From e9a4f0f8d48bfc87e209f06f06ba538d297d1c5d Mon Sep 17 00:00:00 2001 From: Neil Deakin Date: Mon, 14 Jul 2014 13:39:04 -0400 Subject: [PATCH] Bug 1026310, set the open state only once the opening transition is finished, rename the internal popup states to be clearer, fix UITour.jsm to check the showing state, r=MattN,neil --- browser/modules/UITour.jsm | 4 +-- layout/xul/nsMenuPopupFrame.cpp | 18 ++++++++++--- layout/xul/nsMenuPopupFrame.h | 25 +++++++++++++------ layout/xul/nsPopupBoxObject.cpp | 21 +++++++--------- layout/xul/nsXULPopupManager.cpp | 13 ++++++---- .../content/tests/chrome/test_arrowpanel.xul | 3 +++ 6 files changed, 54 insertions(+), 30 deletions(-) diff --git a/browser/modules/UITour.jsm b/browser/modules/UITour.jsm index c9edd94abf5e..05dd265c41c3 100644 --- a/browser/modules/UITour.jsm +++ b/browser/modules/UITour.jsm @@ -842,7 +842,7 @@ this.UITour = { highlighter.style.width = highlightWidth + "px"; // Close a previous highlight so we can relocate the panel. - if (highlighter.parentElement.state == "open") { + if (highlighter.parentElement.state == "showing" || highlighter.parentElement.state == "open") { highlighter.parentElement.hidePopup(); } /* The "overlap" position anchors from the top-left but we want to centre highlights at their @@ -909,7 +909,7 @@ this.UITour = { let tooltipIcon = document.getElementById("UITourTooltipIcon"); let tooltipButtons = document.getElementById("UITourTooltipButtons"); - if (tooltip.state == "open") { + if (tooltip.state == "showing" || tooltip.state == "open") { tooltip.hidePopup(); } diff --git a/layout/xul/nsMenuPopupFrame.cpp b/layout/xul/nsMenuPopupFrame.cpp index eb8cb2a4f81b..ae608896cffc 100644 --- a/layout/xul/nsMenuPopupFrame.cpp +++ b/layout/xul/nsMenuPopupFrame.cpp @@ -333,6 +333,12 @@ nsMenuPopupFrame::GetShadowStyle() NS_IMETHODIMP nsXULPopupShownEvent::Run() { + nsMenuPopupFrame* popup = do_QueryFrame(mPopup->GetPrimaryFrame()); + // Set the state to visible if the popup is still open. + if (popup && popup->IsOpen()) { + popup->SetPopupState(ePopupShown); + } + WidgetMouseEvent event(true, NS_XUL_POPUP_SHOWN, nullptr, WidgetMouseEvent::eReal); return EventDispatcher::Dispatch(mPopup, mPresContext, &event); @@ -486,8 +492,11 @@ nsMenuPopupFrame::LayoutPopup(nsBoxLayoutState& aState, nsIFrame* aParentMenu, rect.x = rect.y = 0; viewManager->ResizeView(view, rect); + if (mPopupState == ePopupOpening) { + mPopupState = ePopupVisible; + } + viewManager->SetViewVisibility(view, nsViewVisibility_kShow); - mPopupState = ePopupOpenAndVisible; nsContainerFrame::SyncFrameViewProperties(pc, this, nullptr, view, 0); } @@ -774,7 +783,7 @@ nsMenuPopupFrame::ShowPopup(bool aIsContextMenu, bool aSelectFirstItem) InvalidateFrameSubtree(); if (mPopupState == ePopupShowing) { - mPopupState = ePopupOpen; + mPopupState = ePopupOpening; mIsOpenChanged = true; nsMenuFrame* menuFrame = do_QueryFrame(GetParent()); @@ -1980,12 +1989,13 @@ nsMenuPopupFrame::MoveToAnchor(nsIContent* aAnchorContent, int32_t aXPos, int32_t aYPos, bool aAttributesOverride) { - NS_ASSERTION(mPopupState == ePopupOpenAndVisible, "popup must be open to move it"); + NS_ASSERTION(IsVisible(), "popup must be visible to move it"); + nsPopupState oldstate = mPopupState; InitializePopup(aAnchorContent, mTriggerContent, aPosition, aXPos, aYPos, aAttributesOverride); // InitializePopup changed the state so reset it. - mPopupState = ePopupOpenAndVisible; + mPopupState = oldstate; // Pass false here so that flipping and adjusting to fit on the screen happen. SetPopupPosition(nullptr, false, false); diff --git a/layout/xul/nsMenuPopupFrame.h b/layout/xul/nsMenuPopupFrame.h index e221fb89ead7..2456685fb163 100644 --- a/layout/xul/nsMenuPopupFrame.h +++ b/layout/xul/nsMenuPopupFrame.h @@ -27,10 +27,15 @@ class nsIWidget; // state changes as follows: // ePopupClosed - initial state // ePopupShowing - during the period when the popupshowing event fires -// ePopupOpen - between the popupshowing event and being visible. Creation -// of the child frames, layout and reflow occurs in this state. -// ePopupOpenAndVisible - layout is done and the popup's view and widget are -// made visible. The popupshown event fires. +// ePopupOpening - between the popupshowing event and being visible. Creation +// of the child frames, layout and reflow occurs in this +// state. The popup is stored in the popup manager's list of +// open popups during this state. +// ePopupVisible - layout is done and the popup's view and widget are made +// visible. The popup is visible on screen but may be +// transitioning. The popupshown event has not yet fired. +// ePopupShown - the popup has been shown and is fully ready. This state is +// assigned just before the popupshown event fires. // When closing a popup: // ePopupHidden - during the period when the popuphiding event fires and // the popup is removed. @@ -42,9 +47,11 @@ enum nsPopupState { // popupshowing event has been fired. ePopupShowing, // state while a popup is open but the widget is not yet visible - ePopupOpen, + ePopupOpening, + // state while a popup is visible and waiting for the popupshown event + ePopupVisible, // state while a popup is open and visible on screen - ePopupOpenAndVisible, + ePopupShown, // state from when a popup is requested to be hidden to when it is closed. ePopupHiding, // state which indicates that the popup was hidden without firing the @@ -251,7 +258,11 @@ public: nsPopupType PopupType() const { return mPopupType; } bool IsMenu() MOZ_OVERRIDE { return mPopupType == ePopupTypeMenu; } - bool IsOpen() MOZ_OVERRIDE { return mPopupState == ePopupOpen || mPopupState == ePopupOpenAndVisible; } + bool IsOpen() MOZ_OVERRIDE { return mPopupState == ePopupOpening || + mPopupState == ePopupVisible || + mPopupState == ePopupShown; } + bool IsVisible() { return mPopupState == ePopupVisible || + mPopupState == ePopupShown; } bool IsMouseTransparent() { return mMouseTransparent; } diff --git a/layout/xul/nsPopupBoxObject.cpp b/layout/xul/nsPopupBoxObject.cpp index 4491c1f2b7e3..bf2871bf728d 100644 --- a/layout/xul/nsPopupBoxObject.cpp +++ b/layout/xul/nsPopupBoxObject.cpp @@ -128,7 +128,7 @@ nsPopupBoxObject::MoveToAnchor(nsIDOMElement* aAnchorElement, nsCOMPtr anchorContent(do_QueryInterface(aAnchorElement)); nsMenuPopupFrame *menuPopupFrame = do_QueryFrame(mContent->GetPrimaryFrame()); - if (menuPopupFrame && menuPopupFrame->PopupState() == ePopupOpenAndVisible) { + if (menuPopupFrame && menuPopupFrame->IsVisible()) { menuPopupFrame->MoveToAnchor(anchorContent, aPosition, aXPos, aYPos, aAttributesOverride); } } @@ -226,13 +226,14 @@ nsPopupBoxObject::GetPopupState(nsAString& aState) nsMenuPopupFrame *menuPopupFrame = mContent ? do_QueryFrame(mContent->GetPrimaryFrame()) : nullptr; if (menuPopupFrame) { switch (menuPopupFrame->PopupState()) { - case ePopupShowing: - case ePopupOpen: - aState.AssignLiteral("showing"); - break; - case ePopupOpenAndVisible: + case ePopupShown: aState.AssignLiteral("open"); break; + case ePopupShowing: + case ePopupOpening: + case ePopupVisible: + aState.AssignLiteral("showing"); + break; case ePopupHiding: case ePopupInvisible: aState.AssignLiteral("hiding"); @@ -284,13 +285,9 @@ nsPopupBoxObject::GetOuterScreenRect(nsIDOMClientRect** aRect) NS_ADDREF(*aRect = rect); - nsMenuPopupFrame *menuPopupFrame = do_QueryFrame(GetFrame(false)); - if (!menuPopupFrame) - return NS_OK; - // Return an empty rectangle if the popup is not open. - nsPopupState state = menuPopupFrame->PopupState(); - if (state != ePopupOpen && state != ePopupOpenAndVisible) + nsMenuPopupFrame *menuPopupFrame = do_QueryFrame(GetFrame(false)); + if (!menuPopupFrame || !menuPopupFrame->IsOpen()) return NS_OK; nsView* view = menuPopupFrame->GetView(); diff --git a/layout/xul/nsXULPopupManager.cpp b/layout/xul/nsXULPopupManager.cpp index 8e64e1a2d28d..7dd55d2bf215 100644 --- a/layout/xul/nsXULPopupManager.cpp +++ b/layout/xul/nsXULPopupManager.cpp @@ -394,7 +394,7 @@ nsMenuPopupFrame* GetPopupToMoveOrResize(nsIFrame* aFrame) return nullptr; // no point moving or resizing hidden popups - if (menuPopupFrame->PopupState() != ePopupOpenAndVisible) + if (!menuPopupFrame->IsVisible()) return nullptr; nsIWidget* widget = menuPopupFrame->GetWidget(); @@ -1390,7 +1390,11 @@ nsXULPopupManager::FirePopupHidingEvent(nsIContent* aPopup, // from hiding. if (status == nsEventStatus_eConsumeNoDefault && !popupFrame->IsInContentShell()) { - popupFrame->SetPopupState(ePopupOpenAndVisible); + // XXXndeakin + // If an attempt was made to hide this popup before the popupshown event + // fired, then ePopupShown is set here even though it should be + // ePopupVisible. This probably isn't worth the hassle of handling. + popupFrame->SetPopupState(ePopupShown); } else { // If the popup has an animate attribute and it is not set to false, assume @@ -1503,10 +1507,9 @@ nsXULPopupManager::GetVisiblePopups(nsTArray& aPopups) nsMenuChainItem* item = mPopups; for (int32_t list = 0; list < 2; list++) { while (item) { - // Skip panels which are not open and visible as well as popups that + // Skip panels which are not visible as well as popups that // are transparent to mouse events. - if (item->Frame()->PopupState() == ePopupOpenAndVisible && - !item->Frame()->IsMouseTransparent()) { + if (item->Frame()->IsVisible() && !item->Frame()->IsMouseTransparent()) { aPopups.AppendElement(item->Frame()); } diff --git a/toolkit/content/tests/chrome/test_arrowpanel.xul b/toolkit/content/tests/chrome/test_arrowpanel.xul index e0a809d0e88e..6bed0a50dece 100644 --- a/toolkit/content/tests/chrome/test_arrowpanel.xul +++ b/toolkit/content/tests/chrome/test_arrowpanel.xul @@ -186,10 +186,12 @@ function nextTest() transitions++; // Two properties transition so continue on the second one finishing. if (!(transitions % 2)) { + is($("animatepanel").state, "open", "state is open after transitionend"); ok(animatedPopupShown, "popupshown now fired") SimpleTest.executeSoon(() => runNextTest.next()); } else { + is($("animatepanel").state, "showing", "state is showing during transitionend"); ok(!animatedPopupShown, "popupshown not fired yet") } } @@ -197,6 +199,7 @@ function nextTest() // Check that the transition occurs for an arrow panel with animate="true" window.addEventListener("transitionend", transitionEnded, false); $("animatepanel").openPopup($("topleft"), "after_start", 0, 0, false, false, null, "start"); + is($("animatepanel").state, "showing", "state is showing"); yield; window.removeEventListener("transitionend", transitionEnded, false);