Skip to content

fix: focus outlines only on keyboard focus#2306

Merged
pfulton merged 6 commits into
mainfrom
mdt2/css-637-focus-audit-adjustments
Dec 1, 2023
Merged

fix: focus outlines only on keyboard focus#2306
pfulton merged 6 commits into
mainfrom
mdt2/css-637-focus-audit-adjustments

Conversation

@mdt2
Copy link
Copy Markdown
Collaborator

@mdt2 mdt2 commented Nov 16, 2023 •
edited
Loading

Description

Some of our components (breadcrumb, card, dropzone, floatingactionbutton, tabs, textfield) were showing and persisting a keyboard focus indicator on mouse focus. This PR adjusts those components to show the correct focus indicators for keyboard and mouse.

How and where has this been tested?

Please tag yourself on the tests you've marked complete to confirm the tests have been run by someone other than the author.

Validation steps

Breadcrumb component

  • Focus outline only shows on keyboard focus

In Storybook:

  • Focus outline only shows on keyboard focus (manually tab through, compare to prod to see behavior before changes)

Card component

  • Focus outline only shows on keyboard focus

In Storybook:

  • Has a focused control
  • Focus outline only shows on keyboard focus (manually tab through, compare to prod to see behavior before changes)

Docs site includes:

  • Example of focus

Dropzone component

  • Focus outline only shows on keyboard focus

In Storybook:

  • Focus outline only shows on keyboard focus (manually tab through, compare to prod to see behavior before changes)

Floatingactionbutton component

  • Focus outline only shows on keyboard focus

In Storybook:

  • Focus outline only shows on keyboard focus (manually tab through, compare to prod to see behavior before changes)

Tabs component

  • Focus outline only shows on keyboard focus

In Storybook:

  • Focus outline only shows on keyboard focus (manually tab through, compare to prod to see behavior before changes)

Textfield component

  • Focus outline only shows on keyboard focus

In Storybook:

  • Has a focused control
  • Has a keyboard focused control
  • Focus outline only shows on keyboard focus (compare to prod to see behavior before changes)

Docs site includes:

  • Example of focus
  • Example of keyboard focus

Regression testing

Validate:

  1. The documentation pages for at least two other components are still loading, including:
  • The pages render correctly, are accessible, and are responsive.
  1. If components have been modified, VRTs have been run on this branch:
  • VRTs have been run and looked at.
  • Any VRT changes have been accepted (by reviewer and/or PR author), or there are no changes.

Screenshots

To-do list

  • I have read the contribution guidelines.
  • I have updated relevant storybook stories and templates.
  • I have tested these changes in Windows High Contrast mode.
  • If my change impacts other components, I have tested to make sure they don't break.
  • If my change impacts documentation, I have updated the documentation accordingly.
  • ✨ This pull request is ready to merge. ✨

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Nov 16, 2023 •
edited
Loading

File metrics

Overall Δ: -1.82 KB ⬇ (-0.05%)

card

-< 1KB ⬇

File Size Diff Δ Δ%
Total changes 70.23 KB 69.84 KB -< 1KB ⬇ -0.56%
index-base.css 22.31 KB 22.18 KB -< 1KB ⬇ -0.59%
index-theme.css < 1KB < 1KB No change 🎉 0%
index-vars.css 22.31 KB 22.18 KB -< 1KB ⬇ -0.59%
index.css 22.31 KB 22.18 KB -< 1KB ⬇ -0.59%
mods.json 2.14 KB 2.14 KB No change 🎉 0%
themes/express.css < 1KB < 1KB No change 🎉 0%
themes/spectrum.css < 1KB < 1KB No change 🎉 0%

dropzone

-< 1KB ⬇

File Size Diff Δ Δ%
Total changes 31.76 KB 31.68 KB -< 1KB ⬇ -0.25%
index-base.css 9.63 KB 9.60 KB -< 1KB ⬇ -0.27%
index-theme.css < 1KB < 1KB No change 🎉 0%
index-vars.css 9.63 KB 9.60 KB -< 1KB ⬇ -0.27%
index.css 9.63 KB 9.60 KB -< 1KB ⬇ -0.27%
mods.json 1.71 KB 1.71 KB No change 🎉 0%
themes/express.css < 1KB < 1KB No change 🎉 0%
themes/spectrum.css < 1KB < 1KB No change 🎉 0%

floatingactionbutton

-< 1KB ⬇

File Size Diff Δ Δ%
Total changes 22.51 KB 22.05 KB -< 1KB ⬇ -2.04%
index-base.css 7.16 KB 7.01 KB -< 1KB ⬇ -2.14%
index-theme.css < 1KB < 1KB No change 🎉 0%
index-vars.css 7.16 KB 7.01 KB -< 1KB ⬇ -2.14%
index.css 7.16 KB 7.01 KB -< 1KB ⬇ -2.14%
mods.json 1.02 KB 1.02 KB No change 🎉 0%
themes/express.css < 1KB < 1KB No change 🎉 0%
themes/spectrum.css < 1KB < 1KB No change 🎉 0%

tabs

-< 1KB ⬇

File Size Diff Δ Δ%
Total changes 56.72 KB 56.52 KB -< 1KB ⬇ -0.36%
index-base.css 17.79 KB 17.72 KB -< 1KB ⬇ -0.38%
index-theme.css < 1KB < 1KB No change 🎉 0%
index-vars.css 17.97 KB 17.90 KB -< 1KB ⬇ -0.38%
index.css 17.97 KB 17.90 KB -< 1KB ⬇ -0.38%
mods.json 1.48 KB 1.48 KB No change 🎉 0%
themes/express.css < 1KB < 1KB No change 🎉 0%
themes/spectrum.css < 1KB < 1KB No change 🎉 0%

textfield

-< 1KB ⬇

File Size Diff Δ Δ%
Total changes 116.47 KB 115.79 KB -< 1KB ⬇ -0.58%
index-base.css 36.20 KB 35.97 KB -< 1KB ⬇ -0.62%
index-theme.css 1.52 KB 1.52 KB No change 🎉 0%
index-vars.css 37.14 KB 36.91 KB -< 1KB ⬇ -0.61%
index.css 37.14 KB 36.91 KB -< 1KB ⬇ -0.61%
mods.json 2.96 KB 2.96 KB No change 🎉 0%
themes/express.css 1.05 KB 1.05 KB No change 🎉 0%
themes/spectrum.css < 1KB < 1KB No change 🎉 0%

@mdt2 mdt2 force-pushed the mdt2/css-637-focus-audit-adjustments branch from 0f2ee60 to 65321c2 Compare November 16, 2023 17:33
@mdt2 mdt2 marked this pull request as ready for review November 16, 2023 19:56
@mdt2 mdt2 force-pushed the mdt2/css-637-focus-audit-adjustments branch from 65321c2 to 2328f5a Compare November 16, 2023 20:12
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Nov 16, 2023 •
edited
Loading

🚀 Deployed on https://pr-2306--spectrum-css.netlify.app

@mdt2 mdt2 force-pushed the mdt2/css-637-focus-audit-adjustments branch from 2328f5a to da25430 Compare November 16, 2023 20:37
@mdt2 mdt2 requested review from jawinn, jenndiaz and pfulton November 16, 2023 20:37
jawinn
jawinn suggested changes Nov 17, 2023
View reviewed changes
Copy link
Copy Markdown
Collaborator

@jawinn jawinn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The functionality is looking good to no longer show various focus indicators on click.

My biggest question is that we are adding the is-keyboardFocused class to some components that do not need that additional requirement. For example, on tabs, the issue is resolved by removing the :focus selector, as focus-visible already does not trigger on click with a div that has a tabindex. This applies to all the components here except for Textfield, which needs that class because :focus-visible is triggered differently when the field allows keyboard input.

Is the only benefit that we can have examples that show the keyboard focused state? We have been trying to clean up a lot of the unnecessary pseudos and state classes, so this seems to be going in the other direction. But may be desired if we want it for examples and Storybook.

Comment thread components/breadcrumb/stories/breadcrumb.stories.js
Comment thread components/breadcrumb/stories/breadcrumb.stories.js
Comment thread components/breadcrumb/stories/template.js Outdated
Comment thread components/card/index.css Outdated
Comment thread components/textfield/index.css Outdated
Comment thread components/textfield/metadata/textfield.yml
Comment thread components/dropzone/stories/template.js Outdated
Comment thread components/floatingactionbutton/stories/template.js Outdated
@mdt2 mdt2 force-pushed the mdt2/css-637-focus-audit-adjustments branch from da25430 to 0d66f23 Compare November 20, 2023 15:07
@mdt2
Copy link
Copy Markdown
Collaborator Author

mdt2 commented Nov 20, 2023

The functionality is looking good to no longer show various focus indicators on click.

My biggest question is that we are adding the is-keyboardFocused class to some components that do not need that additional requirement. For example, on tabs, the issue is resolved by removing the :focus selector, as focus-visible already does not trigger on click with a div that has a tabindex. This applies to all the components here except for Textfield, which needs that class because :focus-visible is triggered differently when the field allows keyboard input.

Is the only benefit that we can have examples that show the keyboard focused state? We have been trying to clean up a lot of the unnecessary pseudos and state classes, so this seems to be going in the other direction. But may be desired if we want it for examples and Storybook.

Ah, yep. I went on a little "Chromatic Coverage!!!" rampage that didn't make sense in hindsight (we don't want coverage of is-keyboardFocused classes when the component shouldn't have that class 🤦‍♀️). I've just pushed updates to remove those unneeded is-keyboardFocused classes, and to address your comments. Let me know if these changes address your concerns, and thanks for your comments!

@mdt2 mdt2 force-pushed the mdt2/css-637-focus-audit-adjustments branch from 0d66f23 to a720a78 Compare November 20, 2023 15:14
@mdt2 mdt2 requested a review from jawinn November 21, 2023 16:19
@mdt2 mdt2 force-pushed the mdt2/css-637-focus-audit-adjustments branch from a720a78 to ab912eb Compare November 21, 2023 18:13
jenndiaz
jenndiaz approved these changes Nov 22, 2023
View reviewed changes
Copy link
Copy Markdown
Contributor

@jenndiaz jenndiaz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Checked each component and compared against main, I saw no issues!

@pfulton
Copy link
Copy Markdown
Collaborator

pfulton commented Nov 27, 2023

@jnjosh would you mind taking a peek at this one, too?

@pfulton pfulton requested a review from jnjosh November 27, 2023 20:49
jnjosh
jnjosh approved these changes Nov 28, 2023
View reviewed changes
Copy link
Copy Markdown
Collaborator

@jnjosh jnjosh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me! :shipit:

@jnjosh jnjosh mentioned this pull request Nov 30, 2023
Closed
6 tasks
@pfulton pfulton force-pushed the mdt2/css-637-focus-audit-adjustments branch from ab912eb to 1f72c3e Compare December 1, 2023 15:20
@pfulton pfulton added the run_vrt For use on PRs looking to kick off VRT label Dec 1, 2023
@pfulton pfulton merged commit 1f72c3e into main Dec 1, 2023
@pfulton pfulton deleted the mdt2/css-637-focus-audit-adjustments branch December 1, 2023 16:02
@rise-erpelding rise-erpelding mentioned this pull request Mar 13, 2025
Merged
23 tasks
rise-erpelding added a commit that referenced this pull request Mar 14, 2025
@rise-erpelding
fix: remove :focus-visible selectors, see #2306
b764148
rise-erpelding added a commit that referenced this pull request Mar 14, 2025
@rise-erpelding
fix: remove :focus-visible selectors, see #2306
07d6dce
castastrophe pushed a commit that referenced this pull request Mar 21, 2025
@rise-erpelding @castastrophe
fix: remove :focus-visible selectors, see #2306
adfb73e
rise-erpelding added a commit that referenced this pull request Mar 28, 2025
@rise-erpelding
fix: remove :focus-visible selectors, see #2306
9dca22d
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jnjosh jnjosh jnjosh approved these changes

@pfulton pfulton pfulton approved these changes

+2 more reviewers

@jawinn jawinn jawinn approved these changes

@jenndiaz jenndiaz jenndiaz approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

run_vrt For use on PRs looking to kick off VRT

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@mdt2 @pfulton @jnjosh @jawinn @jenndiaz