Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Proposal: Improve the browser.commands API #300

Closed
hanguokai opened this issue Oct 17, 2022 · 29 comments
Closed

Proposal: Improve the browser.commands API #300

hanguokai opened this issue Oct 17, 2022 · 29 comments
Labels
proposal Proposal for a change or new feature

Comments

@hanguokai
Copy link
Member

hanguokai commented Oct 17, 2022

Note: I have updated this to reflect the feedback and new design.

Related Doc Links: Chrome, MDN. Tracking bugs: crbug-1375790 and crbug-1173375.

Use Case

Some extensions support lots of user defined keyboard shortcuts, so users may forget some shortcuts. To help user check current shortcuts quickly(read only), developers want to display a table of all current shortcuts for users in the popup page or an extension page.

Proposal 1: Return the command for "_execute_action"

At present, commands.getAll() doesn't return the command of "_execute_action" in MV3 (It returns in MV2).

Proposal: commands.getAll() should return the command of "_execute_action".

Proposal 2: New api: shortcut changed event

At present, when the user changed a shortcut, there is no changed event for extension to know it for updating the shortcuts table.

Proposal: Add a new event to notify extensions the shortcuts has changed, like below:

browser.commands.onRegistrationChanged.addListener( command => {} )

PS: I don't care about the api name or the details, as long as it meets the functional requirements.

Proposal 3: Unify the format of Command.shortcut (key combination) or add a new property shortcutKeys in Command

Another problem is that the format of Command.shortcut is not defined. Developers need to parse the shortcut string to styled display it. For example, if the shortcut string is "Ctrl + F", then use <kbd>Ctrl</kbd> + <kbd>F</kbd> to display in web pages.

But I found the shortcut string is not uniform across platforms on Chrome. For example, on macOS, it is "^F" (no plus sign and use ^ for Ctrl), but on ChromeOS, it is "Ctrl+F".

So I propose to unify and clearly define its format or add a new property in Command object like below:

class Command {
  name, // existing property
  description, // existing property
  shortcut, // existing property, like "Ctrl+F" on ChromeOS or "^F" on macOS
  shortcutKeys // new property, it is an array of keys, like ['Ctrl', 'F'] or ['^', 'F']
}

Then I can use it to format shortcut in page, like this:

browser.commands.getAll(commands => {
  for (let cmd of commands) {
    let keys = cmd.shortcutKeys;
    // format [key1, key2, key3] to html <kbd>key1</kbd> + <kbd>key2</kbd> + <kbd>key3</kbd>
  }
});

Proposal 4: New API: Open Shortcut.

/**
 * Open the browser extension shortcut page and locate to (scroll to):
 * 1. if no name parameter, locate to the beginning of this extension.
 * 2. if specify the name, locate to the specified shortcut of this extension.
 * The name parameter is `Command.name` of a shortcut.
 */
browser.commands.openShortcut( name ?: string );

I reported this feature request a year and a half ago at crbug-1173375.

Why need this API?
Take Chrome as an example, the shortcut page is chrome://extensions/shortcuts, which centralizes all extensions' shortcuts settings. We need to let users locate to current extension (not other extensions) in this page. Assuming that the user has dozens of extensions installed, if we cannot directly locate to current extension, then the user will spend the effort to find this extension first. Assuming that if the extension supports 30 shortcuts, it is also necessary to directly locate to a specified shortcut (not other shortcuts) in this extension.

@hanguokai hanguokai added agenda Discuss in future meetings proposal Proposal for a change or new feature labels Oct 17, 2022
@yankovichv
Copy link

+1

@hanguokai
Copy link
Member Author

hanguokai commented Oct 17, 2022

Another problem is that the format of Command.shortcut is not defined. Developers need to parse the shortcut string to styled display it. For example, if the shortcut string is "Ctrl + F", then use <kbd>Ctrl</kbd> + <kbd>F</kbd> to display in web pages.

But I found the shortcut string is not uniform across platforms. For example, on macOS, it is "^F" (no plus sign), but on ChromeOS, it is "Ctrl+F".

So I propose to unify and clearly define its format.

@bershanskiy
Copy link
Member

This would be useful in some of products I work/worked on.

Another consideration: MV2 has two commands called _execute_page_action and _execute_browser_action, which were merged and renamed to _execute_action in MV3. Personally, I would prefer if MV2 also supported this feature under the new name (_execute_action) to reduce extension code complexity. There should be no backwards compatibility concerns, because, well, querying this command was not possible in MV2 either.

@bershanskiy
Copy link
Member

Unify and define the format of Command.shortcut string

This would also be very nice. I believe that in some contexts some keyboard keys are named inconsistently in different browsers. I believe, at some point I saw Ctrl vs Control inconsistency within Chromium and Chromium's own Selenium/WebDriver component. Having standard naming and capitalization for keys would be great (e.g., Ctrl vs CTRL).

@hanguokai
Copy link
Member Author

@bershanskiy Because I don't care about the key, I only care about how to parse the shortcut string. I have another proposal for this problem, adding a new property in Command object like below:

class Command {
  name, // existing property
  description, // existing property
  shortcut, // existing property, like "Ctrl+F" on ChromeOS or "^F" on macOS
  shortcutKeys // new property, it is an array of keys, like ['Ctrl', 'F'] or ['^', 'F']
}

Then I can use it to format shortcut in page, like this:

browser.commands.getAll(commands => {
  for (let cmd of commands) {
    let keys = cmd.shortcutKeys;
    // format [key1, key2, key3] to html <kbd>key1</kbd> + <kbd>key2</kbd> + <kbd>key3</kbd>
  }
});

@carlosjeurissen
Copy link
Contributor

Is _execute_action, _execute_page_action and _execute_browser_action never returned by these APIs in any browser? If so, as @bershanskiy mentioned we can return _execute_action in MV2 as well for code simplicity / convenience. Related to this: #301

@hanguokai
Copy link
Member Author

Welcome to support it in MV2 if possible. But given other similar situations and the current schedule, I don't think Chrome will support new feature in MV2.

@carlosjeurissen
Copy link
Contributor

@hanguokai could still be supported in MV2 for Firefox and Safari. Tho maybe they already do as we did not test it yet?

@carlosjeurissen
Copy link
Contributor

@hanguokai Can we split the "Unify and define the format of Command.shortcut string" into a separate issue for clarification? My first thoughts would be to use the key names as given by KeyboardEvent.key.

@hanguokai
Copy link
Member Author

@carlosjeurissen as my previous comment, this problem can be resolved by another way. And here, I don't mean the key string, I mean the format of key combination(e.g. key1+key2 or key1key2).

@carlosjeurissen
Copy link
Contributor

@hanguokai You mentioned the key string is different across operating systems (Ctrl vs ^), unifying this with a web standard seems like a good way forward right? As for the key combination part, an Array would be great indeed. Yet again a separate issue in this group for this would help.

@hanguokai
Copy link
Member Author

@carlosjeurissen Yes, welcome to add a new issue for that.

@hanguokai
Copy link
Member Author

I just tested. On Firefox(MV2), commands.getAll() includes "_execute_browser_action".
Screen Shot 2022-10-18 at 21 37 47

So for Firefox, it can ignore problem-1 and focus on problem-2 and 3.

@hanguokai
Copy link
Member Author

And on Chrome, chrome also return "_execute_browser_action" in MV2. So for problem-1, this is a MV3 bug.

@hanguokai
Copy link
Member Author

I have updated my first post to reflect the feedback and new design(for problem-3).

@hanguokai
Copy link
Member Author

Complement another API proposal: Open Shortcut for editing.

/**
 * Open the browser extension shortcut page and locate to (scroll to):
 * 1. if no name parameter, locate to the beginning of this extension.
 * 2. if specify the name, locate to the specified shortcut of this extension.
 * The name parameter is `Command.name` of a shortcut.
 */
browser.commands.openShortcut( name ?: string );

I reported this feature request a year and a half ago at crbug-1173375.

Why need this API?

Take Chrome as an example, the shortcut page is chrome://extensions/shortcuts, which centralizes all extensions' shortcuts settings. We need to let users locate to current extension (not other extensions) in this page. Assuming that the user has dozens of extensions installed, if we cannot directly locate to current extension, then the user will spend the effort to find this extension first. Assuming that if the extension supports 30 shortcuts, it is also necessary to directly locate to a specified shortcut (not other shortcuts) in this extension.

@hanguokai hanguokai changed the title Proposal: get Command for "_execute_action" and commands.onRegistrationChanged() event Proposal: Improve the browser.commands API Oct 24, 2022
@siadatmark
Copy link

siadatmark commented Oct 24, 2022 via email

@carlosjeurissen
Copy link
Contributor

carlosjeurissen commented Oct 25, 2022

@hanguokai Again it would help a lot if we create separate issues in the group for features which could be implemented separately. This allows vendors to indicate support for specific features individually and keep the issue thread understandable. Having the ability to open the keyboard shortcuts page would be great. It's a perfect subject for a new issue in the group. In which we can discuss how this would work in different browsers.

@hanguokai
Copy link
Member Author

@carlosjeurissen These functional requirements are all about browser.commands and come from the same or similar use cases (for me, they are from one real use case). If browser vendors want to implement it separately, they are free to create multiple issues in their bug trackers, which is not necessary here. Put them together, browser vendors can pay more attention to it and understand them easily as a whole.

@carlosjeurissen
Copy link
Contributor

carlosjeurissen commented Oct 27, 2022

@hanguokai during the 2022-10-27 discussion. It was brought up (not by me), it would be productive if we can split this issue up in separately discussable / implementable issues so we can discuss them in the next meeting.

@hanguokai
Copy link
Member Author

@carlosjeurissen I see the meeting note, it will be discussed in next meeting. This proposal contains a number of trivial but related little features. If you're very, very sure it needs to create 4 new issues, I'm willing to do so. Or wait until after the next meeting.

@hanguokai
Copy link
Member Author

@dotproto @Rob--W @xeenon Do you want to split these 4 small features into multiple issues?

@Rob--W
Copy link
Member

Rob--W commented Oct 28, 2022

While these feature requests may be related to you, they are essentially different feature requests. Having them all bundled together means that it would be impossible to use the github labels to express support/opposition to the individual feature requests. Therefore I recommend to split up the issue.

On the specific proposals:

  • 1 seems to have been resolved already, hasn't it? It's a Chrome bug.
  • 2 seems reasonable.
  • The need for 3 is not obvious. The format is clearly documented, not just for Chrome, but also for Firefox (including Ctrl and MacCtrl): https://developer.mozilla.org/en-US/docs/Mozilla/Add-ons/WebExtensions/manifest.json/commands . If ChromeOS does something special, then that's a bug in Chrome. Not a WECG issue.
  • 4 may be a topic of discussion. There is already precedent for opening the extension settings page via runtime.openOptionsPage. Something similar for shortcuts (provided that the browser supports it) is reasonable. FYI Firefox supports the commands.update API to update a shortcut, but it cannot do the kind of conflict resolution that is available at the shortcuts UI page: https://bugzilla.mozilla.org/show_bug.cgi?id=1654403

In short I think that it's only worthwhile to file follow-ups for 2 and 4.

@hanguokai
Copy link
Member Author

@Rob--W OK, I will create 3 new issues for 2-4.

For proposal 3, you're missing my point. I don't mean the format of "suggested_key" in manifest. I mean the value of Command.shortcut https://developer.mozilla.org/en-US/docs/Mozilla/Add-ons/WebExtensions/API/commands/Command . There is no document for it's format, and it is different format cross platforms in Chrome, I didn't test it in Firefox.

@Rob--W
Copy link
Member

Rob--W commented Oct 28, 2022

@Rob--W OK, I will create 3 new issues for 2-4.

For proposal 3, you're missing my point. I don't mean the format of "suggested_key" in manifest. I mean the value of Command.shortcut https://developer.mozilla.org/en-US/docs/Mozilla/Add-ons/WebExtensions/API/commands/Command . There is no document for it's format, and it is different format cross platforms in Chrome, I didn't test it in Firefox.

In Firefox the shortcut format is identical to suggested_key, minus whitespace if any to have a canonical format.
It looks like Chrome behaves differently. Since Chrome does not offer a programmatic way to change the shortcut (whereas Firefox has commands.update), the shortcut could only be used for informational purposes and the impact is limited. In any case, the desired behavior here seems obvious, so I suggest to report a bug to Chromium, not here. A WECG issue could be considered if the Chromium people disagree with updating the format of shortcut, since then some discussion may be needed to converge to a common solution (e.g. the introduction of a new property name shortcut_key).

For next reports, please test in Firefox (and/or Safari), not just Chrome. That would make it easier to determine whether a bug is an implementation-specific bug or possibly a broader cross-browser issue.

@hanguokai
Copy link
Member Author

the shortcut could only be used for informational purposes and the impact is limited

I created #309 for this. I think the problem is there is no definition of the format for this property. If it is only used for informational purposes, then I suggest add a new property like Solution 2 in #309

@Rob--W
Copy link
Member

Rob--W commented Oct 28, 2022

Let's continue the discussion in the individual issues. I copied over some recent threads at the end of this issue; if you felt that any prior discussion is unresolved and needs to be repeated in the new issues, just copy it there.

I'm closing this issue to encourage the use of the new issues (#308 #309 #310).

@hanguokai
Copy link
Member Author

1 seems to have been resolved already, hasn't it? It's a Chrome bug.

I reported a bug for it. At present, it has not been fixed.

@hanguokai
Copy link
Member Author

FYI: my original proposal-1 has fixed in Chrome. Since Chrome 110, chrome.commands.getAll() returns the Command of "_execute_action". Thanks @oliverdunk for fixing this bug.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
proposal Proposal for a change or new feature
Projects
None yet
Development

No branches or pull requests

6 participants