Closed Bug 1226877 Opened 4 years ago Closed 4 years ago

[a11y] Invisible elements in devtools shouldn't be tabbable (aka accessibility is broken)

Categories

(DevTools :: General, defect)

defect
Not set

Tracking

(firefox45 affected, firefox50 verified)

VERIFIED FIXED
Firefox 50
Tracking Status
firefox45 --- affected
firefox50 --- verified

People

(Reporter: arni2033, Assigned: yzen)

References

(Depends on 1 open bug, Blocks 1 open bug, )

Details

(Keywords: regression)

Attachments

(2 files)

>>>   My Info:   Win7_64, Nightly 45, 32bit, ID 20151119030404
STR:
 1. Open this "data:" url (only to log something reliably)
>   data:text/html,<script> for (i=0;i<99;i++) console.log(i); </script>
 2. Open developer tools -> Console, then switch to Inspector tab
 3. Open "Rules" tab in Inspector sidebar, then close sidebar
 4. Click <body> node in markup-view, make sure it's focused
 5. Press Tab key twice
 6. Press Tab key 20 times

Result:       
 After Step 5 it's not clear what element is focused.
  (But actually it's an element in Inspector Sidebar which is collapsed)
 After Step 6 a link in console is focused:
  you can tell it by url tooltip in the bottom left corner of content area

Expectations: 
 After Step 5 selected <tab> in TabsToolbar should become focused
Has STR: --- → yes
See Also: → 1248297
Blocks: devtoolsa11y
Thanks for filing this, I noticed that as well but have not opened a bug for it. This is actually a regression as Tabbing was not so crazy a couple of weeks ago.
Keywords: regression
Duplicate of this bug: 1278535
Assignee: nobody → yzenevich
Status: NEW → ASSIGNED
OK this patch does not actually fix this particular issue but does other ones, will add a fix to this review request shortly.
Comment on attachment 8766373 [details]
Bug 1226877 - make collapsed details pane hidden using visibility: hidden to be inaccessible by both keyboard and a11y tools.

https://reviewboard.mozilla.org/r/61302/#review58174

The focus on the web console input field is not being set when the split console opens with this patch applied (it does without it)

::: devtools/client/framework/sidebar.js:458
(Diff revision 1)
>        tab.allTabsMenuItem.setAttribute("checked", true);
>      }
>    },
>  
>    /**
> +   * Set visibility for sidebar panels.

The sidebar panels are actively being re-done to use HTML in Bug 1259819, so I'd prefer not to land this in between.  Maybe leave a comment in that bug, or file a follow up to address this is needed on top of that work

::: devtools/client/framework/test/browser_toolbox_sidebar.js:113
(Diff revision 1)
>      let label = 1;
>      for (let tab of tabs) {
>        is(tab.getAttribute("label"), label++, "Tab has the right title");
>      }
>  
> +    testVisibility(panel, 0);

Same

::: devtools/client/inspector/inspector.css:37
(Diff revision 1)
>     * Override `-moz-user-focus:ignore;` from toolkit/content/minimal-xul.css
>     */
>    -moz-user-focus: normal;
>  }
> +
> +/**

Same
Attachment #8766373 - Flags: review?(bgrinstead) → review-
Another bug with this applied: open the split console (escape), close the split console (escape), try to open again (escape) and it doesn't open
(In reply to Brian Grinstead [:bgrins] from comment #6)
> Comment on attachment 8766373 [details]
> Bug 1226877 - fixing keyboard navigation around hidden devtools
> panels/controls.
> 
> https://reviewboard.mozilla.org/r/61302/#review58174
> 
> The focus on the web console input field is not being set when the split
> console opens with this patch applied (it does without it)
> 
> ::: devtools/client/framework/sidebar.js:458
> (Diff revision 1)
> >        tab.allTabsMenuItem.setAttribute("checked", true);
> >      }
> >    },
> >  
> >    /**
> > +   * Set visibility for sidebar panels.
> 
> The sidebar panels are actively being re-done to use HTML in Bug 1259819, so
> I'd prefer not to land this in between.  Maybe leave a comment in that bug,
> or file a follow up to address this is needed on top of that work
> 
> ::: devtools/client/framework/test/browser_toolbox_sidebar.js:113
> (Diff revision 1)
> >      let label = 1;
> >      for (let tab of tabs) {
> >        is(tab.getAttribute("label"), label++, "Tab has the right title");
> >      }
> >  
> > +    testVisibility(panel, 0);
> 
> Same
> 
> ::: devtools/client/inspector/inspector.css:37
> (Diff revision 1)
> >     * Override `-moz-user-focus:ignore;` from toolkit/content/minimal-xul.css
> >     */
> >    -moz-user-focus: normal;
> >  }
> > +
> > +/**
> 
> Same

Thanks yes, sidebar tabs are working as expected with changes in bug 1259819
Comment on attachment 8766373 [details]
Bug 1226877 - make collapsed details pane hidden using visibility: hidden to be inaccessible by both keyboard and a11y tools.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/61302/diff/1-2/
Attachment #8766373 - Flags: review- → review?(bgrinstead)
Comment on attachment 8766373 [details]
Bug 1226877 - make collapsed details pane hidden using visibility: hidden to be inaccessible by both keyboard and a11y tools.

https://reviewboard.mozilla.org/r/61302/#review60372

Works for me locally, but seeing bustage on the try push

::: devtools/client/framework/toolbox.js:1473
(Diff revision 2)
>      this._refreshConsoleDisplay();
>      this.emit("split-console");
>  
>      if (this._lastFocusedElement) {
>        this._lastFocusedElement.focus();
> +    } else if (this.doc.activeElement) {

Should we be doing a check here to see if this.doc.activeElement is the webconsole frame?

::: devtools/client/framework/toolbox.js:1476
(Diff revision 2)
>      if (this._lastFocusedElement) {
>        this._lastFocusedElement.focus();
> +    } else if (this.doc.activeElement) {
> +      // Do not let the focus remain on a hidden frame, move it to the previous
> +      // active frame in the deck.
> +      Services.focus.moveFocus(this.win, this.doc.activeElement,

We are actively working on removing chrome priv from our frontend, so I'd like to have a discussion about how we can replace some of the functionality in Services.focus for a normal web page.  For instance, in this case is there a way that we could do this without Services?  (we don't necessarily need to do it in this bug)
Attachment #8766373 - Flags: review?(bgrinstead) → review-
Attachment #8766373 - Flags: review-
(In reply to Yura Zenevich [:yzen] from comment #10)
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=8813659a398c

I think here we can simply move focus back onto currently visible iframe in the deck. Ill update the patch.
Comment on attachment 8766373 [details]
Bug 1226877 - make collapsed details pane hidden using visibility: hidden to be inaccessible by both keyboard and a11y tools.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/61302/diff/2-3/
Comment on attachment 8766373 [details]
Bug 1226877 - make collapsed details pane hidden using visibility: hidden to be inaccessible by both keyboard and a11y tools.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/61302/diff/3-4/
Attachment #8766373 - Flags: review?(bgrinstead)
Comment on attachment 8766373 [details]
Bug 1226877 - make collapsed details pane hidden using visibility: hidden to be inaccessible by both keyboard and a11y tools.

https://reviewboard.mozilla.org/r/61302/#review62080

::: devtools/client/netmonitor/netmonitor.css:11
(Diff revision 4)
>  #toolbar-labels {
>    overflow: hidden;
>  }
>  
> +/**
> + * Use visibility:; hidden instead of display: none to still receieve events

Nit: extra semicolon here

::: devtools/client/netmonitor/netmonitor.css:14
(Diff revision 4)
>  
> +/**
> + * Use visibility:; hidden instead of display: none to still receieve events
> + * from a hidden pane.
> + */
> +#details-pane.pane-collapsed {

What is the utility of getting events from the hidden details pane?  Do we actually need to be able to navigate inside it when it's hidden?

r=me on the toolbox.css changes, but I want to understand this part more before agreeing to it.  If you want to land the toolbox parts, feel free to break the netmonitor stuff out into a follow-up bug (up to you).
Attachment #8766373 - Flags: review?(bgrinstead) → review+
As per conversation , will split into 2 patches with more explanation in comments/commit messages.
---
 devtools/client/themes/toolbox.css | 22 +++++++++++++++++++++-
 1 file changed, 21 insertions(+), 1 deletion(-)

Review commit: https://reviewboard.mozilla.org/r/66096/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/66096/
Attachment #8766373 - Attachment description: Bug 1226877 - fixing keyboard navigation around hidden devtools panels/controls. → Bug 1226877 - make collapsed details pane hidden using visibility: hidden to be inaccessible by both keyboard and a11y tools.
Attachment #8773399 - Flags: review?(bgrinstead)
Comment on attachment 8766373 [details]
Bug 1226877 - make collapsed details pane hidden using visibility: hidden to be inaccessible by both keyboard and a11y tools.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/61302/diff/4-5/
https://reviewboard.mozilla.org/r/61302/#review63000

r+, looks like there's some extra junk at the bottom of the commit message that might need cleanup
https://reviewboard.mozilla.org/r/66096/#review63006

Same comment about the end of the commit message

::: devtools/client/themes/toolbox.css:379
(Diff revision 1)
> + * inaccessible by keyboard. This is not the case by default because the are
> + * predominantly hidden using visibility: collapse; style or collapsed
> + * attribute.
> + */
> +.toolbox-panel *,
> +#toolbox-deck[collapsed] *,

The only time the toolbox-deck is collapsed is when the console is selected (so none of the tools in the deck will be selected and won't have their -moz-user-focus reset by the rule below).  As such, I don't think this rule is needed.

Make sense?  Or is there a case I'm not thinking of
Comment on attachment 8773399 [details]
Bug 1226877 - make toolbox panels inaccessible with keyboard when they are hidden or collapsed.

ni? for Comment 22
Flags: needinfo?(yzenevich)
Attachment #8773399 - Flags: review?(bgrinstead)
(In reply to Brian Grinstead [:bgrins] from comment #23)
> Comment on attachment 8773399 [details]
> Bug 1226877 - make toolbox panels and decks inaccessible with keyboard when
> they are hidden or collapsed.
> 
> ni? for Comment 22

Yep, tested, you are right!
Flags: needinfo?(yzenevich)
Comment on attachment 8766373 [details]
Bug 1226877 - make collapsed details pane hidden using visibility: hidden to be inaccessible by both keyboard and a11y tools.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/61302/diff/5-6/
Attachment #8773399 - Attachment description: Bug 1226877 - make toolbox panels and decks inaccessible with keyboard when they are hidden or collapsed. → Bug 1226877 - make toolbox panels inaccessible with keyboard when they are hidden or collapsed.
Attachment #8773399 - Flags: review?(bgrinstead)
Comment on attachment 8773399 [details]
Bug 1226877 - make toolbox panels inaccessible with keyboard when they are hidden or collapsed.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/66096/diff/1-2/
Attachment #8773399 - Flags: review?(bgrinstead) → review+
Comment on attachment 8773399 [details]
Bug 1226877 - make toolbox panels inaccessible with keyboard when they are hidden or collapsed.

https://reviewboard.mozilla.org/r/66096/#review63222
Pushed by yura.zenevich@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b45c5745abc0
make collapsed details pane hidden using visibility: hidden to be inaccessible by both keyboard and a11y tools. r=bgrins
https://hg.mozilla.org/integration/mozilla-inbound/rev/a5ee1d48d2f5
make toolbox panels inaccessible with keyboard when they are hidden or collapsed. r=bgrins
https://hg.mozilla.org/mozilla-central/rev/b45c5745abc0
https://hg.mozilla.org/mozilla-central/rev/a5ee1d48d2f5
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 50
I have reproduced this bug with Nightly 45.0a1 (2015-11-21) on Windows 7, 64 Bit!

This bug's fix is verified with latest Aurora


Build    ID   20160803004014
User  Agent   Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:50.0) Gecko/20100101 Firefox/50.0

[bugday-20160803]
Reproduced this bug in firefox nightly 45.0a1 (2015-11-21) as comment 0 with ubuntu 16.04 (64 bit)

Verified this bug as fixed with latest firefox aurora 50.0a2 (Build ID: 20160804004004)
Mozilla/5.0 (X11; Linux x86_64; rv:50.0) Gecko/20100101 Firefox/50.0
QA Whiteboard: [bugday-20160803]
Status: RESOLVED → VERIFIED
Depends on: 1318048
Depends on: 1327789
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.