Bug 1650835 - Use a "button-and-view" widget for the profiler button. r=florian

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

browser/themes/shared/customizableui/customizeMode.inc.css

@@ -294,11 +294,13 @@ toolbarpaletteitem > #search-container > #searchbar > .searchbar-textbox {
 
 /* Squeeze together the multi-button toolbarpaletteitems: */
 #wrapper-zoom-controls[place="palette"] > #zoom-controls > #zoom-out-button,
-#wrapper-edit-controls[place="palette"] > #edit-controls > #cut-button {
+#wrapper-edit-controls[place="palette"] > #edit-controls > #cut-button,
+#wrapper-profiler-button[place="palette"] > #profiler-button > #profiler-button-button {
   -moz-box-align: end;
 }
 #wrapper-zoom-controls[place="palette"] > #zoom-controls > #zoom-in-button,
-#wrapper-edit-controls[place="palette"] > #edit-controls > #paste-button {
+#wrapper-edit-controls[place="palette"] > #edit-controls > #paste-button,
+#wrapper-profiler-button[place="palette"] > #profiler-button > #profiler-button-dropmarker {
   -moz-box-align: start;
 }
 #wrapper-edit-controls[place="palette"] > #edit-controls > #copy-button {

browser/themes/shared/toolbarbutton-icons.inc.css

@@ -256,41 +256,28 @@ toolbar[brighttext] {
   list-style-image: url("chrome://browser/skin/developer.svg");
 }
 
-#profiler-button > .toolbarbutton-icon {
+#profiler-button-button > .toolbarbutton-icon {
   list-style-image: url("chrome://devtools/skin/images/tool-profiler.svg");
 }
 
-#profiler-button:not(.profiler-active) > image {
+#profiler-button-button:not(.profiler-active) > image {
   transform: scaleX(-1);
 }
 
-#profiler-button.profiler-active > image {
+#profiler-button-button.profiler-active > image {
   fill: #0060df;
   fill-opacity: 1;
   background-color: #0060df33;
 }
 
-#profiler-button.profiler-active:hover > image {
+#profiler-button-button.profiler-active:hover > image {
   background-color: #0060df22;
 }
 
-#profiler-button.profiler-paused > image {
+#profiler-button-button.profiler-paused > image {
   opacity: 0.4;
 }
 
-#profiler-button > .toolbarbutton-menu-dropmarker {
-  /* Inherit the fill and fill-opacity values from the toolbarbutton
-     instead of using the values from .toolbarbutton-menu-dropmarker */
-  -moz-context-properties: fill, fill-opacity;
-  fill: inherit;
-  fill-opacity: inherit;
-}
-
-.widget-overflow-list #profiler-button > .toolbarbutton-menu-dropmarker,
-#customization-palette #profiler-button > .toolbarbutton-menu-dropmarker {
-  display: none;
-}
-
 #preferences-button {
   list-style-image: url("chrome://global/skin/icons/settings.svg");
 }

devtools/client/performance-new/popup/menu-button.jsm.js

@@ -144,7 +144,7 @@ function initialize(toggleProfilerKeyShortcuts) {
 
   const item = {
     id: WIDGET_ID,
-    type: "view",
+    type: "button-and-view",
     viewId,
     tooltiptext: "profiler-button.tooltiptext",
 
@@ -229,32 +229,34 @@ function initialize(toggleProfilerKeyShortcuts) {
      * This method is used when we need to operate upon the button element itself.
      * This is called once per browser window.
      *
-     * @type {(buttonElement: HTMLElement) => void}
+     * @type {(node: HTMLElement) => void}
      */
-    onCreated: buttonElement => {
-      const window = buttonElement?.ownerDocument?.defaultView;
+    onCreated: node => {
+      const window = node.ownerDocument?.defaultView;
       if (!window) {
         console.error(
-          "Unable to find the window of the profiler button element."
+          "Unable to find the window of the profiler toolbar item."
         );
         return;
       }
 
+      const firstButton = node.firstElementChild;
+      if (!firstButton) {
+        console.error(
+          "Unable to find the button element inside the profiler toolbar item."
+        );
+        return;
+      }
+
+      // Assign the null-checked button element to a new variable so that
+      // TypeScript doesn't require additional null checks in the functions
+      // below.
+      const buttonElement = firstButton;
+
       // This class is needed to show the subview arrow when our button
       // is in the overflow menu.
       buttonElement.classList.add("subviewbutton-nav");
 
-      // Our buttons has 2 targets: the icon which starts the profiler or
-      // captures the profile, and the dropmarker which opens the popup.
-      // To have this behavior, we need to enable showing the dropmarker...
-      buttonElement.setAttribute("wantdropmarker", "true");
-      // ... and to implement our custom click and keyboard event handling
-      // to decide which of the 2 behaviors should be triggered.
-      buttonElement.addEventListener("click", item);
-      // This would be better as a keypress handler, but CustomizableUI's
-      // keypress handler runs before keypress handlers we could add here.
-      buttonElement.addEventListener("keydown", item);
-
       function setButtonActive() {
         buttonElement.setAttribute(
           "tooltiptext",
@@ -295,79 +297,20 @@ function initialize(toggleProfilerKeyShortcuts) {
         Services.obs.removeObserver(setButtonActive, "profiler-started");
         Services.obs.removeObserver(setButtonInactive, "profiler-stopped");
         Services.obs.removeObserver(setButtonPaused, "profiler-paused");
-        buttonElement.removeEventListener("click", item);
-        buttonElement.removeEventListener("keydown", item);
       });
     },
 
     // @ts-ignore - Bug 1674368
-    handleEvent: event => {
-      function startOrCapture() {
-        if (Services.profiler.IsPaused()) {
-          // A profile is already being captured, ignore this event.
-          return;
-        }
-        const { startProfiler, captureProfile } = lazy.Background();
-        if (Services.profiler.IsActive()) {
-          captureProfile("aboutprofiling");
-        } else {
-          startProfiler("aboutprofiling");
-        }
+    onCommand: event => {
+      if (Services.profiler.IsPaused()) {
+        // A profile is already being captured, ignore this event.
+        return;
       }
-
-      if (event.type == "click") {
-        // Only handle clicks on the left button.
-        if (event.button != 0) {
-          return;
-        }
-
-        const button = event.target;
-
-        // Ignore the click event while in the overflow menu
-        if (button.getAttribute("cui-anchorid") == "nav-bar-overflow-button") {
-          return;
-        }
-
-        // If we are within the area of the icon, handle the event.
-        // Otherwise we are on the dropmarker and CustomizableUI will take
-        // care of opening the panel.
-        const win = button.ownerGlobal;
-        const iconBounds = win.windowUtils.getBoundsWithoutFlushing(
-          button.icon
-        );
-        if (
-          win.RTL_UI ? event.x >= iconBounds.left : event.x <= iconBounds.right
-        ) {
-          startOrCapture();
-          event.stopPropagation();
-          event.preventDefault();
-        }
-      } else if (event.type == "keydown") {
-        if (event.key == " " || event.key == "Enter") {
-          startOrCapture();
-          event.stopPropagation();
-          event.preventDefault();
-          return;
-        }
-        if (event.key == "ArrowDown") {
-          // Trigger a "command" event that CustomizableUI will handle.
-          const button = event.target;
-          const cmdEvent = button.ownerDocument.createEvent("xulcommandevent");
-          cmdEvent.initCommandEvent(
-            "command",
-            true,
-            true,
-            button.ownerGlobal,
-            0,
-            event.ctrlKey,
-            event.altKey,
-            event.shiftKey,
-            event.metaKey,
-            null,
-            event.mozInputSource
-          );
-          event.currentTarget.dispatchEvent(cmdEvent);
-        }
+      const { startProfiler, captureProfile } = lazy.Background();
+      if (Services.profiler.IsActive()) {
+        captureProfile("aboutprofiling");
+      } else {
+        startProfiler("aboutprofiling");
       }
     },
   };

devtools/client/performance-new/test/browser/browser_popup-private-browsing.js

@@ -12,6 +12,7 @@ add_task(async function test() {
 
   const getRecordingButton = () =>
     getElementByLabel(document, "Start Recording");
+
   const getDisabledMessage = () =>
     getElementFromDocumentByText(
       document,

devtools/client/performance-new/test/browser/browser_popup-profiler-states.js

@@ -17,7 +17,7 @@ add_task(async function test() {
     "resource://devtools/client/performance-new/popup/background.jsm.js"
   );
 
-  const button = document.getElementById("profiler-button");
+  const button = document.getElementById("profiler-button-button");
   if (!button) {
     throw new Error("Could not find the profiler button.");
   }

devtools/client/performance-new/test/browser/browser_split-toolbar-button.js

@@ -34,6 +34,7 @@ async function waitForProfileAndCloseTab() {
   });
 }
 var button;
+var dropmarker;
 
 add_task(async function setup() {
   info(
@@ -43,34 +44,35 @@ add_task(async function setup() {
     "http://example.com/browser/devtools/client/performance-new/test/browser/fake-frontend.html"
   );
   await makeSureProfilerPopupIsEnabled();
-  button = document.getElementById("profiler-button");
+  button = document.getElementById("profiler-button-button");
+  dropmarker = document.getElementById("profiler-button-dropmarker");
 });
 
 add_task(async function click_icon() {
   info("Test that the profiler icon starts and captures a profile.");
 
-  ok(!button.hasAttribute("open"), "should start with the panel closed");
+  ok(!dropmarker.hasAttribute("open"), "should start with the panel closed");
   ok(!isActive(), "should start with the profiler inactive");
 
-  await EventUtils.synthesizeMouseAtCenter(button.icon, {});
+  button.click();
   ok(isActive(), "should have started the profiler");
 
-  await EventUtils.synthesizeMouseAtCenter(button.icon, {});
+  button.click();
   await waitForProfileAndCloseTab();
 });
 
 add_task(async function click_dropmarker() {
   info("Test that the profiler icon dropmarker opens the panel.");
 
-  ok(!button.hasAttribute("open"), "should start with the panel closed");
+  ok(!dropmarker.hasAttribute("open"), "should start with the panel closed");
   ok(!isActive(), "should start with the profiler inactive");
 
   const popupShownPromise = waitForProfilerPopupEvent("popupshown");
-  await EventUtils.synthesizeMouseAtCenter(button.dropmarker, {});
+  dropmarker.click();
   await popupShownPromise;
 
   info("Ensure the panel is open and the profiler still inactive.");
-  ok(button.getAttribute("open") == "true", "panel should be open");
+  ok(dropmarker.getAttribute("open") == "true", "panel should be open");
   ok(!isActive(), "profiler should still be inactive");
   await getElementByLabel(document, "Start Recording");
 
@@ -78,13 +80,13 @@ add_task(async function click_dropmarker() {
   const popupHiddenPromise = waitForProfilerPopupEvent("popuphidden");
   EventUtils.synthesizeKey("KEY_Escape");
   await popupHiddenPromise;
-  ok(!button.hasAttribute("open"), "panel should be closed");
+  ok(!dropmarker.hasAttribute("open"), "panel should be closed");
 });
 
 add_task(async function space_key() {
   info("Test that the Space key starts and captures a profile.");
 
-  ok(!button.hasAttribute("open"), "should start with the panel closed");
+  ok(!dropmarker.hasAttribute("open"), "should start with the panel closed");
   ok(!isActive(), "should start with the profiler inactive");
   forceFocus(button);
 
@@ -100,10 +102,17 @@ add_task(async function space_key() {
 add_task(async function enter_key() {
   info("Test that the Enter key starts and captures a profile.");
 
-  ok(!button.hasAttribute("open"), "should start with the panel closed");
+  ok(!dropmarker.hasAttribute("open"), "should start with the panel closed");
   ok(!isActive(), "should start with the profiler inactive");
   forceFocus(button);
 
+  const isMacOS = Services.appinfo.OS === "Darwin";
+  if (isMacOS) {
+    // On macOS, pressing Enter on a focused toolbarbutton does not fire a
+    // command event, so we do not expect Enter to start the profiler.
+    return;
+  }
+
   info("Pressing Enter should start the profiler.");
   EventUtils.synthesizeKey("KEY_Enter");
   ok(isActive(), "should have started the profiler");
@@ -112,24 +121,3 @@ add_task(async function enter_key() {
   EventUtils.synthesizeKey("KEY_Enter");
   await waitForProfileAndCloseTab();
 });
-
-add_task(async function arrowDown_key() {
-  info("Test that ArrowDown key dropmarker opens the panel.");
-
-  ok(!button.hasAttribute("open"), "should start with the panel closed");
-  ok(!isActive(), "should start with the profiler inactive");
-  forceFocus(button);
-
-  info("Pressing the down arrow should open the panel.");
-  const popupShownPromise = waitForProfilerPopupEvent("popupshown");
-  EventUtils.synthesizeKey("KEY_ArrowDown");
-  await popupShownPromise;
-  ok(!isActive(), "profiler should still be inactive");
-  ok(button.getAttribute("open") == "true", "panel should be open");
-
-  info("Press Escape to close the panel.");
-  const popupHiddenPromise = waitForProfilerPopupEvent("popuphidden");
-  EventUtils.synthesizeKey("KEY_Escape");
-  await popupHiddenPromise;
-  ok(!button.hasAttribute("open"), "panel should be closed");
-});

devtools/client/performance-new/test/browser/head.js

@@ -233,17 +233,21 @@ async function _toggleOpenProfilerPopup(window) {
   info("Toggle open the profiler popup.");
 
   info("> Find the profiler menu button.");
-  const profilerButton = document.getElementById("profiler-button");
-  if (!profilerButton) {
-    throw new Error("Could not find the profiler button in the menu.");
+  const profilerDropmarker = document.getElementById(
+    "profiler-button-dropmarker"
+  );
+  if (!profilerDropmarker) {
+    throw new Error(
+      "Could not find the profiler button dropmarker in the toolbar."
+    );
   }
 
   const popupShown = waitForProfilerPopupEvent("popupshown");
 
   info("> Trigger a click on the profiler button dropmarker.");
-  await EventUtils.synthesizeMouseAtCenter(profilerButton.dropmarker, {});
+  await EventUtils.synthesizeMouseAtCenter(profilerDropmarker, {});
 
-  if (profilerButton.getAttribute("open") !== "true") {
+  if (profilerDropmarker.getAttribute("open") !== "true") {
     throw new Error(
       "This test assumes that the button will have an open=true attribute after clicking it."
     );