1264907 - Remove the firstChild "feature" from the breadcrumbs widget

Status: RESOLVED FIXED

(DevTools :: Inspector, defect, P1)

Description

[Patrick Brosset <:pbro>]

The breadcrumbs widget has a feature where it would show the first child of the current selection even the DOM tree hadn't been expanded that far yet.

I'm assuming this is to allow keyboard navigating the DOM through the breadcrumbs (pressing RIGHT would go down the tree).

The breadcrumbs is a very rarely used widget and most probably not a widget used to navigate the DOM (at most it provides visual context to people and might be used to go back UP the DOM).

The code used to do this is also complex. I'd like to remove it.

See some discussion in bug 1262491 comment 6 and bug 1262491 comment 7.

If you want to try this, follow these steps:

- open the inspector on this page
- click on the body node in the breadcrumbs widget
- notice how the first child of the body node is visible (this is different, btw, to how Chrome does it)
- press RIGHT a few times
- notice how the widget auto-loads the first child every time and allows you to go down the DOM.

This is what I'd like to remove.

Comment 1

[Patrick Brosset <:pbro>]

Attachment: Bug_1264907_-_Don_t_show_the_firstChild_of_the_cur.diff

Helen, what do you think about removing this? Any compelling reason to keep this around?
Here's a pending try build that should have osX builds read soon if you want to try it: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d11572c82f7b
(spoiler alert: it works like in chrome devtools now).

Julian, pinged you for review since you're the one who brought this to my attention today.

Comment 2

[Julian Descottes [:jdescottes]]

Comment on attachment 8741724 [details] [diff] [review]
Bug_1264907_-_Don_t_show_the_firstChild_of_the_cur.diff

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

The code change looks good. Switching f+ because I would like to discuss something before going forward.

I have a small issue with the change: as soon as you click on the breadcrumbs, the keyboard navigation is handled by the breadcrumbs instead of the markup view. With the "first-child", the navigation used to be similar to the markup view. With this patch, the keyboard navigation is more limited. It is unclear for the user that the focus moved from the markup view to the breadcrumbs, and I think the keyboard navigation should remain similar. (sorry about that, I know I said on IRC that keyboard navigation in the breadcrumbs was useless, I didn't realize how easily the focus could be lost to the breadcrumbs)

Maybe we can go one step further and stop handling keyboard navigation in the breadcrumbs. Instead we could delegate it to the markup-view which already has all the logic to handle UP/DOWN/LEFT/RIGHT depending on the current selection.
I think we should still be able to focus and tab through the breadcrumbs for accessibility, the goal would just be to rely on the markup view for handling the navigation.

::: devtools/client/inspector/test/browser_inspector_breadcrumbs_keybinding.js
@@ +33,1 @@
>  }, {

Keyboard navigation with UP/DOWN is still possible with the current implementation. If we want to keep it (big if) I would add the following test cases for completeness here : 

> {
>   desc: "Pressing up should move to #i1",
>   key: "VK_UP",
>   newSelection: "#i1"
> }, {
>   desc: "Pressing down should move back to #i2",
>   key: "VK_DOWN",
>   newSelection: "#i2"
> },
>

::: devtools/client/inspector/test/browser_inspector_breadcrumbs_mutations.js
@@ +61,2 @@
>  }, {
>    desc: "Updating a non id/class attribute to a displayed element should not refresh",

Since you started fixing linting issues in this file, we have 3 remaining errors: line 62, 99 and 204 :)

Comment 3

[Helen V. Holmes (:helenvholmes) (:✨)]

Comment on attachment 8741724 [details] [diff] [review]
Bug_1264907_-_Don_t_show_the_firstChild_of_the_cur.diff

I'm fine with removing this.

Comment 4

[Patrick Brosset <:pbro>]

(In reply to Julian Descottes [:jdescottes] from comment #2)
> Maybe we can go one step further and stop handling keyboard navigation in
> the breadcrumbs. Instead we could delegate it to the markup-view which
> already has all the logic to handle UP/DOWN/LEFT/RIGHT depending on the
> current selection.
> I think we should still be able to focus and tab through the breadcrumbs for
> accessibility, the goal would just be to rely on the markup view for
> handling the navigation.
I've tried this just now. It's nice because it means that shortcuts like H (to hide a node) and others, do work for free.
It also means that bug 1193248 gets (at least partially) fixed. All other toolbox/browser shortcuts now work.
However, it's kind of weird sometimes, if you press PREV on an expanded node in the markup-view, this doesn't select the previous node, instead, the node gets collapsed. So, if you're just looking at the breadcrumbs, and expect to be able to navigate quickly in the tree by using PREV/NEXT, then you might be surprised why it sometimes take 2 key strokes to do 1 action.

Comment 5

[Patrick Brosset <:pbro>]

Attachment: MozReview Request: Bug 1264907 - Don't show the firstChild of the current selection in breadcrumbs; r=jdescottes

The breadcrumbs widget used to have a feature where it would show the first
child of the current selection even the DOM tree hadn't been expanded that
far yet.
This was to allow keyboard navigating the DOM through the breadcrumbs.
The breadcrumbs is a very rarely used widget and this code was unnecessarily
making things complex.
It was decided that this feature would be removed.
Instead, key events are now delegated to the markup-view.

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

Comment 6

[Julian Descottes [:jdescottes]]

Comment on attachment 8750799 [details]
MozReview Request: Bug 1264907 - Don't show the firstChild of the current selection in breadcrumbs; r=jdescottes

https://reviewboard.mozilla.org/r/51639/#review48491

Thanks! The keyboard navigation feels much better now

Few nits and comments on the new changes.

::: .eslintignore:91
(Diff revision 1)
>  devtools/client/inspector/computed/**
>  devtools/client/inspector/fonts/**
>  devtools/client/inspector/shared/test/**
>  devtools/client/inspector/test/**
>  devtools/client/inspector/*.js
> +!devtools/client/inspector/breadcrumbs.js

Just want to ensure this is intentional.
I assumed we were trying to un-ignore folder by folder, not file by file.

::: devtools/client/inspector/breadcrumbs.js:294
(Diff revision 1)
>  
> +      FocusManager.moveFocus(this.chromeWin, elm, type, 0);
> -    event.preventDefault();
> +      event.preventDefault();
> -    event.stopPropagation();
> +      event.stopPropagation();
> +
> +      return;

Just a suggestion.

The late return could make the emit easy to miss.
One option would be to reverse the logic :
"if the key is not handled by breadcrumbs, notify listeners and return".

::: devtools/client/inspector/inspector-panel.js:966
(Diff revision 1)
>          "inspector.menu.selectElement.label", [popupNode.dataset.link], 1));
>      }
>    },
>  
> +  _onBreadcrumbsKeyPress: function(name, event) {
> +    this.markup.handleKeyDown(event, true);

See my comment in markup.js::handleKeyDown. If you decide to go against it, maybe just add a comment here?

::: devtools/client/inspector/markup/markup.js:616
(Diff revision 1)
>      evt.stopPropagation();
>      evt.preventDefault();
>    },
>  
>    /**
> -   * Key handling.
> +   * Handle a key down event in the markup-view.

nit: This method also handles keypress events now. The jsdoc and method name should not make any reference to the specific keydown event.

I think I would rename to handleKeyboardEvent. It will also make the event forwarding in inspector-panel less awkward.

nit: add a new line before the @param jsdoc
nit: the description of the parameter should be on a newline, aligned with {Boolean}

(sorry for the comment nits, but the file is pretty consistent about this)

::: devtools/client/inspector/markup/markup.js:621
(Diff revision 1)
> -   * Key handling.
> +   * Handle a key down event in the markup-view.
> +   * @param {DOMEvent} event
> +   * @param {Boolean} ignoreFocus Optional. If set to true, handling the key
> +   * event won't lead to focusing a node in the view.
>     */
> -  _onKeyDown: function (event) {
> +  handleKeyDown: function (event, ignoreFocus) {

Crazy theory for later: Do we actually need ignoreFocus? The fact that the breadcrumbs gain focus is more a technical matter than a real feature. If we have no complaints about this change, I think we could have a follow up to completely prevent focus on the breadcrumbs. A click would simply select a node in the markup-view, and focus it. What do you think?

::: devtools/client/inspector/test/browser_inspector_breadcrumbs_keybinding.js:8
(Diff revision 1)
>     http://creativecommons.org/publicdomain/zero/1.0/ */
>  "use strict";
>  
>  // Test that the breadcrumbs keybindings work.
> +// Note that the breadcrumbs delegate keyboard navigation to the markup-view,
> +// and so we don't expect to test all cases here, just that a simple arrow keys

nit: "just that a simple arrow keys navigation works" s/keys/key?

::: devtools/client/inspector/test/browser_inspector_search-06.js:23
(Diff revision 1)
>  
>    yield inspector.search.once("search-result");
>    assertHasResult(inspector, true);
>  
>    info("Removing node #d1");
>    yield mutatePage(inspector, testActor,

nit: Rather than adding a new parameter to the mutatePage() method, maybe we can directly yield on inspector-updated here.

That would also be a good opportunity to add a comment explaining why we expect an inspector-updated event in this case? 

I'm guessing it's because the breadcrumbs are updated in this case, which calls inspector.updating("breadcrumbs").

Comment 7

[Patrick Brosset <:pbro>]

Thanks Julian.
I've addressed the review comments.
Some answers below, and a question for :yzen :

(In reply to Julian Descottes [:jdescottes] from comment #6)
> ::: .eslintignore:91
> (Diff revision 1)
> >  devtools/client/inspector/computed/**
> >  devtools/client/inspector/fonts/**
> >  devtools/client/inspector/shared/test/**
> >  devtools/client/inspector/test/**
> >  devtools/client/inspector/*.js
> > +!devtools/client/inspector/breadcrumbs.js
> 
> Just want to ensure this is intentional.
> I assumed we were trying to un-ignore folder by folder, not file by file.
Yes this was intentional. I thought I'd clean the few eslint errors I saw in this file while working on it.
I started looking at other files in the same directory too, but there are many errors in inspector-panel.js and cleaning them now will result in a not so nice merge for Brian (who's working on the context-menu rewriting). So I'll do that later.

> ::: devtools/client/inspector/markup/markup.js:621
> (Diff revision 1)
> > -   * Key handling.
> > +   * Handle a key down event in the markup-view.
> > +   * @param {DOMEvent} event
> > +   * @param {Boolean} ignoreFocus Optional. If set to true, handling the key
> > +   * event won't lead to focusing a node in the view.
> >     */
> > -  _onKeyDown: function (event) {
> > +  handleKeyDown: function (event, ignoreFocus) {
> 
> Crazy theory for later: Do we actually need ignoreFocus? The fact that the
> breadcrumbs gain focus is more a technical matter than a real feature. If we
> have no complaints about this change, I think we could have a follow up to
> completely prevent focus on the breadcrumbs. A click would simply select a
> node in the markup-view, and focus it. What do you think?
Let's ask :yzen about this. Right now, focus goes to the breadcrumbs so you can still TAB through all elements of the toolbox like normal, like after having TABed through the breadcrumbs, you end up in the sidebar, and you can TAB back from it to the breadcrumbs, and then TAB back again to the DOM tree, etc... If we delegate the focus from the breadcrumbs to the inspector, then it means you can't really go to the sidebar anymore I guess.

Comment 8

[Patrick Brosset <:pbro>]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=7d280a26c082&group_state=expanded

Comment 9

[Julian Descottes [:jdescottes]]

(In reply to Patrick Brosset <:pbro> from comment #7)
> > Crazy theory for later: Do we actually need ignoreFocus? The fact that the
> > breadcrumbs gain focus is more a technical matter than a real feature. If we
> > have no complaints about this change, I think we could have a follow up to
> > completely prevent focus on the breadcrumbs. A click would simply select a
> > node in the markup-view, and focus it. What do you think?
> Let's ask :yzen about this. Right now, focus goes to the breadcrumbs so you
> can still TAB through all elements of the toolbox like normal, like after
> having TABed through the breadcrumbs, 

Good call!

Adding some more details about the behavior of the breadcrumbs when using TAB.
1 - When the focus is on a breadcrumbs button, pressing TAB will focus the inspector sidebar, pressing SHIFT+TAB will focus the markup view. Which means it's not really possible to focus the other breadcrumbs buttons using tab.
2 - This issue is OSX only. You can not reach the breadcrumbs by using TAB. TAB jumps from the markup view to the inspector sidebar (and vice versa).

So to elaborate on my suggestion, another option is to really support TAB in the breadcrumbs, addressing the two issues above.

Comment 10

[Marco Zehe (:MarcoZ)]

(In reply to Julian Descottes [:jdescottes] from comment #9)
> 1 - When the focus is on a breadcrumbs button, pressing TAB will focus the
> inspector sidebar, pressing SHIFT+TAB will focus the markup view. Which
> means it's not really possible to focus the other breadcrumbs buttons using
> tab.
This is actually by design. Like in various other desktop apps on both Windows and OS X, buttons in a tool bar can be reached by arrowing left and right, whereas tab takes you out of that toolbar container to the next item past it. This is to reduce the amount of presses of the tab key. Say that I am on a node 30 levels deep, which on a page like Gmail is very likely to happen, and would have to tab 30 or more times through all breadcrumb buttons until I reached the side bar. For me who cannot use a mouse, this would be tideous and terribly inefficient. So, if I want to interact with any of the breadcrumb buttons other than the current one that gained focus when tabbing into that container, I use left and right arrows. But if I actually want to go somewhere else, I press tab once, instead of 30 times.

> 2 - This issue is OSX only. You can not reach the breadcrumbs by using TAB.
> TAB jumps from the markup view to the inspector sidebar (and vice versa).
This probably has to do with the setting in System Preferences -> Keyboard where the tab key only moves to inputs instead of all focusable controls. Firefox honors that system setting.

Comment 11

[Patrick Brosset <:pbro>]

(In reply to Patrick Brosset <:pbro> from comment #8)
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=7d280a26c082&group_state=expanded
This shows font-inspector failures. My analysis so far is that we've always had a race between the font-inspector initialization and the rest of the inspector.
Most panels/widgets in the inspector participate in a shared update mechanism: breadcrumbs, rule-view, computed-view, ... tell the inspector when they are updating, and the inspector then waits for all of them to signal they are done before sending an inspector-updated event.

For some reason, the font-inspector doesn't participate in this mechanism, it does its own init on the side, which races with the inspector's.

This means that in tests, waiting for the inspector-updated event isn't enough.
It turns out that this wasn't a problem so far because of the way the breadcrumbs widget initialized. Making an async request to get the first child probably delayed it enough that the font-inspector was ready by then.

So, now that I have changed the way the breadcrumbs initializes (by removing the firstChild part), the timing changed a bit and tests that used to pass now fail.

Comment 12

[Patrick Brosset <:pbro>]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=20d082baf12c&group_state=expanded

Comment 13

[Patrick Brosset <:pbro>]

I discussed today with :yzen on IRC.

Deferring arrow events to the markup view makes makes it confusing to sighted users: breadcrumbs navigation is linear and horizontal as perceived (visually), so users expect navigating the hierarchy (via breadcrumbs) by arrowing left and right, which is no longer the case with the patch.

Keyboard navigation can be improved further in breadcrumbs by never shifting focus onto the individual breadcrumb and instead keeping it on the breadcrumbs container. This way we can keep track of selected breadcrumbs (for accessibility purposes) via aria-activedescendant attribute, since breadcrumb focus is somewhat meaningless.
Filed bug 1272011 for this.

The position here is that the breadcrumbs are not really a navigation tool to start with, the markup-view is.
The breadcrumbs are more of a context tool, and maybe, sometimes, a way to navigate up to parents.

Therefore, it makes sense to implement the most basic keyboard navigation mechanism we can here.
One option would be to avoid focusing it at all (tab would go from the markup-view to the sidebar directly), but since you can click on individual breadcrumbs and because we want mouse/keyboard parity, we need to at least make sure you can use the left/right arrows to navigate linearly through the visible breadcrumbs.

Comment 14

[Patrick Brosset <:pbro>]

Comment on attachment 8750799 [details]
MozReview Request: Bug 1264907 - Don't show the firstChild of the current selection in breadcrumbs; r=jdescottes

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

Comment 15

[Patrick Brosset <:pbro>]

Comment on attachment 8750799 [details]
MozReview Request: Bug 1264907 - Don't show the firstChild of the current selection in breadcrumbs; r=jdescottes

I think it makes sense to take another look at this.
Based on my last comment (discussion with yzen), I re-introduced a simple LEFT/RIGHT navigation and fixed the tests.

Comment 16

[Julian Descottes [:jdescottes]]

Comment on attachment 8750799 [details]
MozReview Request: Bug 1264907 - Don't show the firstChild of the current selection in breadcrumbs; r=jdescottes

https://reviewboard.mozilla.org/r/51639/#review48839

Looks good! Sorry about the many round-trips for this bug :(

One comment about modifiers, plus we have a small issue with UP/DOWN.
The commit comment should be updated, it still mentions forwarding the events to the markup-view.

Issue with UP/DOWN:
- focus first element of the breadcrumbs
- press UP
-> Focus moves to the markup view

- focus last element of the breadcrumbs
- press DOWN
-> Focus moves to the inspector sidebar

If you are on OSX and have the keyboard setting Marco mentioned in a previous comment (the one that prevents focusing non-input elements, and which I imagine is the default?), this gets worse. Pressing UP or DOWN from anywhere in the breadcrumbs moves the focus out of them.

In a regular toolbar, I think UP should be the same as LEFT, and DOWN the same as RIGHT. IMO we should handle them. 
Could avoid frustration from users like me who got used to having a markup-view-like navigation after clicking on the breadcrumbs.

::: devtools/client/inspector/breadcrumbs.js:272
(Diff revision 2)
>    handleKeyPress: function (event) {
> -    let navigate = promise.resolve(null);
> +    let win = this.chromeWin;
> +    let {keyCode, shiftKey} = event;
>  
> -    this._keyPromise = (this._keyPromise || promise.resolve(null)).then(() => {
> -      switch (event.keyCode) {
> +    // Only handle left, right, and tab
> +    if (keyCode === win.KeyEvent.DOM_VK_LEFT ||

We should check modifiers here, we are swallowing events we should let bubble, for example alt+LEFT (cmd+LEFT on OSX) that should navigate to the previous page etc ...

This is not a regression, and we have another bug already logged for this, so up to you.

Comment 17

[Patrick Brosset <:pbro>]

(In reply to Julian Descottes [:jdescottes] from comment #16)
> Issue with UP/DOWN:
> - focus first element of the breadcrumbs
> - press UP
> -> Focus moves to the markup view
> 
> - focus last element of the breadcrumbs
> - press DOWN
> -> Focus moves to the inspector sidebar
Yeah, I've seen this too. I discussed about this with :yzen and he confirmed that this happened on other XUL toolbars too. Maybe this goes away with the HTML rewrite? (bug 1259812). In any case, I'm going to leave this as is for now and file a follow-up.

> If you are on OSX and have the keyboard setting Marco mentioned in a
> previous comment (the one that prevents focusing non-input elements, and
> which I imagine is the default?), this gets worse. Pressing UP or DOWN from
> anywhere in the breadcrumbs moves the focus out of them.
> 
> In a regular toolbar, I think UP should be the same as LEFT, and DOWN the
> same as RIGHT. IMO we should handle them. 
> Could avoid frustration from users like me who got used to having a
> markup-view-like navigation after clicking on the breadcrumbs.
Ah I see (btw, I don't think this setting is the default, I've had to switch it ON in the past to investigate similar bugs, and it was OFF by default). I'm going to mention this in the follow-up bug too.

Comment 18

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/6191f8b4e99d

Comment 19

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/6191f8b4e99d