Bug 1725430: disable screenshot shortcut for extension when screenshots.browser.component.enabled is true. r=emalysz,fluent-reviewers,flod
☠☠ backed out by 7b4e73e86e75 ☠ ☠
authorEmma Malysz <emalysz@mozilla.com>
Mon, 27 Sep 2021 17:30:35 +0000
changeset 593334 ec019c0205a9c801c20950096bd1de4a54155c7c
parent 593333 572f860441621731e78c327d2edf348ccc45c14e
child 593335 1ccdf3fc54183e922e326c4cd1dedaa53576049f
push id150384
push useremalysz@mozilla.com
push dateMon, 27 Sep 2021 17:32:55 +0000
treeherderautoland@ec019c0205a9 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersemalysz, fluent-reviewers, flod
bugs1725430
milestone94.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 1725430: disable screenshot shortcut for extension when screenshots.browser.component.enabled is true. r=emalysz,fluent-reviewers,flod Differential Revision: https://phabricator.services.mozilla.com/D125850
browser/base/content/browser-sets.inc
browser/base/content/browser.js
browser/base/content/browser.xhtml
browser/base/content/macWindow.inc.xhtml
browser/base/content/nsContextMenu.js
browser/components/customizableui/CustomizableWidgets.jsm
browser/components/screenshots/ScreenshotsUtils.jsm
browser/extensions/screenshots/background/main.js
browser/extensions/screenshots/background/startBackground.js
browser/extensions/screenshots/experiments/screenshots/api.js
browser/extensions/screenshots/manifest.json
browser/extensions/screenshots/test/browser/browser_screenshots_injection.js
browser/locales/en-US/browser/screenshots.ftl
--- a/browser/base/content/browser-sets.inc
+++ b/browser/base/content/browser-sets.inc
@@ -86,16 +86,18 @@
     <command id="Browser:NewUserContextTab" oncommand="openNewUserContextTab(event.sourceEvent);"/>
     <command id="Browser:OpenAboutContainers" oncommand="openPreferences('paneContainers');"/>
     <command id="Tools:Search" oncommand="BrowserSearch.webSearch();"/>
     <command id="Tools:Downloads" oncommand="BrowserDownloadsUI();"/>
     <command id="Tools:Addons" oncommand="BrowserOpenAddonsMgr();"/>
     <command id="Tools:Sanitize" oncommand="Sanitizer.showUI(window);"/>
     <command id="Tools:PrivateBrowsing"
       oncommand="OpenBrowserWindow({private: true});"/>
+    <command id="Browser:Screenshot" oncommand="ScreenshotsUtils.notify(window, 'shortcut')"/>
+    
 #ifdef NIGHTLY_BUILD
     <command id="Tools:FissionWindow"
       oncommand="OpenBrowserWindow({fission: true, private: !!window?.browsingContext?.usePrivateBrowsing});"
       hidden="true"/>
     <command id="Tools:NonFissionWindow"
       oncommand="OpenBrowserWindow({fission: false, private: !!window?.browsingContext?.usePrivateBrowsing});"
       hidden="true"/>
 #endif
@@ -298,16 +300,17 @@
     <key                          data-l10n-id="full-zoom-reset-shortcut-alt"   command="cmd_fullZoomReset"   modifiers="accel"/>
 
     <key id="key_showAllTabs" keycode="VK_TAB" modifiers="control,shift"/>
 
     <key id="key_switchTextDirection" data-l10n-id="bidi-switch-direction-shortcut" command="cmd_switchTextDirection" modifiers="accel,shift" />
 
     <key id="key_privatebrowsing" command="Tools:PrivateBrowsing" data-l10n-id="private-browsing-shortcut"
          modifiers="accel,shift" reserved="true"/>
+    <key id="key_screenshot" data-l10n-id="screenshot-shortcut" command="Browser:Screenshot" modifiers="accel,shift"/>
     <key id="key_sanitize" command="Tools:Sanitize" keycode="VK_DELETE" modifiers="accel,shift"/>
 #ifdef XP_MACOSX
     <key id="key_sanitize_mac" command="Tools:Sanitize" keycode="VK_BACK" modifiers="accel,shift"/>
 #endif
     <key id="key_quitApplication" data-l10n-id="quit-app-shortcut"
 #ifdef XP_WIN
          modifiers="accel,shift"
 #else
--- a/browser/base/content/browser.js
+++ b/browser/base/content/browser.js
@@ -64,16 +64,17 @@ XPCOMUtils.defineLazyModuleGetters(this,
   PromiseUtils: "resource://gre/modules/PromiseUtils.jsm",
   PromptUtils: "resource://gre/modules/SharedPromptUtils.jsm",
   // TODO (Bug 1529552): Remove once old urlbar code goes away.
   ReaderMode: "resource://gre/modules/ReaderMode.jsm",
   RFPHelper: "resource://gre/modules/RFPHelper.jsm",
   SafeBrowsing: "resource://gre/modules/SafeBrowsing.jsm",
   Sanitizer: "resource:///modules/Sanitizer.jsm",
   SaveToPocket: "chrome://pocket/content/SaveToPocket.jsm",
+  ScreenshotsUtils: "resource:///modules/ScreenshotsUtils.jsm",
   SessionStartup: "resource:///modules/sessionstore/SessionStartup.jsm",
   SessionStore: "resource:///modules/sessionstore/SessionStore.jsm",
   ShortcutUtils: "resource://gre/modules/ShortcutUtils.jsm",
   SimpleServiceDiscovery: "resource://gre/modules/SimpleServiceDiscovery.jsm",
   SiteDataManager: "resource:///modules/SiteDataManager.jsm",
   SitePermissions: "resource:///modules/SitePermissions.jsm",
   SubDialog: "resource://gre/modules/SubDialog.jsm",
   SubDialogManager: "resource://gre/modules/SubDialog.jsm",
--- a/browser/base/content/browser.xhtml
+++ b/browser/base/content/browser.xhtml
@@ -71,16 +71,17 @@
   <link rel="localization" href="preview/interventions.ftl"/>
   <link rel="localization" href="browser/sidebarMenu.ftl"/>
   <link rel="localization" href="browser/allTabsMenu.ftl"/>
   <link rel="localization" href="browser/places.ftl"/>
   <link rel="localization" href="toolkit/printing/printUI.ftl"/>
   <link rel="localization" href="browser/tabbrowser.ftl"/>
   <link rel="localization" href="preview/firefoxSuggest.ftl"/>
   <link rel="localization" href="browser/toolbarContextMenu.ftl"/>
+  <link rel="localization" href="browser/screenshots.ftl"/>
 
   <title data-l10n-id="browser-main-window-title"></title>
 
 # All JS files which are needed by browser.xhtml and other top level windows to
 # support MacOS specific features *must* go into the global-scripts.inc file so
 # that they can be shared with macWindow.inc.xhtml.
 #include global-scripts.inc
 
--- a/browser/base/content/macWindow.inc.xhtml
+++ b/browser/base/content/macWindow.inc.xhtml
@@ -11,16 +11,17 @@
 # browser-doctype.inc must also be included.
 
 <linkset>
   <html:link rel="localization" href="branding/brand.ftl"/>
   <html:link rel="localization" href="browser/branding/sync-brand.ftl"/>
   <html:link rel="localization" href="toolkit/global/textActions.ftl"/>
   <html:link rel="localization" href="browser/browserSets.ftl"/>
   <html:link rel="localization" href="browser/menubar.ftl"/>
+  <html:link rel="localization" href="browser/screenshots.ftl"/>
 </linkset>
 
 # All JS files which are needed by browser.xhtml and other top level windows to
 # support MacOS specific features *must* go into the global-scripts.inc file so
 # that they can be shared with browser.xhtml.
 #include global-scripts.inc
 
 <script src="chrome://browser/content/nonbrowser-mac.js"></script>
--- a/browser/base/content/nsContextMenu.js
+++ b/browser/base/content/nsContextMenu.js
@@ -1296,17 +1296,21 @@ class nsContextMenu {
       triggeringPrincipal: this.browser.contentPrincipal,
     });
   }
 
   takeScreenshot() {
     if (SCREENSHOT_BROWSER_COMPONENT) {
       Services.obs.notifyObservers(window, "menuitem-screenshot", true);
     } else {
-      Services.obs.notifyObservers(null, "menuitem-screenshot-extension", true);
+      Services.obs.notifyObservers(
+        null,
+        "menuitem-screenshot-extension",
+        "contextMenu"
+      );
     }
   }
 
   // View Partial Source
   viewPartialSource() {
     let { browser } = this;
     let openSelectionFn = function() {
       let tabBrowser = gBrowser;
--- a/browser/components/customizableui/CustomizableWidgets.jsm
+++ b/browser/components/customizableui/CustomizableWidgets.jsm
@@ -520,25 +520,30 @@ if (Services.prefs.getBoolPref("identity
       aEvent.target.syncedTabsPanelList = null;
     },
   });
 }
 
 if (!screenshotsDisabled) {
   CustomizableWidgets.push({
     id: "screenshot-button",
+    shortcutId: "key_screenshot",
     l10nId: "screenshot-toolbarbutton",
     onCommand(aEvent) {
       if (SCREENSHOT_BROWSER_COMPONENT) {
         Services.obs.notifyObservers(
           aEvent.currentTarget.ownerGlobal,
           "menuitem-screenshot"
         );
       } else {
-        Services.obs.notifyObservers(null, "menuitem-screenshot-extension");
+        Services.obs.notifyObservers(
+          null,
+          "menuitem-screenshot-extension",
+          "toolbar"
+        );
       }
     },
     onCreated(aNode) {
       aNode.ownerGlobal.MozXULElement.insertFTLIfNeeded(
         "browser/screenshots.ftl"
       );
       Services.obs.addObserver(this, "toggle-screenshot-disable");
     },
--- a/browser/components/screenshots/ScreenshotsUtils.jsm
+++ b/browser/components/screenshots/ScreenshotsUtils.jsm
@@ -21,9 +21,23 @@ var ScreenshotsUtils = {
 
     Services.obs.notifyObservers(subj, "toggle-screenshot-disable", "true");
 
     return dialogBox.open(
       `chrome://browser/content/screenshots/screenshots.html?browsingContextId=${browser.browsingContext.id}`,
       { features: "resizable=no", sizeTo: "available" }
     );
   },
+  notify(window, type) {
+    if (window.document.getElementById("screenshot-button").disabled) {
+      return;
+    }
+
+    if (Services.prefs.getBoolPref("screenshots.browser.component.enabled")) {
+      Services.obs.notifyObservers(
+        window.event.currentTarget.ownerGlobal,
+        "menuitem-screenshot"
+      );
+    } else {
+      Services.obs.notifyObservers(null, "menuitem-screenshot-extension", type);
+    }
+  },
 };
--- a/browser/extensions/screenshots/background/main.js
+++ b/browser/extensions/screenshots/background/main.js
@@ -69,17 +69,17 @@ this.main = (function() {
   exports.onClicked = catcher.watchFunction(tab => {
     _startShotFlow(tab, "toolbar-button");
   });
 
   exports.onClickedContextMenu = catcher.watchFunction(tab => {
     _startShotFlow(tab, "context-menu");
   });
 
-  exports.onCommand = catcher.watchFunction(tab => {
+  exports.onShortcut = catcher.watchFunction(tab => {
     _startShotFlow(tab, "keyboard-shortcut");
   });
 
   const _startShotFlow = (tab, inputType) => {
     if (!tab) {
       // Not in a page/tab context, ignore
       return;
     }
--- a/browser/extensions/screenshots/background/startBackground.js
+++ b/browser/extensions/screenshots/background/startBackground.js
@@ -59,57 +59,36 @@ this.startBackground = (function() {
     "build/thumbnailGenerator.js",
     "background/analytics.js",
     "background/deviceInfo.js",
     "background/takeshot.js",
     "background/main.js",
   ];
 
   browser.experiments.screenshots.onScreenshotCommand.addListener(
-    async isContextMenuClick => {
+    async type => {
       try {
         let [[tab]] = await Promise.all([
           browser.tabs.query({ currentWindow: true, active: true }),
           loadIfNecessary(),
         ]);
         zoomFactor = await browser.tabs.getZoom(tab.id);
-        isContextMenuClick
-          ? main.onClickedContextMenu(tab)
-          : main.onClicked(tab);
+        if (type === "contextMenu") {
+          main.onClickedContextMenu(tab);
+        } else if (type === "toolbar") {
+          main.onClicked(tab);
+        } else if (type === "shortcut") {
+          main.onShortcut(tab);
+        }
       } catch (error) {
         console.error("Error loading Screenshots:", error);
       }
     }
   );
 
-  browser.commands.onCommand.addListener(cmd => {
-    if (cmd !== "take-screenshot") {
-      return;
-    }
-    loadIfNecessary()
-      .then(() => {
-        browser.tabs
-          .query({ currentWindow: true, active: true })
-          .then(async tabs => {
-            const activeTab = tabs[0];
-            zoomFactor = await browser.tabs.getZoom(activeTab.id);
-            main.onCommand(activeTab);
-          })
-          .catch(error => {
-            throw error;
-          });
-      })
-      .catch(error => {
-        console.error(
-          "Error toggling Screenshots via keyboard shortcut: ",
-          error
-        );
-      });
-  });
-
   browser.runtime.onMessage.addListener((req, sender, sendResponse) => {
     loadIfNecessary()
       .then(() => {
         return communication.onMessage(req, sender, sendResponse);
       })
       .catch(error => {
         console.error("Error loading Screenshots:", error);
       });
--- a/browser/extensions/screenshots/experiments/screenshots/api.js
+++ b/browser/extensions/screenshots/experiments/screenshots/api.js
@@ -42,18 +42,18 @@ this.screenshots = class extends Extensi
               isActive
             );
           },
           onScreenshotCommand: new EventManager({
             context,
             name: "experiments.screenshots.onScreenshotCommand",
             register: fire => {
               let observer = (subject, topic, data) => {
-                let isContexMenuClick = data;
-                fire.sync(isContexMenuClick);
+                let type = data;
+                fire.sync(type);
               };
               Services.obs.addObserver(observer, TOPIC);
               return () => {
                 Services.obs.removeObserver(observer, TOPIC);
               };
             },
           }).api(),
         },
--- a/browser/extensions/screenshots/manifest.json
+++ b/browser/extensions/screenshots/manifest.json
@@ -13,24 +13,16 @@
     }
   },
   "l10n_resources": [ "browser/screenshots.ftl" ],
   "background": {
     "scripts": [
       "background/startBackground.js"
     ]
   },
-  "commands": {
-    "take-screenshot": {
-      "suggested_key": {
-        "default": "Ctrl+Shift+S"
-      },
-      "description": "Open the Firefox Screenshots UI"
-    }
-  },
   "content_scripts": [
     {
       "matches": ["https://screenshots.firefox.com/*"],
       "js": [
         "log.js",
         "catcher.js",
         "selector/callBackground.js",
         "sitehelper.js"
--- a/browser/extensions/screenshots/test/browser/browser_screenshots_injection.js
+++ b/browser/extensions/screenshots/test/browser/browser_screenshots_injection.js
@@ -60,17 +60,23 @@ add_task(async function test_inject_srcd
           ) {
             error = msg;
             resolve();
           }
         });
       });
 
       // Now try to start the screenshot flow:
-      document.querySelector("keyset[id*=screenshot] > key").doCommand();
+      CustomizableUI.addWidgetToArea(
+        "screenshot-button",
+        CustomizableUI.AREA_NAVBAR
+      );
+
+      let screenshotBtn = document.getElementById("screenshot-button");
+      screenshotBtn.click();
       await Promise.race([errorPromise, responsePromise]);
       ok(error, "Should get the relevant error: " + error?.message);
       ok(!response, "Should not get a response from the webpage.");
 
       SpecialPowers.postConsoleSentinel();
     }
   );
 });
--- a/browser/locales/en-US/browser/screenshots.ftl
+++ b/browser/locales/en-US/browser/screenshots.ftl
@@ -1,16 +1,19 @@
 # This Source Code Form is subject to the terms of the Mozilla Public
 # License, v. 2.0. If a copy of the MPL was not distributed with this
 # file, You can obtain one at http://mozilla.org/MPL/2.0/.
 
 screenshot-toolbarbutton =
   .label = Screenshot
   .tooltiptext = Take a screenshot
 
+screenshot-shortcut =
+  .key = S
+
 screenshots-instructions = Drag or click on the page to select a region. Press ESC to cancel.
 screenshots-cancel-button = Cancel
 screenshots-save-visible-button = Save visible
 screenshots-save-page-button = Save full page
 screenshots-download-button = Download
 screenshots-download-button-tooltip = Download screenshot
 screenshots-copy-button = Copy
 screenshots-copy-button-tooltip = Copy screenshot to clipboard