Bug 1706479 - Wrap page action icons in an hbox. r=morgan,adw
authorHarry Twyford <htwyford@mozilla.com>
Wed, 19 May 2021 20:37:19 +0000
changeset 580083 23a910bc6d4604b49e7aed5dc6a36b6e4431e431
parent 580082 04e17064fb366cfecfeee3019f80b477a43b1a9d
child 580084 50f0907c08deb8e37a300a4188a4fcb12a9ea3c9
push id38477
push usernbeleuzu@mozilla.com
push dateThu, 20 May 2021 04:32:45 +0000
treeherdermozilla-central@746a4efcd8b7 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersmorgan, adw
bugs1706479, 1482025
milestone90.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 1706479 - Wrap page action icons in an hbox. r=morgan,adw To resolve this bug, we need page action icons and semantic page action nodes to be separate. That way, we can apply filters to the icons without also filtering the nodes' outlines. This means the semantic meaning of the page action button must move up a level, to the enclosing hbox. This means reverting bug 1482025, so I'd like a11y review on this patch. Differential Revision: https://phabricator.services.mozilla.com/D114131
browser/base/content/browser-pageActions.js
browser/base/content/browser-places.js
browser/base/content/browser.xhtml
browser/base/content/test/keyboard/browser_toolbarButtonKeyPress.js
browser/base/content/test/keyboard/browser_toolbarKeyNav.js
browser/components/extensions/parent/ext-pageAction.js
browser/themes/shared/urlbar-searchbar.inc.css
--- a/browser/base/content/browser-pageActions.js
+++ b/browser/base/content/browser-pageActions.js
@@ -527,28 +527,32 @@ var BrowserPageActions = {
     this.mainButtonNode.parentNode.insertBefore(node, insertBeforeNode);
     this.updateAction(action, null, {
       urlbarNode: node,
     });
     action.onPlacedInUrlbar(node);
   },
 
   _makeUrlbarButtonNode(action) {
-    let buttonNode = document.createXULElement("image");
-    buttonNode.classList.add("urlbar-icon", "urlbar-page-action");
+    let buttonNode = document.createXULElement("hbox");
+    buttonNode.classList.add("urlbar-icon-wrapper", "urlbar-page-action");
     if (action.extensionID) {
       buttonNode.classList.add("urlbar-addon-page-action");
     }
     buttonNode.setAttribute("actionid", action.id);
     buttonNode.setAttribute("role", "button");
     let commandHandler = event => {
       this.doCommandForAction(action, event, buttonNode);
     };
     buttonNode.addEventListener("click", commandHandler);
     buttonNode.addEventListener("keypress", commandHandler);
+
+    let imageNode = document.createXULElement("image");
+    imageNode.classList.add("urlbar-icon");
+    buttonNode.appendChild(imageNode);
     return buttonNode;
   },
 
   /**
    * Removes all the DOM nodes of the given action.
    *
    * @param  action (PageActions.Action, required)
    *         The action to remove.
@@ -679,24 +683,17 @@ var BrowserPageActions = {
         document.l10n.setAttributes(panelNode, action.panelFluentID, {
           tabCount,
         });
       } else {
         panelNode.setAttribute("label", title);
       }
     }
     if (urlbarNode) {
-      // Some actions (e.g. Save Page to Pocket) have a wrapper node with the
-      // actual controls inside that wrapper. The wrapper is semantically
-      // meaningless, so it doesn't get reflected in the accessibility tree.
-      // In these cases, we don't want to set aria-label because that will
-      // force the element to be exposed to accessibility.
-      if (urlbarNode.nodeName != "hbox") {
-        urlbarNode.setAttribute("aria-label", title);
-      }
+      urlbarNode.setAttribute("aria-label", title);
       // tooltiptext falls back to the title, so update it too if necessary.
       let tooltip = action.getTooltip(window);
       if (!tooltip) {
         if (action.urlbarFluentID) {
           document.l10n.setAttributes(urlbarNode, action.urlbarFluentID, {
             tabCount,
           });
         } else {
@@ -795,23 +792,25 @@ var BrowserPageActions = {
    */
   actionForNode(node) {
     if (!node) {
       return null;
     }
     let actionID = this._actionIDForNodeID(node.id);
     let action = PageActions.actionForID(actionID);
     if (!action) {
-      // The given node may be an ancestor of a node corresponding to an action,
-      // like how #star-button is contained in #star-button-box, the latter
-      // being the bookmark action's node.  Look up the ancestor chain.
+      // When a page action is clicked, `node` will be an ancestor of
+      // a node corresponding to an action. `node` will be the page action node
+      // itself when a page action is selected with the keyboard. That's because
+      // the semantic meaning of page action is on an hbox that contains an
+      // <image>.
       for (let n = node.parentNode; n && !action; n = n.parentNode) {
         if (n.id == "page-action-buttons" || n.localName == "panelview") {
           // We reached the page-action-buttons or panelview container.
-          // Stop looking; no acton was found.
+          // Stop looking; no action was found.
           break;
         }
         actionID = this._actionIDForNodeID(n.id);
         action = PageActions.actionForID(actionID);
       }
     }
     return action && !action.__isSeparator ? action : null;
   },
--- a/browser/base/content/browser-places.js
+++ b/browser/base/content/browser-places.js
@@ -1913,17 +1913,17 @@ var BookmarkingUI = {
     }
 
     // Update the tooltip for elements that require it.
     let shortcut = document.getElementById(this.BOOKMARK_BUTTON_SHORTCUT);
     let l10nArgs = {
       shortcut: ShortcutUtils.prettifyShortcut(shortcut),
     };
     document.l10n.setAttributes(
-      this.star,
+      this.starBox,
       starred ? "urlbar-star-edit-bookmark" : "urlbar-star-add-bookmark",
       l10nArgs
     );
 
     // Update the Bookmark This Page menuitem when bookmarked state changes.
     this.updateBookmarkPageMenuItem();
 
     Services.obs.notifyObservers(
--- a/browser/base/content/browser.xhtml
+++ b/browser/base/content/browser.xhtml
@@ -1983,48 +1983,54 @@
                      onclick="gURLBar.handleCommand(event);"
                      data-l10n-id="urlbar-go-button"/>
               <hbox id="page-action-buttons" context="pageActionContextMenu">
                 <toolbartabstop/>
                 <hbox id="contextual-feature-recommendation" role="button" hidden="true">
                   <hbox id="cfr-label-container">
                     <label id="cfr-label"/>
                   </hbox>
-                  <image id="cfr-button"
-                         class="urlbar-icon urlbar-page-action"
-                         role="presentation"/>
+                  <hbox id="cfr-button"
+                        role="presentation"
+                        class="urlbar-page-action urlbar-icon-wrapper">
+                    <image class="urlbar-icon"/>
+                  </hbox>
                 </hbox>
                 <hbox id="userContext-icons" hidden="true">
                   <label id="userContext-label"/>
                   <image id="userContext-indicator"/>
                 </hbox>
-                <image id="reader-mode-button"
-                       class="urlbar-icon urlbar-page-action"
-                       tooltip="dynamic-shortcut-tooltip"
-                       role="button"
-                       hidden="true"
-                       onclick="AboutReaderParent.buttonClick(event);"/>
+                <hbox id="reader-mode-button"
+                      class="urlbar-page-action urlbar-icon-wrapper"
+                      role="button"
+                      hidden="true"
+                      tooltip="dynamic-shortcut-tooltip"
+                      onclick="AboutReaderParent.buttonClick(event);">
+                  <image class="urlbar-icon"/>
+                </hbox>
                 <toolbarbutton id="urlbar-zoom-button"
                        onclick="FullZoom.reset(); FullZoom.resetScalingZoom();"
                        tooltip="dynamic-shortcut-tooltip"
                        hidden="true"/>
                 <box id="pageActionSeparator" class="urlbar-page-action"/>
-                <image id="pageActionButton"
-                       class="urlbar-icon urlbar-page-action"
-                       role="button"
-                       data-l10n-id="urlbar-page-action-button"
-                       onmousedown="BrowserPageActions.mainButtonClicked(event);"
-                       onkeypress="BrowserPageActions.mainButtonClicked(event);"/>
+                <hbox id="pageActionButton"
+                      class="urlbar-page-action urlbar-icon-wrapper"
+                      role="button"
+                      data-l10n-id="urlbar-page-action-button"
+                      onmousedown="BrowserPageActions.mainButtonClicked(event);"
+                      onkeypress="BrowserPageActions.mainButtonClicked(event);">
+                  <image class="urlbar-icon"/>
+                </hbox>
                 <hbox id="star-button-box"
                       hidden="true"
-                      class="urlbar-icon-wrapper urlbar-page-action"
+                      role="button"
+                      class="urlbar-page-action urlbar-icon-wrapper"
                       onclick="BrowserPageActions.doCommandForAction(PageActions.actionForID('bookmark'), event, this);">
                   <image id="star-button"
-                         class="urlbar-icon"
-                         role="button"/>
+                         class="urlbar-icon"/>
                 </hbox>
               </hbox>
             </hbox>
           </hbox>
           <toolbartabstop/>
         </toolbaritem>
 
         <toolbarspring cui-areatype="toolbar" class="chromeclass-toolbar-additional"/>
--- a/browser/base/content/test/keyboard/browser_toolbarButtonKeyPress.js
+++ b/browser/base/content/test/keyboard/browser_toolbarButtonKeyPress.js
@@ -251,17 +251,17 @@ add_task(async function testSidebarsButt
 
 // Test activation of the Bookmark this page button from the keyboard.
 // This is an image with a click handler on its parent and no command handler,
 // but the toolbar keyboard navigation code should handle keyboard activation.
 add_task(async function testBookmarkButtonPress() {
   await BrowserTestUtils.withNewTab("https://example.com", async function(
     aBrowser
   ) {
-    let button = document.getElementById("star-button");
+    let button = document.getElementById("star-button-box");
     forceFocus(button);
     StarUI._createPanelIfNeeded();
     let panel = document.getElementById("editBookmarkPanel");
     let focused = BrowserTestUtils.waitForEvent(panel, "focus", true);
     // The button ignores activation while the bookmarked status is being
     // updated. So, wait for it to finish updating.
     await TestUtils.waitForCondition(
       () => BookmarkingUI.status != BookmarkingUI.STATUS_UPDATING
--- a/browser/base/content/test/keyboard/browser_toolbarKeyNav.js
+++ b/browser/base/content/test/keyboard/browser_toolbarKeyNav.js
@@ -258,17 +258,17 @@ add_task(async function testArrowsToolba
 });
 
 // Test that right/left arrows move through buttons wihch aren't toolbarbuttons
 // but have role="button".
 add_task(async function testArrowsRoleButton() {
   await BrowserTestUtils.withNewTab("https://example.com", async function() {
     startFromUrlBar();
     await expectFocusAfterKey("Tab", "pageActionButton");
-    await expectFocusAfterKey("ArrowRight", "star-button");
+    await expectFocusAfterKey("ArrowRight", "star-button-box");
     await expectFocusAfterKey("ArrowLeft", "pageActionButton");
   });
 });
 
 // Test that right/left arrows do not land on disabled buttons.
 add_task(async function testArrowsDisabledButtons() {
   await BrowserTestUtils.withNewTab("https://example.com", async function(
     aBrowser
--- a/browser/components/extensions/parent/ext-pageAction.js
+++ b/browser/components/extensions/parent/ext-pageAction.js
@@ -247,21 +247,36 @@ this.pageAction = class extends Extensio
     this.handleClick(window, { button: 0, modifiers: [] });
   }
 
   handleEvent(event) {
     switch (event.type) {
       case "popupshowing":
         const menu = event.target;
         const trigger = menu.triggerNode;
-
+        let actionId = trigger?.getAttribute("actionid");
+        if (trigger && !actionId) {
+          // When a page action is clicked, triggerNode will be an ancestor of
+          // a node corresponding to an action. triggerNode will be the page
+          // action node itself when a page action is selected with the
+          // keyboard. That's because the semantic meaning of page action is on
+          // an hbox that contains an <image>.
+          for (let n = trigger.parentNode; n && !actionId; n = n.parentNode) {
+            if (n.id == "page-action-buttons" || n.localName == "panelview") {
+              // We reached the page-action-buttons or panelview container.
+              // Stop looking; no action was found.
+              break;
+            }
+            actionId = n.getAttribute("actionid");
+          }
+        }
         if (
           menu.id === "pageActionContextMenu" &&
           trigger &&
-          trigger.getAttribute("actionid") === this.browserPageAction.id &&
+          actionId === this.browserPageAction.id &&
           !this.browserPageAction.getDisabled(trigger.ownerGlobal)
         ) {
           global.actionContextMenu({
             extension: this.extension,
             onPageAction: true,
             menu: menu,
           });
         }
--- a/browser/themes/shared/urlbar-searchbar.inc.css
+++ b/browser/themes/shared/urlbar-searchbar.inc.css
@@ -586,24 +586,24 @@
   /* Just remove this element once Proton is released */
   display: none;
 }
 
 /* The background can be very dark and if the add-on uses a black-ish svg it
    will be barely visible. In the future we should have a standardized SVG
    solution we can apply to add-on icons, for now we can only try to make them
    visible through some filtering tricks */
-:root[lwt-toolbar-field-brighttext] #urlbar:not([focused="true"]) .urlbar-addon-page-action[style*=".svg"],
-:root[lwt-toolbar-field-focus-brighttext] #urlbar[focused="true"] .urlbar-addon-page-action[style*=".svg"] {
+:root[lwt-toolbar-field-brighttext] #urlbar:not([focused="true"]) .urlbar-addon-page-action[style*=".svg"] > .urlbar-icon,
+:root[lwt-toolbar-field-focus-brighttext] #urlbar[focused="true"] .urlbar-addon-page-action[style*=".svg"] > .urlbar-icon {
   filter: grayscale(100%) brightness(20%) invert();
 }
 
 @media (prefers-color-scheme: dark) {
   /* As above, but for the default theme in dark mode, which suffers from the same issue */
-  :root:not(:-moz-lwtheme) .urlbar-addon-page-action[style*=".svg"] {
+  :root:not(:-moz-lwtheme) .urlbar-addon-page-action[style*=".svg"] > .urlbar-icon {
     filter: grayscale(100%) brightness(20%) invert();
   }
 }
 } /*** END proton ***/
 
 @media not (-moz-proton) {
 #pageActionSeparator {
   /* This draws the separator the same way that #urlbar-label-box draws its
@@ -824,21 +824,21 @@
   mask-position-x: calc(var(--cfr-label-width) * -1);
 }
 #urlbar[cfr-recommendation-state="expanded"] #urlbar-input:-moz-locale-dir(rtl) {
   mask-position-x: calc(var(--cfr-label-width));
 }
 
 /* Reader mode icon */
 
-#reader-mode-button {
+#reader-mode-button > .urlbar-icon {
   list-style-image: url(chrome://browser/skin/reader-mode.svg);
 }
 
-#reader-mode-button[readeractive] {
+#reader-mode-button[readeractive] > .urlbar-icon {
   fill: var(--toolbarbutton-icon-fill-attention);
   fill-opacity: 1;
 }
 
 /* Zoom button */
 
 #urlbar-zoom-button {
   appearance: none;