Bug 1668119 - Update macOS menupopup metrics for modern appearance. r=desktop-theme-reviewers,emilio,reusable-components-reviewers,dao

While bug 1861671 improved menupopup styling to closer match modern macOS, there were still several discrepancies between native and non-native menu metrics. The following adjustments were made in this patch to match native styles:
- Don't render the checkmark containers for menus with no checked or checkable items
- Adjusted various spacings to match native metrics
- Apply padding to the accelerator container only if an accelerator is present
- Fixed regression from bug 1861671 resulting in menuitems being misaligned with sub-menus

These changes result in non-native menupopups appearing nearly indistinguishable from native menus.

Differential Revision: https://phabricator.services.mozilla.com/D199787

accessible/tests/mochitest/tree/test_combobox.xhtml

@@ -24,15 +24,6 @@
       //////////////////////////////////////////////////////////////////////////
       // menulist
 
-      var selectedOptionChildren = [];
-      if (MAC) {
-        // checkmark is part of the Mac menu styling
-        selectedOptionChildren = [{
-          role: ROLE_STATICTEXT,
-          children: []
-        }];
-      }
-
       var accTree = {
         role: ROLE_COMBOBOX,
         children: [
@@ -41,7 +32,7 @@
             children: [
               {
                 role: ROLE_COMBOBOX_OPTION,
-                children: selectedOptionChildren
+                children: []
               },
               {
                 role: ROLE_COMBOBOX_OPTION,

toolkit/content/tests/chrome/test_menulist_position.xhtml

@@ -52,11 +52,10 @@ function popupShown()
   let index = menulist.selectedIndex;
   if (menulist.selectedItem && navigator.platform.includes("Mac")) {
     let menulistlabelrect = menulist.shadowRoot.getElementById("label").getBoundingClientRect();
-    let paddingTop = parseFloat(getComputedStyle(menulist.selectedItem).paddingTop);
     let mitemlabelrect = menulist.selectedItem.querySelector(".menu-iconic-text").getBoundingClientRect();
 
-    ok(isWithinHalfPixel(menulistlabelrect.top - paddingTop, mitemlabelrect.top),
-       `Labels vertically aligned for ${index} : ${menulistlabelrect.top} - ${paddingTop} vs. ${mitemlabelrect.top}`);
+    ok(isWithinHalfPixel(menulistlabelrect.top, mitemlabelrect.top),
+       `Labels vertically aligned for ${index} : ${menulistlabelrect.top} vs. ${mitemlabelrect.top}`);
 
     // Store the current value and reset it afterwards.
     let current = menulist.selectedIndex;

toolkit/content/widgets/menupopup.js

@@ -7,6 +7,10 @@
 // This is loaded into all XUL windows. Wrap in a block to prevent
 // leaking to window scope.
 {
+  const { AppConstants } = ChromeUtils.importESModule(
+    "resource://gre/modules/AppConstants.sys.mjs"
+  );
+
   // For the non-native context menu styling, we need to know if we need
   // a gutter for checkboxes. To do this, check whether there are any
   // radio/checkbox type menuitems in a menupopup when showing it. We use a
@@ -19,7 +23,12 @@
     function (e) {
       if (e.target.nodeName == "menupopup") {
         let haveCheckableChild = e.target.querySelector(
-          ":scope > menuitem:not([hidden]):is([type=checkbox],[type=radio])"
+          `:scope > menuitem:not([hidden]):is([type=checkbox],[type=radio]${
+            // On macOS, selected menuitems are checked regardless of type
+            AppConstants.platform == "macosx"
+              ? ",[checked=true],[selected=true]"
+              : ""
+          })`
         );
         e.target.toggleAttribute("needsgutter", haveCheckableChild);
       }

toolkit/themes/osx/global/menu.css

@@ -18,21 +18,14 @@
 .menu-iconic-icon {
   height: 16px;
   margin-block: -2px;
-  margin-inline-end: 5px;
+  margin-inline-end: 6px;
   /* Empty icons shouldn't take up room, so we need to compensate
-   * the 5px margin-end with a negative margin-start.
+   * the 6px margin-end with a negative margin-start.
    */
-  margin-inline-start: -5px;
+  margin-inline-start: -6px;
 }
 
 /* menuitems with icons */
-.menuitem-iconic,
-.menu-iconic,
-menuitem[image] {
-  /* 2px higher than those without icons */
-  padding-block: 1px 3px;
-}
-
 .menuitem-iconic > .menu-iconic-left > .menu-iconic-icon,
 .menu-iconic > .menu-iconic-left > .menu-iconic-icon,
 menuitem[image] > .menu-iconic-left > .menu-iconic-icon {
@@ -92,23 +85,26 @@ menulist > menupopup > menu {
 
 /* checked menuitems */
 
-menupopup > menuitem {
-  padding-inline: 0;
+menupopup[needsgutter] {
+  /* although only menuitems may be checked, apply this to
+     menus and menucaptions as well to maintain alignment */
+  > menu,
+  > menuitem,
+  > menucaption {
+    padding-inline-start: 0;
 
-  /* We only show the ::before pseudo-element, but we'd like the spacing to be
-   * symmetric, so we render an invisible checkmark on both sides in order to
-   * reserve the right amount of space. */
-  &::before,
-  &::after {
+    &::before {
       content: '\2713'; /* a checkmark */
       display: inline-block;
       vertical-align: middle;
       line-height: 0;
       visibility: hidden;
       padding-inline: 4px;
+      font-weight: bold;
     }
   }
 
-menupopup > menuitem:is([checked="true"], [selected="true"])::before {
+  > menuitem:is([checked="true"], [selected="true"])::before {
     visibility: inherit;
   }
+}

toolkit/themes/shared/menu-shared.css

@@ -24,6 +24,11 @@ menuseparator {
      separators (e.g. in bookmarks menus) can be dragged or have a context
      menu. */
   padding-block: 4px;
+
+  @media (-moz-platform: macos) {
+    padding-block: 5px;
+    margin-inline: 9px;
+  }
 }
 
 menuseparator::before {
@@ -100,7 +105,7 @@ menucaption {
   }
 
   @media (-moz-platform: macos) {
-    padding: 2px 21px;
+    padding: 3px 9px;
   }
 }
 
@@ -147,14 +152,17 @@ menucaption {
     margin-inline: 6px 0;
   }
 
+  @media (-moz-platform: macos) {
+    margin-inline: 22px -3px;
+  }
+
   @media (-moz-platform: windows) {
     margin-inline-end: 1em;
   }
 }
 
 @media (-moz-platform: macos) {
-  .menu-right,
-  .menu-accel-container {
-    margin-inline: 21px -9px;
+  :is(.menu-accel, .menu-iconic-accel)[value] {
+    margin-inline-start: 25px;
   }
 }

toolkit/themes/shared/popup.css

@@ -125,7 +125,7 @@ menulist > menupopup {
   @media (-moz-platform: macos) {
     &:not([position]) {
       margin-inline-start: -13px;
-      margin-top: -2px;
+      margin-top: -1px;
     }
   }
 }