1122605 - Markup view should be focused after selecting an element on the page

Status: RESOLVED FIXED

(DevTools :: Inspector, defect)

Description

[Brian Grinstead [:bgrins]]

After selecting an element via "Inspect Element" context menu or with the element picker, the markup view should be focused so the keyboard traversal keys should be immediately available.

Comment 1

[Abhishek Bhattacharya [:babhishek21]]

Successfully reproduced.

STR:
1. Open inspector via "Inspect Element" context menu or by using "Pick an Element" button.
2. Click on an element to select it.
3. Markup view switches to corresponding line in source.

PROBLEM: The focus has not shifted to the the markup view. It remains in the selection view. Hence keyboard traversal keys do not come into effect. Markup view has to be brought to focus by clicking on it.

Comment 2

[Patrick Brosset <:pbro>]

Attachment: MozReview Request: bz://1122605/pbrosset

/r/9017 - Bug 1122605 - Focus selected inspector node after user selection; r=bgrins

Pull down this commit:

hg pull -r a7eaa09833eed10a0a6d3950180f1d8516d200ec https://reviewboard-hg.mozilla.org/gecko/

Comment 3

[Patrick Brosset <:pbro>]

Comment on attachment 8607433 [details]
MozReview Request: bz://1122605/pbrosset

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b29ce16052de
Try shows that this patch breaks a lot of tests, probably those tests are expecting the focus to be somewhere else.
Unflagging Brian for now until I resolve these.

Comment 4

[Patrick Brosset <:pbro>]

Comment on attachment 8607433 [details]
MozReview Request: bz://1122605/pbrosset

/r/9017 - Bug 1122605 - Focus selected inspector node after user selection; r=bgrins

Pull down this commit:

hg pull -r 3b1eb55aa4518163f5d3717ca41a77797ede15c4 https://reviewboard-hg.mozilla.org/gecko/

Comment 5

[Patrick Brosset <:pbro>]

This second patch is quite different from the first one, so no need to try and interdiff between the 2 on mozreview. Thanks to the previous failing tests I discovered a mess in the way we marked nodes as selected in various situations. I cleaned that up a little bit and we shouldn't be calling _onNewSelection several times in a row now.
Try push with this: https://treeherder.mozilla.org/#/jobs?repo=try&revision=cb4eb9a4f5b8

Comment 6

[Brian Grinstead [:bgrins]]

Comment on attachment 8607433 [details]
MozReview Request: bz://1122605/pbrosset

https://reviewboard.mozilla.org/r/9015/#review7755

Works great!  Just a couple of notes before landing

::: browser/devtools/markupview/markup-view.js:459
(Diff revision 2)
> +      // Mark the node as selected only if it isn't already (i.e. if we were

Doesn't the initial check in markNodeAsSelected already catch this case?

if (this._selectedContainer === container) {
  return false;
}

If so we could call it unconditionally and remove the comment in the function header about infinite loops

::: browser/devtools/inspector/inspector-panel.js:454
(Diff revision 2)
> -    if (reason !== "navigateaway" &&
> +    if (this.canGetUniqueSelector &&

Curious, why remove the navigateaway condition here?

Comment 7

[Patrick Brosset <:pbro>]

(In reply to Brian Grinstead [:bgrins] from comment #6)
> Comment on attachment 8607433 [details]
> MozReview Request: bz://1122605/pbrosset
> 
> https://reviewboard.mozilla.org/r/9015/#review7755
> 
> Works great!  Just a couple of notes before landing
> 
> ::: browser/devtools/markupview/markup-view.js:459
> (Diff revision 2)
> > +      // Mark the node as selected only if it isn't already (i.e. if we were
> 
> Doesn't the initial check in markNodeAsSelected already catch this case?
> 
> if (this._selectedContainer === container) {
>   return false;
> }
> 
> If so we could call it unconditionally and remove the comment in the
> function header about infinite loops
That's right, with my changes, this condition isn't needed at all, good catch, thanks.

> ::: browser/devtools/inspector/inspector-panel.js:454
> (Diff revision 2)
> > -    if (reason !== "navigateaway" &&
> > +    if (this.canGetUniqueSelector &&
> 
> Curious, why remove the navigateaway condition here?
I had to remove it to make the test browser_inspector_select-last-selected.js pass again.
This condition was here previously because we were setting the current selection several times in a row. With my patch that's not the case anymore, and when there is a page navigation, the "navigateaway" selection reason is actually the one we need to listen to in order to store the unique Css Selector.

Comment 8

[Patrick Brosset <:pbro>]

Comment on attachment 8607433 [details]
MozReview Request: bz://1122605/pbrosset

/r/9017 - Bug 1122605 - Focus selected inspector node after user selection; r=bgrins

Pull down this commit:

hg pull -r 4f269c9cc65a021ed47ec0b9f7c5198265a61fc7 https://reviewboard-hg.mozilla.org/gecko/

Comment 9

[Patrick Brosset <:pbro>]

New try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=662d7ea58d58

Comment 10

[Brian Grinstead [:bgrins]]

Comment on attachment 8607433 [details]
MozReview Request: bz://1122605/pbrosset

https://reviewboard.mozilla.org/r/9015/#review7837

Ship It!

Comment 11

[Pulsebot]

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

Comment 12

[Phil Ringnalda (:philor)]

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

Comment 14

[Patrick Brosset <:pbro>]

Attachment: MozReview Request: Bug 1122605 - Focus selected inspector node after user selection; r=bgrins