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(select): placeholder button text #2740

Closed bennypowers closed 2 months ago

bennypowers commented 3 months ago

What I did

  1. ensure placeholder value is used for button label unless there is a selected item

Testing Instructions

  1. add placeholder attr
  2. see that it is used as button text
  3. select an item
  4. see that the item's value is used as button text

Notes to Reviewers

  1. is the use of the placeholder as a disabled item correct?
changeset-bot[bot] commented 3 months ago

🦋 Changeset detected

Latest commit: 9f449904ae31465fab8fb92593805e48bc963601

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 | Patch |

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 3 months ago

Deploy Preview for patternfly-elements ready!

Name Link
Latest commit c7bc562e89c93254f38949876c82073f3b0fa608
Deploy Preview https://deploy-preview-2740--patternfly-elements.netlify.app/

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

bennypowers commented 3 months ago

@nikkimk @adamjohnson is this better? Now, the button will have text content from either the selected item or the placeholder, be described by the placeholder, and be labelled by the input (in the case of typeahead) or directly with the computed label text (based on FACE labels).

these changes cause an axe failure on the button, which you can see on the actions page, so i went back to using the placeholder in certain circumstances

When reviewing, maybe forget about the usage of the word "placeholder", which isn't exactly the same thing here as <input placeholder>.

hellogreg commented 2 months ago

We shouldn't use values from the options to label the entire select list--especially considering those values can change.

For example, if we have a select menu for choosing a system's status, its label should always be something like "Current system status," and not "Running". Here's a screencap from our docs:

Select list with the value Running selected

For an example of what we could do, the WAI Select-Only Combobox Example uses aria-labelledby to point to the text just preceding the select list to label it. We could do something like this, even if that text is placed offscreen.

bennypowers commented 2 months ago

@nikkimk 's notes from chat

  1. what text must appear in the combobutton before the user opens the select menu? If an option is pre-sepected, that option, if not but there is placeholder, the placeholder, otherwise, the first option. Note: What appears in the box is NOT a label.
  2. what text must be announced when sr focuses on the combobox? First a label and then the text in the box as the selected value. Note that text is different from a label. The text should not be labelled as the combobox's label.
  3. how many focusable items must appear in the select list - is there a fourth unselectable item? If there is a placeholder, it is an unselectable item that is only read when the select has not value. The placeholder text can also serve as aria-describedby for the combobox.
  4. when the users selects an item, what effect must that have on the answers to questions 1 and 2? What appears in the comboxbox is the selected item. The screenreader should read a label (which is distinct from an option/placeholder) followed by the current value as the text.
  5. pf-select is a FACE with no role. I can
hellogreg commented 2 months ago

Only thing I noticed with screen readers is that the placeholder text is always read when the button receives focus, even when another value in the list is currently selected. (If the placeholder is currently selected, it is read twice.)

PFE demo screencap

For example, when the above button receives focus after I've selected "Orange" (which is not the placeholder value), NVDA announces Color, combobox, Orange, collapsed, opens list, Select a value on focus:

NVDA screencap

So you get the label, role, selected value, expanded state, button action, and placeholder value.

WAI combobox screencap

By comparison, the above WAI Combobox announces Favorite Fruit, combobox, Blueberry, collapsed, opens list. In other words, everything but the placeholder value from our demo.

Other screen readers act similarly. VoiceOver on the Mac actually does something especially odd. It trims the last letter of the placeholder (e.g., "Select a valu" without the "e" at the end).

bennypowers commented 2 months ago

I think we can add an aria hidden on the placeholder text whenever there's a selected value

bennypowers commented 2 months ago

@hellogreg 73014286 addresses this, you can test in at https://patternflyelements.org/elements/select/demo/

hellogreg commented 2 months ago

Thanks, @bennypowers . The current version has this odd flickering (video link) when triggering the button via either mouse or keyboard. You also then can't collapse the list, except by choosing an option.