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

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

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

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
+//   ePopupOpening - between the popupshowing event and being visible. Creation
-//                of the child frames, layout and reflow occurs in this state.
+//                   of the child frames, layout and reflow occurs in this
-//   ePopupOpenAndVisible - layout is done and the popup's view and widget are
+//                   state. The popup is stored in the popup manager's list of
-//                          made visible. The popupshown event fires.
+//                   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; }
 

layout/xul/nsPopupBoxObject.cpp

@@ -128,7 +128,7 @@ nsPopupBoxObject::MoveToAnchor(nsIDOMElement* aAnchorElement,
     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);
     }
   }
@@ -226,13 +226,14 @@ nsPopupBoxObject::GetPopupState(nsAString& aState)
   nsMenuPopupFrame *menuPopupFrame = mContent ? do_QueryFrame(mContent->GetPrimaryFrame()) : nullptr;
   if (menuPopupFrame) {
     switch (menuPopupFrame->PopupState()) {
-      case ePopupShowing:
+      case ePopupShown:
-      case ePopupOpen:
-        aState.AssignLiteral("showing");
-        break;
-      case ePopupOpenAndVisible:
         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();
+  nsMenuPopupFrame *menuPopupFrame = do_QueryFrame(GetFrame(false));
-  if (state != ePopupOpen && state != ePopupOpenAndVisible)
+  if (!menuPopupFrame || !menuPopupFrame->IsOpen())
     return NS_OK;
 
   nsView* view = menuPopupFrame->GetView();

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<nsIFrame *>& 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 &&
+      if (item->Frame()->IsVisible() && !item->Frame()->IsMouseTransparent()) {
-          !item->Frame()->IsMouseTransparent()) {
         aPopups.AppendElement(item->Frame());
       }
 

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