feat(focusable): add focusable component

[seanforyou23]

What: This PR adds a new Focusable component that can be used to wrap other elements/components to make them focusable. I haven't had a chance to add cypress tests yet, but wanted to go ahead and get the work up for review in case people have time to take a look and provide feedback.

Closes #3791

It also serves as an alternative approach to #3817

Additional issues: Steps toward closing #1894

[patternfly-build]

PF3 preview: https://patternfly-react-pr-3845-pf3.surge.sh
PF4 preview: https://patternfly-react-pr-3845.surge.sh
A11y report: https://patternfly-react-pr-3845-coverage.surge.sh

[codecov-io]

Codecov Report

Merging #3845 into master will increase coverage by 0.05%.
The diff coverage is 90.9%.

@@            Coverage Diff             @@
##           master    #3845      +/-   ##
==========================================
+ Coverage    60.3%   60.35%   +0.05%     
==========================================
  Files         405      406       +1     
  Lines        6225     6236      +11     
  Branches     2306     2313       +7     
==========================================
+ Hits         3754     3764      +10     
  Misses       2042     2042              
- Partials      429      430       +1

Flag	Coverage Δ
#patternfly4	60.35% <90.9%> (+0.05%)	⬆️

Impacted Files	Coverage Δ
packages/react-core/src/components/Tabs/Tab.tsx	100% <ø> (ø)	⬆️
.../react-core/src/components/Focusable/Focusable.tsx	90.9% <90.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 34098e9...a581d6e. Read the comment docs.

[dlabrecq]

Overall, I like this approach much better. We're not altering the underlying HTML button element and instead relying on standard HTML behavior for the disabled state. And this functionality can be used with just about any component!

However, I see one potential issue. When I remove the isDisabled prop from button, both the button and container are focusable.

Can we ensure that the example is mutually exclusive? For example, we could add an isDisabled property to the focusable component. Then, if the button is enabled, the focusable component is not.

Alternatively, the focusable component could test the disabled state of its immediate child. Or, use a component ref to test the disabled state of a specific child?

Does this need to be experimental / beta?

Probably don't need to update the yarn.lock, right?

[dlabrecq]

Looking good. Just a nit about the interface prefix.

[dlabrecq]

Looks great!

[tlabaj]

Looks great! One little nit.

[boaz0]

LGTM 👍

[dlabrecq]

Could the default div tag use pf-u-display-inline-block? That way, the focus ring won't extend beyond the underlying button, radio, etc.

<Tooltip content={...}>
  <div className="pf-u-display-inline-block">
    <Button isDisabled>I have a tooltip!</Button>
  </div>
</Tooltip>

[tlabaj]

LGTM

[jgiardino]

When I test the examples of Focusable with tooltips in VO, the tooltip contents are not announced.

Additionally for the disabled buttons, there are slight differences with how VO behaves depending on navigation method.

Using the tab key, VO announces the element as a group instead of a button. It does add "you are currently on a button," but the semantics in this case are slightly different. For example, the disabled button is announced as:

Disabled button text group main. [this is where the tooltip text would be announced if working as expected] You are currently on a button. This item is dimmed.

Using the next element navigation or navigating by form control, the element is announced as a button and the button is announced as disabled.

Disabled button text dimmed button

Assuming the issue with the tooltip getting announced is fixed, there are still a couple of concerns I have with this method:

1. If different methods of navigation are focusing on different elements (i.e. the outer div vs the inner button), then the tooltip should be applied to all focusable elements to ensure that it's announced regardless of which element receives focus.
2. How does this behave in other screen readers? VO does eventually announce that it's a button, but is that the case with all screen readers? It's really important that the semantics of elements are preserved. If an element is a button, it should always be announced as a button. Those semantics should not change simply because it's disabled. user is expecting a button element and comes across that element at a time when it's disabled, it , and the element should still be announced as a button even when disabled.

I still favor the original implementation of adding the prop directly on the element to state that it's disabled but focusable. It avoids the issues mentioned above, since we're not adding addltional wrapping elements that alter the semantics of the normal state of the element. It also seemed like a much cleaner solution, both in implementation, but also when using it (as a consumer, I should be able to just add a prop to say that something is disabled).

[mcoker]

Could the default div tag use pf-u-display-inline-block

I'm not sure if you would need to import the entire display utility CSS (which is 3.7k) but an alternative could be to use a <span>.

---

This looks pretty cool! My main concern is supporting layouts that depend on a .parent > .child relationship, where the child is disabled. Adding an extra element around .child will break those layouts. In some cases we may be able to refactor the CSS to support an extra element being added between the parent and child, but not all. For example, something like this https://codepen.io/mcoker/pen/ZEGvyWw

I included in there an example of display: contents, which is great at resolving the layout issue where an element is added between the .parent > .child, but that also removes the tabindex.

Side note, this may be something that is resolved in later implementations of display: contents as the current implementation by browsers has a11y issues due to incorrectly implementing the spec. Namely removing semantics and including the ability to tab to an element.

[seanforyou23]

Thanks for the feedback! Wanted to address a couple of things while we collect more comments/feedback.

When I test the examples of Focusable with tooltips in VO, the tooltip contents are not announced.

I noticed they are not announced when focusing with VO cursor (reader focus), however if you tab onto the element with the standard tab key, the tooltip content does get announced - however, it's announced later after the reader has finished reading other contextual aspects of the current view. So (for this implementation strategy) I don't think it's as much a matter of getting the announcements "fixed" as it is just realizing when we should and shouldn't expect the announcements to take place. Seems like descriptions/tooltips are generally only announced on TAB focus, not screen reader focus.

I think exploring the impact/differences of screen reader focus vs keybaord focus is in play here, and adjusting our expectations around how these things should function can make the path forward a little more clear and flexible.

I'm starting to see how both implementations we're comparing here can be useful as tools in the toolset to build accessible experiences, and maybe there's less value in forcing to pick one as the "correct" path forward. IMO the need here is becoming more about offering tools to enable developers to build good AX rather than doing it for them or constructing tools in such a way that "prevents" them from composing solutions in a way that may be inaccessible.

Could the default div tag use pf-u-display-inline-block

I'm not sure if you would need to import the entire display utility CSS (which is 3.7k) but an alternative could be to use a <span>.

Yes would need to import the display utilities, the size difference I think is nominal though. Using a span isn't a good default because certain types of markup isn't valid as children