Skip to content

fix(stepper): focus bugs#2358

Merged
pfulton merged 9 commits into
mainfrom
mdt2/css-648-stepper-focus-bugs
Dec 15, 2023
Merged

fix(stepper): focus bugs#2358
pfulton merged 9 commits into
mainfrom
mdt2/css-648-stepper-focus-bugs

Conversation

@mdt2
Copy link
Copy Markdown
Collaborator

@mdt2 mdt2 commented Dec 11, 2023 •
edited
Loading

⚠️ Rebased on #2288

Description

This PR fixes two focus bugs in the Stepper component:

  • Active state for the Infield Buttons sticks around after the mouse click is released
  • Invalid variant keyboard focus outline doesn't fit the component
  • Infield Button isn't focusable within the Stepper component (aligning with implementations from SWC and React teams per Patrick's guidance)
  • Invalid hover state border colors aren't mismatched (infield buttons were getting a different color than the text field)

And also adds kitchen sink style Chromatic coverage for all the variants of Stepper.

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

In the docs site (check Spectrum and Express theme):

  • Infield buttons don't show their focus state when Stepper has focus/keyboard focus
  • Infield buttons show their focus state when they get focus/keyboard focus state within Stepper
  • Invalid Stepper keyboard focus outline looks the same as the keyboard focus outline on valid Stepper
  • Invalid Stepper hover border is just one color (be extra sure to check Express on this one too)
  • Infield Buttons aren't keyboard accessible in the Stepper (aligning with implementations from SWC and React teams per Patrick's guidance)

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.

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. ✨

@mdt2 mdt2 changed the title Mdt2/css 648 stepper focus bugs fix(stepper): focus bugs Dec 11, 2023
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Dec 11, 2023 •
edited
Loading

File metrics

Summary

Total size: 3.97 MB*
Total change (Δ): ⬇ 0.27 KB (0.01%)

Package Size Δ
infieldbutton 18.37 KB 🚨 deleted, moved, or renamed
stepper 20.62 KB 🚨 deleted, moved, or renamed
Details

infieldbutton

File Size Base Δ
Total 18.72 KB 18.37 KB ⬇ 0.36 KB (1.86%)
index-base.css 16.57 KB 16.22 KB ⬇ 0.36 KB (2.10%)
index-theme.css 2.72 KB 2.72 KB -
index-vars.css 18.72 KB 18.37 KB ⬇ 0.36 KB (1.86%)
index.css 18.72 KB 18.37 KB ⬇ 0.36 KB (1.86%)
mods.json 2.16 KB 2.16 KB -
themes/express.css 1.64 KB 1.64 KB -
themes/spectrum.css 1.08 KB 1.08 KB -

stepper

File Size Base Δ
Total 20.39 KB 20.62 KB ⬆ 0.23 KB (1.11%)
index-base.css 17.00 KB 17.22 KB ⬆ 0.23 KB (1.33%)
index-theme.css 3.98 KB 3.98 KB -
index-vars.css 20.39 KB 20.62 KB ⬆ 0.23 KB (1.11%)
index.css 20.39 KB 20.62 KB ⬆ 0.23 KB (1.11%)
mods.json 1.27 KB 1.38 KB ⬆ 0.11 KB (8.51%)
themes/express.css 2.30 KB 2.30 KB -
themes/spectrum.css 1.67 KB 1.67 KB -
* Size determined by adding together the size of the main file for all packages in the library.
* Results are not gzipped or minified.
* An ASCII character in UTF-8 is 8 bits or 1 byte.

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Dec 11, 2023 •
edited
Loading

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

@mdt2 mdt2 requested review from jawinn, jenndiaz and pfulton December 11, 2023 16:52
pfulton
pfulton reviewed Dec 13, 2023
View reviewed changes
Copy link
Copy Markdown
Collaborator

@pfulton pfulton left a comment

Choose a reason for hiding this comment

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

Thanks for taking care of this one!

It looks like this is slightly different in Firefox vs. Chrome, which is maybe ok. I'm a bit more concerned about the missing (overlapping?) focus indicator border when the stepper buttons are focused.

Firefox:
Screenshot 2023-12-13 at 2 27 28 PM

Chrome:
Screenshot 2023-12-13 at 2 29 07 PM

Also, and I think we could do this as a separate/follow-up ticket: can we get representation in Chromatic for the focus states?

@mdt2 mdt2 force-pushed the mdt2/css-648-stepper-focus-bugs branch from e99062d to 9b7665e Compare December 13, 2023 20:53
pfulton
pfulton reviewed Dec 14, 2023
View reviewed changes
Copy link
Copy Markdown
Collaborator

@pfulton pfulton left a comment

Choose a reason for hiding this comment

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

Looking much better! Thank you.

Just one question regarding Storybook: if I flip the focused control to true, it doesn't appear to affect the component in any visual way. Should it be turning off and on the focus indicator?

Comment thread components/stepper/stories/stepper.stories.js
...args
})}

${isChromatic() ? chromaticKitchenSink(args) : null}
Copy link
Copy Markdown
Collaborator

@pfulton pfulton Dec 14, 2023

Choose a reason for hiding this comment

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

😍

@mdt2 mdt2 force-pushed the mdt2/css-648-stepper-focus-bugs branch from 2a5e198 to 28804db Compare December 14, 2023 15:57
@mdt2
Copy link
Copy Markdown
Collaborator Author

mdt2 commented Dec 14, 2023

Looking much better! Thank you.

Just one question regarding Storybook: if I flip the focused control to true, it doesn't appear to affect the component in any visual way. Should it be turning off and on the focus indicator?

Good catch! I broke it in the Chromatic Coverage commit by passing args manually when I didn't need to. Should be fixed now.

@mdt2 mdt2 force-pushed the mdt2/css-648-stepper-focus-bugs branch from 28804db to e31e7b0 Compare December 14, 2023 16:23
@mdt2 mdt2 force-pushed the mdt2/css-648-stepper-focus-bugs branch from e31e7b0 to 8172ec9 Compare December 15, 2023 16:34
@pfulton pfulton added the run_vrt For use on PRs looking to kick off VRT label Dec 15, 2023
@pfulton pfulton merged commit 8172ec9 into main Dec 15, 2023
@pfulton pfulton deleted the mdt2/css-648-stepper-focus-bugs branch December 15, 2023 21:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@pfulton pfulton pfulton approved these changes

@jawinn jawinn Awaiting requested review from jawinn

@jenndiaz jenndiaz Awaiting requested review from jenndiaz

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.

2 participants

@mdt2 @pfulton