Fixing a bug in Image where IE throws a native JavaScript exception if an image is attached and detached immediately. The problem occurs when we try to dispatch a synthetic load event on an unattached element, which IE doesn't like. Now, if we detect that the widget is detached, we mark an expando so that we trigger the event the next time the image is attached.
Also, I fixed a bug where we were firing multiple load events after multiple synchronous changes to a clipped state when we should only fire one load event. The test cases were incorrectly testing that we received multiple events, but all four load events would fire AFTER the final synchronous change, which is not useful, and contradictary to how load events are fired if you change the src of an img multiple times in a single event loop.
Review at http://gwt-code-reviews.appspot.com/1421801
Review by: pdr@google.com
git-svn-id: https://google-web-toolkit.googlecode.com/svn/trunk@10005 8db76d5a-ed1c-0410-87a9-c151d255dfc7
diff --git a/user/src/com/google/gwt/user/client/ui/Image.java b/user/src/com/google/gwt/user/client/ui/Image.java
index 6e711b3..45b4d42 100644
--- a/user/src/com/google/gwt/user/client/ui/Image.java
+++ b/user/src/com/google/gwt/user/client/ui/Image.java
@@ -16,6 +16,8 @@
package com.google.gwt.user.client.ui;
import com.google.gwt.core.client.GWT;
+import com.google.gwt.core.client.Scheduler;
+import com.google.gwt.core.client.Scheduler.ScheduledCommand;
import com.google.gwt.dom.client.Document;
import com.google.gwt.dom.client.Element;
import com.google.gwt.dom.client.ImageElement;
@@ -72,8 +74,6 @@
import com.google.gwt.event.dom.client.TouchStartHandler;
import com.google.gwt.event.shared.HandlerRegistration;
import com.google.gwt.resources.client.ImageResource;
-import com.google.gwt.user.client.Command;
-import com.google.gwt.user.client.DeferredCommand;
import com.google.gwt.user.client.Event;
import com.google.gwt.user.client.ui.impl.ClippedImageImpl;
@@ -137,6 +137,7 @@
private int height = 0;
private int left = 0;
+ private boolean pendingNativeLoadEvent = true;
private int top = 0;
private String url = null;
private int width = 0;
@@ -186,6 +187,13 @@
}
@Override
+ public void onLoadEvent(Image image) {
+ // A load event has fired.
+ pendingNativeLoadEvent = false;
+ super.onLoadEvent(image);
+ }
+
+ @Override
public void setUrl(Image image, String url) {
image.changeState(new UnclippedState(image));
// Need to make sure we change the state before an onload event can fire,
@@ -194,10 +202,15 @@
}
@Override
- public void setUrlAndVisibleRect(Image image, String url, int left,
- int top, int width, int height) {
- if (!this.url.equals(url) || this.left != left || this.top != top
- || this.width != width || this.height != height) {
+ public void setUrlAndVisibleRect(Image image, String url, int left, int top, int width,
+ int height) {
+ /*
+ * In the event that the clipping rectangle has not changed, we want to
+ * skip all of the work required with a getImpl().adjust, and we do not
+ * want to fire a load event.
+ */
+ if (!this.url.equals(url) || this.left != left || this.top != top || this.width != width
+ || this.height != height) {
this.url = url;
this.left = left;
@@ -206,29 +219,22 @@
this.height = height;
impl.adjust(image.getElement(), url, left, top, width, height);
- fireSyntheticLoadEvent(image);
+
+ /*
+ * The native load event hasn't fired yet, so we don't need to
+ * synthesize an event. If we did synthesize an event, we would get two
+ * load events.
+ */
+ if (!pendingNativeLoadEvent) {
+ fireSyntheticLoadEvent(image);
+ }
}
}
@Override
public void setVisibleRect(Image image, int left, int top, int width,
int height) {
- /*
- * In the event that the clipping rectangle has not changed, we want to
- * skip all of the work required with a getImpl().adjust, and we do not
- * want to fire a load event.
- */
- if (this.left != left || this.top != top || this.width != width
- || this.height != height) {
-
- this.left = left;
- this.top = top;
- this.width = width;
- this.height = height;
-
- impl.adjust(image.getElement(), url, left, top, width, height);
- fireSyntheticLoadEvent(image);
- }
+ setUrlAndVisibleRect(image, url, left, top, width, height);
}
/* This method is used only by unit tests */
@@ -244,6 +250,11 @@
*/
private abstract static class State {
+ /**
+ * The pending command to create a synthetic event.
+ */
+ private ScheduledCommand syntheticEventCommand = null;
+
public abstract int getHeight(Image image);
public abstract ImageElement getImageElement(Image image);
@@ -256,16 +267,30 @@
public abstract int getWidth(Image image);
+ /**
+ * Called when the widget is attached to the page. Not to be confused with
+ * the load event that fires when the image loads.
+ *
+ * @param image the widget
+ */
public void onLoad(Image image) {
// If an onload event fired while the image wasn't attached, we need to
// synthesize one now.
- String unhandledEvent = getImageElement(image).getPropertyString(
- UNHANDLED_EVENT_ATTR);
+ String unhandledEvent = getImageElement(image).getPropertyString(UNHANDLED_EVENT_ATTR);
if ("load".equals(unhandledEvent)) {
fireSyntheticLoadEvent(image);
}
}
+ /**
+ * Called when a load event is handled by the widget.
+ *
+ * @param image the widget
+ */
+ public void onLoadEvent(Image image) {
+ // Overridden by ClippedState.
+ }
+
public abstract void setUrl(Image image, String url);
public abstract void setUrlAndVisibleRect(Image image, String url,
@@ -287,12 +312,33 @@
* that a second load event would occur while you are in the load event
* handler.
*/
- DeferredCommand.addCommand(new Command() {
+ syntheticEventCommand = new ScheduledCommand() {
public void execute() {
+ /*
+ * The state has been replaced, or another load event is already
+ * pending.
+ */
+ if (image.state != State.this || this != syntheticEventCommand) {
+ return;
+ }
+ syntheticEventCommand = null;
+
+ /*
+ * The image is not attached, so we cannot safely fire the event. We
+ * still want the event to fire eventually, so we mark an unhandled
+ * load event, which will trigger a new synthetic event the next time
+ * the widget is attached.
+ */
+ if (!image.isAttached()) {
+ getImageElement(image).setPropertyString(UNHANDLED_EVENT_ATTR, "load");
+ return;
+ }
+
NativeEvent evt = Document.get().createLoadEvent();
getImageElement(image).dispatchEvent(evt);
}
- });
+ };
+ Scheduler.get().scheduleDeferred(syntheticEventCommand);
}
// This method is used only by unit tests.
@@ -685,6 +731,7 @@
// handlers could trigger onLoad, which would refire the event.
if (event.getTypeInt() == Event.ONLOAD) {
clearUnhandledEvent();
+ state.onLoadEvent(this);
}
super.onBrowserEvent(event);
diff --git a/user/test/com/google/gwt/user/client/ui/ImageTest.java b/user/test/com/google/gwt/user/client/ui/ImageTest.java
index 7007839..0827501 100644
--- a/user/test/com/google/gwt/user/client/ui/ImageTest.java
+++ b/user/test/com/google/gwt/user/client/ui/ImageTest.java
@@ -117,6 +117,23 @@
}
/**
+ * The default timeout of asynchronous tests. This should be larger than
+ * LOAD_EVENT_TIMEOUT and SYNTHETIC_LOAD_EVENT_TIMEOUT.
+ */
+ private static final int DEFAULT_TEST_TIMEOUT = 10000;
+
+ /**
+ * The amount of time to wait for a load event to fire in milliseconds.
+ */
+ private static final int LOAD_EVENT_TIMEOUT = 7000;
+
+ /**
+ * The amount of time to wait for a clipped image to fire a synthetic load
+ * event in milliseconds.
+ */
+ private static final int SYNTHETIC_LOAD_EVENT_TIMEOUT = 1000;
+
+ /**
* Helper method that allows us to 'peek' at the private <code>state</code>
* field in the Image object, and call the <code>state.getStateName()</code>
* method.
@@ -130,6 +147,10 @@
return imgState.@com.google.gwt.user.client.ui.Image.State::getStateName() ();
}-*/;
+ private static native boolean isIE6() /*-{
+ return @com.google.gwt.dom.client.DOMImplIE6::isIE6()();
+ }-*/;
+
private int firedError;
private int firedLoad;
@@ -151,6 +172,35 @@
}
/**
+ * Test that attaching and immediately detaching an element does not cause an
+ * error.
+ */
+ public void testAttachDetach() {
+ /*
+ * This test fails on IE6 due to the way ImageSrcIE6 delays setting the
+ * src of images. It may be a real error, but it doesn't seem to affect
+ * apps and its too obscure to spend time fixing it.
+ */
+ if (isIE6()) {
+ return;
+ }
+
+ final Image image = new Image("counting-forwards.png");
+ RootPanel.get().add(image);
+ RootPanel.get().remove(image);
+
+ // Wait for the synthetic event to attempt to fire.
+ delayTestFinish(DEFAULT_TEST_TIMEOUT);
+ new Timer() {
+ @Override
+ public void run() {
+ // The synthetic event did not cause an error.
+ finishTest();
+ }
+ }.schedule(SYNTHETIC_LOAD_EVENT_TIMEOUT);
+ }
+
+ /**
* Tests the transition from the clipped state to the unclipped state.
*/
@DoNotRunWith(Platform.HtmlUnitUnknown)
@@ -158,7 +208,7 @@
final Image image = new Image("counting-forwards.png", 12, 13, 8, 8);
assertEquals("clipped", getCurrentImageStateName(image));
- delayTestFinish(5000);
+ delayTestFinish(DEFAULT_TEST_TIMEOUT);
image.addErrorHandler(new TestErrorHandler(image));
image.addLoadHandler(new LoadHandler() {
private int onLoadEventCount = 0;
@@ -189,7 +239,7 @@
final Image image = new Image("counting-forwards.png");
assertEquals("unclipped", getCurrentImageStateName(image));
- delayTestFinish(5000);
+ delayTestFinish(DEFAULT_TEST_TIMEOUT);
image.addErrorHandler(new TestErrorHandler(image));
image.addLoadHandler(new LoadHandler() {
private int onLoadEventCount = 0;
@@ -214,13 +264,68 @@
}
/**
+ * Tests the transition from the unclipped state to the clipped state
+ * before a load event fires.
+ */
+ @DoNotRunWith(Platform.HtmlUnitUnknown)
+ public void testChangeImageToClippedSynchronously() {
+ /*
+ * This test fails on IE6 due to the way ImageSrcIE6 delays setting the
+ * src of images. It may be a real error, but it doesn't seem to affect
+ * apps and its too obscure to spend time fixing it.
+ */
+ if (isIE6()) {
+ return;
+ }
+
+ final Image image = new Image("counting-forwards.png");
+ assertEquals("unclipped", getCurrentImageStateName(image));
+
+ image.addErrorHandler(new TestErrorHandler(image));
+ final TestLoadHandler loadHandler = new TestLoadHandler() {
+ public void onLoad(LoadEvent event) {
+ if (isFinished()) {
+ fail("LoadHandler fired twice. Expected it to fire only once.");
+ }
+
+ assertEquals("clipped", getCurrentImageStateName(image));
+ assertEquals(12, image.getOriginLeft());
+ assertEquals(13, image.getOriginTop());
+ assertEquals(8, image.getWidth());
+ assertEquals(8, image.getHeight());
+ finish();
+ }
+ };
+ image.addLoadHandler(loadHandler);
+
+ /*
+ * Change the image to a clipped image before a load event fires. We only
+ * expect one asynchronous load event to fire for the final state. This is
+ * consistent with the expected behavior of changing the source URL multiple
+ * times.
+ */
+ RootPanel.get().add(image);
+ image.setVisibleRect(12, 13, 8, 8);
+ assertEquals("clipped", getCurrentImageStateName(image));
+
+ delayTestFinish(DEFAULT_TEST_TIMEOUT);
+ new Timer() {
+ @Override
+ public void run() {
+ assertTrue(loadHandler.isFinished());
+ finishTest();
+ }
+ }.schedule(SYNTHETIC_LOAD_EVENT_TIMEOUT);
+ }
+
+ /**
* Tests the creation of an image in unclipped mode.
*/
@DoNotRunWith(Platform.HtmlUnitUnknown)
public void testCreateImage() {
final Image image = new Image("counting-forwards.png");
- delayTestFinish(5000);
+ delayTestFinish(DEFAULT_TEST_TIMEOUT);
image.addErrorHandler(new TestErrorHandler(image));
image.addLoadHandler(new LoadHandler() {
private int onLoadEventCount = 0;
@@ -247,7 +352,7 @@
public void testCreateImageWithError() {
final Image image = new Image("imageDoesNotExist.png");
- delayTestFinish(5000);
+ delayTestFinish(DEFAULT_TEST_TIMEOUT);
image.addErrorHandler(new ErrorHandler() {
public void onError(ErrorEvent event) {
finishTest();
@@ -270,7 +375,7 @@
public void testSetUrlAndLoadEventsOnUnclippedImage() {
final Image image = new Image();
- delayTestFinish(5000);
+ delayTestFinish(DEFAULT_TEST_TIMEOUT);
image.addErrorHandler(new TestErrorHandler(image));
image.addLoadHandler(new LoadHandler() {
private int onLoadEventCount = 0;
@@ -298,7 +403,7 @@
final Image image = new Image("counting-backwards.png");
assertEquals("unclipped", getCurrentImageStateName(image));
- delayTestFinish(5000);
+ delayTestFinish(DEFAULT_TEST_TIMEOUT);
image.addErrorHandler(new TestErrorHandler(image));
image.addLoadHandler(new LoadHandler() {
private int onLoadEventCount = 0;
@@ -329,7 +434,7 @@
public void testCreateClippedImage() {
final Image image = new Image("counting-forwards.png", 16, 16, 16, 16);
- delayTestFinish(5000);
+ delayTestFinish(DEFAULT_TEST_TIMEOUT);
final TestLoadListener listener = new TestLoadListener(image) {
private int onLoadEventCount = 0;
@@ -393,13 +498,48 @@
}
/**
+ * Test that attaching an image multiple times results in only one load event.
+ */
+ public void testMultipleAttach() {
+ final Image image = new Image("counting-forwards.png");
+
+ final TestLoadHandler loadHandler = new TestLoadHandler() {
+ public void onLoad(LoadEvent event) {
+ if (isFinished()) {
+ fail("LoadHandler fired multiple times.");
+ }
+ finish();
+ }
+ };
+ image.addErrorHandler(new TestErrorHandler(image));
+ image.addLoadHandler(loadHandler);
+
+ RootPanel.get().add(image);
+ RootPanel.get().remove(image);
+ RootPanel.get().add(image);
+ RootPanel.get().remove(image);
+ RootPanel.get().add(image);
+ RootPanel.get().remove(image);
+ RootPanel.get().add(image);
+
+ delayTestFinish(DEFAULT_TEST_TIMEOUT);
+ new Timer() {
+ @Override
+ public void run() {
+ assertTrue(loadHandler.isFinished());
+ finishTest();
+ }
+ }.schedule(LOAD_EVENT_TIMEOUT);
+ }
+
+ /**
* Verify that detaching and reattaching an image in a handler does not fire a
* second onload event.
*/
public void testNoEventOnReattachInHandler() {
final Image image = new Image("counting-forwards.png");
- delayTestFinish(5000);
+ delayTestFinish(DEFAULT_TEST_TIMEOUT);
image.addErrorHandler(new TestErrorHandler(image));
image.addLoadHandler(new LoadHandler() {
private int onLoadEventCount = 0;
@@ -430,18 +570,18 @@
finish();
}
};
- delayTestFinish(6000);
+ image.addErrorHandler(new TestErrorHandler(image));
+ image.addLoadHandler(loadHandler);
+
+ RootPanel.get().add(image);
+ delayTestFinish(DEFAULT_TEST_TIMEOUT);
new Timer() {
@Override
public void run() {
assertTrue(loadHandler.isFinished());
finishTest();
}
- }.schedule(5000);
- image.addErrorHandler(new TestErrorHandler(image));
- image.addLoadHandler(loadHandler);
-
- RootPanel.get().add(image);
+ }.schedule(LOAD_EVENT_TIMEOUT);
}
public void testOneEventOnlyClippedImage() {
@@ -455,19 +595,19 @@
finish();
}
};
- delayTestFinish(6000);
+ image.addErrorHandler(new TestErrorHandler(image));
+ image.addLoadHandler(loadHandler);
+
+ RootPanel.get().add(image);
+ delayTestFinish(DEFAULT_TEST_TIMEOUT);
new Timer() {
@Override
public void run() {
assertTrue(loadHandler.isFinished());
finishTest();
}
- }.schedule(5000);
- image.addErrorHandler(new TestErrorHandler(image));
- image.addLoadHandler(loadHandler);
-
- RootPanel.get().add(image);
- }
+ }.schedule(LOAD_EVENT_TIMEOUT);
+ }
public void testResourceConstructor() {
Bundle b = GWT.create(Bundle.class);
@@ -490,47 +630,56 @@
@SuppressWarnings("deprecation")
public void testSetUrlAndVisibleRectOnClippedImage() {
final Image image = new Image("counting-backwards.png", 12, 12, 12, 12);
- delayTestFinish(5000);
final TestLoadListener listener = new TestLoadListener(image) {
- private int onLoadEventCount = 0;
-
public void onLoad(Widget sender) {
- if (++onLoadEventCount == 2) {
- assertEquals(0, image.getOriginLeft());
- assertEquals(16, image.getOriginTop());
- assertEquals(16, image.getWidth());
- assertEquals(16, image.getHeight());
- assertEquals("clipped", getCurrentImageStateName(image));
- finish();
+ if (isFinished()) {
+ fail("LoadListener fired twice. Expected it to fire only once.");
}
+
+ assertEquals(0, image.getOriginLeft());
+ assertEquals(16, image.getOriginTop());
+ assertEquals(16, image.getWidth());
+ assertEquals(16, image.getHeight());
+ assertEquals("clipped", getCurrentImageStateName(image));
+ finish();
}
};
image.addLoadListener(listener);
- image.addLoadHandler(new LoadHandler() {
- private int onLoadEventCount = 0;
-
+ final TestLoadHandler loadHandler = new TestLoadHandler() {
public void onLoad(LoadEvent event) {
- if (++onLoadEventCount == 2) {
- assertEquals(0, image.getOriginLeft());
- assertEquals(16, image.getOriginTop());
- assertEquals(16, image.getWidth());
- assertEquals(16, image.getHeight());
- assertEquals("clipped", getCurrentImageStateName(image));
- if (listener.isFinished()) {
- finishTest();
- } else {
- fail("Listener did not fire first");
- }
+ if (isFinished()) {
+ fail("LoadHandler fired twice. Expected it to fire only once.");
+ }
+ if (listener.isFinished()) {
+ finish();
+ } else {
+ fail("Listener did not fire first");
}
}
- });
+ };
+ image.addLoadHandler(loadHandler);
image.addErrorHandler(new TestErrorHandler(image));
RootPanel.get().add(image);
assertEquals("clipped", getCurrentImageStateName(image));
+
+ /*
+ * Change the url and visible rect, but we only expect one asynchronous load
+ * event to fire. This is consistent with the expected behavior of changing
+ * the source URL in the same event loop.
+ */
image.setUrlAndVisibleRect("counting-forwards.png", 0, 16, 16, 16);
+
+ delayTestFinish(DEFAULT_TEST_TIMEOUT);
+ new Timer() {
+ @Override
+ public void run() {
+ assertTrue(loadHandler.isFinished());
+ finishTest();
+ }
+ }.schedule(SYNTHETIC_LOAD_EVENT_TIMEOUT);
}
/**
@@ -542,38 +691,51 @@
public void testSetVisibleRectAndLoadEventsOnClippedImage() {
final Image image = new Image("counting-backwards.png", 16, 16, 16, 16);
- delayTestFinish(5000);
final TestLoadListener listener = new TestLoadListener(image) {
- private int onLoadEventCount = 0;
-
public void onLoad(Widget sender) {
- if (++onLoadEventCount == 4) {
- finish();
+ if (isFinished()) {
+ fail("LoadListener fired twice. Expected it to fire only once.");
}
+ finish();
}
};
image.addLoadListener(listener);
- image.addLoadHandler(new LoadHandler() {
- private int onLoadEventCount = 0;
-
+ final TestLoadHandler loadHandler = new TestLoadHandler() {
public void onLoad(LoadEvent event) {
- if (++onLoadEventCount == 4) {
- if (listener.isFinished()) {
- finishTest();
- } else {
- fail("Listener did not fire first");
- }
+ if (isFinished()) {
+ fail("LoadHandler fired twice. Expected it to fire only once.");
+ }
+ if (listener.isFinished()) {
+ finish();
+ } else {
+ fail("Listener did not fire first");
}
}
- });
+ };
+ image.addLoadHandler(loadHandler);
image.addErrorHandler(new TestErrorHandler(image));
RootPanel.get().add(image);
+
+ /*
+ * Change the visible rect multiple times, but we only expect one
+ * asynchronous load event to fire after the final change. This is consistent
+ * with the expected behavior of changing the source URL multiple times.
+ */
image.setVisibleRect(0, 0, 16, 16);
image.setVisibleRect(0, 0, 16, 16);
image.setVisibleRect(16, 0, 16, 16);
image.setVisibleRect(16, 8, 8, 8);
+
+ delayTestFinish(DEFAULT_TEST_TIMEOUT);
+ new Timer() {
+ @Override
+ public void run() {
+ assertTrue(loadHandler.isFinished());
+ finishTest();
+ }
+ }.schedule(SYNTHETIC_LOAD_EVENT_TIMEOUT);
}
/**
