1277501 - Convert markupview breadcrumbs to keyboard shortcut module

Status: VERIFIED FIXED

(DevTools :: Inspector, defect, P1)

Description

[Julian Descottes [:jdescottes]]

Convert devtools/client/inspector/breadcrumbs.js to use devtools/client/shared/key-shortcuts.js

Comment 1

[Julian Descottes [:jdescottes]]

Alex: I couldn't find an existing bug for converting the breadcrumbs widget to the new shortcut API, let me know if I missed it.

Comment 2

[Julian Descottes [:jdescottes]]

Attachment: Bug 1277501 - convert inspector breadcrumbs to shortcut-key API;

Existing keyboard shortcuts have been reimplemented using the new shortcut-key
api.

A keypress handler was removed  on the breadcrumbs buttons, it had no use
since a focused button is always selected.

Review commit: https://reviewboard.mozilla.org/r/57200/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/57200/

Comment 3

[Julian Descottes [:jdescottes]]

Comment on attachment 8759134 [details]
Bug 1277501 - convert inspector breadcrumbs to shortcut-key API;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/57200/diff/1-2/

Comment 4

[Julian Descottes [:jdescottes]]

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

Comment 5

[Alexandre Poirot [:ochameau]]

Comment on attachment 8759134 [details]
Bug 1277501 - convert inspector breadcrumbs to shortcut-key API;

https://reviewboard.mozilla.org/r/57200/#review54126

Looks good.

I couldn't apply it. May be my checkout was out of date.
But I imagine you played with your patch and verified the keypress change?

::: devtools/client/inspector/breadcrumbs.js:549
(Diff revision 2)
> +   *        Name of the keyboard shortcut received.
> +   * @param {DOMEvent} event
> +   *        Original event that triggered the shortcut.
>     */
> -  handleKeyPress: function (event) {
> -    let win = this.chromeWin;
> +  handleShortcut: function (name, event) {
> +    if (!this.selection.isElementNode() || !this.outer.contains(event.target)) {

Note that I'm about to land bug 1271988, which introduce a `target` option passed to KeyShortcuts, which allows to listen to only a given dom element.
(I'm waiting for green try)

You may want to wait for it to land to use that instead of doing this this.outer.contains() call.

::: devtools/client/inspector/breadcrumbs.js
(Diff revision 2)
> -    button.onkeypress = function onBreadcrumbsKeypress(e) {
> -      if (e.charCode == Ci.nsIDOMKeyEvent.DOM_VK_SPACE ||
> -          e.keyCode == Ci.nsIDOMKeyEvent.DOM_VK_RETURN) {
> -        button.click();
> -      }
> -    };

This is really mysterious!

The original version may be more meaningful as we weren't only focusing the button on click:
https://hg.mozilla.org/mozilla-central/diff/394cd944912b/browser/devtools/inspector/Breadcrumbs.jsm#l1.403

Comment 6

[Julian Descottes [:jdescottes]]

Thanks for the review!

In reply to Alexandre Poirot [:ochameau] from comment #5)
> Comment on attachment 8759134 [details]
> MozReview Request: Bug 1277501 - convert inspector breadcrumbs to
> shortcut-key API;r=ochameau
> 
> https://reviewboard.mozilla.org/r/57200/#review54126
> 
> Looks good.
> 
> I couldn't apply it. May be my checkout was out of date.
> But I imagine you played with your patch and verified the keypress change?

I played with it :) Could not see any difference with the current behavior. Only tried on OSX though, I will give it a spin on other OSes before landing.

> 
> ::: devtools/client/inspector/breadcrumbs.js:549
> (Diff revision 2)
> > +   *        Name of the keyboard shortcut received.
> > +   * @param {DOMEvent} event
> > +   *        Original event that triggered the shortcut.
> >     */
> > -  handleKeyPress: function (event) {
> > -    let win = this.chromeWin;
> > +  handleShortcut: function (name, event) {
> > +    if (!this.selection.isElementNode() || !this.outer.contains(event.target)) {
> 
> Note that I'm about to land bug 1271988, which introduce a `target` option
> passed to KeyShortcuts, which allows to listen to only a given dom element.
> (I'm waiting for green try)
> 
> You may want to wait for it to land to use that instead of doing this
> this.outer.contains() call.
> 

Sounds good! Will wait for 1271988 to land.

> ::: devtools/client/inspector/breadcrumbs.js
> (Diff revision 2)
> > -    button.onkeypress = function onBreadcrumbsKeypress(e) {
> > -      if (e.charCode == Ci.nsIDOMKeyEvent.DOM_VK_SPACE ||
> > -          e.keyCode == Ci.nsIDOMKeyEvent.DOM_VK_RETURN) {
> > -        button.click();
> > -      }
> > -    };
> 
> This is really mysterious!
> 
> The original version may be more meaningful as we weren't only focusing the
> button on click:
> https://hg.mozilla.org/mozilla-central/diff/394cd944912b/browser/devtools/
> inspector/Breadcrumbs.jsm#l1.403

Glad I'm not the only one puzzled by this. 
Even in the original version the click handler would only do a focus here, since the rest of the code runs only is the event comes from a right click.

Comment 7

[Julian Descottes [:jdescottes]]

Comment on attachment 8759134 [details]
Bug 1277501 - convert inspector breadcrumbs to shortcut-key API;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/57200/diff/2-3/

Comment 8

[Julian Descottes [:jdescottes]]

Rebased and now using the new {target} constructor parameter for KeyShortcuts.

Pushed to try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ab8a9120f6b0

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/fx-team/rev/58398659c4e9
convert inspector breadcrumbs to shortcut-key API;r=ochameau

Comment 10

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

https://hg.mozilla.org/mozilla-central/rev/58398659c4e9

Comment 11

[Alexandre Poirot [:ochameau]]

Shortcuts en the breadcrumbs are not documented. This page may be a good place holder:
https://developer.mozilla.org/en-US/docs/Tools/Page_Inspector/How_to/Examine_and_edit_HTML#HTML_breadcrumbs

Or we could contribute and link to this one:
https://developer.mozilla.org/en-US/docs/Tools/Page_Inspector/Keyboard_shortcuts

So, once the breadcrumb is focused we can do:
- left/right to navigate to the previous or next element in the breadcrumb widget
- Shift tab is going to focus the markup view
- tab is going to focus the current panel header on the right(rule/computed/box model/animations), but only if one is opened.

Comment 12

[Mihai Boldan, Desktop QA [:mboldan]]

I managed to test this fix on Firefox 50.0a1 (2016-06-09) across platforms[1] and found only an issue that was already logged as Bug 1185349.
Since there are no other issues related to this fix, I am marking this bug Verified Fixed.

[1] Windows 10 x64, Mac OS X 10.11.1, Ubuntu 14.04 x64

Comment 13

[Will Bamberg [:wbamberg]]

I've added a new bit in the keyboard shortcuts page: https://developer.mozilla.org/en-US/docs/Tools/Page_Inspector/Keyboard_shortcuts

... and linked to it from https://developer.mozilla.org/en-US/docs/Tools/Page_Inspector/How_to/Examine_and_edit_HTML#HTML_breadcrumbs.

Let me know if that covers it!

Comment 14

[Alexandre Poirot [:ochameau]]

Looks good, thanks Will!