search

patternfly / patternfly-elements

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

fix(dropdown): slotted trigger #2703

Closed bennypowers closed 3 months ago

bennypowers commented 4 months ago

What I did

  1. added a slot for trigger buttons (already documented)
  2. moved event listeners and aria relationships to a div which now wraps that slot

Testing Instructions

  1. SR usability, ax tree. check the custom-trigger demo

Notes to reviewers

This is required in order to replace the docs version <select> with pf-dropdown

changeset-bot[bot] commented 4 months ago

⚠️ No Changeset found

Latest commit: c58361337afb644efc73e1f6dc642b05cfc94ef2

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

github-actions[bot] commented 4 months ago

Deploy Preview for patternfly-elements ready!

Name Link
Latest commit 580764be2d626ab7539730f9c9552505ff5c5dc1
Deploy Preview https://deploy-preview-2703--patternfly-elements.netlify.app/

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

netlify[bot] commented 4 months ago

Deploy Preview for patternfly-elements ready!

Name Link
Latest commit c58361337afb644efc73e1f6dc642b05cfc94ef2
Latest deploy log https://app.netlify.com/sites/patternfly-elements/deploys/65fc34ddeca5ef000810736b
Deploy Preview https://deploy-preview-2703--patternfly-elements.netlify.app
Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

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

bennypowers commented 3 months ago

@hellogreg ptal at https://deploy-preview-2703--patternfly-elements.netlify.app/components/dropdown/demo/custom-trigger/

hellogreg commented 3 months ago

Looks like the button and list may be getting misaligned upon expansion in Chrome and Safari (Firefox is okay):

dropdown alignment issue
hellogreg commented 3 months ago

With VoiceOver/Safari on Mac, the button is not currently announcing an expanded or collapsed state upon focus.

bennypowers commented 3 months ago

@hellogreg PTAL, thanks

hellogreg commented 3 months ago

Looks like the aria-expanded attribute on <pf-button> isn't being applied to the actual button--and thus the state is not getting announced by screen readers:

accessibility tree showing no aria expanded state on button
hellogreg commented 3 months ago

WIth the Windows Firefox/NVDA browser and screen reader combo, the screen reader stays in browse/read mode when focus is on the button. The user can manually activate focus mode to use the button, but it should auto-detect.

This isn't an issue with any other browser/reader combo. (Typically, this issue crops up for more than one of them at a time, so this is a little unusual.)

bennypowers commented 3 months ago

i think we'll need to remove the native <button> from pf-button's shadow root, and make the pf-button host a role: button with element internals. this could affect chip, select, and others.

nikkimk commented 3 months ago

i think we'll need to remove the native <button> from pf-button's shadow root, and make the pf-button host a role: button with element internals. this could affect chip, select, and others.

Yep. Until we have a cross-root ARIA solution, this is probably a necessary evil.

bennypowers commented 3 months ago

blocked by https://github.com/patternfly/patternfly-elements/issues/2706

bennypowers commented 3 months ago

@hellogreg PTAL

hellogreg commented 3 months ago

Visibility issue

There may be a CSS issue right now where the dropdown button isn't displaying. However, I am still able to place keyboard focus on it and activate it for testing: invisible dropdown button

Mac Safari/VoiceOver, Win Firefox/NVDA, Win Chrome/JAWS, Android Chome/Talkback

Dropdown works well with these combos. I do have one question. Do you want/need to have a default designated focus destination when a menu item is activated and the dropdown closes? After pressing esc, focus returns to the button, as I'd expect. But when activating a menu item, visible focus disappears. (It seems like it's then being set back to the beginning of the page, maybe.)

iOS Safari/VoiceOver

Works as expected--and focus returns to the button by default upon menu item activation.

bennypowers commented 3 months ago

Visibility issue

Please see #2710

Mac Safari/VoiceOver, Win Firefox/NVDA, Win Chrome/JAWS, Android Chome/Talkback

Dropdown works well with these combos. I do have one question. Do you want/need to have a default designated focus destination when a menu item is activated and the dropdown closes? After pressing esc, focus returns to the button, as I'd expect. But when activating a menu item, visible focus disappears. (It seems like it's then being set back to the beginning of the page, maybe.)

@nikkimk wdyt?

bennypowers commented 3 months ago

I'm going to merge this now, and if @nikkimk you want to adjust the default focus behaviour that @hellogreg mentioned above, we can follow up later or do a patch release