1268441 - Convert inspector and markup view to use new key shortcut API.

Status: VERIFIED FIXED

(DevTools :: Inspector, defect, P1)

Description

[Alexandre Poirot [:ochameau]]

In inspector.xul, we need to step using xul:
http://mxr.mozilla.org/mozilla-central/source/devtools/client/inspector/inspector.xul#33

And in markup-view, we need to unify to the new key shortcut module:
http://mxr.mozilla.org/mozilla-central/source/devtools/client/inspector/markup/markup.js#609

Comment 1

[Alexandre Poirot [:ochameau]]

Attachment: Convert inspector key shortcut to stop using XUL - v1

Comment 2

[Alexandre Poirot [:ochameau]]

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

Cancelling the event here, instead of the key-shortcuts module.

Comment 3

[Alexandre Poirot [:ochameau]]

Attachment: Convert inspector key shortcut to stop using XUL - v2

Rebased against last changes of the key-shortcut module.

Comment 4

[Alexandre Poirot [:ochameau]]

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

Comment 5

[Alexandre Poirot [:ochameau]]

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

Fix whitespace.

Comment 6

[Alexandre Poirot [:ochameau]]

Comment on attachment 8748252 [details] [diff] [review]
Convert inspector key shortcut to stop using XUL - v2

I was wondering if it was really useful to put the "# LOCALIZATION NOTE" given the description key right after??

Comment 7

[Alexandre Poirot [:ochameau]]

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

Same thing about localization notes here.

Comment 8

[Brian Grinstead [:bgrins]]

Comment on attachment 8748252 [details] [diff] [review]
Convert inspector key shortcut to stop using XUL - v2

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

This is going to need test fixes at: https://dxr.mozilla.org/mozilla-central/source/devtools/client/inspector/test/head.js#346 - synthesizeKeyFromKeyTag(panelWin.document.getElementById("nodeSearchKey"));

::: devtools/client/inspector/inspector-panel.js
@@ +322,5 @@
> +    let shortcuts = new KeyShortcuts({
> +      window: this.panelDoc.defaultView,
> +    });
> +    let key = strings.GetStringFromName("inspector.searchHTML.key");
> +    shortcuts.on(key, (name, event) => {

Even though it matches the EventEmitter api, I don't know if any consumer of this will ever want to use `name`, but fine for now.

::: devtools/client/locales/en-US/inspector.properties
@@ +138,5 @@
> +
> +# LOCALIZATION NOTE (inspector.searchHTML.key):
> +# Key shortcut used to focus the DOM element search box on top-right corner of
> +# the markup view
> +inspector.searchHTML.key=CmdOrCtrl+F

I wonder if we need to point to the documentation for our key syntax (esp the Accelerator keys).  Any thoughts? If there's a shared place to document this for localizers so we don't need to add it to every string that would be good.

Comment 9

[Brian Grinstead [:bgrins]]

Maybe we'll need a similar function as synthesizeKeyFromKeyTag (https://dxr.mozilla.org/mozilla-central/source/devtools/client/framework/test/shared-head.js#140), but it parses the string similarly to how the API does, and passes it into EventUtils.synthesizeKey?  Then tests fetch that string from the properties file.

Comment 10

[Brian Grinstead [:bgrins]]

(In reply to Brian Grinstead [:bgrins] from comment #8)
> ::: devtools/client/inspector/inspector-panel.js
> @@ +322,5 @@
> > +    let shortcuts = new KeyShortcuts({
> > +      window: this.panelDoc.defaultView,
> > +    });
> > +    let key = strings.GetStringFromName("inspector.searchHTML.key");
> > +    shortcuts.on(key, (name, event) => {
> 
> Even though it matches the EventEmitter api, I don't know if any consumer of
> this will ever want to use `name`, but fine for now.

Never mind -- I see you are using it in the markup view patch :)

Comment 11

[Brian Grinstead [:bgrins]]

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

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

r=me with the comments assuming this passes on try

::: devtools/client/inspector/markup/markup.js
@@ +622,5 @@
>  
> +    this._onShortcut = this._onShortcut.bind(this);
> +
> +    // Process localizable keys
> +    ["hide", "edit", "scrollInto"].forEach(name => {

I prefer to use ["markupView.hide.key", "markupView.edit.key", "markupView.scrollInto.key"] instead of doing concatenation below, because it's easier when searching around the code base (like trying to figure out where a particular key is used).

::: devtools/client/locales/en-US/inspector.properties
@@ +144,5 @@
> +
> +# LOCALIZATION NOTE (markupView.hide.key):
> +# Key shortcut used to hide the selected node in the markup view.
> +markupView.hide.key=h
> +markupView.hide.description=Hide the selected node

Does the .description value have any use right now?  If no, we should probably remove until they are displayed in the UI to not cause extra work for localizers, especially if they spend time trying to figure out where it shows up

Comment 12

[Alexandre Poirot [:ochameau]]

Attachment: Convert inspector key shortcut to stop using XUL - v3

- introduces `synthesizeKeyShortcut` in shared-head
- fixed head.js by using synthesizeKeyShortcut
- removed .description in the .properties

(In reply to Brian Grinstead [:bgrins] from comment #8)
> ::: devtools/client/locales/en-US/inspector.properties
> @@ +138,5 @@
> > +
> > +# LOCALIZATION NOTE (inspector.searchHTML.key):
> > +# Key shortcut used to focus the DOM element search box on top-right corner of
> > +# the markup view
> > +inspector.searchHTML.key=CmdOrCtrl+F
> 
> I wonder if we need to point to the documentation for our key syntax (esp
> the Accelerator keys).  Any thoughts? If there's a shared place to document
> this for localizers so we don't need to add it to every string that would be
> good.

I don't know much about l10n processes...
But yes, I wish we are not going to paste this url on every single key!!

Comment 13

[Alexandre Poirot [:ochameau]]

Comment on attachment 8748672 [details] [diff] [review]
Convert inspector key shortcut to stop using XUL - v3

Also, this patch prevents eating the same shortcut used in the computed view.

Comment 14

[Alexandre Poirot [:ochameau]]

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

- prevents cancelling the split console Escape shortcuts when hitting Escape and there is no drag to stop
- fix an issue with loose shift modifiers. Only accept unexpected Shift modifiers for non-character keys.
  Before that patch we accepted Shift for a-ZA-Z characters, which we assert against in some markupview test
- removed the .description keys in .properties
- used l10n full key names in _initShortcuts

Comment 15

[Alexandre Poirot [:ochameau]]

Comment on attachment 8748672 [details] [diff] [review]
Convert inspector key shortcut to stop using XUL - v3

Oops, wrong obsolete...

Comment 16

[Brian Grinstead [:bgrins]]

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

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

::: devtools/client/inspector/markup/markup.js
@@ +134,5 @@
>    // Listening to various events.
>    this._elt.addEventListener("click", this._onMouseClick, false);
>    this._elt.addEventListener("mousemove", this._onMouseMove, false);
>    this._elt.addEventListener("mouseleave", this._onMouseLeave, false);
> +  this.doc.body.addEventListener("mouseup", this._onMouseUp);

Why did this change to doc.body instead of win?

Comment 17

[Alexandre Poirot [:ochameau]]

(In reply to Brian Grinstead [:bgrins] from comment #16)
> Comment on attachment 8748680 [details] [diff] [review]
> Convert markup view key shortcuts to use the shortcut helper module - v4
> 
> Review of attachment 8748680 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: devtools/client/inspector/markup/markup.js
> @@ +134,5 @@
> >    // Listening to various events.
> >    this._elt.addEventListener("click", this._onMouseClick, false);
> >    this._elt.addEventListener("mousemove", this._onMouseMove, false);
> >    this._elt.addEventListener("mouseleave", this._onMouseLeave, false);
> > +  this.doc.body.addEventListener("mouseup", this._onMouseUp);
> 
> Why did this change to doc.body instead of win?

I imagine that's a wrong rebase... I'll remove that change.

Comment 18

[Brian Grinstead [:bgrins]]

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

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

Works for me with the this.doc.body change you mentioned

::: devtools/client/inspector/markup/markup.js
@@ +745,5 @@
>          if (this.isDragging) {
>            this.cancelDragging();
> +        } else {
> +          // Return early to prevent cancelling this event when not dragging.
> +          // That, to keep opening the split console.

I'd prefer something like this:

// Return early to prevent cancelling the event when not
// dragging, to allow the split console to be toggled.

Comment 19

[Alexandre Poirot [:ochameau]]

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

Comment 20

[Alexandre Poirot [:ochameau]]

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

Comment 21

[Alexandre Poirot [:ochameau]]

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

Addressed review comments and eslint.

Comment 22

[Alexandre Poirot [:ochameau]]

Attachment: Convert inspector key shortcut to stop using XUL - v4

Comment 23

[Alexandre Poirot [:ochameau]]

Comment on attachment 8750368 [details] [diff] [review]
Convert inspector key shortcut to stop using XUL - v4

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

I don't know what went wrong last week with patches, but I meant to r? this one.

This patch introduces a "synthesizeKeyShortcut" helper in tests.

::: devtools/client/debugger/test/mochitest/browser_dbg_chrome-debugging.js
@@ +16,5 @@
>  
>  var { DevToolsLoader } = Cu.import("resource://devtools/shared/Loader.jsm", {});
> +var customLoader = new DevToolsLoader();
> +customLoader.invisibleToDebugger = true;
> +var { DebuggerServer } = customLoader.require("devtools/server/main");

I'm changing this due to a collision with loader defined in shared-head.js
I think it is better to assume we have a `loader` global as in modules?

::: devtools/client/framework/test/browser_toolbox_split_console.js
@@ +16,5 @@
>  add_task(function*() {
>    let tab = yield addTab(URL);
>    let target = TargetFactory.forTab(tab);
> +  gToolbox = yield gDevTools.showToolbox(target, "jsdebugger");
> +  panelWin = gToolbox.getPanel("jsdebugger").panelWin;

This change is related to the removal of <keyset> from inspector.xul.
It means that we will most likely drop this test once the last tool having a <keyset> is refactored. I assume it will be the debugger here...

Comment 24

[Alexandre Poirot [:ochameau]]

Attachment: Make Shift modifier only strict for letter characters - v1

I pulled out the modification related to Shift from the previous patch.
And made it simplier with additional tests.

So the shortcut Shift+A is equal to Shift+a and will require Shift to be pressed to fire an event.
Whereas for any non-letter keys, if the DOMEvent comes with the matching character,
we ignore Shift mismatch.

The typical example is @ on a qwerty keyword or % on a french keyboard.
Both these character requires Shift to be typed, but we want the shortcut string "@"
to work even if we didn't explicitely asked for Shift to be pressed.
So "Shift+@" if equal to "@".

Comment 25

[Alexandre Poirot [:ochameau]]

Attachment: Fix support of all non-letter characters - v1

Fix something stupid in key-shortcuts which prevent "[" shortcut from working...

Comment 26

[Alexandre Poirot [:ochameau]]

Attachment: Fix synthesizeKeyShortcut with non-character shortcuts - v1

I forgot to fold this fix into attachment 8750368 [details] [diff] [review].
It makes synthesizeKeyShortcut work with non-character shortcuts like "Up",
which uses keyCode instead of key.
Note that EventUtils.synthesizeKey requires first arg to be non-null
even if it ends up using second argument 'keyCode' property.

Comment 27

[Brian Grinstead [:bgrins]]

(In reply to Alexandre Poirot [:ochameau] from comment #12)
> > I wonder if we need to point to the documentation for our key syntax (esp
> > the Accelerator keys).  Any thoughts? If there's a shared place to document
> > this for localizers so we don't need to add it to every string that would be
> > good.
> 
> I don't know much about l10n processes...
> But yes, I wish we are not going to paste this url on every single key!!

Hi :flod, we are beginning to migrate some devtools keyboard shortcuts away from xul <key> elements with separate attributes for 'modifiers' and 'key', and towards a syntax borrowed from Electron that lets us declare the shortcuts in JS instead of markup: https://github.com/electron/electron/blob/master/docs/api/accelerator.md.  So, instead of <key key="a" modifiers="accel" /> where the key attribute may or may not be in a .dtd file it would be "CommandOrControl+A", where that string may or may not be in a .properties file.

How should we document this / point to existing documentation so that localizers are aware of this syntax?  I suspect that we will never want to change modifiers across localizations since previously only the 'key' attribute was localized, so maybe it just needs to be more of a 'heads up' that we are doing it.

Comment 28

[Francesco Lodolo [:flod]]

(In reply to Brian Grinstead [:bgrins] from comment #27)
> How should we document this / point to existing documentation so that
> localizers are aware of this syntax?  I suspect that we will never want to
> change modifiers across localizations since previously only the 'key'
> attribute was localized, so maybe it just needs to be more of a 'heads up'
> that we are doing it.

There are two parts to explain: don't localize the first part of the string, only change the letter if you need to.

Some notes could be added to this document, besides sending an email to dev-l10n to explain it
https://developer.mozilla.org/en-US/docs/Mozilla/Localization/Localization_content_best_practices

Comment 29

[Alexandre Poirot [:ochameau]]

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

Comment 30

[Alexandre Poirot [:ochameau]]

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

Comment 31

[Brian Grinstead [:bgrins]]

Comment on attachment 8750382 [details] [diff] [review]
Make Shift modifier only strict for letter characters - v1

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

::: devtools/client/shared/test/browser_key_shortcuts.js
@@ +186,5 @@
>      "a",
> +    { shiftKey: true},
> +    window);
> +  yield onShiftKey;
> +  ok(!hitFirst, "Didn't fired the explicit shift+a");

'Didn't fire'

Comment 32

[Brian Grinstead [:bgrins]]

Comment on attachment 8750368 [details] [diff] [review]
Convert inspector key shortcut to stop using XUL - v4

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

This all looks generally good and ready to go, just a couple of notes

::: devtools/client/framework/test/browser_toolbox_split_console.js
@@ +16,5 @@
>  add_task(function*() {
>    let tab = yield addTab(URL);
>    let target = TargetFactory.forTab(tab);
> +  gToolbox = yield gDevTools.showToolbox(target, "jsdebugger");
> +  panelWin = gToolbox.getPanel("jsdebugger").panelWin;

This makes sense to change since the debugger is the only tool that uses useKeyWithSplitConsole.  But, I don't think we'll remove that functionality once we get to the debugger, we'll instead need to migrate it to the new key API (which should be straightforward work).

@@ +68,1 @@
>    gToolbox.useKeyWithSplitConsole(keyElm, "jsdebugger");

It seems like this test isn't going to work anymore - the point was that it was adding a key for a panel that wasn't focused and making sure triggering the key wouldn't call the command.  But now the debugger is focused and this test appears to be the same as testUseKeyWithSplitConsole but with a different assertion

::: devtools/client/framework/test/shared-head.js
@@ +27,5 @@
>  const DevToolsUtils = require("devtools/shared/DevToolsUtils");
>  let promise = require("promise");
>  const Services = require("Services");
>  
> +loader.lazyRequireGetter(this, "KeyShortcuts", "devtools/client/shared/key-shortcuts", true);

Why lazyRequireGetter and not require?  Don't think one extra require() is going to have any impact on the tests

::: devtools/client/inspector/inspector-panel.js
@@ +324,5 @@
> +    });
> +    let key = strings.GetStringFromName("inspector.searchHTML.key");
> +    shortcuts.on(key, (name, event) => {
> +      // Prevent overriding same shortcut from the computed/rule views
> +      if (event.target.closest("#sidebar-panel-ruleview") ||

I wonder why we are using _onKeypress in the rule and computed view instead of _onKeyDown.  Regardless, we should check and see if these conditions are no longer necessary:

https://dxr.mozilla.org/mozilla-central/source/devtools/client/inspector/rules/rules.js#1506
https://dxr.mozilla.org/mozilla-central/source/devtools/client/inspector/computed/computed.js#504

Seems they shouldn't be since keydown will now be prevented which I think should stop the keypress event also.

Of course, those should also switch over to the new API eventually (in another bug)

::: devtools/client/inspector/test/head.js
@@ +344,5 @@
>  
>    panelWin.focus();
> +  let strings = Services.strings.createBundle(
> +    "chrome://devtools/locale/inspector.properties");
> +  synthesizeKeyShortcut(strings.GetStringFromName("inspector.searchHTML.key"));

Nit: no leading whitespace

Comment 33

[Alexandre Poirot [:ochameau]]

Attachment: Convert inspector key shortcut to stop using XUL - v5

(In reply to Brian Grinstead [:bgrins] from comment #32)
> Comment on attachment 8750368 [details] [diff] [review]
> @@ +68,1 @@
> >    gToolbox.useKeyWithSplitConsole(keyElm, "jsdebugger");
> 
> It seems like this test isn't going to work anymore - the point was that it
> was adding a key for a panel that wasn't focused and making sure triggering
> the key wouldn't call the command.  But now the debugger is focused and this
> test appears to be the same as testUseKeyWithSplitConsole but with a
> different assertion

Weird... The test still pass :/

I changed it to:
+  info("useKeyWithSplitConsole on inspector while debugger is focused");
+  gToolbox.useKeyWithSplitConsole(keyElm, "inspector");
in this second test.

> 
> ::: devtools/client/inspector/inspector-panel.js
> @@ +324,5 @@
> > +    });
> > +    let key = strings.GetStringFromName("inspector.searchHTML.key");
> > +    shortcuts.on(key, (name, event) => {
> > +      // Prevent overriding same shortcut from the computed/rule views
> > +      if (event.target.closest("#sidebar-panel-ruleview") ||
> 
> I wonder why we are using _onKeypress in the rule and computed view instead
> of _onKeyDown.  Regardless, we should check and see if these conditions are
> no longer necessary:
> 
> https://dxr.mozilla.org/mozilla-central/source/devtools/client/inspector/
> rules/rules.js#1506
> https://dxr.mozilla.org/mozilla-central/source/devtools/client/inspector/
> computed/computed.js#504
> 
> Seems they shouldn't be since keydown will now be prevented which I think
> should stop the keypress event also.

These checks are still necessary or they would conflict between each others.
One way to better address that would be to listen on a given DOM element instead of the whole document.
That's something I supported in early version of key-shortcuts module.

> Of course, those should also switch over to the new API eventually (in
> another bug)

Opened bug 1271988.

> 
> ::: devtools/client/inspector/test/head.js
> @@ +344,5 @@
> >  
> >    panelWin.focus();
> > +  let strings = Services.strings.createBundle(
> > +    "chrome://devtools/locale/inspector.properties");
> > +  synthesizeKeyShortcut(strings.GetStringFromName("inspector.searchHTML.key"));
> 
> Nit: no leading whitespace

Sorry, I don't see any spacing issue here?

Comment 34

[Alexandre Poirot [:ochameau]]

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

Comment 35

[Alexandre Poirot [:ochameau]]

Attachment: Convert inspector key shortcut to stop using XUL - v6

Rebase and fix KeyShortcuts is already defined exception.

Comment 36

[Alexandre Poirot [:ochameau]]

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

Comment 37

[Alexandre Poirot [:ochameau]]

https://hg.mozilla.org/integration/fx-team/rev/17d6daaf84cb9f1cb60ee827538981aa3ea219b8
Bug 1268441 - Convert inspector key shortcut to stop using XUL. r=bgrins

https://hg.mozilla.org/integration/fx-team/rev/81ed39510b4db4621b945a9fc05ee347bd6e697e
Bug 1268441 - Convert markup view key shortcuts to use the shortcut helper module. r=bgrins

https://hg.mozilla.org/integration/fx-team/rev/ac6e9f60e5c7d4db59357cc9612b8b7694330de4
Bug 1268441 - Make Shift modifier only strict for letter characters. r=bgrins

https://hg.mozilla.org/integration/fx-team/rev/092d7ffc5eeed5c0096117fff18638bbda0ebf6e
Bug 1268441 - Fix support of all non-letter characters. r=bgrins

Comment 38

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/17d6daaf84cb
https://hg.mozilla.org/mozilla-central/rev/81ed39510b4d
https://hg.mozilla.org/mozilla-central/rev/ac6e9f60e5c7
https://hg.mozilla.org/mozilla-central/rev/092d7ffc5eee

Comment 39

[Ada [:adalucinet]]

Confirming that the keyboard shortcuts while in Inspector's HTML pane [1] work as expected with latest Nightly 49.0a1, across platforms [2].

[1] https://developer.mozilla.org/en-US/docs/Tools/Keyboard_shortcuts#HTML_pane
[2] Windows 10 64-bit, Mac OS X 10.10.5 and Ubuntu 16.04 64-bit