Bug 1519917 - Consider <menulist> with sizetopopup unset equal to sizetopopup="pref" r=NeilDeakin
authorTimothy Guan-tin Chien <timdream@gmail.com>
Wed, 16 Jan 2019 16:30:45 +0000
changeset 514095 7484d3a99bfc3de6b40f28bd1fe2c71dd456eb3f
parent 514094 cbfa31396fa4b47a6162423664995167bab86c4d
child 514096 0eafc7a464331f4955977d169485c24b8b57661c
push id1953
push userffxbld-merge
push dateMon, 11 Mar 2019 12:10:20 +0000
treeherdermozilla-release@9c35dcbaa899 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersNeilDeakin
bugs1519917
milestone66.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 1519917 - Consider <menulist> with sizetopopup unset equal to sizetopopup="pref" r=NeilDeakin sizetopopup is set to "pref" by default by the menulist XBL binding, however when converting the binding to custom element, it did not set the attribute value at a time that is early enough. This patch updates nsMenuPopupFrame and nsMenuFrame so that it considers <menulist> with unset sizetopopup attribute as equal to "pref" to avoid the problem above. This reftest layout/reftests/xul/menulist-shrinkwrap-2.xul can detect this failure. The sizetopopup attribute is never meant to be set dynamically; the fix here does not allow us to do so. Differential Revision: https://phabricator.services.mozilla.com/D16410
browser/base/content/test/popups/browser_popup_blocker_identity_block.js
layout/xul/nsMenuFrame.cpp
layout/xul/nsMenuPopupFrame.cpp
toolkit/content/widgets/menulist.xml
--- a/browser/base/content/test/popups/browser_popup_blocker_identity_block.js
+++ b/browser/base/content/test/popups/browser_popup_blocker_identity_block.js
@@ -122,16 +122,17 @@ add_task(async function check_permission
 
   // Wait for popup block.
   await BrowserTestUtils.waitForCondition(() =>
     gBrowser.getNotificationBox().getNotificationWithValue("popup-blocked"));
 
   // Open identity popup and change permission state to allow.
   await openIdentityPopup();
   let menulist = document.getElementById("identity-popup-popup-menulist");
+  menulist.menupopup.openPopup(); // Open the allow/block menu
   let menuitem = menulist.getElementsByTagName("menuitem")[0];
   menuitem.click();
   await closeIdentityPopup();
 
   state = SitePermissions.get(URI, "popup", gBrowser).state;
   Assert.equal(state, SitePermissions.ALLOW);
 
   // Store the popup that opens in this array.
@@ -155,16 +156,17 @@ add_task(async function check_permission
 
   ok(popup.linkedBrowser.currentURI.spec.endsWith("popup_blocker_a.html"), "Popup a");
 
   gBrowser.removeTab(popup);
 
   // Open identity popup and change permission state to block.
   await openIdentityPopup();
   menulist = document.getElementById("identity-popup-popup-menulist");
+  menulist.menupopup.openPopup(); // Open the allow/block menu
   menuitem = menulist.getElementsByTagName("menuitem")[1];
   menuitem.click();
   await closeIdentityPopup();
 
   // Clicking on the "Block" menuitem should remove the permission object(same behavior as UNKNOWN state).
   // We have already confirmed that popups are blocked when the permission state is BLOCK.
   state = SitePermissions.get(URI, "popup", gBrowser).state;
   Assert.equal(state, SitePermissions.BLOCK);
--- a/layout/xul/nsMenuFrame.cpp
+++ b/layout/xul/nsMenuFrame.cpp
@@ -657,18 +657,21 @@ void nsMenuFrame::CloseMenu(bool aDesele
     pm->HidePopup(GetPopup()->GetContent(), false, aDeselectMenu, true, false);
 }
 
 bool nsMenuFrame::IsSizedToPopup(nsIContent* aContent, bool aRequireAlways) {
   MOZ_ASSERT(aContent->IsElement());
   nsAutoString sizedToPopup;
   aContent->AsElement()->GetAttr(kNameSpaceID_None, nsGkAtoms::sizetopopup,
                                  sizedToPopup);
+  bool sizedToPopupSetToPref =
+      sizedToPopup.EqualsLiteral("pref") ||
+      (sizedToPopup.IsEmpty() && aContent->IsXULElement(nsGkAtoms::menulist));
   return sizedToPopup.EqualsLiteral("always") ||
-         (!aRequireAlways && sizedToPopup.EqualsLiteral("pref"));
+         (!aRequireAlways && sizedToPopupSetToPref);
 }
 
 nsSize nsMenuFrame::GetXULMinSize(nsBoxLayoutState& aBoxLayoutState) {
   nsSize size = nsBoxFrame::GetXULMinSize(aBoxLayoutState);
   DISPLAY_MIN_SIZE(this, size);
 
   if (IsSizedToPopup(mContent, true)) SizeToPopup(aBoxLayoutState, size);
 
--- a/layout/xul/nsMenuPopupFrame.cpp
+++ b/layout/xul/nsMenuPopupFrame.cpp
@@ -403,24 +403,47 @@ bool nsMenuPopupFrame::IsLeafDynamic() c
 
   if (mPopupType != ePopupTypeMenu) {
     // any panel with a type attribute, such as the autocomplete popup,
     // is always generated right away.
     return !mContent->AsElement()->HasAttr(kNameSpaceID_None, nsGkAtoms::type);
   }
 
   // menu popups generate their child frames lazily only when opened, so
-  // behave like a leaf frame. However, generate child frames normally if
-  // the parent menu has a sizetopopup attribute. In this case the size of
-  // the parent menu is dependent on the size of the popup, so the frames
-  // need to exist in order to calculate this size.
+  // behave like a leaf frame. However, generate child frames normally if the
+  // following is true:
+  //
+  // - If the parent menu is a <menulist> unless its sizetopopup is set to
+  // "none".
+  // - For other elements, if the parent menu has a sizetopopup attribute.
+  //
+  // In these cases the size of the parent menu is dependent on the size of
+  // the popup, so the frames need to exist in order to calculate this size.
   nsIContent* parentContent = mContent->GetParent();
-  return parentContent && (!parentContent->IsElement() ||
-                           !parentContent->AsElement()->HasAttr(
-                               kNameSpaceID_None, nsGkAtoms::sizetopopup));
+  if (!parentContent) {
+    return false;
+  }
+
+  if (parentContent->IsXULElement(nsGkAtoms::menulist)) {
+    Element* parent = parentContent->AsElement();
+    if (!parent->HasAttr(kNameSpaceID_None, nsGkAtoms::sizetopopup)) {
+      // No prop set, generate child frames normally for the
+      // default value ("pref").
+      return false;
+    }
+
+    nsAutoString sizedToPopup;
+    parent->GetAttr(kNameSpaceID_None, nsGkAtoms::sizetopopup, sizedToPopup);
+    // Don't generate child frame only if the property is set to none.
+    return sizedToPopup.EqualsLiteral("none");
+  }
+
+  return (!parentContent->IsElement() ||
+          !parentContent->AsElement()->HasAttr(kNameSpaceID_None,
+                                               nsGkAtoms::sizetopopup));
 }
 
 void nsMenuPopupFrame::UpdateWidgetProperties() {
   if (nsIWidget* widget = GetWidget()) {
     widget->SetWindowOpacity(StyleUIReset()->mWindowOpacity);
     widget->SetWindowTransform(ComputeWidgetTransform());
   }
 }
--- a/toolkit/content/widgets/menulist.xml
+++ b/toolkit/content/widgets/menulist.xml
@@ -7,17 +7,17 @@
 <bindings id="menulistBindings"
    xmlns="http://www.mozilla.org/xbl"
    xmlns:html="http://www.w3.org/1999/xhtml"
    xmlns:xul="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul"
    xmlns:xbl="http://www.mozilla.org/xbl">
 
   <binding id="menulist"
            extends="chrome://global/content/bindings/general.xml#basecontrol">
-    <content sizetopopup="pref">
+    <content>
       <xul:hbox class="menulist-label-box" flex="1">
         <xul:image class="menulist-icon" xbl:inherits="src=image"/>
         <xul:label class="menulist-label" xbl:inherits="value=label,crop,accesskey,highlightable" crop="right" flex="1"/>
         <xul:label class="menulist-highlightable-label" xbl:inherits="xbl:text=label,crop,accesskey,highlightable" crop="right" flex="1"/>
       </xul:hbox>
       <xul:dropmarker class="menulist-dropmarker" type="menu" xbl:inherits="disabled,open"/>
       <children includes="menupopup"/>
     </content>
@@ -329,13 +329,13 @@
           }
         ]]>
       </destructor>
     </implementation>
   </binding>
 
   <binding id="menulist-popuponly"
            extends="chrome://global/content/bindings/menulist.xml#menulist">
-    <content sizetopopup="pref">
+    <content>
       <children includes="menupopup"/>
     </content>
   </binding>
 </bindings>