feat(ContextSelector): Add PF4 ContextSelector Component

[tlabaj]

#1293, #1525

What:

Additional issues:

[patternfly-build]

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

[jgiardino]

One thing I noticed is that during a search, if I enter a lowercase "a", it returns nothing. Should the search be case-insensitive for this component?

The component isn't keyboard accessible at the moment. For the first implementation, I think the simplest solution is to handle the panel similar to how we handle the popover or modal with regard to trapping and managing focus.

• Given the toggle has focus, when the user hits Enter (or the user clicks the toggle), then:
  • The panel .pf-c-context-selector__menu traps focus - This behavior was added to the modal in feat(PF4-Modal): Adds keyboard and screen reader focus trapping #1011. I'm not sure if the popover uses a similar or different method, and if so, which one would make the most sense for the dropdown. The popover might make the most sense given that it also requires the ability to handle positioning relative to the toggle.
  • The first element in the panel that can receive focus has focus. In this case it would be the search field.
• Given the panel is open,
  • When the user hits Tab, then the next element that can receive focus gets focus (and Shift-Tab goes to the previous item that can receive focus)
  • And given the last item has focus, when the user hits Tab...?
    • In the modal or popover, we would shift focus to the first element that can receive focus. I think for now, that might be the simplest thing to do here. We can always refine this behavior once we have something that works across all experiences (mouse, keyboard, screen reader), but I would recommend handling this as a separate PR given the amount of effort involved.

[tlabaj]

@jgiardino the application implements the search functionality. I can change the example to be case insensitive if you would like.

Not sure what I changed. I added the focus trap similar to popover like we talked about... I will have to investigate and figure out what is going on.
Thanks for feedback.

[jgiardino]

Thanks, @tlabaj!

@jgiardino the application implements the search functionality. I can change the example to be case insensitive if you would like.

I'm not sure what the general expectation is here. That makes sense that we would be more hands-off and rely on consumers to handle that functionality. But I could also see the case where consumers would want to see an example that shows how we would recommend handling that functionality. 🤷‍♀️ I will totally defer to anyone who has a strong opinion about that.

Not sure what I changed. I added the focus trap similar to popover like we talked about... I will have to investigate and figure out what is going on.

One thing you might want to check is whether there is anything else in the component that might be managing focus, and remove that. E.g. if you included some of the keyboard navigation for the menu, maybe there's a conflict there.

[kmcfaul]

Also noticed what Jen mentioned, it looks like the search is case sensitive at the moment. Additionally, if you backspace out some characters (Aw > A > AW to search AWS, or Opens > Open > OpenS to search OpenShift) to correct the case it doesn't look like it re-searches, you have to back out the entire string.

When the list is filtered by the text search and you click to select the item, it looks like it is selecting based on the current index for that filtered list, so it selects the wrong item based on the whole refCollection.

[codecov-io]

Codecov Report

Merging #1526 into master will decrease coverage by 0.15%.
The diff coverage is 69.11%.

@@            Coverage Diff             @@
##           master    #1526      +/-   ##
==========================================
- Coverage   83.05%   82.89%   -0.16%     
==========================================
  Files         568      573       +5     
  Lines        6184     6249      +65     
  Branches       75       75              
==========================================
+ Hits         5136     5180      +44     
- Misses       1018     1039      +21     
  Partials       30       30

Flag	Coverage Δ
#patternfly3	84.75% <ø> (ø)	⬆️
#patternfly4	79.58% <69.11%> (-0.33%)	⬇️
#patternflymisc	95.68% <ø> (ø)	⬆️

Impacted Files	Coverage Δ
...s/patternfly-4/react-core/src/helpers/constants.ts	100% <100%> (ø)	⬆️
.../components/ContextSelector/ContextSelectorItem.js	100% <100%> (ø)
...onents/ContextSelector/contextSelectorConstants.js	100% <100%> (ø)
...4/react-core/src/components/TextInput/TextInput.js	91.66% <100%> (ø)	⬆️
...omponents/ContextSelector/ContextSelectorToggle.js	41.93% <41.93%> (ø)
.../src/components/ContextSelector/ContextSelector.js	84.61% <84.61%> (ø)
...ponents/ContextSelector/ContextSelectorMenuList.js	88.88% <88.88%> (ø)
... and 2 more

---

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 82f2d8f...82ac932. Read the comment docs.

[christiemolloy]

Hey Titani this looks spot on! Was wondering if you could add some more items to the list in the menu, feel free to just repeat the info that is already there like I did in Core. We want to show the scroll functionality

[jschuler]

Looks great! mostly just some minor comments

[jschuler]

The props are spread to a button element, so should probably be HTMLButtonElement

[jschuler]

Suggested change

	onClick={_event => {
	onClick={() => {

[tlabaj]

I passed the event incase the consumer may want to look at it.

[christiemolloy]

CSS looks great

[christiemolloy]

@mcarrano should we have an example where you type in the search input, click the search icon and then it searches vs the implementation here where it automatically searches - it just seems as though the button is redundant, and I wanted to get your input on it.

[jgiardino]

Can you also add the Context Selector to the page demos?

👍 I agree, it would be nice to test this in the context of the demo.

@mcarrano @jgiardino so will the search button be changing to an icon?

I would say that change could be a separate issue, and that we address it in core first. I wouldn't hold this PR up for that since it's not a bug, just a refinement.

I just reviewed this again. The issue I noted with handling Enter key on the toggle is fixed, which is awesome!

I tested in JAWS to see if adding role="dialog" aria-modal="true" would have any effect on trapping screen reader focus, and it doesn't. 😿

Considering the differences between popover (which uses tippy) and modal, I wonder if tippy would be our best option for this component. Both require separating the popup element from the <div> that contains the main contents, but the modal doesn't require positioning, whereas the popover does. Also, being able to separate the panel and position it on top of the other contents might also let us avoid issues like #1230. I'm fine with capturing this last a11y item in a separate issue if this component is needed and/or this would require a significant amount of work. If we do capture this in a separate issue, then I would remove aria-modal="true" since it isn't really modal yet.

Other than that, there are just a couple of more things I noticed that would be simple updates:

• I just noticed that the order of ids on the toggle is reversed. The label id should be first, followed by the button.
• We can keep role="dialog" on the panel, but we should add either aria-label or aria-labelledby to describe it.
  • One suggestion is to set aria-labelledby to the id of the string defined for screenReaderLabel, to avoid adding additional props. But "Selected Project [colon] [dialog]" sounds a little off as the dialog title. Maybe just setting screenReaderLabel to "Project" in our example would work for both the toggle and the dialog? This would result in screen readers announcing "Project AWS button collapsed" for the toggle and "Project dialog" for the panel.
  • Another suggestion is to add a prop, similar to screenReaderLabel for the toggle, but this prop would be for the panel, and would take a text string and add aria-label="[text string]" to the panel. Then in our example, we could define "Select Project" for that prop. This would result in screen readers announcing "Selected Project [colon] AWS button collapsed" for the toggle and "Select Project dialog" for the panel.
  • Let me know if you have a preference on this and I can follow up with design on strings if needed.

[tlabaj]

Issue #1615 has been opened and will address remaining accessibility concerns.

[dlabaj]

@jgiardino @mcarrano Is enter after typing in search suppose to select the first item in the dropdown from the search results?

[jschuler]

IMO button does not make sense if you use typeahead select

[dlabaj]

@jschuler @tlabaj I'm going to take this in and if we want to change what happens on enter we can create a separate issue.

[jgiardino]

The latest updates look good to me. Thanks!