Closed
Bug 1428078
Opened 6 years ago
Closed 6 years ago
Enable keyboard navigation in the sidebar
Categories
(DevTools :: Console, enhancement, P1)
DevTools
Console
Tracking
(firefox61 fixed)
RESOLVED
FIXED
Firefox 61
Tracking | Status | |
---|---|---|
firefox61 | --- | fixed |
People
(Reporter: nchevobbe, Assigned: nchevobbe)
References
(Blocks 1 open bug)
Details
(Whiteboard: [newconsole-mvp])
Attachments
(1 file)
The sidebar uses the ObjectInspector, but we disable the keyboard navigation on purpose (see https://searchfox.org/mozilla-central/rev/b24e6342d744c5a83fab5c15972e11eeb69d68e6/devtools/client/webconsole/new-console-output/utils/object-inspector.js#49-51). Although the keyboard navigation should be supported by the ObjectInspector, it seems that there's something weird. In this bug, we should override the disabledFocus attribute for the Sidebar (passing `disabledFocus: false` in https://searchfox.org/mozilla-central/rev/b24e6342d744c5a83fab5c15972e11eeb69d68e6/devtools/client/webconsole/new-console-output/components/SideBar.js#43-46), and see if adjustments are needed on the ObjectInspector side.
Updated•6 years ago
|
Priority: -- → P2
Whiteboard: [newconsole-mvp]
Comment hidden (mozreview-request) |
Assignee | ||
Comment 2•6 years ago
|
||
mozreview-review |
Comment on attachment 8969636 [details] Bug 1428078 - Enable keyboard navigation in the object sidebar; . https://reviewboard.mozilla.org/r/238436/#review244196 The patch is built upon Bug 1452566 patch
Updated•6 years ago
|
Assignee: nobody → nchevobbe
Status: NEW → ASSIGNED
Priority: P2 → P1
Comment 3•6 years ago
|
||
mozreview-review |
Comment on attachment 8969636 [details] Bug 1428078 - Enable keyboard navigation in the object sidebar; . https://reviewboard.mozilla.org/r/238438/#review244286 Looks pretty good! Just a couple comments ::: devtools/client/webconsole/components/GripMessageBody.js:62 (Diff revision 1) > } > > let objectInspectorProps = { > autoExpandDepth: shouldAutoExpandObjectInspector(props) ? 1 : 0, > mode, > + // TODO: we disable focus since the tabing trail is a bit weird in the output (e.g. Typo: "tabbing" ::: devtools/client/webconsole/components/GripMessageBody.js:64 (Diff revision 1) > let objectInspectorProps = { > autoExpandDepth: shouldAutoExpandObjectInspector(props) ? 1 : 0, > mode, > + // TODO: we disable focus since the tabing trail is a bit weird in the output (e.g. > + // location links are not focused). > + // Let's remove the property below when we found and fixed the issue. Could you file a bug with more details and link to it in the comment? ::: devtools/client/webconsole/components/SideBar.js:59 (Diff revision 1) > - sidebarVisible, > grip, > serviceContainer, > } = this.props; > > + const roots = createRootsFromGrip(grip); Why do we need to call createRootsFromGrip and pass it in here? Won't getObjectInspector assign roots to the same thing if we don't pass it in as an override?
Attachment #8969636 -
Flags: review?(bgrinstead)
Assignee | ||
Comment 4•6 years ago
|
||
mozreview-review-reply |
Comment on attachment 8969636 [details] Bug 1428078 - Enable keyboard navigation in the object sidebar; . https://reviewboard.mozilla.org/r/238438/#review244286 > Could you file a bug with more details and link to it in the comment? Sure, I created Bug 1456060 for this. > Why do we need to call createRootsFromGrip and pass it in here? Won't getObjectInspector assign roots to the same thing if we don't pass it in as an override? we use it here because we need the root for focusedItem: `focusedItem: roots[0]` (l.64). We could pass only the grip and have a dedicated `autoSelectGrip` that we pass to ObjectInspectorUtils, but when I tried that it felt not that great (it hides some implementation detail, we have to remove the property from the props we pass to the ObjectInspector, …). I don't feel strongly about this, what would make sense to you ?
Comment hidden (mozreview-request) |
Comment 6•6 years ago
|
||
mozreview-review |
Comment on attachment 8969636 [details] Bug 1428078 - Enable keyboard navigation in the object sidebar; . https://reviewboard.mozilla.org/r/238438/#review244658 This looks good to go after making the change that we discussed separately about not exporting `createRootsFromGrip` and rather creating an option like `autoFocusRoot` that does the `focusedItem: roots[0]` inside the util function.
Attachment #8969636 -
Flags: review?(bgrinstead)
Comment hidden (mozreview-request) |
Comment 8•6 years ago
|
||
mozreview-review |
Comment on attachment 8969636 [details] Bug 1428078 - Enable keyboard navigation in the object sidebar; . https://reviewboard.mozilla.org/r/238438/#review244672
Attachment #8969636 -
Flags: review?(bgrinstead) → review+
Pushed by nchevobbe@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/4c56b4540243 Enable keyboard navigation in the object sidebar; r=bgrins.
Comment 10•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/4c56b4540243
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox61:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 61
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•