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(dropdown): trigger button should never be disabled #2671

Closed nikkimk closed 3 months ago

nikkimk commented 5 months ago

What I did

  1. Removed disabled from toggle button.

Testing Instructions

  1. In Dropdown docs -> disabled, use a keyboard to navigate to toggle the dropdown.
  2. Verify the menuitems can be read, by a screenreader but are disabled.

Notes to Reviewers

  1. WAI-ARIA recommends elements of a larger composite widget remain focusable.
changeset-bot[bot] commented 5 months ago

šŸ¦‹ Changeset detected

Latest commit: 7894a071846dd8fe44eee7c31842bef7acca8831

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

This PR includes changesets to release 1 package | Name | Type | | -------------------- | ----- | | @patternfly/elements | Minor |

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

netlify[bot] commented 5 months ago

Deploy Preview for patternfly-elements ready!

Name Link
Latest commit 1ee0316361c542c5bc996e8245c8b9d981c93a95
Deploy Preview https://deploy-preview-2671--patternfly-elements.netlify.app/

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

bennypowers commented 5 months ago

http://v4-archive.patternfly.org/v4/components/dropdown#disabled-toggles demonstrates a disabled trigger button, as such we must align with upstream behaviours. if this is an accessibility issue upstream, we should file there

update: @nikkimk received the go ahead from upstream to make this change. thanks for reaching out to them :)

zeroedin commented 3 months ago

If a keyboard user is able to open the menu, then a mouse click should do the same. BOTH shouldn't be able to select a sub item.

nikkimk commented 3 months ago

If a keyboard user is able to open the menu, then a mouse click should do the same. BOTH shouldn't be able to select a sub item.

@hellogreg @zeroedin and I decided that disabled menu should open for both keyboard and mouse but that menu items can't be clicked

nikkimk commented 3 months ago

@bennypowers ptal

hellogreg commented 3 months ago

FYI: looks like the disabled button and menu items are not giving an indication to VoiceOver/Safari or JAWS/Chrome that they are disabled:

disabled button not announcing dimmed in VoiceOver disabled menu item not announcing dimmed in VoiceOver

The menu items do announce "unavailable" to NVDA/Firefox.

bennypowers commented 3 months ago

@nikkimk @hellogreg does d8eaf8d help?

github-actions[bot] commented 3 months ago

"\n## šŸ‘• Commitlint Problems for this PR: \n\nšŸ”Ž found 1 errors, 0 warnings\nā„¹ļø Get help: https://github.com/conventional-changelog/commitlint/#what-is-commitlint\n \n\n

\n802194f5 - fix(dropdown): Update elements/pf-dropdown/pf-dropdown.ts \n\nCo-authored-by: Benny Powers - עם יש×Øאל חי! bennypowers@users.noreply.github.com\n\n
\n\n\n- āŒ subject must not be sentence-case, start-case, pascal-case, upper-case "

nikkimk commented 3 months ago

@bennypowers in the DP's disabled example, the second menu item has a pointer cursor.

hellogreg commented 3 months ago

A little weirdness here with VoiceOver. The first time a menu item receives focus, VO announces, "[text], menu item." But on subsequent focus, it just reads the text.

Action announced as menu item Action announced, but not menu item

This may not be a showstopper. Other menu button examples (e.g., WAI's Navigation Menu Button) don't announce "menu item" with the initial text at all. And VoiceOver does ultimately indicate it is a menu item (as it does in the WAI example):

VoiceOver does indicate the option is a menu item

But the inconsistency might be worth investigating--especially if it causes any side effects. If we're okay with launching it with this, I can open another issue afterward.

Windows and mobile testing results to follow...

hellogreg commented 3 months ago

Windows (Chrome/JAWS and Firefox/NVDA) and mobile (Android and iOS) working as expected.