Closed
Bug 1122605
Opened 10 years ago
Closed 9 years ago
Markup view should be focused after selecting an element on the page
Categories
(DevTools :: Inspector, defect)
DevTools
Inspector
Tracking
(firefox41 fixed)
RESOLVED
FIXED
Firefox 41
Tracking | Status | |
---|---|---|
firefox41 | --- | fixed |
People
(Reporter: bgrins, Assigned: pbro)
References
(Blocks 1 open bug)
Details
Attachments
(1 file, 1 obsolete file)
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.
Reporter | ||
Updated•10 years ago
|
Reporter | ||
Updated•10 years ago
|
Blocks: DevToolsPaperCuts
Comment 1•10 years ago
|
||
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.
Assignee | ||
Updated•9 years ago
|
Assignee: nobody → pbrosset
Status: NEW → ASSIGNED
Assignee | ||
Comment 2•9 years ago
|
||
/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/
Attachment #8607433 -
Flags: review?(bgrinstead)
Assignee | ||
Comment 3•9 years ago
|
||
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.
Attachment #8607433 -
Flags: review?(bgrinstead)
Assignee | ||
Updated•9 years ago
|
Attachment #8607433 -
Flags: review?(bgrinstead)
Assignee | ||
Comment 4•9 years ago
|
||
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/
Assignee | ||
Comment 5•9 years ago
|
||
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
Reporter | ||
Comment 6•9 years ago
|
||
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?
Attachment #8607433 -
Flags: review?(bgrinstead)
Assignee | ||
Comment 7•9 years ago
|
||
(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.
Assignee | ||
Updated•9 years ago
|
Attachment #8607433 -
Flags: review?(bgrinstead)
Assignee | ||
Comment 8•9 years ago
|
||
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/
Assignee | ||
Comment 9•9 years ago
|
||
New try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=662d7ea58d58
Reporter | ||
Comment 10•9 years ago
|
||
Comment on attachment 8607433 [details] MozReview Request: bz://1122605/pbrosset https://reviewboard.mozilla.org/r/9015/#review7837 Ship It!
Attachment #8607433 -
Flags: review?(bgrinstead) → review+
Assignee | ||
Updated•9 years ago
|
Keywords: checkin-needed
Comment 11•9 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/ea293c3f7e32
Keywords: checkin-needed
Comment 12•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/ea293c3f7e32
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox41:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 41
Assignee | ||
Comment 13•9 years ago
|
||
Attachment #8607433 -
Attachment is obsolete: true
Attachment #8619167 -
Flags: review+
Assignee | ||
Comment 14•9 years ago
|
||
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•