feat(Select): add typeahead variants (single and multi)

[kmcfaul]

…, updated tests

What: adding typeahead single and multi variant

See issue: #1215, #1216

[patternfly-build]

PatternFly-React preview: https://1839-pr-patternfly-react-patternfly.surge.sh

[codecov-io]

Codecov Report

❗ No coverage uploaded for pull request base (master@0974e51). Click here to learn what that means.
The diff coverage is 58.82%.

@@            Coverage Diff            @@
##             master    #1839   +/-   ##
=========================================
  Coverage          ?   82.59%           
=========================================
  Files             ?      621           
  Lines             ?     6844           
  Branches          ?       93           
=========================================
  Hits              ?     5653           
  Misses            ?     1151           
  Partials          ?       40

Flag	Coverage Δ
#patternfly3	84.87% <ø> (?)
#patternfly4	79.29% <58.82%> (?)
#patternflymisc	95.68% <ø> (?)

Impacted Files	Coverage Δ
...4/react-core/src/components/Select/SelectOption.js	60% <ø> (ø)
...eact-core/src/components/Select/selectConstants.js	100% <ø> (ø)
...4/react-core/src/components/Select/SingleSelect.js	80% <100%> (ø)
...4/react-core/src/components/Select/SelectToggle.js	54.54% <50%> (ø)
...rnfly-4/react-core/src/components/Select/Select.js	73.52% <61.9%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0974e51...3a4c218. Read the comment docs.

[mcarrano]

This is looking good @kmcfaul The only question I have is about desired keyboard interaction. So right now, if I start typing to narrow the list and then press Enter, it acts as an escape. @jgiardino can you check this? What would you expect?

[mcoker]

In the typeahead version of the select dropdown, we wrap the toggle icon on the right in a button.pf-c-button.pf-c-select__toggle-button since the parent element is a <div>. That's what's creating the extra space there.

[jgiardino]

The behavior we have in the PF3 TypeAhead Select works really well, and matches what I would expect. I suggest reviewing that component and how it behaves. One behavior to check out is how the arrow keys are able to change the selected value while focus remains in the input field. The ability to switch between arrow keys and typing is pretty seamless.

The way the PF3 component handles the menu keyboard interaction is different in that they use aria-activedescendant to manage focus instead of tabindex, which is what we have with the original Select variants. Both methods are valid, but the aria-activedescendant method lets you keep focus on the input, but still use the arrow keys to change the selection, which is more appropriate for this case. WAI-ARIA describes this method here: https://www.w3.org/TR/wai-aria-practices-1.1/#kbd_focus_activedescendant.

Let me know if you have any questions about this.

[kmcfaul]

Added multi typeahead variant
Added missing button wrapper in toggle to typeahead variants

[mcoker]

very nice!

[jgiardino]

Per discussion with @dgutride, I captured my previous comment in this issue: #1842

[kmcfaul]

Regarding the typeahead logic, the code is currently converting the text input to a regex to filter options. Special characters are ignored as they break the regex and would crash the page.

[tlabaj]

Looks great. A few small things

[jgiardino]

Great job on pulling the multi-select typeahead together!

Now that keyboard and screen reader stuff is off the table, here are my other comments :-)

• There are some differences between the core and react implementations. In this implementation, I see a lot more in this div that what we have in core:
  <div aria-labelledby="title-id pf-toggle-id-128" id="pf-toggle-id-128" class="pf-c-select__toggle pf-m-typeahead" aria-expanded="false" aria-haspopup="listbox">
  This is what we have in core (i.e. just the class names):
  <div class="pf-c-select__toggle pf-m-typeahead">

• The following is the input element in the Typeahead select input example:
  <input class="pf-c-form-control pf-c-select__toggle-typeahead" id="select-single-typeahead-typeahead" aria-label="Type to filter" placeholder="Select a state" type="text" value="">
  Is there a prop available to let the user provide a text string for aria-label? This string works for a default value, but we need to allow consumers to specify their own value.

  • For a prop name, does ariaLabelTypeAhead (fyi, as part of fix(aria): make aria-* component props consistent #1731, we came to some conclusions about how to handle prop names for aria-label attributes, so in this case it's ariaLabel + name of component feature, like Toggle, Search, Menu, etc...)

• I get different behaviors for button.pf-c-select__toggle-clear between click and Enter keys. These events should always result in the same action (unless there's some edge case I'm not thinking of). The button works as expected on click, but when the button has focus and I hit Enter, it opens the menu instead of clearing the value.

  • I'm also seeing that this button is included in the react implementation for single select typeahead, but isn't included in the Core example. Is that intentional? Is it missing in core, or should it not be included here? (cc'ing @mcarrano on this one)

• Can you update the text strings that are defined in both examples to use the same string for the hidden span, the aria-label, and the placeholder text, like I show below? I think this is helpful in illustrating what the purpose of these different elements/attributes are (i.e. that they should be communicating the same info).

      <div>
        <span id={titleId} hidden>
          Select a state
        </span>
        <Select
          variant={SelectVariant.typeahead}
          aria-label="Select a state"
          onToggle={this.onToggle}
          onSelect={this.onSelect}
          onClear={this.clearSelection}
          selections={selected}
          isExpanded={isExpanded}
          ariaLabelledBy={titleId}
          placeholderText="Select a state"
        >
  

[mcarrano]

Regarding the multi-select version, there are a few issues here with the keyboard and mouse interactions. I will leave the keyboard-only issues to the side for now, but these are the things I see.

• As I type, the list does open and filter, but after I select an item, the text string I type still remains. This is unexpected.
• If I click back into the field after I select the first item, the drop-down closes. After more testing, it looks like any time I click in the field it also toggles the state of the list. I don't expect that. My initial click into the text field should open the list. Selecting an item should close the list. Putting focus back into the field should open the list again.
• After I select the second item, I'm getting a badge that says "1 more" even though there is plenty of room to shown both selections. The "n more" badge was only intended as an overflow when there is not enough space to show all selections.

The PatternFly 3 type-ahead works well. If we can model after that I think we will be in good shape.

[kmcfaul]

@mcarrano

1. Change input to clear on selection.
2. Toggling on click is now prevented for type ahead variants. Clicking will only open the menu, and clicking outside the menu, selecting an item (for single), or pressing escape will close the menu.
3. As far as I can tell from the code, the overflow of ChipGroup is hardcoded to happen at 2+ chips. It's also possible the large margin of the clear all selections icon is making the div fill up early effectively.

[kmcfaul]

@jgiardino

1. The extra attributes on the toggle label come from the toggle class that is reused by all variants. If there's something specific that shouldn't be on the tag for typeahead let me know.

2. Added the areaLabelTypeAhead parameter that is passed to the aria label here.

3. Removed the toggle's open on enter for type ahead variants. The clear selection button should behave as expected now.

4. Updated the examples with your suggestions.

[jgiardino]

Thanks for making updates! I included some more comments below, but if any of these seem like updates for a later PR, just let me know.

The extra attributes on the toggle label come from the toggle class that is reused by all variants. If there's something specific that shouldn't be on the tag for typeahead let me know.

The following are the differences in how attributes are defined between these implementations and core for the single select. I'm assuming the multi-select would need similar updates, but let me know if you have questions about that one.

react	core	diff
<div aria-labelledby="title-id pf-toggle-id-12" id="pf-toggle-id-12" class="pf-c-select__toggle pf-m-typeahead" aria-expanded="false" aria-haspopup="listbox">	<div class="pf-c-select__toggle pf-m-typeahead">	aria-labelledby, id, aria-expanded, aria-haspopup should not be included on this element
<button class="pf-c-button pf-c-select__toggle-button">	<button class="pf-c-button pf-c-select__toggle-button" id="select-single-typeahead-expanded-toggle" aria-haspopup="true" aria-expanded="true" aria-labelledby="select-single-typeahead-expanded-label select-single-typeahead-expanded-toggle" aria-label="Select">	aria-labelledby, id, aria-expanded, aria-haspopup should be included on this element

Added the areaLabelTypeAhead parameter that is passed to the aria label here.

Awesome! There are a couple of other text strings that we should expose via props if they aren't already. They are on these elements:

• <button class="pf-c-button pf-m-plain pf-c-select__toggle-clear" aria-label="Clear all">
  • Does ariaLabelClear work?
• <button class="pf-c-button pf-c-select__toggle-button" id="select-single-typeahead-expanded-toggle" aria-haspopup="true" aria-expanded="true" aria-labelledby="select-single-typeahead-expanded-label select-single-typeahead-expanded-toggle" aria-label="Select">
  • I'd use ariaLabelToggle for this one.
• <li class="pf-c-chip"> ... <button class="pf-c-button pf-m-plain" aria-label="Remove" ...> in the multiselect
  • ariaLabelRemove probably works
  • Also, in core, this value is set to "Remove", but the default string "close" that's defined for the Chip is currently being used.

One more thing I notice that is really minor... The new examples have the same id values for the hidden span, which results in duplicate ids on the page:
const titleId = 'title-id';
But other examples provide something more specific, like:
const titleId = 'grouped-checkbox-select-id';

[mcarrano]

This is looking much better @kmcfaul ! Just a couple of additional questions/comments:

Toggling on click is now prevented for type ahead variants. Clicking will only open the menu, and clicking outside the menu, selecting an item (for single), or pressing escape will close the menu.

Thanks for making this change. the interaction is much better. Seems like the user should also be able to click the toggle (down arrow) itself to close the menu.

As far as I can tell from the code, the overflow of ChipGroup is hardcoded to happen at 2+ chips. It's also possible the large margin of the clear all selections icon is making the div fill up early effectively.

Where is this hardcoded? In the ChipGroup component? That may be a problem as that's not what I think was intended.

It would be great if we could have a string like "No matches found" display in the dropdown when there is a null query result.

[kmcfaul]

@jgiardino

• Added a control to change where the aria attributes are assigned based on variant. They should remain the same for normal/checkbox select and for typeahead variants the attributes will appear on the toggle button.
• Added the mentioned aria attributes as parameters
• Updated the titleIds on the examples

@mcarrano

• The carat button should now behave as a toggle, opening and closing the menu
• Added a 'No results found' message when the filtering returns nothing

[mcarrano]

Approving this, and have created #1855 to address issues with ChipGroup that were raised here.

[jgiardino]

Your updates look great! Here are a few more minor things I noticed. Let me know if you have any concerns about getting these addressed in time.

1. The ariaLabelToggle should only be used for the type-ahead select variants and is not recommended for other variants because they will have text already which will conflict with the aria-label value. I'm not sure if that's easy to address or not.

   1. I'd at least update the description from "Label for toggle of select variants" to "Label for toggle of type ahead select variants" to discourage consumers from defining that for other variants.

2. In the options menu for similar situations (i.e. a button with an icon instead of text), we use "Options menu" as the default value for aria-label unless it is specified by the user. Can we do the same here? I.e. if ariaLabelToggle is not defined, automatically set aria-label to "Options menu" on the .pf-c-select__toggle-button button?

   1. Ideally, we should have a default string, but if not possible in this PR, should we at least add the prop to the example?

3. When ariaLabelRemove is not defined in the multi-typeahead example, there isn't a default string defined for aria-label on the chip close buttons (even the default that I see on the Chip component isn't showing up)

4. The ariaLabelClear prop should behave similar to what I describe above—when defined, its value is used as the aria-label value, but when not defined we should still have aria-label with a default value. I'm seeing slightly different behaviors between the two typeahead variants:

   1. This prop seems to have no effect on the multi-select typeahead variant. The text string is always "Clear all" regardless of whether this prop is defined or not.
   2. If this prop is defined on the single-select typeahead, it works. But if I remove this prop, then there is no default text string that displays.
   3. In both of these examples, I would recommend not defining this prop (or ariaLabelRemove), and just have the default text strings used.

5. This might be completely unrelated to this PR... If I remove the prop ariaLabelledBy from the Select component, it will result in this value for the attribute:
   aria-labelledby="null pf-toggle-id-60"
   But instead I would expect that attribute to just not be included (i.e. an element is already labeled by itself, so having aria-labelledby reference only itself is unnecessary).

[tlabaj]

Nice Job. Look great!

[mcoker]

👍

[jgiardino]

Great job on making the updates! All the props work as expected! There are just a few minor text string updates needed to match core:

3. For multi-select, in core, the value for aria-label on li.pf-c-chip>button is set to "Remove", but in the latest updates the default string is set as "Remove selection". This should match core.

4. For both variants, in core, the value for aria-label on pf-c-select__toggle-clear is "Clear all", but in the latest updates the default string is set as "Clear all selections". This should match core.

   • Additionally, the prop ariaLabelClear is defined in the example as "Clear all", which is the correct string, but in these examples we can leave the prop off and just use the default string.

[tlabaj]

LGTM

[dlabaj]

Great Job!

[jgiardino]

Great job on this!