[Amsterdam] Fixing some focus states by cchaos · Pull Request #4876 · elastic/eui

cchaos · 2021-06-11T18:04:24Z

Preface

The Amsterdam theme prioritizes using the browser's internal focus management through the outline property. However, browsers not only display these slightly differently, but also controls when to display them differently. For instance, specifically on Mac OS:

Chrome: Always displays the outline on regular :focus, but supports the new :focus-visible property to control the display of the focus outline only when using the keyboard.
Safari: Does not support :focus-visible (yet), but already chooses not to show the outline on :focus unless using the keyboard to navigate.
Firefox: Supports :focus-visible but does not support customizing the outline-color.

This portion of the reset file also explains further alongside the code.

eui/src/themes/eui-amsterdam/global_styling/reset/_reset.scss

Lines 72 to 89 in 7dc7da7

    
           *:focus { 
        
             // 👉 Safari and Firefox innately respect only showing the outline with keyboard only 
        
             // 💔 But they don't allow coloring of the 'auto'/default outline, so contrast is no good in dark mode. 
        
             // 👉 For these browsers we use the solid type in order to match with `currentColor`. 
        
             // 😦 Which does means the outline will be square 
        
             outline: $euiFocusRingSize solid currentColor; 
        
             outline-offset: -($euiFocusRingSize / 2); 
        
             // 👀 Chrome respects :focus-visible and allows coloring the `auto` style 
        
             &:focus-visible { 
        
               outline-style: auto; 
        
             } 
        
             //🙅‍♀️ But Chrome also needs to have the outline forcefully removed from regular `:focus` state 
        
             &:not(:focus-visible) { 
        
               outline: none; 
        
             } 
        
           }

By adding that to the reset file, all focusable elements will have the same focus behavior (within each browser). By all "focusable" elements, that also means anything with a forced tabindex. There are a few caveats and gotchas that the following components ran into and therefore needed custom focus states/fixes.

EuiPopover

Slightly fixes #4443.

Accessibility note: Accessibility standards require that elements with overflow scrolling ability should be focusable.

I saw this PR only slightly fixes #4443, because the issue still remains for Safari users. This is because, again, Safari uses it's own "magic" to determine when to show focus outlines. While it de-prioritizes outline on most interactive elements until using they keyboard to navigate, it does prioritize it when content "pops" in, focus is moved to a new element via JS.

One of the main problems that was happening in the issue, though, was that when using EuiContextMenu, there are then 2 panels that get a tabindex for shifting focus. I removed the visible outline solely from the context menu panel, but left it on the normal EuiPopover.

So that instead of multiple focus rings when tabbing like:

We only get one that is placed (somewhat, at least the arrow is visible) correctly.

EuiHeaderBreadcrumbs

Fixes #4719

Technical note: Header breadcumbs use clip-path to create their arrow shape. Using clip-path means any outer style like outline and box-shadow are not visible outside of the clip-path.

Because of the technical note above, without any customizations, the EuiHeaderBreadcrumbs focus state looked odd because it was cut off by the clip-path.

Even using box-shadow did not work because of the clip-path.

So the best solution I found was to just eliminate the clip-path when the breadcrumb is :focus-visible.

Note that the "overflow" breadcrumb has a slightly inner focus ring, this is because of how the display had to be handled with a popover. I didn't think this variant compromising enough to warrant a complete re-work of the elements.

Also, while this still doesn't fix Safari, it's a good compromise of effective focus states and waiting for support.

EuiButtonGroup

Fixes #4684

Technical note: The current functionality of :focus-visible cannot be used to target elements via :focus-within. This discussion is on going and may be supported in the future via :has().

#4056 refactored EuiButtonGroup for much better (innate) accessibility usability. There are essentially two types that render different HTML elements.

multi renders <button>s because their selection are independent of one another and we just use the aria-pressed attribute for selection.
single renders visually hidden radio inputs encompassed by a clickable <label> element.

The single type of button group is what got tricky because of not supporting :focus-within so originally, and probably mistakenly, we left off the focus outlines hoping we'd get support soon.

Instead, now this PR prioritizes showing some sort of focus state if there's no support for :focus-visible-within. The result is that multi groups work similarly to the regular buttons (only show outline during keyboard nav), but single groups will always show focus ring even when using the mouse (in Chrome & Firefox).

Checklist

Check against all themes for compatibility in both light and dark modes
Checked in mobile
Checked in Chrome, Safari, Edge, and Firefox
[ ] Props have proper autodocs and playground toggles
[ ] Added documentation
~~[ ] Checked Code Sandbox works for the any docs examples~~
Added or updated jest tests
~~[ ] Checked for breaking changes and labeled appropriately~~
Checked for accessibility including keyboard-only and screenreader modes
A changelog entry exists and is marked appropriately

… support

cchaos · 2021-06-11T18:05:51Z

Note to reviewers: This needs to be a quick fix so that we can get it into Kibana 7.14. There is certinaly more we can do to tackle focus states, but let's keep this scope limited and make issues for follow up.

kibanamachine · 2021-06-11T18:28:02Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_4876/

myasonik

Tested in FF@next, Safari, and Chrome

src/components/accordion/_accordion.scss

src/components/color_picker/_saturation.scss

myasonik · 2021-06-14T19:25:08Z

src/components/context_menu/_context_menu_panel.scss

+  &:focus {
+    outline: none; // Hide focus ring because of `tabindex=0` on Safari
+  }


(not blocking)

Rather than removing the focus outline, can we set tabindex to -1 instead? (L519)

I don't imagine the context menu scrolls? (Or, if it does, that'd probably be the edge so I'd rather optimize for the general case and remove a tab stop).

Hmm, the problem is that Safari still puts focus rings on tabindex=-1 so we'll still have this effect:

That focus ring around the whole popover is, just that, around the popover.

There used to be two tab stops: the popover and the context menu. Now there's one.

You could go into the example and set tabIndex="-1" for the popover panel to remove it.

Created #4884 to further discuss

kibanamachine · 2021-06-15T14:44:47Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_4876/

myasonik

Retested in FF, Safari, Chrome on Mac.
Spot checked in FF and ChromiEdge on Windows.

cchaos · 2021-06-16T03:44:37Z

Update

I added just a few more updates to the mix:

EuiBetaBadge

The new accent color was still getting the focus ring color from currentColor, but the text color was white so I just made them all black (white in dark mode).

EuiCard

Not a focus state, but the disabled state was indiscernable on subdued backgrounds so I just made it match disabled buttons.

kibanamachine · 2021-06-16T04:05:00Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_4876/

cchaos · 2021-06-16T16:25:57Z

And one last thing...

I was told the EuiCard.hasBorder was working but TS didn't like it. So I just pushed a fix to allow passing this prop through to the EuiPanel.

cchaos · 2021-06-16T16:38:06Z

One more one last thing 😉

When I was fixing the cards ^^, I realized that the border-radius' of EuiPanel's were completely wrong. It was still getting the default theme value of 4px instead of the updated value of 6px 🤦 .

Before

After

This will actually make quite the impact on consuming applications. 😬 But it's technically a bug fix.

kibanamachine · 2021-06-16T17:02:18Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_4876/

kibanamachine · 2021-06-16T21:34:56Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_4876/

elizabetdev

Tested in Chrome, Safari, Edge and Firefox, and LGTM! 🎉

After working with focus states I can say that now you're my focus states hero! 🦸‍♀️

I really like the extra updates. The panels look much better with a higher border-radius and the disabled card looks great.

kibanamachine · 2021-06-17T17:43:31Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_4876/

cchaos added 4 commits June 9, 2021 16:45

Removing a few intrusive focus rings

e9b89d9

[EuiHeaderBreadcrumbs] Fixing focus states

3c9560d

[EuiButtonGroup] Prioritizing visible focus rings over :focus-visible…

a0a0e2d

… support

Fix position of popover panel outline

260ce53

cchaos requested review from elizabetdev and myasonik June 11, 2021 18:04

myasonik suggested changes Jun 14, 2021

View reviewed changes

cchaos added 4 commits June 15, 2021 09:25

Merge remote-tracking branch 'upstream/master' into ams_fixes/more_focus

30d9c4e

Addressing some feedback for color picker, context menu, and accordion

195d539

cl

6151fa8

Revert saturation snap updates

c4d1c26

myasonik mentioned this pull request Jun 15, 2021

[EuiContextMenu] Should render EuiPopover for you? #4884

Closed

myasonik self-requested a review June 15, 2021 18:32

myasonik approved these changes Jun 15, 2021

View reviewed changes

Also fixing EuiBetaBadge and disabled EuiCard

486b2d9

[EuiCard] Allowing a hasBorder option

5622e5b

Fixed EuiPanel border radius

e8e6d51

cl

f1e375b

Merge branch 'master' into ams_fixes/more_focus

f8542d4

elizabetdev approved these changes Jun 17, 2021

View reviewed changes

Merge branch 'master' into ams_fixes/more_focus

5f9de6b

cchaos merged commit 80e2489 into elastic:master Jun 17, 2021

cchaos deleted the ams_fixes/more_focus branch June 17, 2021 17:57

thompsongl mentioned this pull request Jun 24, 2021

Upgrade EUI to v34.5.1 elastic/kibana#103297

Merged

GerardoPM mentioned this pull request Apr 5, 2022

eui-card-41.4.0 prysmex/ember-eui#130

Merged

	*:focus {
	// 👉 Safari and Firefox innately respect only showing the outline with keyboard only
	// 💔 But they don't allow coloring of the 'auto'/default outline, so contrast is no good in dark mode.
	// 👉 For these browsers we use the solid type in order to match with `currentColor`.
	// 😦 Which does means the outline will be square
	outline: $euiFocusRingSize solid currentColor;
	outline-offset: -($euiFocusRingSize / 2);

	// 👀 Chrome respects :focus-visible and allows coloring the `auto` style
	&:focus-visible {
	outline-style: auto;
	}

	//🙅‍♀️ But Chrome also needs to have the outline forcefully removed from regular `:focus` state
	&:not(:focus-visible) {
	outline: none;
	}
	}

Conversation

cchaos commented Jun 11, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Preface

EuiPopover

EuiHeaderBreadcrumbs

EuiButtonGroup

Checklist

Uh oh!

cchaos commented Jun 11, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kibanamachine commented Jun 11, 2021

Uh oh!

myasonik left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

myasonik Jun 14, 2021

Choose a reason for hiding this comment

Uh oh!

cchaos Jun 15, 2021

Choose a reason for hiding this comment

Uh oh!

myasonik Jun 15, 2021

Choose a reason for hiding this comment

Uh oh!

myasonik Jun 15, 2021

Choose a reason for hiding this comment

Uh oh!

kibanamachine commented Jun 15, 2021

Uh oh!

myasonik left a comment

Choose a reason for hiding this comment

Uh oh!

cchaos commented Jun 16, 2021

Update

EuiBetaBadge

EuiCard

Uh oh!

kibanamachine commented Jun 16, 2021

Uh oh!

cchaos commented Jun 16, 2021

And one last thing...

Uh oh!

cchaos commented Jun 16, 2021

One more one last thing 😉

Uh oh!

kibanamachine commented Jun 16, 2021

Uh oh!

kibanamachine commented Jun 16, 2021

Uh oh!

elizabetdev left a comment

Choose a reason for hiding this comment

Uh oh!

kibanamachine commented Jun 17, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

cchaos commented Jun 11, 2021 •

edited

Loading

cchaos commented Jun 11, 2021 •

edited

Loading