Bug 1706479 - Part 2 - Stop overriding tooltiptext on bookmark icon. r=adw
authorHarry Twyford <htwyford@mozilla.com>
Wed, 19 May 2021 20:37:19 +0000
changeset 580100 50f0907c08deb8e37a300a4188a4fcb12a9ea3c9
parent 580099 23a910bc6d4604b49e7aed5dc6a36b6e4431e431
child 580101 51a187c21494dd5079ce70aebd33cc4419729803
push id143404
push userhtwyford@mozilla.com
push dateWed, 19 May 2021 21:06:01 +0000
treeherderautoland@50f0907c08de [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
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 - Part 2 - Stop overriding tooltiptext on bookmark icon. r=adw This resolves a failure in browser/base/content/test/pageActions-proton/browser_PageActions_bookmark.js. That test checks the tooltiptext on the bookmark button hbox. Before part 1 of this patch, the hbox always had one of the generic tooltiptexts "Bookmark Current Tab"/"Edit This Bookmark". This tooltiptext was set in updateBookmarkPageMenuItem. Then, in __updateStar, we set the tooltiptext on the <image> contained in the hbox to be "Bookmark this page ([shortcut])"/"Edit this bookmark ([shortcut])". Since the <image> filled the entire hbox, the tooltiptext on the hbox underneath was never seen. Since Part 1 of this patch moved the semantic meaning of the page action buttons from the <image> to the hbox, the tooltiptext containing the shortcut was also moved to the hbox. However, updateBookmarkPageMenuItem was overriding the shortcut tooltiptext with the generic text and thus browser_PageActions_bookmark.js was failing intermittently. The timing of when the tooltiptext was overridden was variable. This patch stops setting the generic tooltiptext on the hbox altogether. I'm splitting this out into its own patch since I think it needs its own review. With Part 1 applied, when actually using the browser, the hbox had the tooltiptext with the shortcut. I'm not totally clear on why that was working despite updateBookmarkPageMenuItem overriding it, so I'd like to make sure this change is okay. Depends on D114131 Differential Revision: https://phabricator.services.mozilla.com/D115485
--- a/browser/base/content/browser-places.js
+++ b/browser/base/content/browser-places.js
@@ -1899,17 +1899,17 @@ var BookmarkingUI = {
       if (starred) {
         element.setAttribute("starred", "true");
       } else {
-    if (!this.star) {
+    if (!this.starBox) {
       // The BOOKMARK_BUTTON_SHORTCUT exists only in browser.xhtml.
       // Return early if we're not in this context, but still reset the
       // Bookmark This Page items.
     // Update the tooltip for elements that require it.
@@ -2006,30 +2006,23 @@ var BookmarkingUI = {
         // a newer l10n id pending to be set.
         if (this._latestMenuItemL10nId != menuItemL10nId) {
         // We assume that menuItemL10nId has a single attribute.
         let label = l10n[0].attributes[0].value;
-        // Update the label, tooltip, and the starred state for the
-        // page action panel.
+        // Update the label for the page action panel.
         let panelButton = BrowserPageActions.panelButtonNodeForActionID(
         if (panelButton) {
           panelButton.setAttribute("label", label);
-        let urlbarButton = BrowserPageActions.urlbarButtonNodeForActionID(
-          PageActions.ACTION_ID_BOOKMARK
-        );
-        if (urlbarButton) {
-          urlbarButton.setAttribute("tooltiptext", label);
-        }
   onMainMenuPopupShowing: function BUI_onMainMenuPopupShowing(event) {
     // Don't handle events for submenus.
     if (event.target != event.currentTarget) {
--- a/browser/base/content/test/pageActions-proton/browser_PageActions_bookmark.js
+++ b/browser/base/content/test/pageActions-proton/browser_PageActions_bookmark.js
@@ -24,23 +24,25 @@ add_task(async function starButtonCtrlCl
     await hiddenPromise;
 add_task(async function bookmark() {
   // Open a unique page.
   let url = "http://example.com/browser_page_action_menu";
   await BrowserTestUtils.withNewTab(url, async () => {
-    // The bookmark button should read "Bookmark Current Tab" and not be starred.
+    // The bookmark button should read "Bookmark this page ([shortcut])" and not
+    // be starred.
     let bookmarkButton = BrowserPageActions.urlbarButtonNodeForActionID(
-    Assert.equal(
-      bookmarkButton.getAttribute("tooltiptext"),
-      "Bookmark Current Tab"
+    let tooltipText = bookmarkButton.getAttribute("tooltiptext");
+    Assert.ok(
+      tooltipText.startsWith("Bookmark this page"),
+      `Expecting the tooltip text to be updated. Tooltip text: ${tooltipText}`
     info("Click the button.");
     // 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
@@ -55,20 +57,22 @@ add_task(async function bookmark() {
     await promise;
     await onItemAddedPromise;
       "Star has open attribute"
-    // The bookmark button should now read "Edit This Bookmark" and be starred.
-    Assert.equal(
-      bookmarkButton.getAttribute("tooltiptext"),
-      "Edit This Bookmark"
+    // The bookmark button should now read "Edit this bookmark ([shortcut])" and
+    // be starred.
+    tooltipText = bookmarkButton.getAttribute("tooltiptext");
+    Assert.ok(
+      tooltipText.startsWith("Edit this bookmark"),
+      `Expecting the tooltip text to be updated. Tooltip text: ${tooltipText}`
     Assert.equal(bookmarkButton.firstChild.getAttribute("starred"), "true");
       "Star no longer has open attribute"