1414447 - Third-party devtools webextension panels not included in keyboard navigation

Status: VERIFIED FIXED

(DevTools :: Framework, defect, P3)

Description

[Marcy Sutton]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_11_6) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/61.0.3163.100 Safari/537.36

Steps to reproduce:

-Install a third-party devtools addon in any recent version of Firefox (I could reproduce in Nightly and Developer Edition).

-You can observe this behavior with one of the example extensions: https://github.com/mdn/webextensions-examples/tree/master/devtools-panels

-Use the keyboard to cycle through devtools panels: `Cmd + }` (Mac) or `Ctrl + }` (Win)


Actual results:

The tab switcher code that cycles you through devtools tabs skips over third-party extensions, going from the last native panel straight to the devtools toolbox options button. This means that keyboard users can't get to third-party extensions. This happens on Mac and Windows.

It looks like there's some devtools code that changes the tabindex of those tab buttons for an aria-activedescendant type experience, but the JavaScript that cycles you through them doesn't know about the dynamically added tab panels.


Expected results:

Devtools extensions should be reachable using keyboard shortcuts to cycle through all devtools panels.

Comment 1

[Eitan Isaacson [:eeejay]]

Yura,

Is this something you fixed in the past? Is this a regression?

Comment 2

[Yura Zenevich [:yzen] (please use "needinfo")]

I don't think I've touched any Cmd + }/{ shortcuts, but I would treat anything to do with tabs in DevTools as a regression as that component went through a rewrite from XUL to React, iirc.

Comment 3

[Eitan Isaacson [:eeejay]]

I didn't know about the ctrl+} shortcut before. I noticed that if you tab through the tool tabs the keyboard focus cycles through them correctly, but you can't activate the focused too. So that's bad too.

Comment 4

[Eitan Isaacson [:eeejay]]

Attachment: Use a combined sorted list of toolbox items.

Comment 5

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8927499 [details] [diff] [review]
Use a combined sorted list of toolbox items.

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

Thanks for working on this! :)

Overall, the idea makes sense, but I'd like to avoid the data duplication as mentioned below.

::: devtools/client/framework/toolbox.js
@@ +254,1 @@
>      this.component.setPanelDefinitions(definitions);

This data gets stored as part of the state of the `ToolboxController` React component.

Instead of creating a copy local to this module, let's add an accessor for the existing data.  In toolbox-controller.js, add something like:

```
getPanelDefinitions() {
  return this.state.panelDefinitions;
}
```

and then when navigating tools in this module's `selectNextTool` etc., you can access the definitions with `this.component.getPanelDefinitions()`.

Comment 6

[Eitan Isaacson [:eeejay]]

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #5)
> This data gets stored as part of the state of the `ToolboxController` React
> component.
> 
> Instead of creating a copy local to this module, let's add an accessor for
> the existing data.

Sounds good! I didn't originally do that because I thought react components are not supposed to leak state upwards, only to child components. I'm happy to add an accessor.

Comment 7

[Eitan Isaacson [:eeejay]]

Attachment: Use displayed definitions in toolbox component for kb nav. r?jryans

Comment 8

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

(In reply to Eitan Isaacson [:eeejay] from comment #6)
> (In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #5)
> > This data gets stored as part of the state of the `ToolboxController` React
> > component.
> > 
> > Instead of creating a copy local to this module, let's add an accessor for
> > the existing data.
> 
> Sounds good! I didn't originally do that because I thought react components
> are not supposed to leak state upwards, only to child components. I'm happy
> to add an accessor.

Ah, I would probably agree in a more pure, React-only situation...

In this case, it's feels more fuzzy to me, since we're dealing with wrapper components at the boundary between React and legacy UI components, so I don't feel like there's a clear policy to follow.

Comment 9

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8927913 [details] [diff] [review]
Use displayed definitions in toolbox component for kb nav. r?jryans

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

Thanks, this looks good to me! :)

Comment 10

[Eitan Isaacson [:eeejay]]

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #8)
> (In reply to Eitan Isaacson [:eeejay] from comment #6)
> > (In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #5)
> > > This data gets stored as part of the state of the `ToolboxController` React
> > > component.
> > > 
> > > Instead of creating a copy local to this module, let's add an accessor for
> > > the existing data.
> > 
> > Sounds good! I didn't originally do that because I thought react components
> > are not supposed to leak state upwards, only to child components. I'm happy
> > to add an accessor.
> 
> Ah, I would probably agree in a more pure, React-only situation...
> 
> In this case, it's feels more fuzzy to me, since we're dealing with wrapper
> components at the boundary between React and legacy UI components, so I
> don't feel like there's a clear policy to follow.

I guess ideally the component should be listening to the keypresses and updating its own state, but it sounds like this module is mid-transition.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a0e98ae937a2
Use displayed definitions in toolbox component for kb nav. r=jryans

Comment 12

[Natalia Csoregi [:nataliaCs]]

Backed out changeset  for multiple failures on devtools/client

Backout: https://hg.mozilla.org/integration/mozilla-inbound/rev/57e412af505d1bf9538a69b4140ef769bf6bbdc0

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=a0e98ae937a20699d244f65e89ca2a8c41f92f8c

Comment 13

[Gabriel Luong [:gl] (ΦωΦ)]

Comment on attachment 8927913 [details] [diff] [review]
Use displayed definitions in toolbox component for kb nav. r?jryans

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

::: devtools/client/framework/components/toolbox-controller.js
@@ +147,5 @@
>      this.updateButtonIds();
>    }
>  
> +  get panelDefinitions() {
> +    retun this.state.panelDefinitions;

Ran into this problem locally before the backout.

I think it is the misspelling of retun here.

Comment 14

[Eitan Isaacson [:eeejay]]

That's embarrassing.. i forgot to build locally before testing my changed patch.

Comment 15

[Eitan Isaacson [:eeejay]]

Attachment: Use displayed definitions in toolbox component for kb nav.

Fixed typo. And properly tested.

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b67b072f3c58
Use displayed definitions in toolbox component for kb nav. r=jryans

Comment 17

[Noemi Erli[:noemi_erli]]

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

Comment 18

[Eitan Isaacson [:eeejay]]

Comment on attachment 8928021 [details] [diff] [review]
Use displayed definitions in toolbox component for kb nav.

Approval Request Comment
[Feature/Bug causing the regression]: devtools react rewrite
[User impact if declined]: keyboard users cannot access addon devtools panels
[Is this code covered by automated tests?]: No
[Has the fix been verified in Nightly?]: Yes
[Needs manual test from QE? If yes, steps to reproduce]: The STR is in the bug summary.
[List of other uplifts needed for the feature/fix]: None
[Is the change risky?]: No
[Why is the change risky/not risky?]: It's a limited change that is easily tested.
[String changes made/needed]: None.

Comment 19

[Gerry Chang [:gchang]]

Comment on attachment 8928021 [details] [diff] [review]
Use displayed definitions in toolbox component for kb nav.

Fix a userability issue when using devtools. Beta58+.

Comment 20

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/releases/mozilla-beta/rev/13271ee1aac0

Comment 21

[Oana Botisan, Desktop QA]

I managed to reproduce the bug using an older version of Nightly (2017-11-03) on Windows 10 x64. I installed DevTools Media Panel and when I tried to navigate with the keyboard shortcuts, it always skipped the "Media" panel.
I retested everything on latest Nightly 59 and beta 58.0b5 on Windows 10x64, Ubuntu 16.04 x64 and Mac OS 10.11, but the bug is not reproducing anymore.