Closed Bug 1428078 Opened 6 years ago Closed 6 years ago

Enable keyboard navigation in the sidebar

Categories

(DevTools :: Console, enhancement, P1)

enhancement

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.
Priority: -- → P2
Whiteboard: [newconsole-mvp]
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
Assignee: nobody → nchevobbe
Status: NEW → ASSIGNED
Priority: P2 → P1
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)
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 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 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.
https://hg.mozilla.org/mozilla-central/rev/4c56b4540243
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 61
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.