Skip to content
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.

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

feat: listbox elements #2570

Merged
merged 405 commits into from
Jan 19, 2024
Merged

feat: listbox elements #2570

merged 405 commits into from
Jan 19, 2024

Conversation

bennypowers
Copy link
Member

@bennypowers bennypowers commented Aug 1, 2023

This is a staging branch for dropdown, menu, and select.

What I did

  1. Added directional support to roving-tabindex-controller and fixed an issue with the controller stealing focus when items were updated.
  2. Added a listbox-controller.
  3. Added a toggle-controller.
  4. Added listbox components in order to test the support and demo how we can create an accessible listbox component: pf-select, pf-select-listbox, pf-select-group, and pf-select-option.
  5. Added chip components pf-chip and pf-chip-group.
  6. Added pf-dropdown, pf-dropdown-menu, pf-dropdown-group, and pf-dropdown-item.
  7. Updated popover API to be consistent with other components. Deprecated close-label in favore of accessible-close-label.

Testing Instructions

View the following docs, and check out all the examples:

  • Accordion - test keyboard nav for breaking changes
  • Chip - test new component
  • Dropdown - test new component
  • Jump links - test keyboard nav for breaking changes
  • Select - test new component
  • Popover - test for close-label deprecation
  • Tabs - test keyboard nav for breaking changes

Notes to Reviewers

roving-tabindex-controller

The controller's updateItems function was stealing focus every time it was called. This function was meant to update the controller's list of focusable items as items were added or removed from the host. Since not all updates require focus, the controller will now only re-set focus if the controller had focus in the first place.

Upstream Patternfly

Due to the number of accessibility improvements to Patternfly's current release (v5), these components are based on their v5 counterparts instead of earlier versions. There are also additional changes to improve accessibility in these components.

Chip

Upstream Patternfly's Select has options that use upstream Patternfly's Chip so I had to also do 1-1 for pf-chip in order to provide those options in pf-select.

Dropdown

See the following references for components with role="menu":

  1. Toggle and Items Accessibility
  2. Web component benchmarking
  3. WAI ARIA Menu Button Pattern
  4. WAI ARIA Actions Menu Button Example Using aria-activedescendant
  5. Upstream Patternfly's Dropdown

Select

Components with role=listbox can be used as standalone elements, as part of a combobox. See the following references:

  1. Toggle and Items Accessibility
  2. Web component benchmarking
  3. WAI ARIA Listbox Pattern
  4. WAI ARIA Example Listbox Grouped
  5. WAI ARIA Combobox Pattern
  6. Upstream Patternfly's Select

@changeset-bot
Copy link

changeset-bot bot commented Aug 1, 2023

🦋 Changeset detected

Latest commit: bd10aed

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

This PR includes changesets to release 2 packages
Name Type
@patternfly/pfe-core Major
@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 work in progress POC / Not ready for review label Aug 1, 2023
@github-actions github-actions bot added the AT passed Automated testing has passed label Aug 1, 2023
@github-actions
Copy link
Contributor

github-actions bot commented Aug 1, 2023

Deploy Preview for patternfly-elements ready!

Name Link
🔨 Latest commit 9bd964b
😎 Deploy Preview https://deploy-preview-2570--patternfly-elements.netlify.app/

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

Copy link
Member Author

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

Do not merge until ready

.changeset/rh-listbox-elements.md Outdated Show resolved Hide resolved
@bennypowers bennypowers changed the title docs: wip changeset feat: listbox elements Aug 2, 2023
@github-actions github-actions bot added demo Updating demo pages doc functionality Functionality, typically pertaining to the JavaScript. styles An issue or PR pertaining only to CSS/Sass tests Related to testing labels Aug 10, 2023
@netlify
Copy link

netlify bot commented Aug 16, 2023

Deploy Preview for patternfly-elements ready!

Name Link
🔨 Latest commit bd10aed
🔍 Latest deploy log https://app.netlify.com/sites/patternfly-elements/deploys/65aa289d49b1ef00089be77e
😎 Deploy Preview https://deploy-preview-2570--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.

@hellogreg
Copy link
Collaborator

hellogreg commented Aug 17, 2023

@nikkimk Chip is looking good! Here are a couple notes--the first of which we chatted about:

  1. The close button target size needs to be at least 24px by 24px (per WCAG 2.5.8), and I think our preferred dimensions are at least 44px by 44px. Currently, looks like it's 24px wide by 18px high. You had also mentioned the possibility of making the entire chip a button target.

  2. In Safari (Mac and Mobile) and Firefox, the "Really long chip..." wraps and displays all content.

This screencap demonstrates both issues:
"Really long chip..." screenshot

  1. Also noticed that the tooltip persists after the chip is closed:
    screencap of the lingering tooltip

@bennypowers
Copy link
Member Author

what a boost to come back from vacation and see such a ferment of activity in this branch. looking forward to diving in to the details

@nikkimk
Copy link
Collaborator

nikkimk commented Aug 25, 2023

@hellogreg ptal on chip. Also feel free to look at all of select.

@hellogreg
Copy link
Collaborator

@hellogreg ptal on chip.

The issues look fixed with chip! Two questions:

  1. Where do you want keyboard focus to move (if anywhere) upon dismissal of the chip?
  2. Are you okay with screen readers only reading "close button" when a dismissable chip receives focus (because that's the interactive content), or do you want them to read the chip's text content, too? As it is now, the default and max-width chips just read "close button" upon focus, and the overflow chip reads the entire chip's text content. Just making sure it's working as you intended.

@hellogreg
Copy link
Collaborator

Also feel free to look at all of select.

At the demo page, I'm unable to navigate options with the arrow keys (Mac Chrome/Safari/Firefox). I can expand and collapse the list, but the arrows just scroll the page, as shown below:

https://drive.google.com/file/d/1La1Quwv-SAcmxCh1HEbhdnkJ9VRav5Mo/view?usp=drive_link

In multiselect and typeahead, I can click in the list with the mouse and then use the arrows, with mixed results (only lets me travel between a few options).

@nikkimk
Copy link
Collaborator

nikkimk commented Aug 29, 2023

@hellogreg this should be fixed now.

I had added logic for "create option", but it kept adding options that reset roving tabindex. I just pushed an update that will prevent this.

@hellogreg
Copy link
Collaborator

@hellogreg this should be fixed now.

I can navigate the Select options with all the arrows now (cool!), but I have to tab into them first. I can't just open the list and start using arrows (nor can I open the list with the down arrow, if you want users to have that option).

In other words, here's what I expect (assuming we're going for keyboard behavior like the WAI Select-only pattern):

  1. Place focus on the closed pf-select element.
  2. Press space, return, or down arrow to open.
  3. Use arrow keys to navigate list.

And here's what I have to do:

  1. Place focus on the closed pf-select element.
  2. Press space or return (but not down arrow) to open.
  3. Tab into list.
  4. Use arrow keys to navigate list.

@nikkimk
Copy link
Collaborator

nikkimk commented Aug 30, 2023

@hellogreg thanks much for the feedback. I just uploaded a fix for that. Check it out.

@hellogreg
Copy link
Collaborator

@hellogreg thanks much for the feedback. I just uploaded a fix for that. Check it out.

Nice. Arrows are working. One more thing (because there's always one more thing): after a selection is made with arrows, pressing space or return doesn't currently set the value, close the box, and return focus to the closed box.

@nikkimk
Copy link
Collaborator

nikkimk commented Aug 31, 2023

@hellogreg tag: you're it. I think I fixed it.

@hellogreg
Copy link
Collaborator

@hellogreg tag: you're it. I think I fixed it.

Confirmed that Single, Option, and Grouped option variants all work via keyboard!

Should the Multi-select, Typeahead, and Filtering variants be ready for testing, too? I was still having some issues selecting or typing in those. We can chat about them (and I can demo) when we meet later.

Getting close.

@nikkimk
Copy link
Collaborator

nikkimk commented Aug 31, 2023

@hellogreg try it now

@hellogreg
Copy link
Collaborator

hellogreg commented Sep 1, 2023

@hellogreg try it now

Thanks! Basically, everything does now work: items in each variant can be successfully selected! Here are the (smaller) keyboard-only issues I've noticed.

  1. For the basic single, grouped single, and filter variants, choosing an option by pressing space does select that option and collapse the box, but also scrolls the page and removes focus from the box. Pressing return selects the option, closes the box and removes focus. (The WAI pattern retains focus on the box)

  2. Typeahead with create allows the user to create the same option multiple times.
    A typeahead screencap with Blue created twice

  3. Typeahead with create has this behavior (not sure if by design, but is different from WAI):

  • Type Green. It will show in the options.
  • Press the down arrow to focus on that option.
  • Select it with return/space. The box closes and receives focus.
  • Beginning to type another option (e.g., Blue) does not re-open the box.
  1. Typeahead with checkbox moves focus to the top of the box when selecting items with space or return (rather than keeping focus on the selected item). However, deselecting items does keep focus on the deselected item.

@bennypowers bennypowers enabled auto-merge (rebase) January 18, 2024 18:40
auto-merge was automatically disabled January 19, 2024 07:46

Rebase failed

@bennypowers bennypowers merged commit 0d92145 into main Jan 19, 2024
14 checks passed
@bennypowers bennypowers deleted the menu-dropdown-listbox branch January 19, 2024 07:47
@nikkimk nikkimk restored the menu-dropdown-listbox branch January 19, 2024 13:29
@bennypowers bennypowers deleted the menu-dropdown-listbox branch June 25, 2024 07:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
AT passed Automated testing has passed demo Updating demo pages doc functionality Functionality, typically pertaining to the JavaScript. pfe 2.5 For PFE version 2.5 styles An issue or PR pertaining only to CSS/Sass tests Related to testing tools Development and build tools work in progress POC / Not ready for review
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

[1:1]: pf-select [1:1] pf-dropdown
5 participants