swift-selection-search icon indicating copy to clipboard operation
swift-selection-search copied to clipboard

keyboard shortcuts that are deleted reappear (and are in conflict)

Open grahamperrin opened this issue 3 years ago • 5 comments

Steps

  1. With Firefox 88, enable Swift Selection Search 3.47.3 and Tab Manager Plus for Firefox 5.2.0
  2. observe the conflicting shortcut
  3. delete the two shortcuts for Swift Selection Search
  4. quit
  5. start Firefox

Expected

  1. no conflict

Actual result

  1. conflict; reappearance of the two shortcuts that were deleted.

This is a major issue only in that Swift Selection Search must be disabled to prevent repetitive shortcut conflicts.

For me personally, not a major issue; I can live without the extension.

https://github.com/CanisLupus/swift-selection-search/blame/master/README.md#L6 noted and understood. Thanks for all the fish.

grahamperrin avatar Apr 28 '21 01:04 grahamperrin

Hi Graham,

I think I know what you are seeing. Are you using Firefox's "Manage Extension Shortcuts" to disable the shortcuts?

image

If so, SSS's shortcuts should be disabled from its own settings menu, by deleting the shortcut (the text), here:

image

I realize this is not intuitive for users that expect the other menu to work, and I fully agree. There a reason it's like this, but not a terribly good one. :) SSS had shortcuts (at least one) when it was first created in 2016, even before being a WebExtension. When WebExtensions first got support for shortcuts, I re-added it as a setting with its own menu, as you see above. At that time I think that the "Manage Extension Shortcuts" menu didn't exist and I'm not aware when it was added.

I remember being told by a user later (possibly here on GitHub?) that the menu existed and there was this problem, but it wasn't a big priority to fix things in Firefox's shortcuts menu, so it was left like that.

So yes, I consider this a bug, but it should be possible to solve on the user side (not ideal!) by deleting the shortcut text in SSS's settings menu ("Popup/icons behaviour" section).

I hope this helps! :) Cheers!

CanisLupus avatar Apr 29 '21 15:04 CanisLupus

By the way, the reason it reappears on restart is that SSS stores the shortcut as text in its settings, and it never gets the memo that the user deleted it in Firefox's menu, so it simply registers the shortcut again when it starts.

CanisLupus avatar Apr 29 '21 15:04 CanisLupus

Thanks for the explanation. Apologies for the noise.

It's nice that you made things configurable before Mozilla did so.


Given the workaround here and (more broadly) https://github.com/CanisLupus/swift-selection-search/blame/master/README.md#L6, I'm happy for you to close this issue. Or would you prefer it left open, for the possibility of someone else making a pull request?

grahamperrin avatar Apr 30 '21 04:04 grahamperrin

No problem!

Thanks for understanding, Graham. :) I think we can leave this issue open. It's an actual bug and it's not summarized elsewhere (that I remember). If development resumes at some point, this will be a good reminder.

CanisLupus avatar Apr 30 '21 09:04 CanisLupus

Hey! Here's an update after I made some preliminary research on what it would take to fix this in SSS (it looks like a lot but it's not too bad, but it was more than I anticipated since the browser shortcuts menu only exists after Firefox 66). This mostly serves as a summary and also as a reminder for me, so no need to reply. :) Development hasn't resumed, but I'm okay with dealing with the bugs that show up. 😁

Research:

  • The menu was added in Firefox 66.
  • The menu allows changing and deleting a shortcut, but does not allow resetting it to its default value.
  • Whenever a setting is changed or when the browser starts, SSS will overwrite the shortcuts with the ones it has saved, regardless of what changes the user might have made in the menu.
  • SSS has no way of knowing of any changes in that menu, which means that it's not possible to make it play well with it. It's possible to not override the shortcuts on start/save, but it will never display the correct values if they were changed in the menu.
  • The only solution is to disable the shortcut options in the SSS settings page and direct the user to the new browser menu.
  • Since the menu was added in Firefox 66 but SSS supports Firefox 63, SSS needs to keep all the code and functionality it has currently. The settings will just have to work/display differently only after Firefox 66.

Required changes (if running on Firefox 66+):

  • Remove text boxes from the settings menu for options "Shortcut to open popup" and "Shortcut to toggle auto popup opening".
  • Change description text for the options to explain where the shortcuts can be changed now, and what defaults they have (since there's no option to reset to defaults).
  • Disable "Default" buttons on both settings. They could stay and actually reset the shortcuts, but they would need a confirmation message since the user wouldn't see the difference on the page. Most importantly, it would complicate the code significantly because saving the settings won't touch the shortcuts, but resetting causes a save.
  • Improve shortcut names (descriptions) in the manifest, since they are important to the user now (this must affect versions before 63, but it's not like better names will hurt users on old versions :p).
  • Change code in setup_Commands() to skip updating the shortcuts on start/save.
  • Delete the shortcut settings from the saved data on read (they're not needed anymore).
  • Add info to the "Reset options" button saying it doesn't affect the shortcuts. Resetting all settings or loading a backup (from a file or from Firefox Sync) shouldn't change the shortcuts. This keeps it consistent with not having "Default" buttons for the options avoids possible confusion with the settings page changing things outside itself.

Some of these are a pain if SSS is to still support Firefox versions older than 66. It's either this or up the minimum required Firefox version again just for this fix. :) I'll think about it.

Cheers!

CanisLupus avatar Jun 21 '22 18:06 CanisLupus

Hey! I've updated SSS to version 3.48.0 (actually a few days ago). The shortcut options now direct you to use Firefox's menu to change shortcuts, which is the new way since Firefox 66 (aaaaages ago) :)

Now, when SSS loads, it won't override the shortcuts that you had set in that menu. SSS won't change shortcuts anymore, just respond to them. As a consequence, resetting settings and loading backups also won't touch the shortcuts (which is probably for the best).

Follow up from the previous post:

  • I bumped the minimum Firefox version to 66 from 63. Essentially, looking at the extremely tiny version usage stats I concluded it was not worth it to support versions 63 to 65.

Delete the shortcut settings from the saved data on read (they're not needed anymore).

  • This was not done in the end, just in case the option needs to be reverted for some reason.

Add info to the "Reset options" button saying it doesn't affect the shortcuts.

  • Also felt unnecessary since they are no longer "options" in any case.

Cheers!

CanisLupus avatar Aug 31 '22 18:08 CanisLupus

Thank you!

grahamperrin avatar Sep 02 '22 07:09 grahamperrin