Add button focus styles #2007

pksjce · 2022-03-31T08:35:55Z

In anticipation of Buttons accessibility review, I'm pulling in changes from https://github.com/primer/css/pull/1744/files for button specific focus.

My reference is to the focus styles in this storybook.
https://primer-css-eyj5rcc1n-primer.vercel.app/css/storybook?path=/story/patterns-focusstyles--focus-styles

The main change is to remove focus styles of box-shadow and substitute it with outline. primary variant of the button gets some special treatment but all others have same focus styles.

Before

After

Merge checklist

Take a look at the What we look for in reviews section of the contributing guidelines for more information on how we review PRs.

changeset-bot · 2022-03-31T08:35:57Z

🦋 Changeset detected

Latest commit: 2f436be

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
@primer/react	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

pksjce · 2022-03-31T08:36:58Z

I am more confused about focus-visible in Safari. I can't seem to enable tabbing there, there's no experimental feature and there is no pseudoclass for it in the developer tools.

github-actions · 2022-03-31T08:39:22Z

size-limit report 📦

Path	Size
dist/browser.esm.js	65.75 KB (+0.06% 🔺)
dist/browser.umd.js	66.15 KB (+0.05% 🔺)

langermank

This looks great! The only thing I'm noticing (and I can't find it in your PR for some reason) is the box-shadow being removed for :focus fallback. I had to do that in dotcom as an override, but it shouldn't be needed here 😄

langermank · 2022-03-31T13:59:48Z

I am more confused about focus-visible in Safari. I can't seem to enable tabbing there, there's no experimental feature and there is no pseudoclass for it in the developer tools.

Hmm, I seem to have it as experimental in Safari 15.4

colebemis · 2022-03-31T16:25:03Z

The focus style for the primary button doesn't seem to fit with the rest of them. Is this intentional? cc @langermank

langermank · 2022-04-01T01:13:15Z

src/Button/styles.ts

+      ...fallbackFocus,
+      '&:focus-visible:not([disabled])': {
+        outline: '2px solid',
+        outlineOffset: '0',


@colebemis great catch on the primary style! This code reflects what I've done in PCSS, but I don't think its correct. I can't remember why I didn't negative offset the primary 😅 anyways.. let's try:

outlineOffset: '-2' boxShadow: 'inset 0 0 0 3px'

That should align better. I'll make that same change in PCSS/dotcom

Ah! I saw the difference in your code too and thought that was the spec. There was no other reference other than what you had done. Thanks for the clarification. :) Will make this change.

pksjce · 2022-04-04T12:47:18Z

@langermank - I think I've to update macos to get to safari 15
regarding the box-shadow, I'm not sure what's the change you require. Should we have a box-shadow as a fallback on :focus?

pksjce · 2022-04-04T12:51:57Z

@colebemis - I'm not sure if the invisible button requires any special treatment. It is smaller in width because it doesn't have border of its own.

langermank · 2022-04-04T14:14:37Z

regarding the box-shadow, I'm not sure what's the change you require. Should we have a box-shadow as a fallback on :focus?

The change makes the offset negative and changes the box-shadow to 3px instead of 2px:

outlineOffset: '-2'
boxShadow: 'inset 0 0 0 3px'

The box-shadow creates the solid white line between the primary button and the outline. I'm just setting it for :focus-visible and keeping the fallback simple (I'll confirm that later today).

mperrotti · 2022-04-06T22:33:25Z

I am more confused about focus-visible in Safari. I can't seem to enable tabbing there

Safari is weird and you have to go to System Preferences > Keyboard > Shortcuts, and then check the "Use keyboard navigation..." at the bottom.

there's no experimental feature and there is no pseudoclass for it in the developer tools.

@pksjce - you were right, you have to upgrade to Safari 15.4 to test :focus-visible. I have Safari 14, and I'm not getting any focus outlines. See this recording where I compare Primer CSS with Primer React keyboard navigation for buttons:

Check out how @langermank and team got consistent cross-browser behavior.

pksjce · 2022-04-07T04:41:16Z

Hey @mperrotti - It seems to be working fine on Safari now that I've updated it.

Screen.Recording.2022-04-07.at.2.35.16.pm.mov

mperrotti · 2022-04-07T16:51:11Z

@pksjce - I still think we need to support Safari <15.4 like Primer CSS does. Do you want me to try to take that on since I still have 14 installed locally?

mperrotti

Tested in Safari 14, and it works the same as Primer CSS 🎉

pksjce requested review from a team and siddharthkp March 31, 2022 08:35

pksjce had a problem deploying to needs-approval March 31, 2022 08:36 Failure

pksjce had a problem deploying to needs-approval March 31, 2022 08:37 Failure

github-actions bot deployed to Preview March 31, 2022 08:37 View deployment

pksjce requested review from alliethu and langermank March 31, 2022 08:37

langermank approved these changes Mar 31, 2022

View reviewed changes

langermank reviewed Apr 1, 2022

View reviewed changes

Add button focus styles

60015eb

Fix primary button focus styles

851a40c

pksjce force-pushed the pk/button-focus-styles branch from f82477b to 851a40c Compare April 5, 2022 22:49

pksjce had a problem deploying to needs-approval April 5, 2022 22:49 Failure

github-actions bot deployed to Preview April 5, 2022 22:49 View deployment

mperrotti had a problem deploying to needs-approval April 6, 2022 16:30 Failure

github-actions bot deployed to Preview April 6, 2022 16:30 View deployment

mperrotti had a problem deploying to needs-approval April 6, 2022 17:19 Failure

github-actions bot deployed to Preview April 6, 2022 17:19 View deployment

pksjce temporarily deployed to needs-approval April 7, 2022 04:42 Inactive

github-actions bot deployed to Preview April 7, 2022 04:42 View deployment

Fix safari fallback

40b318a

pksjce force-pushed the pk/button-focus-styles branch from 54e89a2 to 40b318a Compare April 8, 2022 01:11

pksjce temporarily deployed to needs-approval April 8, 2022 01:11 Inactive

github-actions bot deployed to Preview April 8, 2022 01:11 View deployment

Fix toggle switch snapshot

a8f29a8

pksjce had a problem deploying to needs-approval April 8, 2022 01:31 Failure

github-actions bot deployed to Preview April 8, 2022 01:31 View deployment

Create strange-jobs-add.md

2f436be

pksjce temporarily deployed to needs-approval April 8, 2022 01:41 Inactive

github-actions bot deployed to Preview April 8, 2022 01:41 View deployment

mperrotti approved these changes Apr 8, 2022

View reviewed changes

pksjce merged commit a8b3ed1 into main Apr 8, 2022

pksjce deleted the pk/button-focus-styles branch April 8, 2022 08:15

primer-css mentioned this pull request Apr 8, 2022

Release tracking #1993

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add button focus styles #2007

Add button focus styles #2007

pksjce commented Mar 31, 2022 •

edited

Loading

changeset-bot bot commented Mar 31, 2022 •

edited

Loading

pksjce commented Mar 31, 2022

github-actions bot commented Mar 31, 2022 •

edited

Loading

langermank left a comment

langermank commented Mar 31, 2022

colebemis commented Mar 31, 2022

langermank Apr 1, 2022

pksjce Apr 3, 2022

pksjce commented Apr 4, 2022

pksjce commented Apr 4, 2022

langermank commented Apr 4, 2022

mperrotti commented Apr 6, 2022

pksjce commented Apr 7, 2022

mperrotti commented Apr 7, 2022

mperrotti left a comment

Add button focus styles #2007

Add button focus styles #2007

Conversation

pksjce commented Mar 31, 2022 • edited Loading

Merge checklist

changeset-bot bot commented Mar 31, 2022 • edited Loading

🦋 Changeset detected

pksjce commented Mar 31, 2022

github-actions bot commented Mar 31, 2022 • edited Loading

size-limit report 📦

langermank left a comment

Choose a reason for hiding this comment

langermank commented Mar 31, 2022

colebemis commented Mar 31, 2022

langermank Apr 1, 2022

Choose a reason for hiding this comment

pksjce Apr 3, 2022

Choose a reason for hiding this comment

pksjce commented Apr 4, 2022

pksjce commented Apr 4, 2022

langermank commented Apr 4, 2022

mperrotti commented Apr 6, 2022

pksjce commented Apr 7, 2022

mperrotti commented Apr 7, 2022

mperrotti left a comment

Choose a reason for hiding this comment

pksjce commented Mar 31, 2022 •

edited

Loading

changeset-bot bot commented Mar 31, 2022 •

edited

Loading

github-actions bot commented Mar 31, 2022 •

edited

Loading