Remove the glass panel from DOM when Popup#removeFromParent is called.
http://gwt-code-reviews.appspot.com/220801
Review by: jgw@google.com
git-svn-id: https://google-web-toolkit.googlecode.com/svn/trunk@7746 8db76d5a-ed1c-0410-87a9-c151d255dfc7
diff --git a/user/src/com/google/gwt/user/client/ui/PopupPanel.java b/user/src/com/google/gwt/user/client/ui/PopupPanel.java
index ed49e9f..8e9dcd2 100644
--- a/user/src/com/google/gwt/user/client/ui/PopupPanel.java
+++ b/user/src/com/google/gwt/user/client/ui/PopupPanel.java
@@ -34,12 +34,11 @@
import com.google.gwt.event.logical.shared.ValueChangeHandler;
import com.google.gwt.event.shared.HandlerRegistration;
import com.google.gwt.i18n.client.LocaleInfo;
-import com.google.gwt.user.client.Command;
import com.google.gwt.user.client.DOM;
-import com.google.gwt.user.client.DeferredCommand;
import com.google.gwt.user.client.Event;
import com.google.gwt.user.client.EventPreview;
import com.google.gwt.user.client.History;
+import com.google.gwt.user.client.Timer;
import com.google.gwt.user.client.Window;
import com.google.gwt.user.client.Event.NativePreviewEvent;
import com.google.gwt.user.client.Event.NativePreviewHandler;
@@ -134,6 +133,13 @@
private PopupPanel curPanel = null;
/**
+ * Indicates whether or not the {@link PopupPanel} is in the process of
+ * unloading. If the popup is unloading, then the animation just does
+ * cleanup.
+ */
+ private boolean isUnloading;
+
+ /**
* The offset height and width of the current {@link PopupPanel}.
*/
private int offsetHeight, offsetWidth = -1;
@@ -144,6 +150,11 @@
private boolean showing;
/**
+ * The timer used to delay the show animation.
+ */
+ private Timer showTimer;
+
+ /**
* A boolean indicating whether the glass element is currently attached.
*/
private boolean glassShowing;
@@ -166,12 +177,25 @@
*
* @param showing true if the popup is showing, false if not
*/
- public void setState(boolean showing) {
+ public void setState(boolean showing, boolean isUnloading) {
// Immediately complete previous open/close animation
+ this.isUnloading = isUnloading;
cancel();
+ // If there is a pending timer to start a show animation, then just cancel
+ // the timer and complete the show operation.
+ if (showTimer != null) {
+ showTimer.cancel();
+ showTimer = null;
+ onComplete();
+ }
+
+ // Update the logical state.
+ curPanel.showing = showing;
+ curPanel.updateHandlers();
+
// Determine if we need to animate
- boolean animate = curPanel.isAnimationEnabled;
+ boolean animate = !isUnloading && curPanel.isAnimationEnabled;
if (curPanel.animType != AnimationType.CENTER && !showing) {
animate = false;
}
@@ -196,14 +220,21 @@
impl.setClip(curPanel.getElement(), getRectString(0, 0, 0, 0));
RootPanel.get().add(curPanel);
impl.onShow(curPanel.getElement());
- }
- // Wait for the popup panel to be attached before running the animation
- DeferredCommand.addCommand(new Command() {
- public void execute() {
- run(ANIMATION_DURATION);
- }
- });
+ // Wait for the popup panel and iframe to be attached before running
+ // the animation. We use a Timer instead of a DeferredCommand so we
+ // can cancel it if the popup is hidden synchronously.
+ showTimer = new Timer() {
+ @Override
+ public void run() {
+ showTimer = null;
+ ResizeAnimation.this.run(ANIMATION_DURATION);
+ }
+ };
+ showTimer.schedule(1);
+ } else {
+ run(ANIMATION_DURATION);
+ }
} else {
onInstantaneousRun();
}
@@ -213,7 +244,9 @@
protected void onComplete() {
if (!showing) {
maybeShowGlass();
- RootPanel.get().remove(curPanel);
+ if (!isUnloading) {
+ RootPanel.get().remove(curPanel);
+ }
impl.onHide(curPanel.getElement());
}
impl.setClip(curPanel.getElement(), "rect(auto, auto, auto, auto)");
@@ -311,7 +344,9 @@
RootPanel.get().add(curPanel);
impl.onShow(curPanel.getElement());
} else {
- RootPanel.get().remove(curPanel);
+ if (!isUnloading) {
+ RootPanel.get().remove(curPanel);
+ }
impl.onHide(curPanel.getElement());
}
DOM.setStyleAttribute(curPanel.getElement(), "overflow", "visible");
@@ -582,7 +617,7 @@
if (!isShowing()) {
return;
}
- setState(false, true);
+ resizeAnimation.setState(false, false);
CloseEvent.fire(this, this, autoClosed);
}
@@ -967,7 +1002,7 @@
if (showing) {
return;
}
- setState(true, true);
+ resizeAnimation.setState(true, false);
}
/**
@@ -1019,11 +1054,13 @@
@Override
protected void onUnload() {
+ super.onUnload();
+
// Just to be sure, we perform cleanup when the popup is unloaded (i.e.
// removed from the DOM). This is normally taken care of in hide(), but it
// can be missed if someone removes the popup directly from the RootPanel.
if (isShowing()) {
- setState(false, false);
+ resizeAnimation.setState(false, true);
}
}
@@ -1363,22 +1400,20 @@
}
/**
- * Set the showing state of the popup. If maybeAnimate is true, the animation
- * will be used to set the state. If it is false, the animation will be
- * cancelled.
- *
- * @param showing the new state
- * @param maybeAnimate true to possibly run the animation
+ * Register or unregister the handlers used by {@link PopupPanel}.
*/
- private void setState(boolean showing, boolean maybeAnimate) {
- if (maybeAnimate) {
- resizeAnimation.setState(showing);
- } else {
- resizeAnimation.cancel();
+ private void updateHandlers() {
+ // Remove any existing handlers.
+ if (nativePreviewHandlerRegistration != null) {
+ nativePreviewHandlerRegistration.removeHandler();
+ nativePreviewHandlerRegistration = null;
}
- this.showing = showing;
+ if (historyHandlerRegistration != null) {
+ historyHandlerRegistration.removeHandler();
+ historyHandlerRegistration = null;
+ }
- // Create or remove the native preview handler
+ // Create handlers if showing.
if (showing) {
nativePreviewHandlerRegistration = Event.addNativePreviewHandler(new NativePreviewHandler() {
public void onPreviewNativeEvent(NativePreviewEvent event) {
@@ -1392,15 +1427,6 @@
}
}
});
- } else {
- if (nativePreviewHandlerRegistration != null) {
- nativePreviewHandlerRegistration.removeHandler();
- nativePreviewHandlerRegistration = null;
- }
- if (historyHandlerRegistration != null) {
- historyHandlerRegistration.removeHandler();
- historyHandlerRegistration = null;
- }
}
}
}
diff --git a/user/test/com/google/gwt/user/client/ui/PopupTest.java b/user/test/com/google/gwt/user/client/ui/PopupTest.java
index 8277412..d6de326 100644
--- a/user/test/com/google/gwt/user/client/ui/PopupTest.java
+++ b/user/test/com/google/gwt/user/client/ui/PopupTest.java
@@ -271,6 +271,23 @@
}
/**
+ * Test the hiding a popup while it is showing will not result in an illegal
+ * state.
+ */
+ public void testHideWhileShowing() {
+ PopupPanel popup = createPopupPanel();
+
+ // Start showing the popup.
+ popup.setAnimationEnabled(true);
+ popup.show();
+ assertTrue(popup.isShowing());
+
+ // Hide the popup while its showing.
+ popup.hide();
+ assertFalse(popup.isShowing());
+ }
+
+ /**
* Test that the onLoad method is only called once when showing the popup.
*/
public void testOnLoad() {
@@ -366,6 +383,21 @@
popup.hide();
}
+ /**
+ * Issue 4720: Glass is not removed when removeFromParent is called.
+ */
+ public void testRemoveFromParent() {
+ PopupPanel popup = createPopupPanel();
+ popup.setGlassEnabled(true);
+ assertNull(popup.getGlassElement().getParentElement());
+
+ popup.show();
+ assertNotNull(popup.getGlassElement().getParentElement());
+
+ popup.removeFromParent();
+ assertNull(popup.getGlassElement().getParentElement());
+ }
+
public void testSeparateContainers() {
TestablePopupPanel p1 = new TestablePopupPanel();
TestablePopupPanel p2 = new TestablePopupPanel();
