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
authorNeil Deakin <neil@mozilla.com>
Mon, 14 Jul 2014 13:39:04 -0400
changeset 215848 f32dd267971e261704d3cd9a61edb9dd2cfc79b7
parent 215847 40522c4512d0aa6794dd73a4ced5245c0f906cfc
child 215849 f78da59d7cbd95477b67dd82aab09a72672b4205
push id515
push userraliiev@mozilla.com
push dateMon, 06 Oct 2014 12:51:51 +0000
treeherdermozilla-release@267c7a481bef [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersMattN, neil
bugs1026310
milestone33.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
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
layout/xul/nsMenuPopupFrame.cpp
layout/xul/nsMenuPopupFrame.h
layout/xul/nsPopupBoxObject.cpp
layout/xul/nsXULPopupManager.cpp
toolkit/content/tests/chrome/test_arrowpanel.xul
--- a/browser/modules/UITour.jsm
+++ b/browser/modules/UITour.jsm
@@ -837,17 +837,17 @@ this.UITour = {
       } else {
         highlighter.style.borderRadius = "";
       }
 
       highlighter.style.height = highlightHeight + "px";
       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
          minimum size. */
       let highlightWindow = aTargetEl.ownerDocument.defaultView;
       let containerStyle = highlightWindow.getComputedStyle(highlighter.parentElement);
       let paddingTopPx = 0 - parseFloat(containerStyle.paddingTop);
       let paddingLeftPx = 0 - parseFloat(containerStyle.paddingLeft);
@@ -904,17 +904,17 @@ this.UITour = {
 
       let document = aAnchorEl.ownerDocument;
       let tooltip = document.getElementById("UITourTooltip");
       let tooltipTitle = document.getElementById("UITourTooltipTitle");
       let tooltipDesc = document.getElementById("UITourTooltipDescription");
       let tooltipIcon = document.getElementById("UITourTooltipIcon");
       let tooltipButtons = document.getElementById("UITourTooltipButtons");
 
-      if (tooltip.state == "open") {
+      if (tooltip.state == "showing" || tooltip.state == "open") {
         tooltip.hidePopup();
       }
 
       tooltipTitle.textContent = aTitle || "";
       tooltipDesc.textContent = aDescription || "";
       tooltipIcon.src = aIconURL || "";
       tooltipIcon.hidden = !aIconURL;
 
--- a/layout/xul/nsMenuPopupFrame.cpp
+++ b/layout/xul/nsMenuPopupFrame.cpp
@@ -328,16 +328,22 @@ nsMenuPopupFrame::GetShadowStyle()
     case NS_THEME_MENUPOPUP:
       return NS_STYLE_WINDOW_SHADOW_MENU;
   }
   return NS_STYLE_WINDOW_SHADOW_DEFAULT;
 }
 
 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);                 
 }
 
 NS_IMETHODIMP nsXULPopupShownEvent::HandleEvent(nsIDOMEvent* aEvent)
 {
   nsMenuPopupFrame* popup = do_QueryFrame(mPopup->GetPrimaryFrame());
@@ -481,18 +487,21 @@ nsMenuPopupFrame::LayoutPopup(nsBoxLayou
   }
 
   if (isOpen) {
     nsViewManager* viewManager = view->GetViewManager();
     nsRect rect = GetRect();
     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);
   }
 
   // finally, if the popup just opened, send a popupshown event
   if (mIsOpenChanged) {
     mIsOpenChanged = false;
 
 #ifndef MOZ_WIDGET_GTK
@@ -769,17 +778,17 @@ nsMenuPopupFrame::InitializePopupWithAnc
 void
 nsMenuPopupFrame::ShowPopup(bool aIsContextMenu, bool aSelectFirstItem)
 {
   mIsContextMenu = aIsContextMenu;
 
   InvalidateFrameSubtree();
 
   if (mPopupState == ePopupShowing) {
-    mPopupState = ePopupOpen;
+    mPopupState = ePopupOpening;
     mIsOpenChanged = true;
 
     nsMenuFrame* menuFrame = do_QueryFrame(GetParent());
     if (menuFrame) {
       nsWeakFrame weakFrame(this);
       menuFrame->PopupOpened();
       if (!weakFrame.IsAlive())
         return;
@@ -1975,22 +1984,23 @@ nsMenuPopupFrame::MoveTo(int32_t aLeft, 
 }
 
 void
 nsMenuPopupFrame::MoveToAnchor(nsIContent* aAnchorContent,
                                const nsAString& aPosition,
                                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);
 }
 
 bool
 nsMenuPopupFrame::GetAutoPosition()
 {
--- a/layout/xul/nsMenuPopupFrame.h
+++ b/layout/xul/nsMenuPopupFrame.h
@@ -22,34 +22,41 @@
 #include "nsITimer.h"
 
 class nsIWidget;
 
 // XUL popups can be in several different states. When opening a popup, the
 // 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.
 //   ePopupClosed - the popup's widget is made invisible.
 enum nsPopupState {
   // state when a popup is not open
   ePopupClosed,
   // state from when a popup is requested to be shown to after the
   // 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
   // popuphiding or popuphidden events. It is used when executing a menu
   // command because the menu needs to be hidden before the command event
   // fires, yet the popuphiding and popuphidden events are fired after. This
   // state can also occur when the popup is removed because the document is
   // unloaded.
@@ -246,17 +253,21 @@ public:
   // should be opened as a result, this method should return the frame for
   // that menu, or null if no menu should be opened. Also, calling Enter will
   // reset the current incremental search string, calculated in
   // FindMenuWithShortcut.
   nsMenuFrame* Enter(mozilla::WidgetGUIEvent* aEvent);
 
   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; }
 
   static nsIContent* GetTriggerContent(nsMenuPopupFrame* aMenuPopupFrame);
   void ClearTriggerContent() { mTriggerContent = nullptr; }
 
   // returns true if the popup is in a content shell, or false for a popup in
   // a chrome shell
--- a/layout/xul/nsPopupBoxObject.cpp
+++ b/layout/xul/nsPopupBoxObject.cpp
@@ -123,17 +123,17 @@ nsPopupBoxObject::MoveToAnchor(nsIDOMEle
                                const nsAString& aPosition,
                                int32_t aXPos, int32_t aYPos,
                                bool aAttributesOverride)
 {
   if (mContent) {
     nsCOMPtr<nsIContent> 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);
     }
   }
 
   return NS_OK;
 }
 
 NS_IMETHODIMP
@@ -221,23 +221,24 @@ NS_IMETHODIMP
 nsPopupBoxObject::GetPopupState(nsAString& aState)
 {
   // set this here in case there's no frame for the popup
   aState.AssignLiteral("closed");
 
   nsMenuPopupFrame *menuPopupFrame = mContent ? do_QueryFrame(mContent->GetPrimaryFrame()) : nullptr;
   if (menuPopupFrame) {
     switch (menuPopupFrame->PopupState()) {
+      case ePopupShown:
+        aState.AssignLiteral("open");
+        break;
       case ePopupShowing:
-      case ePopupOpen:
+      case ePopupOpening:
+      case ePopupVisible:
         aState.AssignLiteral("showing");
         break;
-      case ePopupOpenAndVisible:
-        aState.AssignLiteral("open");
-        break;
       case ePopupHiding:
       case ePopupInvisible:
         aState.AssignLiteral("hiding");
         break;
       case ePopupClosed:
         break;
       default:
         NS_NOTREACHED("Bad popup state");
@@ -279,23 +280,19 @@ nsPopupBoxObject::GetAnchorNode(nsIDOMEl
 
 NS_IMETHODIMP
 nsPopupBoxObject::GetOuterScreenRect(nsIDOMClientRect** aRect)
 {
   DOMRect* rect = new DOMRect(mContent);
 
   NS_ADDREF(*aRect = rect);
 
+  // Return an empty rectangle if the popup is not open.
   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)
+  if (!menuPopupFrame || !menuPopupFrame->IsOpen())
     return NS_OK;
 
   nsView* view = menuPopupFrame->GetView();
   if (view) {
     nsIWidget* widget = view->GetWidget();
     if (widget) {
       nsIntRect screenRect;
       widget->GetScreenBounds(screenRect);
--- a/layout/xul/nsXULPopupManager.cpp
+++ b/layout/xul/nsXULPopupManager.cpp
@@ -389,17 +389,17 @@ void nsXULPopupManager::AdjustPopupsOnWi
 static
 nsMenuPopupFrame* GetPopupToMoveOrResize(nsIFrame* aFrame)
 {
   nsMenuPopupFrame* menuPopupFrame = do_QueryFrame(aFrame);
   if (!menuPopupFrame)
     return nullptr;
 
   // no point moving or resizing hidden popups
-  if (menuPopupFrame->PopupState() != ePopupOpenAndVisible)
+  if (!menuPopupFrame->IsVisible())
     return nullptr;
 
   nsIWidget* widget = menuPopupFrame->GetWidget();
   if (widget && !widget->IsVisible())
     return nullptr;
 
   return menuPopupFrame;
 }
@@ -1385,17 +1385,21 @@ nsXULPopupManager::FirePopupHidingEvent(
   // get frame again in case it went away
   nsMenuPopupFrame* popupFrame = do_QueryFrame(aPopup->GetPrimaryFrame());
   if (popupFrame) {
     // if the event was cancelled, don't hide the popup, and reset its
     // state back to open. Only popups in chrome shells can prevent a popup
     // 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
       // that it has a closing transition and wait for it to finish. The transition
       // may still occur either way, but the view will be hidden and you won't be
       // able to see it. If there is a next popup, indicating that mutliple popups
       // are rolling up, don't wait and hide the popup right away since the effect
       // would likely be undesirable. This also does a quick check to see if the
@@ -1498,20 +1502,19 @@ void
 nsXULPopupManager::GetVisiblePopups(nsTArray<nsIFrame *>& aPopups)
 {
   aPopups.Clear();
 
   // Iterate over both lists of popups
   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());
       }
 
       item = item->GetParent();
     }
 
     item = mNoHidePanels;
   }
--- a/toolkit/content/tests/chrome/test_arrowpanel.xul
+++ b/toolkit/content/tests/chrome/test_arrowpanel.xul
@@ -181,27 +181,30 @@ function nextTest()
   // Test that a transition occurs when opening or closing the popup. The transition is
   // disabled on Linux.
   if (navigator.platform.indexOf("Linux") == -1) {
     var transitions = 0;
     function transitionEnded(event) {
       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")
       }
     }
 
     // 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);
 
     synthesizeKey("VK_ESCAPE", { });
     ok(!animatedPopupHidden, "animated popup not hidden yet");
     yield;
   }