patternfly / patternfly-elements

PatternFly Elements. A set of community-created web components based on PatternFly design.
https://patternflyelements.org/
MIT License
375 stars 89 forks source link

fix(chip): add toolbar role #2675

Closed bennypowers closed 5 months ago

bennypowers commented 5 months ago

Closes #2666

What I did

  1. added toolbar role to shadow container for chip group
  2. some nitpicky formatting
changeset-bot[bot] commented 5 months ago

⚠️ No Changeset found

Latest commit: 02e20e9756860816e18097cd10bdd388d3b812b5

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

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

netlify[bot] commented 5 months ago

Deploy Preview for patternfly-elements ready!

Name Link
Latest commit 9c8aac11f22f2092042c96162489e7a650fc3e24
Deploy Preview https://deploy-preview-2675--patternfly-elements.netlify.app/

To edit notification comments on pull requests, go to your Netlify site settings.

hellogreg commented 5 months ago

Running some tests in the preferred desktop browser screen reader combos...

Windows Chrome and JAWS: All good

Mac Safari and VoiceOver: All good

Windows Firefox and NVDA: Looks like there's a keyboard navigation issue in Windows and Mac Firefox, even before trying with a screen reader. This issue is also present in the current live version of chip, so I can make a separate issue for it this afternoon. (Basically, if you tab to a chip group and then try using the arrows, you can't navigate between the regular chips in that group. However, if you hit the arrow key enough times, you will navigate to the "x more / see less" chip.)

That said, everything regarding chip that is keyboard navigable in FF looks to be working well with NVDA.

So, I guess what I'm saying is this issue looks good to me, but I can't really test Firefox fully, because keyboard nav isn't totally working.

hellogreg commented 5 months ago

Added the Firefox issue: #2682