Use the built-in keyboard commands system #73

Roy-Orbison · 2021-09-01T00:46:04Z

Using the commands manifest key simplifies registration and conflict resolution of shortcuts (the user automatically gets warnings about existing bindings). This also changes the default shortcut to one that's unused by Firefox itself. It does require an extra permission to find the current tab.

jlebon · 2022-06-06T20:51:01Z

I'll have to test this, but being able to drop shortcut.js would be really nice. Have you observed any regressions using this?

Roy-Orbison · 2022-06-07T03:08:17Z

I use Firefox Dev Edition with my modification manually installed, and it has worked well for me. Something I hadn't fully tested was a cross-domain iframe, but I got that working with the latest changes. I tested in a CodePen (without saving it) using only this HTML:

<iframe width=100% height=400 src=http://www.coconut.com.fj/contact/></iframe>

Roy-Orbison · 2022-06-07T03:16:49Z

The only issues are that any existing customised shortcut cannot be copied to to the updated version, and I set the default to one that does not conflict with the core 'bookmark all tabs'. Not sure about the best way to notify users of that.

Roy-Orbison · 2022-08-10T08:04:17Z

Could you give it a whirl?

https://github.com/Roy-Orbison/textern/releases/tag/v0.8b1

Roy-Orbison · 2022-09-19T04:45:58Z

I'm still using this patched version, regularly, without issue.

jlebon

Thanks for this and apologies for the delay. I've taken it for a spin and it works great. I think we should move in this direction but we need to handle migration carefully. Add-ons are automatically updated by default, and we don't want users' shortcuts to break from one version to the next.

Hmm, I think we'll need to keep carrying the old approach temporarily but mark it as deprecated. Then we can remove it in a future version. I'm thinking something like:

in the next release, roll this out and give users the chance to opt-in to test it
if all goes well, in the next version, mark the previous shortcut setting as deprecated: one way we can do this is when the shortcut is pressed, we send a browser notification with the deprecation message and a link to a GitHub issue detailing the migration
finally, in the next version (maybe after 3 months or so?), remove support for the previous shortcut setting

I can drive this change, but more immediately for this PR, let's restore support for the current approach.

jlebon · 2022-08-31T23:51:30Z

webex/background.js

@@ -19,7 +19,7 @@ function assertNoResponse(response) {
 }

 function notifyError(error) {
-    browser.notifications.create({
+    browser.notifications?.create({


This looks like an unrelated change. Can you either remove it or add it as a separate commit with a commit message explaining it?

When testing, and constantly reloading the add-on from the dev directory, the extension console did not work flawlessly. I added optional chaining in several places when some things did not appear to be working in the debugger.

jlebon · 2022-08-31T23:55:16Z

webex/content.js

@@ -17,7 +17,7 @@ function assertNoResponse(response) {
 }

 function notifyError(error) {
-    browser.notifications.create({
+    browser.notifications?.create({


jlebon · 2022-08-31T23:55:57Z

webex/manifest.json

@@ -5,7 +5,7 @@
  "manifest_version": 2,
  "version": "0.7",

-  "applications": {
+  "browser_specific_settings": {


I was updating the manifest to use the shortcut APIs, it made sense to me to use the recommended keys for the whole document.

jlebon · 2022-09-01T01:28:53Z

webex/manifest.json

+  "commands": {
+    "textern": {
+      "suggested_key": {
+        "default": "Shift+Alt+D"


How about "Ctrl+Shift+F"? Looks like that's not taken by Firefox already, and is close to the current default.

I'm wary of any specifying any shortcut with a Ctrl in it. They are regularly commandeered by both applications, and software running at the desktop level. There aren't many spare Ctrl + Shift + Letter keys available to begin with, so you're practically guaranteed to annoy some of your users.

jlebon · 2022-09-24T12:36:14Z

webex/background.js

@@ -124,3 +124,16 @@ function onMessage(message, sender, respond) {
 }

 browser.runtime.onMessage.addListener(onMessage);
+
+browser.commands.onCommand.addListener(function(command) {
+    if (command === 'textern') {


Hmm, I wonder if the shortcut name should be something more specific, in case we add others in the future. Though if I understand correctly the API docs, we should be free to change this identifier in a future version, so maybe that's OK.

I think these names are internally namespaced to each extension to avoid such collisions. If you want to make it more reflective of the action, that's fine.

jlebon · 2022-09-24T12:46:12Z

webex/background.js

+            if (tabs?.length && tabs[0].id != browser.tabs.TAB_ID_NONE) {
+                browser.tabs.sendMessage(tabs[0].id, {type: 'shortcut'}).then(assertNoResponse, logError);
+            }
+        });


Why not logError here?

jlebon · 2022-09-24T12:46:57Z

webex/background.js

+            currentWindow: true,
+            active: true
+        }).then(function(tabs) {
+            if (tabs?.length && tabs[0].id != browser.tabs.TAB_ID_NONE) {


Do we need the ? here? From the docs, it should never be undefined.

jlebon · 2022-09-24T12:57:30Z

webex/content.js

 function onMessage(message, sender, respond) {
    if (sender.id != "textern@jlebon.com")
        return;
-    if (message.type == "set_text")
+    if (message.type == 'shortcut') {


Let's call this message register?

It's already effectively namespaced by sender.id, so you can call it whatever. I think register is a bit generic. In the context of code, I think of "registering" as attaching a listener, or in some way saying "I want to know about this event", not the occurrence of the relevant event.

jlebon · 2022-09-24T13:10:57Z

webex/content.js

+    if (shadowRoot && shadowRoot.activeElement) {
+        return getActiveElement(shadowRoot.activeElement)
+    }


Interesting, didn't know about shadow roots before. Do you have an example page where a textarea on which you'd want to use Textern is actually nested under a shadow root?

No, it was only to ensure we attach to the right element. I read this answer on SO, and didn't want neglect pages that used more complex DOMs.

jlebon · 2022-09-24T13:12:28Z

webex/content.js

@@ -129,43 +128,37 @@ function setText(id, text) {
    fadeBackground(e);
 }

+function getActiveElement(element = document.activeElement) {
+    if (element.nodeName === 'IFRAME' || element.nodeName === 'FRAME')
+        return null; // skip to allow child documents to handle shortcut


Instead of using all_frames and handling it like this here, would it be more elegant to recurse into it in this function?

Quite possibly, I don't recall if I tried that approach.

Roy-Orbison · 2022-09-26T02:11:25Z

I like your idea of phasing in the new shortcut to preserve people's preferences. Might it be possible to push an update which still has a default shortcut, but uses commands.update() to migrate any existing pref across?

Roy-Orbison · 2022-10-04T06:42:35Z

@jlebon I had an idea for how to smooth out the migration, because I don't think the web extension API provides a way to differentiate updates from new installs, and it's possible to use your extension without setting any prefs.

If you push a minor version where the only change ensures a setting is saved, such as a string containing the current version, then subsequent versions will be able to detect whether they are being updated or not, based on its presence or absence. They can then update the setting after any migration is performed.

In the context of this PR, commands.update() need only be called if updating, and you would pass the users pref or the existing combo if none is set. New installs will just use whatever is specified in the manifest.

jlebon · 2022-10-10T14:30:24Z

I like your idea of phasing in the new shortcut to preserve people's preferences. Might it be possible to push an update which still has a default shortcut, but uses commands.update() to migrate any existing pref across?

Ahh nice, hadn't realized commands.update() existed. This provides the possibility of doing a seamless migration. One concern though is that looking at the supported keys and comparing it to shortcut.js, it seems like the latter supports more things. So there's a chance here that the shortcut is simply not translatable and user action is required. I would think the majority of cases should work though.

@jlebon I had an idea for how to smooth out the migration, because I don't think the web extension API provides a way to differentiate updates from new installs, and it's possible to use your extension without setting any prefs.

Hmm, we should be able to use the presence of the current shortcut key in local storage to tell apart between new installs and updates, no?

Modifying the previous plan, how about something like this:

roll out new version with commands API without any default shortcut value set; this gives time to opt-in to test it out and provide feedback
roll out new version with shortcut.js API marked as deprecated:
- check if shortcut exists in browser.storage.local
  - if no (new install), set the default shortcut value
  - if yes (update), try to copy the shortcut to the new API
    - if we fail, notify user that manual action is required and link to GitHub issue with details on how to migrate (but we still keep supporting the shortcut)
finally, in the next version (maybe after 3 months or so?), remove support for the previous shortcut setting

Roy-Orbison · 2022-10-24T00:51:34Z

Hmm, we should be able to use the presence of the current shortcut key in local storage to tell apart between new installs and updates, no?

As per my previous comment, it's possible to use your extension without setting any prefs, so that key is not guaranteed to exist (I just tested this in a fresh profile). We can't know how many users rely on the default settings. Even if it's only a few, you wouldn't want to break their migration. Best to push an update that uses the existing shortcut system but also guarantees a value in the extension storage.

That timeline seems like a good approach, as long as you do the above to differentiate new installs. I hope having both shortcut methods in the code at the same time doesn't cause conflicts.

jlebon · 2022-12-17T19:30:20Z

Just a note here before I forget, I stumbled upon https://developer.mozilla.org/en-US/docs/Mozilla/Add-ons/WebExtensions/API/runtime/onInstalled which looks like it'd be helpful here.

Roy-Orbison · 2022-12-18T23:38:35Z

@jlebon Well done. I looked for something like that, assuming it would be there but didn't find it.

Registration and conflict resolution is simplified. Change default to one that's unused by Firefox itself. Simplifies code but requires an extra permission to find current tab.

Roy-Orbison force-pushed the patch-commands-system branch from cab8032 to e499201 Compare August 10, 2022 07:49

jlebon reviewed Sep 24, 2022

View reviewed changes

Use the built-in keyboard commands system

c047e59

Registration and conflict resolution is simplified. Change default to one that's unused by Firefox itself. Simplifies code but requires an extra permission to find current tab.

Roy-Orbison force-pushed the patch-commands-system branch from e499201 to c047e59 Compare June 13, 2023 06:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use the built-in keyboard commands system #73

Use the built-in keyboard commands system #73

Roy-Orbison commented Sep 1, 2021

jlebon commented Jun 6, 2022

Roy-Orbison commented Jun 7, 2022

Roy-Orbison commented Jun 7, 2022

Roy-Orbison commented Aug 10, 2022

Roy-Orbison commented Sep 19, 2022

jlebon left a comment

jlebon Aug 31, 2022

Roy-Orbison Sep 26, 2022

jlebon Aug 31, 2022

jlebon Aug 31, 2022

Roy-Orbison Sep 26, 2022

jlebon Sep 1, 2022

Roy-Orbison Sep 26, 2022

jlebon Sep 24, 2022

Roy-Orbison Sep 26, 2022

jlebon Sep 24, 2022

jlebon Sep 24, 2022

jlebon Sep 24, 2022

Roy-Orbison Sep 26, 2022

jlebon Sep 24, 2022

Roy-Orbison Sep 26, 2022

jlebon Sep 24, 2022

Roy-Orbison Sep 26, 2022

Roy-Orbison commented Sep 26, 2022

Roy-Orbison commented Oct 4, 2022

jlebon commented Oct 10, 2022

Roy-Orbison commented Oct 24, 2022

jlebon commented Dec 17, 2022

Roy-Orbison commented Dec 18, 2022

Use the built-in keyboard commands system #73

Are you sure you want to change the base?

Use the built-in keyboard commands system #73

Conversation

Roy-Orbison commented Sep 1, 2021

jlebon commented Jun 6, 2022

Roy-Orbison commented Jun 7, 2022

Roy-Orbison commented Jun 7, 2022

Roy-Orbison commented Aug 10, 2022

Roy-Orbison commented Sep 19, 2022

jlebon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Roy-Orbison commented Sep 26, 2022

Roy-Orbison commented Oct 4, 2022

jlebon commented Oct 10, 2022

Roy-Orbison commented Oct 24, 2022

jlebon commented Dec 17, 2022

Roy-Orbison commented Dec 18, 2022