Skip to content

fix(dropdown): trigger button should never be disabled #2671

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
bennypowers merged 32 commits into from
Mar 29, 2024
Merged

fix(dropdown): trigger button should never be disabled #2671

bennypowers merged 32 commits into from
Mar 29, 2024

Conversation

@nikkimk
Copy link
Contributor

@nikkimk nikkimk commented Jan 19, 2024

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.

@nikkimk nikkimk linked an issue Jan 19, 2024 that may be closed by this pull request
dropdown: disabled dropdown should still be focusable and reveal options #2669
Closed
@changeset-bot
Copy link

changeset-bot bot commented Jan 19, 2024 •
edited
Loading

🦋 Changeset detected

Latest commit: 7894a07

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

@github-actions github-actions bot added the functionality Functionality, typically pertaining to the JavaScript. label Jan 19, 2024
@netlify
Copy link

netlify bot commented Jan 19, 2024 •
edited by github-actions bot
Loading

✅ Deploy Preview for patternfly-elements ready!

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

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

@github-actions github-actions bot added the AT passed Automated testing has passed label Jan 19, 2024
@bennypowers
Copy link
Member

bennypowers commented Jan 20, 2024 •
edited
Loading

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 :)

bennypowers
bennypowers requested changes Jan 22, 2024
View reviewed changes
Copy link
Member

@bennypowers bennypowers left a comment

Choose a reason for hiding this comment

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

please add unit tests

@github-actions github-actions bot removed the functionality Functionality, typically pertaining to the JavaScript. label Mar 21, 2024
@nikkimk nikkimk marked this pull request as draft March 21, 2024 16:01
@github-actions github-actions bot added work in progress POC / Not ready for review functionality Functionality, typically pertaining to the JavaScript. styles An issue or PR pertaining only to CSS/Sass labels Mar 21, 2024
@github-actions github-actions bot added the tests Related to testing label Mar 21, 2024
@nikkimk nikkimk marked this pull request as ready for review March 21, 2024 18:45
@zeroedin
Copy link
Contributor

zeroedin commented Mar 21, 2024

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 nikkimk added the blocked Waiting on some other team or process label Mar 21, 2024
zeroedin
zeroedin approved these changes Mar 21, 2024
View reviewed changes
Copy link
Contributor

@zeroedin zeroedin left a comment •
edited
Loading

Choose a reason for hiding this comment

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

(oops approved wrong PR)

@nikkimk nikkimk requested a review from zeroedin March 21, 2024 19:19
@nikkimk
Copy link
Contributor Author

nikkimk commented Mar 21, 2024

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

bennypowers
bennypowers requested changes Mar 28, 2024
View reviewed changes
@github-actions github-actions bot added the demo Updating demo pages label Mar 28, 2024
hellogreg
hellogreg approved these changes Mar 28, 2024
View reviewed changes
Copy link
Contributor

@hellogreg hellogreg left a comment

Choose a reason for hiding this comment

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

Passed the usual browser and screen reader tests this afternoon.

Lighted Grid, Tron Motorbike.

bennypowers
bennypowers requested changes Mar 29, 2024
View reviewed changes
Copy link
Member

@bennypowers bennypowers left a comment

Choose a reason for hiding this comment

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

Oof GitHub left all these comments pending until now. 😐 Sorry

bennypowers and others added 3 commits March 29, 2024 05:08
@bennypowers
Merge branch 'main' into dropdown-disabled
2be437d
@bennypowers
fix(button): show slotted icon
c1d7ecf
@bennypowers
fix(dropdown)!: change trigger slot to toggle, etc
6caa845 
- move disabled context to pf-dropdown element
- use pfv4 css tokens
- correct toggle styling for variants, in particular the layout
@github-actions github-actions bot added the doc label Mar 29, 2024
@nikkimk
Copy link
Contributor Author

nikkimk commented Mar 29, 2024

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

@hellogreg
Copy link
Contributor

hellogreg commented Mar 29, 2024 •
edited
Loading

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
Copy link
Contributor

hellogreg commented Mar 29, 2024

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

@bennypowers bennypowers merged commit 8e52f62 into main Mar 29, 2024
@bennypowers bennypowers deleted the dropdown-disabled branch March 29, 2024 14:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@bennypowers bennypowers bennypowers approved these changes

@zeroedin zeroedin Awaiting requested review from zeroedin

+1 more reviewer

@hellogreg hellogreg hellogreg approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@nikkimk nikkimk

Labels

AT passed Automated testing has passed demo Updating demo pages doc functionality Functionality, typically pertaining to the JavaScript. ready to merge styles An issue or PR pertaining only to CSS/Sass tests Related to testing tools Development and build tools

Projects

Workflow
Status: Done

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

dropdown: disabled dropdown should still be focusable and reveal options

5 participants

@nikkimk @bennypowers @zeroedin @hellogreg