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();