1260493 - Come up with an alternate API registering tool keyboard shortcuts instead of XUL <key> elements

Status: RESOLVED FIXED

(DevTools :: Framework, enhancement, P1)

Description

[Brian Grinstead [:bgrins]]

We currently use XUL <keyset> and <key> elements, which handle `modifiers`, localized `key` or `keycode` and integration with <command>s
https://dxr.mozilla.org/mozilla-central/search?q=path%3Adevtools%2Fclient+%3Ckey&redirect=true&case=false 

They also handle ‘accel’ nicely as cmd on OSX and ctrl on windows.

Comment 1

[Brian Grinstead [:bgrins]]

Had some discussion about this today.  To summarize: 

Tools register keys in a couple of different ways right now:
* an individual tool's doc has <key> elements that map to <commands>
* an individually tool is manually binding to keypress event, has some kind of case statement and runs logic based on key pressed (for example, markup view)

I think what we should have is something declarative kind of like the first approach, but in JS and works without XUL.

We should look at https://github.com/electron/electron/blob/master/docs/api/global-shortcut.md to see if we could adapt that API to work for us for tool-specific shortcuts.  In particular, we'd need to pass the document / element in so the shortcuts are limited to a tool.

We could use the 'accelerator' format for defining the key shortcuts: https://github.com/electron/electron/blob/master/docs/api/accelerator.md.  And we could also declare them directly into properties files with some kind of naming convention:

xxxx.key=CommandOrControl+F
xxxx.descript=Search for nodes

This would allow the help menu (in Bug 892188) to build a UI around this.  So blocking Bug 892188 so we don't end up coming up with two different systems for this.

Comment 2

[Brian Grinstead [:bgrins]]

Also, we talked about converting the existing inspector key and command as a first demo of this api

Comment 3

[Brian Grinstead [:bgrins]]

Also see https://www.npmjs.com/package/electron-localshortcut for a package that wraps up global-shortcut in a way that's window-specific.

Comment 4

[Alexandre Poirot [:ochameau]]

On it. I do have a WIP patch, polishing it up to attach it tomorrow.

I don't plan to focus much on electron facing API as there is no need for additional privileges here.
We should just stick to use Web API: addEventListener!
For any key listener within the toolbox, using regular key listener is enough.

It would only be necessary to go beyond the web for tool shortcuts related to the browser menu.
Here we could use similar abstraction to electron. But I would like to challenge that against WebExtension. I think it is better to use WebExtension whereever we can and contribute to it if needed.

My current patch uses existing ways to describe a shortcut. Similar to xul:key or DOM KeyboardEvent.
A key (one character or VK_* string for non-character keys + modifiers list [accel, shift, alt]).
If that's any useful, we could easily iterate on top of my patch if we want to come up with smarter strings to describe keys. But I think we can followup on that once we have a better idea about the "HTML browser".

Comment 5

[Alexandre Poirot [:ochameau]]

Attachment: Introduce a module to handle key shortcuts in devtools.

Brian, here is a decent version of such helper.
I started applying it to inspector, markup view and toolbox.
It allows to fullfill all these existing usecases.

If you overall like the approach and the way this modules works,
I'll continue with tests and docs and then continue applying to other tools.

See next patchs to see how looks like the .properties file
and how is it going to affect each tool.

Comment 6

[Alexandre Poirot [:ochameau]]

Attachment: Convert inspector key shortcut to stop using XUL

I'll most likely move all these patchs to followups, one per tool.

Comment 7

[Alexandre Poirot [:ochameau]]

Attachment: Convert markup view key shortcuts to use the shortcut helper module

Comment 8

[Alexandre Poirot [:ochameau]]

Attachment: Convert toolbox key shortcut to stop using XUL

Comment 9

[Brian Grinstead [:bgrins]]

Comment on attachment 8742852 [details] [diff] [review]
Introduce a module to handle key shortcuts in devtools.

Review of attachment 8742852 [details] [diff] [review]:
-----------------------------------------------------------------

It's not exactly the API I had in mind, but it seems to work pretty well in the examples.  There's an exception I see for markup view though, I'll leave a comment inline there

::: devtools/client/shared/key-shortcuts.js
@@ +122,5 @@
> +
> +  handleEvent(event) {
> +    for (let [key, shortcut] of this.keys) {
> +      if (shortcut.enabled && this.doesEventMatchShortcut(event, shortcut)) {
> +        event.stopPropagation();

The fact that calling .on("mykey", () => {}) triggers behavior that prevents default and stops propagation on the event was a little surprising to me.  It seems like something that we'd get bit on where a handler only runs in certain conditions but ends up now preventing in all cases.

An alternative would be to pass the event object into the handlers and let the handler prevent it themselves, or be able to 'return false' a la jQuery to trigger the preventions, or at least an off() function could be implemented to 'disable' the key so the tool could manually stop the listener in certain conditions

Comment 10

[Brian Grinstead [:bgrins]]

Comment on attachment 8742854 [details] [diff] [review]
Convert markup view key shortcuts to use the shortcut helper module

Review of attachment 8742854 [details] [diff] [review]:
-----------------------------------------------------------------

::: devtools/client/inspector/markup/markup.js
@@ -607,5 @@
> -    let handled = true;
> -    let previousNode, nextNode;
> -
> -    // Ignore keystrokes that originated in editors.
> -    if (this._isInputOrTextarea(event.target)) {

There is now a condition that's missing which is any of these keys will now be handled (and prevented) when an input is focused (i.e. when editing an input).  So pressing left in the input looks like it won't work anymore.  This is where I think we need something like in Comment 9 where:

* Each handler is passed in the event object
* It can choose whether to run it's logic (in this case by calling this._isInputOrTextarea(event.target))
* It will return false or manually call preventDefault/stopProp if it should be handled

Comment 11

[Alexandre Poirot [:ochameau]]

Attachment: Returns listeners returned values on EventEmitter.emit.

Do you think that's reasonable to tweak EventEmitter to do this?
If so, I'm going to use that to support "a la jQuery" return false to stop events,
otherwise, I'll let listener call event.stopPropagation/preventDefault manually.

Comment 12

[Alexandre Poirot [:ochameau]]

Attachment: Introduce a module to handle key shortcuts in devtools - v2

Tweak KeyShortcuts to support on-demand event cancelling.

Comment 13

[Alexandre Poirot [:ochameau]]

Attachment: Convert markup view key shortcuts to use the shortcut helper module - v2

Return false to cancel the event and do nothing if the event comes from an input.

Now, I'm going to dispatch these patches to dedicated bugs.

Comment 14

[Brian Grinstead [:bgrins]]

Comment on attachment 8746063 [details] [diff] [review]
Introduce a module to handle key shortcuts in devtools - v2

Review of attachment 8746063 [details] [diff] [review]:
-----------------------------------------------------------------

::: devtools/client/shared/key-shortcuts.js
@@ +37,5 @@
> +      let {key, value} = properties.getNext().QueryInterface(Ci.nsIPropertyElement);
> +      // Each line of the property file should be like this
> +      // ${id}.${type} = ${value}
> +      // So split `key` around dots accordingly.
> +      let parts = key.split(".");

Seems to me the logic involved with parsing the 2 different values from the properties file here is just as complex as it would be to implement the sort of 'CommandOrCtrl+F' logic we discussed earlier.  I'm not totally against pushing forward with this and following up with that, it's just that it will require a bunch of work for localization team to go through all of the changes so if it's similar difficulty I'd rather start with the more final implementation.

Comment 15

[Brian Grinstead [:bgrins]]

Comment on attachment 8746062 [details] [diff] [review]
Returns listeners returned values on EventEmitter.emit.

Review of attachment 8746062 [details] [diff] [review]:
-----------------------------------------------------------------

I'm not sure about this, I can't find examples of other eventemitter apis wrt return values being passed back through emit and I don't know how much we should diverge from them.  I can think of two different ways to handle this without needing to modify eventemitter:

1) Don't use an EventEmitter.  If we strictly define how tools can interact with this API as .on() and .off(), we can keep our own list of registered handlers instead of calling 'on', and then iterate through, apply, and and capture return values directly instead of using 'emit'.  Since we don't need to implement all the functionality / options of an event emitter it shouldn't be much of duplicated code
2) Store listeners in a weakmap using a wrapped function that modifies parent state.  Since it's all sync you could check the state after emitting and it would have a similar effect without much extra code.  Pseudocode: https://gist.github.com/bgrins/df281a4ca7df5042cce9fb4d8e7d6438

I'm in favor of 1 right now, but not too strongly.  What do you think?

Comment 16

[Alexandre Poirot [:ochameau]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=11c1c32614ab

Comment 17

[Alexandre Poirot [:ochameau]]

Attachment: patch v3

Ok, it's now time to focus on this shortcut module.
I opened followups for each tool and will submit new patches there.

Here is the module, cleaned up, with docs and tests.
I no longer include "a la jQuery" event cancellation. I don't want to redo a wheel with event emitter.
Cancelling the event in each listener is much easier than complexifying this module.

See bug 1268441 and bug 1268450 for usage examples now.

Comment 18

[Brian Grinstead [:bgrins]]

Comment on attachment 8746543 [details] [diff] [review]
patch v3

Review of attachment 8746543 [details] [diff] [review]:
-----------------------------------------------------------------

The code looks good overall but I want to make sure we look at the API closely, since we are going to be building on top of this across all the tools.  We've discussed this some on IRC, but I'm not sure if coupling the API with .properties files is going to serve us long term.  I'd rather see:

new KeyShortcuts({ window }).on(String.getStringFromName("inspector.searchHTML"), () => {});

which is equivalent to:

new KeyShortcuts({ window }).on("CmdOrCtrl+F", () => {});

instead of:

new KeyShortcuts({ window, url }).on("inspector.searchHTML", () => {});

This way it would work regardless of the localization system (or in absence of one).  This might be important when we switch to a system that's content-priv friendly (bug 1266075).  And it'd also be convenient for cases where the strings aren't needing to be localized or documented (i.e. arrow navigation in the markup view), and in cases where we just don't want the overhead of adding a properties file (i.e. tests).  But it wouldn't stop us from storing shortcuts / descriptions in a properties file when we need to.

::: devtools/client/shared/key-shortcuts.js
@@ +118,5 @@
> +    }
> +  },
> +
> +  listen() {
> +    this.window.addEventListener("keydown", this);

We should probably have an unlisten() function that unbinds the listener to be symmetrical (or rename this to init() and add a destroy())

@@ +146,5 @@
> +
> +  handleEvent(event) {
> +    for (let [key, shortcut] of this.keys) {
> +      if (shortcut.enabled && this.doesEventMatchShortcut(event, shortcut)) {
> +        this.eventEmitter.emit(key, event);

I'd think swap the order here of the args here so it more closely matches the signature a normal event listener (with the Event object coming first)

::: devtools/client/shared/test/browser_key_shortcuts.js
@@ +70,5 @@
> +  });
> +
> +  // Dispatch the first shortcut and expect only this one to be notified
> +  ok(!hitFirst, "First shortcut isn't notified before firing the key event");
> +  EventUtils.synthesizeKey("0", {}, window);

Interesting that this works.  EventUtils.synthesizeKey must be async - I would expect onFirstKey wouldn't ever resolve otherwise.

@@ +77,5 @@
> +  ok(!hitSecond, "No mixup, second shortcut is still not notified (1/2)");
> +
> +  // Wait an extra time, just to be sure this isn't racy
> +  yield new Promise(done => {
> +    window.setTimeout(done, 2000);

This timeout could be shorter while still covering a race condition (probably '0' would do just as well)

@@ +134,5 @@
> +                           window);
> +
> +  // Wait an extra time to let a chance to call the listener
> +  yield new Promise(done => {
> +    window.setTimeout(done, 2000);

Same comment about the timeout

Comment 19

[Alexandre Poirot [:ochameau]]

Attachment: patch v4

Uses electron like key strings, no longer uses properties and addressed everything, but:

(In reply to Brian Grinstead [:bgrins] from comment #18)
> Comment on attachment 8746543 [details] [diff] [review]
> @@ +146,5 @@
> > +
> > +  handleEvent(event) {
> > +    for (let [key, shortcut] of this.keys) {
> > +      if (shortcut.enabled && this.doesEventMatchShortcut(event, shortcut)) {
> > +        this.eventEmitter.emit(key, event);
> 
> I'd think swap the order here of the args here so it more closely matches
> the signature a normal event listener (with the Event object coming first)

It matches more DOM listeners but not EventEmitter.
Most EventEmitter APIs emits the event name first to be able to listen to multiple
events with the same listener function.

> ::: devtools/client/shared/test/browser_key_shortcuts.js
> @@ +70,5 @@
> > +  });
> > +
> > +  // Dispatch the first shortcut and expect only this one to be notified
> > +  ok(!hitFirst, "First shortcut isn't notified before firing the key event");
> > +  EventUtils.synthesizeKey("0", {}, window);
> 
> Interesting that this works.  EventUtils.synthesizeKey must be async - I
> would expect onFirstKey wouldn't ever resolve otherwise.

Wait. Do you expect synthesizeKey to be *blocking* the thread?
I'm not yielding on synthesizeKey here, so that should block here.
 
> @@ +77,5 @@
> > +  ok(!hitSecond, "No mixup, second shortcut is still not notified (1/2)");
> > +
> > +  // Wait an extra time, just to be sure this isn't racy
> > +  yield new Promise(done => {
> > +    window.setTimeout(done, 2000);
> 
> This timeout could be shorter while still covering a race condition
> (probably '0' would do just as well)

Are you sure synthesizeKey is going to fire the DOM event on next event loop?
I'm not sure next tick is enough to be sure to catch such race.
But may be 2s is also overkill...


Now, I'm waiting for r+ on this patch before modifying the others.

Comment 20

[Brian Grinstead [:bgrins]]

Comment on attachment 8747736 [details] [diff] [review]
patch v4

Review of attachment 8747736 [details] [diff] [review]:
-----------------------------------------------------------------

::: devtools/client/shared/key-shortcuts.js
@@ +9,5 @@
> +const isOSX = Services.appinfo.OS === "Darwin";
> +
> +// List of electron keys that maps directly to DOM API (DOM_VK_*)
> +const ElectronKeys = [
> +  "Plus",

Looks like "Plus" can be removed from this list, right?

@@ +19,5 @@
> +  "Home", "End",
> +  "PrintScreen"
> +];
> +// Other keys, that don't match, with the expected mapping
> +const ElectronKeysMapping = {

I'd rather see a big list of all of the supported keys here in ElectronKeysMapping, including "DOM_VK_"

const ElectronKeysMapping = {
  "Space": "DOM_VK_SPACE",
  "Delete": "DOM_VK_DELETE",
  ...
  "Enter": "DOM_VK_RETURN",
}

By getting rid of ElectronKeys it will make one less branch and put all the keys in a central list that's easier to update in case we need to change the values in the future for more standard DOM support (i.e. to be keyCodes or to match Event.key)

@@ +73,5 @@
> +      // Set for non-character keys
> +      keyCode: undefined,
> +    };
> +    for (let mod of modifiers) {
> +      if (["Command", "Cmd"].includes(mod)) {

I think this should be ["Control", "Ctrl"].includes(mod) based on what it's doing

@@ +75,5 @@
> +    };
> +    for (let mod of modifiers) {
> +      if (["Command", "Cmd"].includes(mod)) {
> +        shortcut.ctrl = true;
> +      } else if (["CommandOrControl", "CmdOrCtrl"].includes(mod)) {

I'd like to see all of these modifiers covered at least once in the test (especially this one).  testModifiers is already covering the case that modifiers need to match exactly, so maybe a new test fn that runs through a list of shortcuts and triggers synthesizeKey to just makes sure it gets hit

@@ +99,5 @@
> +      key = key.toUpperCase();
> +      shortcut.keyCode = this.window.KeyboardEvent["DOM_VK" + key];
> +    } else if (key in ElectronKeysMapping) {
> +      // Maps the others manually to DOM API DOM_VK_*
> +      key = ElectronKeysMapping(key);

This must not work - it's calling ElectronKeysMapping as a function

@@ +123,5 @@
> +    }
> +    if (shortcut.alt != event.altKey) {
> +      return false;
> +    }
> +    // Shift is a special modifiers, it may implicitely be required if the

"modifiers" -> "modifier"

::: devtools/client/shared/test/browser_key_shortcuts.js
@@ +114,5 @@
> +                           window);
> +  yield onKey;
> +  ok(hit, "Got shortcut notified once it is actually fired");
> +
> +  // Some key are only accessible via shift and should prevent listener call if shift is pressed

This shift test could move into a new test function (it doesn't really match with what the rest of testModifiers is doing)

Comment 21

[Brian Grinstead [:bgrins]]

(In reply to Alexandre Poirot [:ochameau] from comment #19)
> Created attachment 8747736 [details] [diff] [review]
> patch v4
> 
> Uses electron like key strings, no longer uses properties and addressed
> everything, but:
> 
> (In reply to Brian Grinstead [:bgrins] from comment #18)
> > Comment on attachment 8746543 [details] [diff] [review]
> > @@ +146,5 @@
> > > +
> > > +  handleEvent(event) {
> > > +    for (let [key, shortcut] of this.keys) {
> > > +      if (shortcut.enabled && this.doesEventMatchShortcut(event, shortcut)) {
> > > +        this.eventEmitter.emit(key, event);
> > 
> > I'd think swap the order here of the args here so it more closely matches
> > the signature a normal event listener (with the Event object coming first)
> 
> It matches more DOM listeners but not EventEmitter.
> Most EventEmitter APIs emits the event name first to be able to listen to
> multiple
> events with the same listener function.
> 
> > ::: devtools/client/shared/test/browser_key_shortcuts.js
> > @@ +70,5 @@
> > > +  });
> > > +
> > > +  // Dispatch the first shortcut and expect only this one to be notified
> > > +  ok(!hitFirst, "First shortcut isn't notified before firing the key event");
> > > +  EventUtils.synthesizeKey("0", {}, window);
> > 
> > Interesting that this works.  EventUtils.synthesizeKey must be async - I
> > would expect onFirstKey wouldn't ever resolve otherwise.
> 
> Wait. Do you expect synthesizeKey to be *blocking* the thread?
> I'm not yielding on synthesizeKey here, so that should block here.

Looking back at the comment, I misread the test as I thought it waiting to bind the key event until after the synthesizeKey happened.

> > @@ +77,5 @@
> > > +  ok(!hitSecond, "No mixup, second shortcut is still not notified (1/2)");
> > > +
> > > +  // Wait an extra time, just to be sure this isn't racy
> > > +  yield new Promise(done => {
> > > +    window.setTimeout(done, 2000);
> > 
> > This timeout could be shorter while still covering a race condition
> > (probably '0' would do just as well)
> 
> Are you sure synthesizeKey is going to fire the DOM event on next event loop?
> I'm not sure next tick is enough to be sure to catch such race.
> But may be 2s is also overkill...

When else would it fire?  I don't fully know the test harness code but I don't see anything to indicate it's waiting for a timeout, nor is anything in your code.  I expect synthesizeKey is synchronously triggering a Key Event, so waiting for next tick should catch any issues: https://dxr.mozilla.org/mozilla-central/source/testing/mochitest/tests/SimpleTest/EventUtils.js#749

Comment 22

[Alexandre Poirot [:ochameau]]

Attachment: patch v5

Comment 23

[Alexandre Poirot [:ochameau]]

(In reply to Brian Grinstead [:bgrins] from comment #20)
> Comment on attachment 8747736 [details] [diff] [review]
> patch v4
> 
> Review of attachment 8747736 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: devtools/client/shared/key-shortcuts.js
> @@ +9,5 @@
> > +const isOSX = Services.appinfo.OS === "Darwin";
> > +
> > +// List of electron keys that maps directly to DOM API (DOM_VK_*)
> > +const ElectronKeys = [
> > +  "Plus",
> 
> Looks like "Plus" can be removed from this list, right?

Hum, no? Why? Given that "+" character is special, we have to have an explicit word for it.

Fixed everything else.

(In reply to Brian Grinstead [:bgrins] from comment #21)
> > > @@ +77,5 @@
> > > > +  ok(!hitSecond, "No mixup, second shortcut is still not notified (1/2)");
> > > > +
> > > > +  // Wait an extra time, just to be sure this isn't racy
> > > > +  yield new Promise(done => {
> > > > +    window.setTimeout(done, 2000);
> > > 
> > > This timeout could be shorter while still covering a race condition
> > > (probably '0' would do just as well)
> > 
> > Are you sure synthesizeKey is going to fire the DOM event on next event loop?
> > I'm not sure next tick is enough to be sure to catch such race.
> > But may be 2s is also overkill...
> 
> When else would it fire?  I don't fully know the test harness code but I
> don't see anything to indicate it's waiting for a timeout, nor is anything
> in your code.  I expect synthesizeKey is synchronously triggering a Key
> Event, so waiting for next tick should catch any issues:
> https://dxr.mozilla.org/mozilla-central/source/testing/mochitest/tests/
> SimpleTest/EventUtils.js#749

I wasn't sure dispatchEvent was synchronous, but it seems to be. It's just to prevent any race from code before/after the event is dispatched. Here EventEmitter is synchronous, so it should be fine. I switched to a timeout of 0.

Comment 24

[Brian Grinstead [:bgrins]]

Comment on attachment 8747956 [details] [diff] [review]
patch v5

Review of attachment 8747956 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, thanks!  Let's go with this and then make adjustments as we need while implementing this in the tools

::: devtools/client/shared/key-shortcuts.js
@@ +169,5 @@
> +  },
> +
> +  on(key, listener) {
> +    if (typeof listener !== "function") {
> +      throw new Error("KeyShortcuts.on() except a function as second argument");

except->expects

Comment 25

[Alexandre Poirot [:ochameau]]

Attachment: patch v6

Fixed nit.
Waiting for MacOS runs to finish:
  https://treeherder.mozilla.org/#/jobs?repo=try&revision=5896a1f91be6&selectedJob=20281896
(there is oranges on other platforms but that's related to other changesets included in this push)

Comment 26

[Alexandre Poirot [:ochameau]]

Attachment: patch v7

Fixed wrong assertion on mac.

Comment 27

[Alexandre Poirot [:ochameau]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=fcb67efa8933

Comment 28

[Alexandre Poirot [:ochameau]]

Attachment: patch v7

Really fixed the MacOS test failure.

Comment 29

[Alexandre Poirot [:ochameau]]

https://hg.mozilla.org/integration/fx-team/rev/f612200731ad2e125e9ae23ecc4f4a6e7110d8d7
Bug 1260493 - Introduce a module to handle key shortcuts in devtools. r=bgrins

Comment 30

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/f612200731ad