Closed Bug 1242851 Opened 8 years ago Closed 8 years ago

[a11y] Ensure keyboard focus has better visibility across toolbox

Categories

(DevTools :: Framework, defect)

defect
Not set
normal

Tracking

(firefox49 fixed)

RESOLVED FIXED
Firefox 49
Tracking Status
firefox49 --- fixed

People

(Reporter: yzen, Assigned: yzen)

References

(Blocks 2 open bugs)

Details

(Keywords: access)

Attachments

(8 files, 1 obsolete file)

Right now when tabbing through attributes of a currently selected tag in the inspector tree, the focus outline is very hard to see. It is even harder to see with the darker background color of a selected tag.
Assignee: nobody → yzenevich
Status: NEW → ASSIGNED
Another example of where it is hard to track keyboard focus is inspector toolbar.
Current focused states, both with the blue selected-line background and without.
Attachment #8713229 - Attachment is obsolete: true
Some suggestions from IRC:

[11:59:29]  <@helenvholmes>	perhaps we could mess with some higher-contrast border colors and thicker ones
[11:59:43]  <@helenvholmes>	see if we can't find one that we think looks good + is more readable
[12:00:16]  <@helenvholmes>	I would prefer a blue/white approach to start, and creep into orange if it isn't good enough
This is a first pass to get some feedback. Thanks!
Attachment #8713821 - Flags: ui-review?(hholmes)
Comment on attachment 8713821 [details] [diff] [review]
1242851 first pass

Only have a couple of comments—mostly I think this looks really good!

I think this is in the scope of your patch, so I'm going to recommend you bump up the contrast on var(--theme-selection-color) to white. (Our current color doesn't stand out very well.)

The selection box-shadow on :-moz-focusring sometimes uses currentColor, which means the Inspector sometimes has black outlines which looks kinda scary. I think we should consider defaulting to the input focus blue unless you know of a reason why it would be good to differentiate colors (which would be valid). If that's the case, maybe we can use a Firefox orange?
Attachment #8713821 - Flags: ui-review?(hholmes) → feedback+
---
 devtools/client/shared/widgets/widgets.css | 11 +++++++++
 devtools/client/themes/common.css          | 14 ++++++++++++
 devtools/client/themes/computed.css        | 16 +++++++++++++
 devtools/client/themes/fonts.css           |  9 ++++++++
 devtools/client/themes/markup.css          | 16 +++++++++++++
 devtools/client/themes/netmonitor.css      | 21 +++++++++++++++++
 devtools/client/themes/performance.css     |  4 ++++
 devtools/client/themes/rules.css           | 13 +++++++++++
 devtools/client/themes/toolbars.css        | 36 +++++++++++++++++++++++++++++-
 devtools/client/themes/variables.css       |  4 ++--
 devtools/client/themes/webconsole.css      |  7 ++++++
 devtools/client/themes/widgets.css         | 23 +++++++++++++++++++
 12 files changed, 171 insertions(+), 3 deletions(-)

Review commit: https://reviewboard.mozilla.org/r/33085/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/33085/
Comment on attachment 8714456 [details]
MozReview Request: Bug 1242851 - adding keyboard focus styles for better accessibility. r=bgrins

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/33085/diff/1-2/
Attachment #8714456 - Attachment description: MozReview Request: Bug 1242851 - adding keyboard focus styles for better accessibility. r=pbro → MozReview Request: Bug 1242851 - adding keyboard focus styles for better accessibility. r=pbrosset
Attachment #8714456 - Flags: review?(pbrosset)
Comment on attachment 8714456 [details]
MozReview Request: Bug 1242851 - adding keyboard focus styles for better accessibility. r=bgrins

I think Brian should ultimately review this patch. I do have a couple of remarks/questions though:

- Do we intend to turn this ON only when you activate an "accessibility" mode? I think we discussed about that on IRC a few days ago, and that's what Firebug does.

- The focus ring around the console, inspector and the debugger search fields is a bit weird in that it doesn't extend to the full width of the fields (there's an extra ~20px padding to the left and right). Is this because the way these fields are implemented is weird and we'd need to change them to fix this?

- Using currentColor in some places looks a bit weird to me. In the rule-view for example, click in a property, then press ESCAPE to have the focus ring appear and then tab through the various properties and values and selectors: the color of the ring alternates from blue to green to orange. I think it would be better if this was consistent. I guess we only really need 2 colors, right? The default (blueish) one, and a contrast one for elements that have a blue background, like selected nodes in the inspector.

- I found some instances of the focus ring only appearing on some edges of an element. For instance, go to about:home, switch the animation inspector panel, and toggle an animation by just moving your mouse over the search submit button in the middle of the about:home page. This will display a toolbar in the animation inspector panel with a play button in it. Now click on this button. It will becomes focused, and the new, blue, focus ring will only appear along the left and bottom edges.
Not sure if we want to fix this now or later.

- I realized that our expand/collapse sidebar panel button in the debugger and inspector aren't keyboard accessible. I.e. go in the debugger, click on the expand/collapse panel button (in the toolbar). The expands/collapses the panel and focuses the button. Now, with the button focused, try to press SPACE or RETURN. Nothing happens. The button seem to activate because I can it flicker a little bit. We're probably only listening to onclick events. I think it's worth filing a bug for.
Attachment #8714456 - Flags: review?(pbrosset) → review?(bgrinstead)
(In reply to Patrick Brosset [:pbrosset] [:pbro] from comment #11)
> Comment on attachment 8714456 [details]
> - Using currentColor in some places looks a bit weird to me. In the
> rule-view for example, click in a property, then press ESCAPE to have the
> focus ring appear and then tab through the various properties and values and
> selectors: the color of the ring alternates from blue to green to orange. I
> think it would be better if this was consistent. I guess we only really need
> 2 colors, right? The default (blueish) one, and a contrast one for elements
> that have a blue background, like selected nodes in the inspector.
I agree that I think we should probably just choose two colors (a focus blue and another color) to stay consistent—currentColor seems to be causing a lot of problems when it's used.
(In reply to Patrick Brosset [:pbrosset] [:pbro] from comment #11)
> Comment on attachment 8714456 [details]
> MozReview Request: Bug 1242851 - adding keyboard focus styles for better
> accessibility. r=pbrosset
> 
> I think Brian should ultimately review this patch. I do have a couple of
> remarks/questions though:
> 
> - Do we intend to turn this ON only when you activate an "accessibility"
> mode? I think we discussed about that on IRC a few days ago, and that's what
> Firebug does.

What do you think, Helen? It's true that firebug has a setting for turning on better accessibility, personally I would not mind having better visibility for things focused in a normal mode..

> 
> - The focus ring around the console, inspector and the debugger search
> fields is a bit weird in that it doesn't extend to the full width of the
> fields (there's an extra ~20px padding to the left and right). Is this
> because the way these fields are implemented is weird and we'd need to
> change them to fix this?

Yes it's the issue with current implementation of those fields.

> 
> - Using currentColor in some places looks a bit weird to me. In the
> rule-view for example, click in a property, then press ESCAPE to have the
> focus ring appear and then tab through the various properties and values and
> selectors: the color of the ring alternates from blue to green to orange. I
> think it would be better if this was consistent. I guess we only really need
> 2 colors, right? The default (blueish) one, and a contrast one for elements
> that have a blue background, like selected nodes in the inspector.

I'll update that.

> 
> - I found some instances of the focus ring only appearing on some edges of
> an element. For instance, go to about:home, switch the animation inspector
> panel, and toggle an animation by just moving your mouse over the search
> submit button in the middle of the about:home page. This will display a
> toolbar in the animation inspector panel with a play button in it. Now click
> on this button. It will becomes focused, and the new, blue, focus ring will
> only appear along the left and bottom edges.
> Not sure if we want to fix this now or later.

I tried finding edge cases like that, thanks for bringing up attention to this one. It's just that by default box-shadow is not inset. I change it to be inset if the element is on the edge like that.

> 
> - I realized that our expand/collapse sidebar panel button in the debugger
> and inspector aren't keyboard accessible. I.e. go in the debugger, click on
> the expand/collapse panel button (in the toolbar). The expands/collapses the
> panel and focuses the button. Now, with the button focused, try to press
> SPACE or RETURN. Nothing happens. The button seem to activate because I can
> it flicker a little bit. We're probably only listening to onclick events. I
> think it's worth filing a bug for.

Yes we already have keyboard accessibility bugs open as part of bug 1242686
Flags: needinfo?(hholmes)
(In reply to Yura Zenevich [:yzen] from comment #13)
> (In reply to Patrick Brosset [:pbrosset] [:pbro] from comment #11)
> > Comment on attachment 8714456 [details]
> > MozReview Request: Bug 1242851 - adding keyboard focus styles for better
> > accessibility. r=pbrosset
> > 
> > I think Brian should ultimately review this patch. I do have a couple of
> > remarks/questions though:
> > 
> > - Do we intend to turn this ON only when you activate an "accessibility"
> > mode? I think we discussed about that on IRC a few days ago, and that's what
> > Firebug does.
> 
> What do you think, Helen? It's true that firebug has a setting for turning
> on better accessibility, personally I would not mind having better
> visibility for things focused in a normal mode..
I'd like it to be available in normal mode. I think our devtools should be accessible by default, not something you have to go turn on.
Flags: needinfo?(hholmes)
(In reply to Helen V. Holmes (:helenvholmes) (:✨) from comment #14)
> (In reply to Yura Zenevich [:yzen] from comment #13)
> > (In reply to Patrick Brosset [:pbrosset] [:pbro] from comment #11)
> > > Comment on attachment 8714456 [details]
> > > MozReview Request: Bug 1242851 - adding keyboard focus styles for better
> > > accessibility. r=pbrosset
> > > 
> > > I think Brian should ultimately review this patch. I do have a couple of
> > > remarks/questions though:
> > > 
> > > - Do we intend to turn this ON only when you activate an "accessibility"
> > > mode? I think we discussed about that on IRC a few days ago, and that's what
> > > Firebug does.
> > 
> > What do you think, Helen? It's true that firebug has a setting for turning
> > on better accessibility, personally I would not mind having better
> > visibility for things focused in a normal mode..
> I'd like it to be available in normal mode. I think our devtools should be
> accessible by default, not something you have to go turn on.

\o/
Comment on attachment 8714456 [details]
MozReview Request: Bug 1242851 - adding keyboard focus styles for better accessibility. r=bgrins

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/33085/diff/2-3/
Attachment #8714456 - Attachment description: MozReview Request: Bug 1242851 - adding keyboard focus styles for better accessibility. r=pbrosset → MozReview Request: Bug 1242851 - adding keyboard focus styles for better accessibility. r=bgrins
Note this patch is for keyboard focus visibility and does not attempt to fix any keyboard navigation/accessibility issues. They are going to be addressed as part of: bug 1242852, bug 1242854 etc
(In reply to Yura Zenevich [:yzen] from comment #13)
> (In reply to Patrick Brosset [:pbrosset] [:pbro] from comment #11)
> > - The focus ring around the console, inspector and the debugger search
> > fields is a bit weird in that it doesn't extend to the full width of the
> > fields (there's an extra ~20px padding to the left and right). Is this
> > because the way these fields are implemented is weird and we'd need to
> > change them to fix this?
> 
> Yes it's the issue with current implementation of those fields.

What specifically is the issue?  Is it something we can fix with different markup?  If not, could we do something like devtools-searchinput:focus and then change the border / outline color instead of moz-focusring?  This really sticks out.
Comment on attachment 8714456 [details]
MozReview Request: Bug 1242851 - adding keyboard focus styles for better accessibility. r=bgrins

https://reviewboard.mozilla.org/r/33085/#review30681

::: devtools/client/shared/widgets/widgets.css:40
(Diff revision 3)
> +  box-shadow: 0 0 2px 2px var(--theme-highlight-blue) inset;

I'd like to pull the commonly used variations things out into variables if possible (in variables.css):

  --var-focus-ring-1: 0 0 1px var(--theme-highlight-blue) inset,
    0 0 4px 1px var(--theme-highlight-blue),
    0 0 1.5px 1px var(--theme-highlight-blue);
  --var-focus-ring-2: 0 0 2px 2px var(--theme-highlight-blue) inset
  
Note the bad variable names - that's because I don't really understand why there are multiple variations throughout this patch.  Is there a method for which element gets which shadow?

::: devtools/client/themes/variables.css:26
(Diff revision 3)
> -  --theme-selection-color: #f5f7fa;
> +  --theme-selection-color: #ffffff;

I'm not sure if this was intentional or not but please don't change --theme-selection-color in this bug - there are other places that need to get updated along with it and we have bug 1242709 opened for that.

::: devtools/client/themes/webconsole.css:144
(Diff revision 3)
>  

The webconsole's input focus ring looks a little off to me (on osx, haven't tested other platforms yet).  I think it'd look better visually if there wasn't a 1 or 2 px vertical space between the border around the input area and the focus ring.  Helen, what do you think?
Attachment #8714456 - Flags: review?(bgrinstead)
(In reply to Brian Grinstead [:bgrins] from comment #19)
> The webconsole's input focus ring looks a little off to me (on osx, haven't
> tested other platforms yet).  I think it'd look better visually if there
> wasn't a 1 or 2 px vertical space between the border around the input area
> and the focus ring.  Helen, what do you think?
Flags: needinfo?(hholmes)
I agree, it looks odd. The focus should be around the entire prompt, not an additional box within the prompt.
Flags: needinfo?(hholmes)
Comment on attachment 8714456 [details]
MozReview Request: Bug 1242851 - adding keyboard focus styles for better accessibility. r=bgrins

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/33085/diff/3-4/
Attachment #8714456 - Flags: review?(bgrinstead)
Comment on attachment 8714456 [details]
MozReview Request: Bug 1242851 - adding keyboard focus styles for better accessibility. r=bgrins

After a quick scan and applying the patch locally, it's is looking much better, thanks!  Helen, can you please review this updated patch and try tabbing through just about everything to see if there's anything that looks out of place, and also to make sure you are good with the visuals.  I think the console focus stands out the most to me as far as an obvious change in the UI when first opening the toolbox.

I really like that this will make it easier for us to be aware when the keyboard navigation is failing.  i.e. in the Inspector once you tab into the search box you can't tab out.  Also in the Inspector you end up focusing the entire sidebar region without being able to do anything.  However, this also makes me think if we should hold off on landing something like this until right after a merge, so we can actually *fix* these issues that are now being highlighted before it rides into Dev Edition.  Just a thought.
Attachment #8714456 - Flags: ui-review?(hholmes)
(In reply to Brian Grinstead [:bgrins] from comment #23)
> Comment on attachment 8714456 [details]
> MozReview Request: Bug 1242851 - adding keyboard focus styles for better
> accessibility. r=bgrins
> 
> After a quick scan and applying the patch locally, it's is looking much
> better, thanks!  Helen, can you please review this updated patch and try
> tabbing through just about everything to see if there's anything that looks
> out of place, and also to make sure you are good with the visuals.  I think
> the console focus stands out the most to me as far as an obvious change in
> the UI when first opening the toolbox.
> 
> I really like that this will make it easier for us to be aware when the
> keyboard navigation is failing.  i.e. in the Inspector once you tab into the
> search box you can't tab out.  Also in the Inspector you end up focusing the
> entire sidebar region without being able to do anything.  However, this also
> makes me think if we should hold off on landing something like this until
> right after a merge, so we can actually *fix* these issues that are now
> being highlighted before it rides into Dev Edition.  Just a thought.

Out of curiosity, when is the merge day for the next version? My next step was to actually tackle the keyboard navigation (and this bug is worked precisely for the reason you mentioned - to be able to see the navigation and issues around it :))
(In reply to Yura Zenevich [:yzen] from comment #24)
> Out of curiosity, when is the merge day for the next version? My next step
> was to actually tackle the keyboard navigation (and this bug is worked
> precisely for the reason you mentioned - to be able to see the navigation
> and issues around it :))

03-07 according to: https://wiki.mozilla.org/RapidRelease/Calendar
(In reply to Brian Grinstead [:bgrins] from comment #25)
> (In reply to Yura Zenevich [:yzen] from comment #24)
> > Out of curiosity, when is the merge day for the next version? My next step
> > was to actually tackle the keyboard navigation (and this bug is worked
> > precisely for the reason you mentioned - to be able to see the navigation
> > and issues around it :))
> 
> 03-07 according to: https://wiki.mozilla.org/RapidRelease/Calendar

Thanks, it should be fine to do that after the merge, if Helen's all good with the ui review, I ll start working on keyboard navigation stuff in the meanwhile.
Comment on attachment 8714456 [details]
MozReview Request: Bug 1242851 - adding keyboard focus styles for better accessibility. r=bgrins

I agree, the console focus is kind of intense. I think a highlight only on the bottom for devtools inputs would make this feel... less so. (The issue with the console is that you're going to be using that input constantly, so the focus feeling heavy and intense is going to feed into a bad experience over time.)

On a sidenote, I notice that focus isn't working in the gcli/developer toolbar.
Attachment #8714456 - Flags: ui-review?(hholmes) → feedback+
(In reply to Helen V. Holmes (:helenvholmes) (:✨) from comment #27)
> On a sidenote, I notice that focus isn't working in the gcli/developer
> toolbar.

I think that's alright for this patch - it's not part of the toolbox or sharing any of the CSS so it's at least a separate issue.
(In reply to Yura Zenevich [:yzen] from comment #26)
> (In reply to Brian Grinstead [:bgrins] from comment #25)
> > (In reply to Yura Zenevich [:yzen] from comment #24)
> > > Out of curiosity, when is the merge day for the next version? My next step
> > > was to actually tackle the keyboard navigation (and this bug is worked
> > > precisely for the reason you mentioned - to be able to see the navigation
> > > and issues around it :))
> > 
> > 03-07 according to: https://wiki.mozilla.org/RapidRelease/Calendar
> 
> Thanks, it should be fine to do that after the merge, if Helen's all good
> with the ui review, I ll start working on keyboard navigation stuff in the
> meanwhile.

That sounds great.  Can you please mark any of these issues as bugs blocking this one?
Attachment #8714456 - Flags: review?(bgrinstead)
Comment on attachment 8714456 [details]
MozReview Request: Bug 1242851 - adding keyboard focus styles for better accessibility. r=bgrins

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/33085/diff/4-5/
Attachment #8714456 - Flags: feedback+
Attachment #8714456 - Flags: ui-review?(hholmes)
Comment on attachment 8714456 [details]
MozReview Request: Bug 1242851 - adding keyboard focus styles for better accessibility. r=bgrins

I like this, it feels less dramatic. 

Looks like the Firefox UX team is settling on some patterns for inputs and focus that we didn't have before. If you could update the patch to match their light styles that would be great:

http://firefoxux.github.io/StyleGuide/#/components/inputs

Relevant CSS:

box-shadow: 0 0 0 2px rgba(97,181,255,.75);
Attachment #8714456 - Flags: ui-review?(hholmes) → ui-review+
As Helen suggested I ask, would you know what would the appropriate color styling be for inputs (when focused: border and box-shadow) when the theme is dark? Thanks
Flags: needinfo?(shorlander)
Comment on attachment 8714456 [details]
MozReview Request: Bug 1242851 - adding keyboard focus styles for better accessibility. r=bgrins

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/33085/diff/5-6/
Attachment #8714456 - Flags: review?(bgrinstead)
Not this now depends on the patch from bug 1242852
Comment on attachment 8714456 [details]
MozReview Request: Bug 1242851 - adding keyboard focus styles for better accessibility. r=bgrins

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/33085/diff/6-7/
I think is ready to be reviewed as well and it requires both patches from bug 1242852 to be applied beforehand.
Renaming / moving the bug to reflect the state of the patch since it's adding styles across the toolbox
Component: Developer Tools: Inspector → Developer Tools: Framework
Summary: [a11y] Ensure keyboard focus has better visibility when tabbing between attributes → [a11y] Ensure keyboard focus has better visibility across toolbox
This is definitely highlighting some bad focus behavior we have across the tools. This one is OSX only (don't see the issue on Win7).  In the options panel if I shift+tab backwards from the first checkbox in the panel (so this box looks like the first focus-able thing in the panel).  Fixes for these sorts of things could be handled in follow ups if you prefer
The entire panel seems to take focus when I shift tab backwards from console input.  It doesn't seem to allow me to do anything useful with the keyboard so maybe it should be skipped.  This one's on both OSX and Win7
Debugger sources seem slightly off balance with the shadow showing more on the left side than the right (more noticeable in OSX than Win but noticeable in both).  Helen, is that something that should be addressed or are you happy with these states?
Flags: needinfo?(hholmes)
Comment on attachment 8714456 [details]
MozReview Request: Bug 1242851 - adding keyboard focus styles for better accessibility. r=bgrins

https://reviewboard.mozilla.org/r/33085/#review32203

::: devtools/client/shared/widgets/widgets.css:40
(Diff revision 4)
>  
> +.side-menu-widget-item-contents:-moz-focusring {
> +  box-shadow: var(--theme-selection-box-shadow-inset);
> +}
> +
> +.selected .side-menu-widget-item-contents:-moz-focusring{

Nit: add whitespace before {

::: devtools/client/themes/toolbars.css:915
(Diff revision 7)
>  .devtools-tab:not([selected])[highlighted] {
>    box-shadow: 0 2px 0 var(--theme-highlight-green) inset;
>  }
>  
> +.devtools-tab:not([selected])[highlighted]:-moz-focusring {
> +  box-shadow: var(--theme-selection-box-shadow-inset-green);

Maybe it's just a failure of the keyboard accesibility right now, but I'm not able to get the debugger into a state to check this UI (need to pause debugger, switch to another panel and then focus the debugger panel).  So I'm inclined to remove this variation + the variable until we are able to test it and do a separate UI review on it (could be that we don't need a special focus style for that state).

::: devtools/client/themes/variables.css:57
(Diff revision 7)
>    --theme-graphs-red: #e57180;
>    --theme-graphs-grey: #cccccc;
>    --theme-graphs-full-red: #f00;
>    --theme-graphs-full-blue: #00f;
> +
> +  --theme-textbox-border-color: #0996f8;

Seems like --theme-textbox-border-color is really --theme-textbox-focus-border-color?  Or shorter: --theme-focus-border-color

::: devtools/client/themes/variables.css:60
(Diff revision 7)
>    --theme-graphs-full-blue: #00f;
> +
> +  --theme-textbox-border-color: #0996f8;
> +  --theme-textbox-box-shadow: rgba(97,181,255,.75);
> +
> +  /* For accessibility purposes we want to enhance the focus styling. This

Is it true that these new `--theme-selection-*` variables are identical between the light and dark themes? I haven't checked all of them, but if so we could at least move them into a new :root section at the bottom of this file so we have only one set of declarations (which would also apply for .theme-firebug)

::: devtools/client/themes/variables.css:61
(Diff revision 7)
> +
> +  --theme-textbox-border-color: #0996f8;
> +  --theme-textbox-box-shadow: rgba(97,181,255,.75);
> +
> +  /* For accessibility purposes we want to enhance the focus styling. This
> +   * should improve keyboard navigation usability.

Nit: please move `*/` onto the same line

::: devtools/client/themes/variables.css:63
(Diff revision 7)
> +  --theme-textbox-box-shadow: rgba(97,181,255,.75);
> +
> +  /* For accessibility purposes we want to enhance the focus styling. This
> +   * should improve keyboard navigation usability.
> +   */
> +  --theme-selection-box-shadow: 0 0 1px var(--theme-highlight-blue) inset,

Throughout this patch, I think `--theme-selection-*` would be more accurate as `--theme-focus-*` since we use 'selection' to mean something else

::: devtools/client/themes/variables.css:74
(Diff revision 7)
> +  --theme-selection-box-shadow-textbox: 0 0 0 2px var(--theme-textbox-box-shadow);
> +  --theme-selection-box-shadow-wide: 0px 2px 2px var(--theme-highlight-blue) inset,
> +    0px -3px 2px var(--theme-highlight-blue) inset,
> +    1px 0px 2px var(--theme-highlight-blue),
> +    -1px 0px 2px var(--theme-highlight-blue);
> +  --theme-selection-box-shadow-wide-dark: 0px 2px 2px var(--theme-highlight-bluegrey) inset,

There's so many variations here.  I wonder if for some of them that are used only once we could switch to a different one so we can remove a variable and the lower amount of choice that devs need to make for their tools.  I guess this is a case-by-case thing and I really do appreciate that you've gone through and picked styles that look good for each widget.   But maybe for something like --theme-selection-box-shadow-wide which is just used by the breadcrumbs we could just let it be --theme-selection-box-shadow.

I wish this were easier and we could apply one or two styles to any focused node, but I get that it isn't since it needs to blend in with the various tools

::: devtools/client/themes/webconsole.css:370
(Diff revision 7)
>  
>  .jsterm-input-node,
>  .jsterm-complete-node {
>    border: none;
> -  padding: 0 0 0 16px;
> +  padding: 4px;
> +  -moz-padding-start: 20px;

We've been using padding-inline-start in place of this
Attachment #8714456 - Flags: review?(bgrinstead)
(In reply to Brian Grinstead [:bgrins] from comment #38)
> Created attachment 8734562 [details]
> weird-options-panel-focus.png
> 
> This is definitely highlighting some bad focus behavior we have across the
> tools. This one is OSX only (don't see the issue on Win7).  In the options
> panel if I shift+tab backwards from the first checkbox in the panel (so this
> box looks like the first focus-able thing in the panel).  Fixes for these
> sorts of things could be handled in follow ups if you prefer

I would definitely want to fix with follow up as the container should probably be non-focusable and the focus should just jump to a first child.
(In reply to Brian Grinstead [:bgrins] from comment #41)
> Comment on attachment 8714456 [details]
> MozReview Request: Bug 1242851 - adding keyboard focus styles for better
> accessibility. r=bgrins
> 
> https://reviewboard.mozilla.org/r/33085/#review32203
> 

> 
> ::: devtools/client/themes/toolbars.css:915
> (Diff revision 7)
> >  .devtools-tab:not([selected])[highlighted] {
> >    box-shadow: 0 2px 0 var(--theme-highlight-green) inset;
> >  }
> >  
> > +.devtools-tab:not([selected])[highlighted]:-moz-focusring {
> > +  box-shadow: var(--theme-selection-box-shadow-inset-green);
> 
> Maybe it's just a failure of the keyboard accesibility right now, but I'm
> not able to get the debugger into a state to check this UI (need to pause
> debugger, switch to another panel and then focus the debugger panel).  So
> I'm inclined to remove this variation + the variable until we are able to
> test it and do a separate UI review on it (could be that we don't need a
> special focus style for that state).

I think you described the steps pretty accurately. Is the issue that you are trapped in the debugger (if so This is going to be addressed in another patch)? For the testing purposes you can focus somewhere else and then do the above steps. I can trigger the style pretty easily this way.
Flags: needinfo?(bgrinstead)
(In reply to Brian Grinstead [:bgrins] from comment #39)
> Created attachment 8734563 [details]
> console-output-panel-focus.png
> 
> The entire panel seems to take focus when I shift tab backwards from console
> input.  It doesn't seem to allow me to do anything useful with the keyboard
> so maybe it should be skipped.  This one's on both OSX and Win7

This is similar issue to the one above. There are a number of places where containers are focusable, I was hoping to deal with them in a separate bug.
(In reply to Brian Grinstead [:bgrins] from comment #41)
> Comment on attachment 8714456 [details]
> MozReview Request: Bug 1242851 - adding keyboard focus styles for better
> accessibility. r=bgrins
> 
> https://reviewboard.mozilla.org/r/33085/#review32203
>
> ::: devtools/client/themes/variables.css:74
> (Diff revision 7)
> > +  --theme-selection-box-shadow-textbox: 0 0 0 2px var(--theme-textbox-box-shadow);
> > +  --theme-selection-box-shadow-wide: 0px 2px 2px var(--theme-highlight-blue) inset,
> > +    0px -3px 2px var(--theme-highlight-blue) inset,
> > +    1px 0px 2px var(--theme-highlight-blue),
> > +    -1px 0px 2px var(--theme-highlight-blue);
> > +  --theme-selection-box-shadow-wide-dark: 0px 2px 2px var(--theme-highlight-bluegrey) inset,
> 
> There's so many variations here.  I wonder if for some of them that are used
> only once we could switch to a different one so we can remove a variable and
> the lower amount of choice that devs need to make for their tools.  I guess
> this is a case-by-case thing and I really do appreciate that you've gone
> through and picked styles that look good for each widget.   But maybe for
> something like --theme-selection-box-shadow-wide which is just used by the
> breadcrumbs we could just let it be --theme-selection-box-shadow.
> 
> I wish this were easier and we could apply one or two styles to any focused
> node, but I get that it isn't since it needs to blend in with the various
> tools
> 

I've tried playing around with it and managed to remove the breadcrumb blue highlight since bug 1242852 fixes a state where breadcrumb could be focused but not highlighted. Not sure how to make it more elegant..

There are 5 variations by color:
 - blue color
 - selection color (which is basically reverse of blue for highlighted inspector nodes)
 - textbox-box-shadow color which is what Helen pointed to in firefox ux pattern for textboxes
 - bluegrey color for nodes that are highlighted (regular blue blends in)
 - green for the debugger tab

Blue and dark blue have an inset variations for cases were nodes only have their content box visible.

The special cases are:
 - console has a shadow only at the bottom
 - breadcrumbs
(In reply to Yura Zenevich [:yzen] from comment #44)
> (In reply to Brian Grinstead [:bgrins] from comment #39)
> > Created attachment 8734563 [details]
> > console-output-panel-focus.png
> > 
> > The entire panel seems to take focus when I shift tab backwards from console
> > input.  It doesn't seem to allow me to do anything useful with the keyboard
> > so maybe it should be skipped.  This one's on both OSX and Win7
> 
> This is similar issue to the one above. There are a number of places where
> containers are focusable, I was hoping to deal with them in a separate bug.

Yes of course, that makes sense to do in another bug
(In reply to Yura Zenevich [:yzen] from comment #43)
> (In reply to Brian Grinstead [:bgrins] from comment #41)
> > Comment on attachment 8714456 [details]
> > MozReview Request: Bug 1242851 - adding keyboard focus styles for better
> > accessibility. r=bgrins
> > 
> > https://reviewboard.mozilla.org/r/33085/#review32203
> > 
> 
> > 
> > ::: devtools/client/themes/toolbars.css:915
> > (Diff revision 7)
> > >  .devtools-tab:not([selected])[highlighted] {
> > >    box-shadow: 0 2px 0 var(--theme-highlight-green) inset;
> > >  }
> > >  
> > > +.devtools-tab:not([selected])[highlighted]:-moz-focusring {
> > > +  box-shadow: var(--theme-selection-box-shadow-inset-green);
> > 
> > Maybe it's just a failure of the keyboard accesibility right now, but I'm
> > not able to get the debugger into a state to check this UI (need to pause
> > debugger, switch to another panel and then focus the debugger panel).  So
> > I'm inclined to remove this variation + the variable until we are able to
> > test it and do a separate UI review on it (could be that we don't need a
> > special focus style for that state).
> 
> I think you described the steps pretty accurately. Is the issue that you are
> trapped in the debugger (if so This is going to be addressed in another
> patch)? For the testing purposes you can focus somewhere else and then do
> the above steps. I can trigger the style pretty easily this way.

I can't get the tabs to appear to be focused at all, or navigate with the keyboard.  See question in https://bugzilla.mozilla.org/show_bug.cgi?id=1242852#c17.  If it would help we can discuss further on IRC.
Flags: needinfo?(bgrinstead)
(In reply to Brian Grinstead [:bgrins] from comment #40)
> Created attachment 8734565 [details]
> debugger-focus-states.png
> 
> Debugger sources seem slightly off balance with the shadow showing more on
> the left side than the right (more noticeable in OSX than Win but noticeable
> in both).  Helen, is that something that should be addressed or are you
> happy with these states?

Hm, if this could be addressed that would be great. It looks a little kooky.
Flags: needinfo?(hholmes)
Comment on attachment 8714456 [details]
MozReview Request: Bug 1242851 - adding keyboard focus styles for better accessibility. r=bgrins

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/33085/diff/7-8/
Attachment #8714456 - Flags: review?(bgrinstead)
(In reply to Helen V. Holmes (:helenvholmes) (:✨)(pls ni?)) from comment #48)
> (In reply to Brian Grinstead [:bgrins] from comment #40)
> > Created attachment 8734565 [details]
> > debugger-focus-states.png
> > 
> > Debugger sources seem slightly off balance with the shadow showing more on
> > the left side than the right (more noticeable in OSX than Win but noticeable
> > in both).  Helen, is that something that should be addressed or are you
> > happy with these states?
> 
> Hm, if this could be addressed that would be great. It looks a little kooky.

I think this should now be addressed
Comment on attachment 8714456 [details]
MozReview Request: Bug 1242851 - adding keyboard focus styles for better accessibility. r=bgrins

Sorry, I'm going to be unavailable for reviews on this and Bug 1242852 until Wednesday.  Forwarding this for feedback from ntim in the meantime.  Tim, if you have a chance can you take a look at this?  See comments in the bug for background if needed, it's basically introducing a global set of focus styles for use across the toolbox to help with keyboard accessibility.  If you are on osx you'll need to flip a system pref to see the focus states for things like the tab bar: https://bugzilla.mozilla.org/show_bug.cgi?id=1242852#c17
Attachment #8714456 - Flags: feedback?(ntim.bugs)
https://reviewboard.mozilla.org/r/33085/#review40179

Have you tested this on Windows? If so, does it add these box-shadows *only* on keyboard focus? I know Windows tends to add focusrings, even after a simple click.

I've tested this on OSX (flipped on the Full keyboard control pref), here are the issues I found:
- The box-shadow sometimes appears on click (without prior keyboard interaction)
- It appears automatically on the first netmonitor item
- I can't seem to trigger focus on the tab bar
- I can't seem to press "Enter" to click/select the item
- The input box-shadow looks too light on the dark theme

We also don't use the same style everywhere, which doesn't feel consistent.

::: devtools/client/themes/variables.css:146
(Diff revision 8)
> +  /* For accessibility purposes we want to enhance the focus styling. This
> +   * should improve keyboard navigation usability. */
> +  --theme-focus-box-shadow: 0 0 1px var(--theme-highlight-blue) inset,
> +    0 0 4px 1px var(--theme-highlight-blue),
> +    0 0 1.5px 1px var(--theme-highlight-blue);
> +  --theme-focus-box-shadow-light: 0 0 1px var(--theme-selection-color) inset,
> +    0 0 4px 1px var(--theme-selection-color),
> +    0 0 1.5px 1px var(--theme-selection-color);
> +  --theme-focus-box-shadow-textbox: 0 0 0 2px var(--theme-textbox-box-shadow);
> +  --theme-focus-box-shadow-wide: 0px 2px 2px var(--theme-highlight-bluegrey) inset,
> +    0px -3px 2px var(--theme-highlight-bluegrey) inset,
> +    1px 0px 2px var(--theme-highlight-bluegrey),
> +    -1px 0px 2px var(--theme-highlight-bluegrey);
> +  --theme-focus-box-shadow-inset: 0 0 2px 2px var(--theme-highlight-blue) inset;
> +  --theme-focus-box-shadow-inset-bottom: 0 -2px 1px var(--theme-textbox-box-shadow) inset,
> +    0px -2px var(--theme-highlight-blue) inset;
> +  --theme-focus-box-shadow-inset-dark: 0 0 2px 2px var(--theme-highlight-bluegrey) inset;
> +  --theme-focus-box-shadow-inset-green: 0 0 2px 2px var(--theme-highlight-green) inset;
> +}

It kind of makes me sad that we're adding so much variables that pretty much have one same purpose.

I'm guessing we're using different colors for better contrast, but why can't we simply use something like `currentColor`, so we can have 1 common box-shadow that simply works in every situation (text color is supposed to have good contrast with the background all the time).
Attachment #8714456 - Flags: feedback?(ntim.bugs)
(In reply to Tim Nguyen [:ntim] from comment #52)
> https://reviewboard.mozilla.org/r/33085/#review40179
> 
> Have you tested this on Windows? If so, does it add these box-shadows *only*
> on keyboard focus? I know Windows tends to add focusrings, even after a
> simple click.

Intention is to display focus and focused element. If a click event triggers focus on an element the highlight must be displayed, it is more than just keyboard only in this case.

> 
> I've tested this on OSX (flipped on the Full keyboard control pref), here
> are the issues I found:
> - The box-shadow sometimes appears on click (without prior keyboard
> interaction)

See above

> - It appears automatically on the first netmonitor item

This is a scope of another bug imo, focus handling within a panel is separate from actually showing focus.

> - I can't seem to trigger focus on the tab bar

Using arrow keys once in the toolbar should work for that .

> - I can't seem to press "Enter" to click/select the item

Use "Space" for that.

> - The input box-shadow looks too light on the dark theme
> 
> We also don't use the same style everywhere, which doesn't feel consistent.
> 
> ::: devtools/client/themes/variables.css:146
> (Diff revision 8)
> > +  /* For accessibility purposes we want to enhance the focus styling. This
> > +   * should improve keyboard navigation usability. */
> > +  --theme-focus-box-shadow: 0 0 1px var(--theme-highlight-blue) inset,
> > +    0 0 4px 1px var(--theme-highlight-blue),
> > +    0 0 1.5px 1px var(--theme-highlight-blue);
> > +  --theme-focus-box-shadow-light: 0 0 1px var(--theme-selection-color) inset,
> > +    0 0 4px 1px var(--theme-selection-color),
> > +    0 0 1.5px 1px var(--theme-selection-color);
> > +  --theme-focus-box-shadow-textbox: 0 0 0 2px var(--theme-textbox-box-shadow);
> > +  --theme-focus-box-shadow-wide: 0px 2px 2px var(--theme-highlight-bluegrey) inset,
> > +    0px -3px 2px var(--theme-highlight-bluegrey) inset,
> > +    1px 0px 2px var(--theme-highlight-bluegrey),
> > +    -1px 0px 2px var(--theme-highlight-bluegrey);
> > +  --theme-focus-box-shadow-inset: 0 0 2px 2px var(--theme-highlight-blue) inset;
> > +  --theme-focus-box-shadow-inset-bottom: 0 -2px 1px var(--theme-textbox-box-shadow) inset,
> > +    0px -2px var(--theme-highlight-blue) inset;
> > +  --theme-focus-box-shadow-inset-dark: 0 0 2px 2px var(--theme-highlight-bluegrey) inset;
> > +  --theme-focus-box-shadow-inset-green: 0 0 2px 2px var(--theme-highlight-green) inset;
> > +}
> 
> It kind of makes me sad that we're adding so much variables that pretty much
> have one same purpose.
> 
> I'm guessing we're using different colors for better contrast, but why can't
> we simply use something like `currentColor`, so we can have 1 common
> box-shadow that simply works in every situation (text color is supposed to
> have good contrast with the background all the time).

Yes, this was my original approach (currentColor) but it makes contrast stand out quite a bit in many cases so the blue box shadow is more consistent with the rest of the firefox UI
I'd like to take a step back to discuss the goal here before spending everyone's time with further code reviews (since I'd still like to lower the number of variations for the focus styling if possible).  I notice for the default browser theme there's a concept of a focus happening with the keyboard vs the mouse.  So if I click on a tab there's no focus state, but if I then press Tab it gets the visible focus state.  I believe this is intentional (shorlander may be able to provide more context).  I'm not sure how difficult it would be to accomplish the same thing (would have to look at the Australis styling).

So Helen, can you please do one more design review with a build using the current patch and make sure we definitely want to have this 'always on' focus state?  It's quite a big change to the UI.
Flags: needinfo?(hholmes)
Yura, could you rebase your current patch so it's up to date with fx-team? I'm having issues applying it.
Flags: needinfo?(yzenevich)
Comment on attachment 8714456 [details]
MozReview Request: Bug 1242851 - adding keyboard focus styles for better accessibility. r=bgrins

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/33085/diff/8-9/
OK, pushed the rebased version. Not you would need to apply 2 patches from bug 1242852 to be able to meaningfully navigate the devtools and inspector with the keyboard.
Flags: needinfo?(yzenevich)
So the ideal state here is to have a focus state that is both a) accessible and b) is really nice looking so all of our users find it pleasant to use whenever it's on. I don't have any design work done for this at all, and since native focus styling affects the entire browser I'd like to bring it up with the larger UX team since I believe some is already working on this issue and I just don't know who.

It will take a little bit as I'm going to be out tomorrow and Friday so I won't meet with the UX team until Monday. Sorry for the wait, hope that explains why this is taking a bit.
(In reply to Helen V. Holmes (:helenvholmes) (:✨)(pls ni?)(unavailable until 4/18)) from comment #58)
> So the ideal state here is to have a focus state that is both a) accessible
> and b) is really nice looking so all of our users find it pleasant to use
> whenever it's on. I don't have any design work done for this at all, and
> since native focus styling affects the entire browser I'd like to bring it
> up with the larger UX team since I believe some is already working on this
> issue and I just don't know who.
> 
> It will take a little bit as I'm going to be out tomorrow and Friday so I
> won't meet with the UX team until Monday. Sorry for the wait, hope that
> explains why this is taking a bit.

Thanks and no worries :)
Comment on attachment 8714456 [details]
MozReview Request: Bug 1242851 - adding keyboard focus styles for better accessibility. r=bgrins

Clearing review for now as per comment 58
Attachment #8714456 - Flags: review?(bgrinstead)
Hi Helen, I was wondering if you were able to discuss this with UX team any further? Thanks!
Sorry for the delay. I chatted with bgrins some and here is some feedback on the patch currently:

- There are a few quirks that seem like they need to be sorted out (see below screenshots). Either looks like focus styling isn't being applied quite right or some small bugs with what the keyboard is trying to latch on to:
https://www.dropbox.com/s/v684tqltm3m35j7/Screenshot%202016-04-25%2016.59.45.png?dl=0 (seems like the keyboard is navigating to something that isn't there)
https://www.dropbox.com/s/bajguj753c7isxw/Screenshot%202016-04-25%2017.00.12.png?dl=0 (issues with styling)
https://www.dropbox.com/s/3plmjgfosmq0lmy/Screenshot%202016-04-25%2017.00.28.png?dl=0 (issues with styling)
https://www.dropbox.com/s/sqh04zaqwevnp3p/Screenshot%202016-04-25%2017.00.37.png?dl=0 (issues with styling)

(Looks like all of those are happening in the Storage Inspector.)

- There are a few instances where it seems like the focus ring is being applied on click instead of on keyboard-navigate. Here, for example, it's happening with the breadcrumbs: http://cl.ly/0X2s233p2o05
- If we could default to what the browser uses for non-input styling and the toolbar tabs (seems like a thin, dotted, black outline) that would be preferable.

Those are the UX points, hope they're enough to get discussion going again!
Flags: needinfo?(hholmes)
Comment on attachment 8714456 [details]
MozReview Request: Bug 1242851 - adding keyboard focus styles for better accessibility. r=bgrins

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/33085/diff/9-10/
Attachment #8714456 - Flags: review?(bgrinstead)
Attachment #8714456 - Flags: review?(bgrinstead) → review+
Comment on attachment 8714456 [details]
MozReview Request: Bug 1242851 - adding keyboard focus styles for better accessibility. r=bgrins

https://reviewboard.mozilla.org/r/33085/#review46371

This is great, thanks for sticking with it!  Forwarding to Helen, but the code changes look good to me

::: devtools/client/themes/variables.css:171
(Diff revision 10)
>                                                       rgba(0, 0, 0, 0.1),
>                                                       transparent) repeat-x;
>  }
> +
> +:root {
> +  --theme-focus-border-color: #0996f8;

I'm thinking this should be called --theme-focus-border-color-textbox or similar.  Or ideally there'll be an existing theme color that can be used instead and this variable can go away, but let's see what colors we land on.

::: devtools/client/themes/variables.css:172
(Diff revision 10)
>                                                       transparent) repeat-x;
>  }
> +
> +:root {
> +  --theme-focus-border-color: #0996f8;
> +  --theme-textbox-box-shadow: rgba(97,181,255,.75);

Helen, can you please have a look at this patch, specifically focus styles for textboxes?  There are two colors here that might need changing: --theme-focus-border-color and --theme-textbox-box-shadow.

Let me know if you need a try build

::: devtools/client/themes/widgets.css:499
(Diff revision 10)
>    transform: translateZ(1px);
>  }
>  
>  /* SideMenuWidget container */
>  
> -.side-menu-widget-container[with-arrows=true] .side-menu-widget-item {
> +.side-menu-widget-container[with-arrows=true] .side-menu-widget-item.selected {

Is this change intentional?
ni? for Comment 64
Flags: needinfo?(hholmes)
https://reviewboard.mozilla.org/r/33085/#review46371

> I'm thinking this should be called --theme-focus-border-color-textbox or similar.  Or ideally there'll be an existing theme color that can be used instead and this variable can go away, but let's see what colors we land on.

I tried using default color that already is in a variable (for dark theme specifically) so I could use just that variable without creating a new one. I was also using black for white theme which is new, I believe

> Helen, can you please have a look at this patch, specifically focus styles for textboxes?  There are two colors here that might need changing: --theme-focus-border-color and --theme-textbox-box-shadow.
> 
> Let me know if you need a try build

Textbox related colors are all taken from the new Firefox guidelines that Helen mentioned in the comments above.

> Is this change intentional?

I believe without it the box is cut off by 1 px on the right but I will double check.
(In reply to Brian Grinstead [:bgrins] from comment #64)
> Comment on attachment 8714456 [details]
> ::: devtools/client/themes/variables.css:172
> (Diff revision 10)
> >                                                       transparent) repeat-x;
> >  }
> > +
> > +:root {
> > +  --theme-focus-border-color: #0996f8;
> > +  --theme-textbox-box-shadow: rgba(97,181,255,.75);
> 
> Helen, can you please have a look at this patch, specifically focus styles
> for textboxes?  There are two colors here that might need changing:
> --theme-focus-border-color and --theme-textbox-box-shadow.
> 
A few final visual nits, sorry!

Can we add a transition on the border colors for text inputs? something like "transition: all 0.2s ease-in-out"?

This looks really great in light theme, but the border + glow difference (while obvious in light theme) isn't as apparently in dark: http://cl.ly/0S3E1Z430x06 can we step the focus color one more color in Shorlander's color guide?

Lastly, also in the above screenshot: there are places where the focus ring is cut off by the div above it. Should we bump down the focus size from 3px > 2px to avoid that problem?

I'm also going to clear the ni? for Shorlander; seems like we've moved past the parts where we needed his help. Happy to reflag him if either of you think that's untrue.
Flags: needinfo?(shorlander)
Flags: needinfo?(hholmes)
This is the latest color guide we have if still applicable: https://developer.mozilla.org/en-US/docs/Tools/DevToolsColors

But it doesn't specifically have a focus color.
Comment on attachment 8714456 [details]
MozReview Request: Bug 1242851 - adding keyboard focus styles for better accessibility. r=bgrins

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/33085/diff/10-11/
Depends on: 1269681
Blocks: 1269681
No longer depends on: 1269681
https://hg.mozilla.org/mozilla-central/rev/e60f6cb27fdd
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 49
Depends on: 1271095
Depends on: 1272179
Depends on: 1272999
Depends on: 1273345
Depends on: 1273904
Depends on: 1289065
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: