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

VERIFIED FIXED in Firefox 49

Status

P1
normal
VERIFIED FIXED
3 years ago
4 months ago

People

(Reporter: ochameau, Assigned: ochameau)

Tracking

(Depends on: 1 bug)

unspecified
Firefox 49
Dependency tree / graph

Firefox Tracking Flags

(firefox49 verified)

Details

(Whiteboard: [devtools-html])

Attachments

(5 attachments, 9 obsolete attachments)

11.52 KB, patch
ochameau
: review+
Details | Diff | Splinter Review
4.49 KB, patch
bgrins
: review+
Details | Diff | Splinter Review
2.87 KB, patch
bgrins
: review+
Details | Diff | Splinter Review
1016 bytes, patch
bgrins
: review+
Details | Diff | Splinter Review
18.89 KB, patch
ochameau
: review+
Details | Diff | Splinter Review
Whiteboard: [devtools-html][triage]
Created attachment 8746539 [details] [diff] [review]
Convert inspector key shortcut to stop using XUL - v1
Created attachment 8746540 [details] [diff] [review]
Convert markup view key shortcuts to use the shortcut helper module - v3

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

Updated

3 years ago
Blocks: 1259121
Flags: qe-verify?
Priority: -- → P1
Whiteboard: [devtools-html][triage] → [devtools-html]

Updated

3 years ago
Status: NEW → ASSIGNED
Iteration: --- → 49.1 - May 9
Created attachment 8748252 [details] [diff] [review]
Convert inspector key shortcut to stop using XUL - v2

Rebased against last changes of the key-shortcut module.
Attachment #8746539 - Attachment is obsolete: true
Created attachment 8748254 [details] [diff] [review]
Convert markup view key shortcuts to use the shortcut helper module - v4
Attachment #8746540 - Attachment is obsolete: true
Created attachment 8748261 [details] [diff] [review]
Convert markup view key shortcuts to use the shortcut helper module - v5

Fix whitespace.
Attachment #8748254 - Attachment is obsolete: true
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??
Attachment #8748252 - Flags: review?(bgrinstead)
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.
Attachment #8748261 - Flags: review?(bgrinstead)
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.
Attachment #8748252 - Flags: review?(bgrinstead)
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.
(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 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
Attachment #8748261 - Flags: review?(bgrinstead) → review+
Created attachment 8748672 [details] [diff] [review]
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!!
Attachment #8748672 - Flags: review?(bgrinstead)
Attachment #8748252 - Attachment is obsolete: true
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.
Created attachment 8748680 [details] [diff] [review]
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
Attachment #8748680 - Flags: review?(bgrinstead)
Attachment #8748672 - Attachment is obsolete: true
Attachment #8748672 - Flags: review?(bgrinstead)
Attachment #8748261 - Attachment is obsolete: true
Comment on attachment 8748672 [details] [diff] [review]
Convert inspector key shortcut to stop using XUL - v3

Oops, wrong obsolete...
Attachment #8748672 - Flags: review?(bgrinstead)
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?
(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 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.
Attachment #8748680 - Flags: review?(bgrinstead) → review+
Flags: qe-verify? → qe-verify+

Updated

3 years ago
QA Contact: alexandra.lucinet
Created attachment 8750366 [details] [diff] [review]
Convert markup view key shortcuts to use the shortcut helper module - v5

Addressed review comments and eslint.
Attachment #8750366 - Flags: review+
Attachment #8748680 - Attachment is obsolete: true
Created attachment 8750368 [details] [diff] [review]
Convert inspector key shortcut to stop using XUL - v4
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...
Attachment #8750368 - Flags: review?(bgrinstead)
Created attachment 8750382 [details] [diff] [review]
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 "@".
Attachment #8750382 - Flags: review?(bgrinstead)
Created attachment 8750384 [details] [diff] [review]
Fix support of all non-letter characters - v1

Fix something stupid in key-shortcuts which prevent "[" shortcut from working...
Attachment #8750384 - Flags: review?(bgrinstead)
Created attachment 8750385 [details] [diff] [review]
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.
Attachment #8750385 - Flags: review?(bgrinstead)
(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.
Flags: needinfo?(francesco.lodolo)
(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
Flags: needinfo?(francesco.lodolo)
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'
Attachment #8750382 - Flags: review?(bgrinstead) → review+
Attachment #8750384 - Flags: review?(bgrinstead) → review+
Attachment #8750385 - Flags: review?(bgrinstead) → review+
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
Attachment #8750368 - Flags: review?(bgrinstead)

Updated

3 years ago
Iteration: 49.1 - May 9 → 49.2 - May 23
Created attachment 8751289 [details] [diff] [review]
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?
Attachment #8751289 - Flags: review?(bgrinstead)
Attachment #8750368 - Attachment is obsolete: true
Attachment #8751289 - Flags: review?(bgrinstead) → review+
Created attachment 8751362 [details] [diff] [review]
Convert inspector key shortcut to stop using XUL - v6

Rebase and fix KeyShortcuts is already defined exception.
Attachment #8751362 - Flags: review+
Attachment #8751289 - Attachment is obsolete: true

Comment 38

3 years ago
bugherder
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
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox49: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 49

Comment 39

2 years ago
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
Status: RESOLVED → VERIFIED
QA Whiteboard: [qe-dthtml]
status-firefox49: fixed → verified
Flags: qe-verify+
Attachment #8748672 - Flags: review?(bgrinstead)
See Also: → bug 1279675

Updated

2 years ago
Depends on: 1327982

Updated

4 months ago
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.