Bug 1725430: disable screenshot shortcut for extension when screensho…

…ts.browser.component.enabled is true. r=emalysz,fluent-reviewers,flod Differential Revision: https://phabricator.services.mozilla.com/D125850
dothq · Sep 21, 2021 · 6fede10 · 6fede10
1 parent 6c11c9c
commit 6fede10
Show file tree

Hide file tree

Showing 11 changed files with 44 additions and 42 deletions.
diff --git a/browser/base/content/browser-sets.inc b/browser/base/content/browser-sets.inc
@@ -91,6 +91,8 @@
     <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});"
@@ -303,6 +305,7 @@
 
     <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"/>

diff --git a/browser/base/content/browser.js b/browser/base/content/browser.js
@@ -69,6 +69,7 @@ XPCOMUtils.defineLazyModuleGetters(this, {
   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",

diff --git a/browser/base/content/browser.xhtml b/browser/base/content/browser.xhtml
@@ -76,6 +76,7 @@
   <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>
 

diff --git a/browser/base/content/nsContextMenu.js b/browser/base/content/nsContextMenu.js
@@ -1300,7 +1300,11 @@ class nsContextMenu {
     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"
+      );
     }
   }
 

diff --git a/browser/components/customizableui/CustomizableWidgets.jsm b/browser/components/customizableui/CustomizableWidgets.jsm
@@ -525,6 +525,7 @@ if (Services.prefs.getBoolPref("identity.fxaccounts.enabled")) {
 if (!screenshotsDisabled) {
   CustomizableWidgets.push({
     id: "screenshot-button",
+    shortcutId: "key_screenshot",
     l10nId: "screenshot-toolbarbutton",
     onCommand(aEvent) {
       if (SCREENSHOT_BROWSER_COMPONENT) {
@@ -533,7 +534,11 @@ if (!screenshotsDisabled) {
           "menuitem-screenshot"
         );
       } else {
-        Services.obs.notifyObservers(null, "menuitem-screenshot-extension");
+        Services.obs.notifyObservers(
+          null,
+          "menuitem-screenshot-extension",
+          "toolbar"
+        );
       }
     },
     onCreated(aNode) {

diff --git a/browser/components/screenshots/ScreenshotsUtils.jsm b/browser/components/screenshots/ScreenshotsUtils.jsm
@@ -26,4 +26,18 @@ var ScreenshotsUtils = {
       { 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);
+    }
+  },
 };
diff --git a/browser/extensions/screenshots/background/main.js b/browser/extensions/screenshots/background/main.js
@@ -74,7 +74,7 @@ this.main = (function() {
     _startShotFlow(tab, "context-menu");
   });
 
-  exports.onCommand = catcher.watchFunction(tab => {
+  exports.onShortcut = catcher.watchFunction(tab => {
     _startShotFlow(tab, "keyboard-shortcut");
   });
 

diff --git a/browser/extensions/screenshots/background/startBackground.js b/browser/extensions/screenshots/background/startBackground.js
@@ -64,47 +64,26 @@ this.startBackground = (function() {
   ];
 
   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(() => {

diff --git a/browser/extensions/screenshots/experiments/screenshots/api.js b/browser/extensions/screenshots/experiments/screenshots/api.js
@@ -47,8 +47,8 @@ this.screenshots = class extends ExtensionAPI {
             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 () => {

diff --git a/browser/extensions/screenshots/manifest.json b/browser/extensions/screenshots/manifest.json
@@ -18,14 +18,6 @@
       "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/*"],

diff --git a/browser/locales/en-US/browser/screenshots.ftl b/browser/locales/en-US/browser/screenshots.ftl
@@ -6,6 +6,9 @@ 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