feat(radio): allow radios to be "inoperable"

[seanforyou23]

What: This PR introduces a new prop isInoperable. When provided as a prop, radios are set into a partially enabled state. This way, users can still attach tooltips when the radios are not fully available for whatever reason. Click, change, and keypress events are suppressed when this prop is present so effectively the only thing you can do is focus the element and view the tooltip.

Closes # #3791

Additional issues: Steps toward closing #1894

[patternfly-build]

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

[codecov-io]

Codecov Report

Merging #3817 into master will decrease coverage by 0.04%.
The diff coverage is 40.9%.

@@            Coverage Diff             @@
##           master    #3817      +/-   ##
==========================================
- Coverage      71%   70.95%   -0.05%     
==========================================
  Files         785      785              
  Lines       10638    10658      +20     
  Branches     2314     2327      +13     
==========================================
+ Hits         7553     7562       +9     
- Misses       2655     2659       +4     
- Partials      430      437       +7

Flag	Coverage Δ
#misc	95.45% <ø> (ø)	⬆️
#patternfly3	85.89% <ø> (ø)	⬆️
#patternfly4	59.96% <40.9%> (-0.05%)	⬇️

Impacted Files	Coverage Δ
...ernfly-4/react-core/src/components/Radio/Radio.tsx	59.45% <40.9%> (-17.02%)	⬇️

---

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 7b461c0...b4f351c. Read the comment docs.

[dlabrecq]

Removing disabled feels like we're breaking the standard. That is, we're breaking expected HTML behavior in order to always display tooltips, forcing HTML elements to be keyboard navigable when intentionally disabled.

Could we not wrap a disabled component; for example, with a div that supports an alternate, disabled tooltip?

[seanforyou23]

@dlabrecq you're right at the heart of what's at stake - thanks for calling this out!

I don't think this is going against any standard. We are removing disabled because we are exposing a tiny bit of functionality, even if it's only focusability and hoverability with tabindex and pointer-events - that counts :)

When this is the case, it's replaced by [aria-disabled] and .pf-m-disabled, which together achieve a partially disabled state both visually and with AT. Screen reader users are made aware that the content is (dimmed) which is the case for disabled elements, and sighted users see the control with standard disabled styling.

Agree it's expected that disabled elements behave a specific way, but also we can acknowledge that this is bleeding edge of what's even possible from an experience standpoint so we're reaching hard for gains even if it requires managing state/perception in unique ways. I think as long as it passes standard html validation then it's fair game if the community is ok with it from the UX side.

All of this is in an effort to facilitate tooltips for disabled elements. It we're ok with limiting the tooltip to being attached to the container element of the Component then we might be able to achieve the same thing without having to remove the disabled attribute on these natively disablable elements. In other words, would we be ok with supporting only this way of using tooltips with disabled elements, and not this one? Notice the difference is that in one case, we simply nest the target element within a tooltip as usual, and in the other we send the tooltip down as props. I think the latter is the potentially dangerous api we might benefit from steering clear of.

I need to explore this a bit more to ensure it is even possible and works in all the cases of how it can be used/consumed.

IMO we should be thinking of this as setting the Component (capitol C) in a disabled/focusable state, not the native controls that may be associated with the control. I think with this distinction we can reason about it better and have more options to consider. Does that help?

I did notice that the .pf-c-radio__description element fails html validation with Element div not allowed as child of element label in this context.. Using a span element instead seems to fix it.

[seanforyou23]

@dlabrecq I just realized I may not have actually answered your question after all of that 😆

Yes, I think that would work also. At the very least, let's limit to managing the events/properties on the container of the React Component we're wrapping, and at best introduce a wrapper component to carry the related properties. It would be great if we don't have to do a lot of surgery on all these components that need this feature, the latter probably does this best. Does that help?

[dlabrecq]

We should ensure the community is really ok with this, perhaps push for a change in the standard, before introducing something unexpected?

It's not necessarily in violation if we never disable a component. However, it's unexpected behavior at the DOM level. If were to test the underlying HTML element, I expect that element to be disabled, not just visually.

[seanforyou23]

If we can use the approach where we introduce a container to carry this functionality, it seems like it would eliminate the problem, right? If so, that also has my vote.

Maybe there is a way to keep the disabled attribute, now that we've got the current implementation in place to work with - I'll explore this some more. Keep you posted!

[dlabrecq]

Yes, that would be my preference. As long as were not circumventing the disabled state (i.e., the DOM element remains disabled).

[dlabrecq]

Although we've made the radio button look disabled, it does not appear to be disabled at the DOM level. When I test the radio button, I expect the HTML element to be disabled.

[seanforyou23]

Does not having the disabled property at the DOM level cause an issue with interaction or behavior of the component? I'm trying to better understand any limitations to the approach we've explored here. In addition to making it look visually disabled, we've also blocked the common/relevant events that people associate with an "enabled" element. The only (intentional) interactions we've exposed are focusability and hoverabilty. I just want to make sure I understand what problem this is presenting. Thanks!

[dlabrecq]

Not so much an issue regarding how the user interacts with it, but it's unexpected at the DOM level. For example, while testing the radio or button (different PR), the HTML element is not disabled, so my existing tests fail. I don't see a way to programmatically determine if the HTML element is truly disabled or just a facade applied to make it look disabled.

[seanforyou23]

Hmm I see. I suppose one way to identify this case would be to check for the presence of aria-disabled and the absence of disabled at the same time. I think the container/wrapper based approach would also use this markup strategy. What do you think of using this as the primary method of verification for testing?

Do you think the name isDisabledFocusable is confusing? I wonder if isLimitedAvailability (or something like that) would be a better reflection of the end resulting effect. I can see how using "disabled" as part of the prop name would invite people to expect to find a disabled property in there.

I can start exploring a container-based approach some we have more options to compare. Also, I think some of these pointer-event adjustments are no longer necessary now that we're preventing default actions for click, keypress, and change events. I think I added the pointer-event bits before I was properly preventing default actions - will send updates for this soon.

[seanforyou23]

Here's an alternate approach we could consider - #3845