search

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(select): variant attr, tests #2677

Closed bennypowers closed 4 months ago

bennypowers commented 5 months ago

Closes #2664

What I did

  1. replace boolean attrs with upstream's variant attr
  2. remove case-sensitive and match-anywhere attrs from select and listbox, use a customFilter prop instead.
  3. adjust tests

Notes to reviewers

please carefully review the spec files

changeset-bot[bot] commented 5 months ago

⚠️ No Changeset found

Latest commit: 292236c13bf2fb3fe463357ab6591c88836c0354

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 54b6975a2a941d952ad9ab962f7080f2d790bae3
Deploy Preview https://deploy-preview-2677--patternfly-elements.netlify.app/

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

nikkimk commented 5 months ago

@nikkimk I'm making assumptions that demo/chips.html and demo/multi-select-with-badge.html can now be removed? Their attributes are no longer available. Does that seem correct to you?

Docs and demos should align of upstream docs imho.

zeroedin commented 5 months ago

Docs and demos should align of upstream docs imho.

My mistake I misunderstood what the demos were supposed to show. Compared to PFv4 and have a better understanding now of what should be shown there.

bennypowers commented 5 months ago

Options count for checkbox variant doesn't update on keyboard navigation selection, only on with mouse click.

image

2d39d804

bennypowers commented 5 months ago

@nikkimk If you write out the specs in prose, I can fill out the test file, e.g.

<pf-select> > variant attribute > "single" > calling focus()) > pressing Enter > opens
<pf-select> > variant attribute > "single" > calling focus()) > pressing Enter > pressing ArrowDown > focuses on first option
<pf-select> > variant attribute > "single" > calling focus()) > pressing Enter > pressing ArrowDown > pressing ArrowDown > focuses on second option
<pf-select> > variant attribute > "single" > calling focus()) > pressing Enter > pressing ArrowDown > pressing ArrowDown > pressing Enter > selects it
<pf-select> > variant attribute > "single" > calling focus()) > pressing Enter > pressing Space > closes

but for all the different scenarios

you're also welcome to take a crack at it, just commit whatever you have by eow so i can pick it back up sunday morning :handshake:

nikkimk commented 5 months ago

@bennypowers:

Single

Checkbox

Typeahead

Typeahead multiple

nikkimk commented 5 months ago

@bennypowers per your question this morning for all select variants, when any listbox items is focused:

bennypowers commented 5 months ago
bennypowers commented 5 months ago

@nikkimk not correct in what way?

hellogreg commented 5 months ago

Nikki may be referring to how the variant demos from "Multi-selectable" on down on the docs page are currently all just the default "Single" select. For example, this is how "Checkbox, with badge" looks:

Screencap of what should be the Checkbox select, which is really just a Single select
hellogreg commented 5 months ago

Also the "Accessibility" heading is currently a <p>:

Accessibility section heading text
hellogreg commented 5 months ago

Screen reader issues as of 1/31/24

It looks like Select is working as expected via keyboard in desktop browsers--when no screen reader is enabled. However, there are currently issues among all the major Mac and Win browser and screen reader combos (Mac Safari and VoiceOver, Win Chrome and JAWS, Win Firefox and NVDA).

The following can be tested/confirmed in the Single variant of Select (among others):

GIVEN a closed <pf-select> with keyboard focus

WHEN the user presses space or enter to open the list
  And uses the arrow keys to highlight an option
  And presses space or enter to select an option

THEN the highlighted option should be selected
  And the list should close

Mac Safari and VoiceOver

Right now, with Safari and VoiceOver, I can open the list with space/enter. And I can navigate the options with the up and down arrows.

However, pressing space/enter to select the highlighted option does not select the highlighted option. The first time you open the list and press space/enter, the list closes with the first option selected (e.g., "Blue" in the Single variant demo)--no matter which option you have highlighted. Upon subsequent openings of the list, the space/enter keys do nothing at all. The list can only be closed via tabbing or the escape key.

Win Chrome/JAWS and Firefox/NVDA

Though the list can be opened via space/enter, the lists cannot be navigated via the arrow keys at all.

Hope that test scenario format is the kind of thing you're looking for, @bennypowers !

bennypowers commented 5 months ago

I noticed some issues with the typeahead multi variant, as well. It could be that the firefox-focus-fix ... broke (😉) the behaviour when expanding the menu for typeaheadmulti

bennypowers commented 4 months ago

i've got a couple more changes i'm planning for this: ListboxController will have to receive a secondary keyboard a11y controller (either rtic or activedescendant) in order to function. this will mean small changes to the interface on rtic

bennypowers commented 4 months ago

@nikkimk, @hellogreg, and @zeroedin this is ready for a comprehensive review. Notes:

bennypowers commented 4 months ago

https://deploy-preview-2677--patternfly-elements.netlify.app/components/select/ https://deploy-preview-2677--patternfly-elements.netlify.app/components/chip/

  • Chip group should be hidden if there are not chips, but it can be seen in DP

see f11741cc

bennypowers commented 4 months ago

@nikkimk thanks for pointing that out. This is another one of those head scratchers

On the one hand, pfv4 uses an isPlaceholder flag on option to populate that text, which is perhaps not the greatest API, and indeed OTOH, pfv5 removes the placeholder flag altogether in favour of the text content of a MenuToggle react component which is passed as a prop to select.

soo..... WDYT of a slot/attr pair placeholder which should default to "Select a value"

<pf-select placeholder="בחרו אופציה">
  <pf-option>...</pf-option>
</pf-select>

<pf-select>
  <span slot="placeholder">choisis une option</span>
  <pf-option>...</pf-option>
</pf-select>
nikkimk commented 4 months ago

soo..... WDYT of a slot/attr pair placeholder which should default to "Select a value"

<pf-select placeholder="בחרו אופציה">
  <pf-option>...</pf-option>
</pf-select>

<pf-select>
  <span slot="placeholder">choisis une option</span>
  <pf-option>...</pf-option>
</pf-select>

@bennypowers I like that idea

bennypowers commented 4 months ago

The failures in rh-tabs will be P.G. fixed in a subsequent PR