-
Notifications
You must be signed in to change notification settings - Fork 355
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Tooltips for disabled elements #1894
Comments
Hi @jgiardino what priority would you consider this issue? |
Given the possibility that consumers would choose a workaround that is not accessible, and given this solution might not be a priority to fix later when this use case is better supported, I'd make this a higher priority. |
We can also split this out with a subset of components that are most likely going to be affected, if that helps. I think components like Button, Dropdown, and all the form components would be a good start. This should also be a relatively easy fix. It's a new prop that changes what attributes are defined. |
I really need a tooltip for disabled radio buttons and it's kind of a blocker for me. Currently, wrapping a radio button with a tooltip doesn't look great, and unfortunately I couldn't find a CSS workaround that's good enough. |
@LeshemNoa we've just pulled this into our current milestone and will bump radio button up in the list so it's worked on soon. Will ping you for review once it's ready! |
Ping @LeshemNoa - got a PR up for Radio that I think will help you be able to get the tooltips going, if you find time please check it out and let me know if you see any issues. Thanks! |
@seanforyou23 This is great progress! Thank you so much for the speedy response. However, when you don't supply a fixed width style for the radio button, the tooltip's location will correspond to the outermost div containing the button rather than the button and its label. This was my problem in the first place. |
@LeshemNoa yep, I noticed that also. I think by default our form controls take up 100% of the available space of their containing element, so it kinda seems like the intended behavior. Looks a little strange in the example because there's no border/background and it's not in any real form layout, which form controls usually are. I think there could be a couple of ways around this though, I will explore this a bit more today. Keep you posted! |
Thanks for sharing the example, @LeshemNoa! In that example, using I wonder if expanding the Tooltip position prop to support more options would be good way to solve this, like "top-left" to be above the tooltip contents but left-aligned instead of center-aligned. But this is probably something that design should weigh in on (cc @mcarrano). I think @seanforyou23 is correct in that the radio is intended to be 100% the width of it's container. |
@LeshemNoa @jgiardino I just pushed an update to that open PR for Radio that adds yet another way of setting the tooltip. To avoid having to add more complex positioning logic to Tooltip, we could consider allowing a tooltip to be passed as a prop, in which case we could nest it within the component however we want. I think either approach requires changes around how we manage pointer-events in core, fwiw. This does add a level of complexity that I can see being challenging to maintain, as we'd need/want to do the same thing for other similar components, but it's an option for us to consider. The consumer side of me would love to have the option 😄 Do we want to gain this capability at the expense of the larger API surface area? I think exploring the disabled/tooltip implementation across a couple more components could help us understand what the ideal solution could look like. The way it stands now (in that PR), you can add tooltips using either approach. Feel free to test/explore them and let me know what you think! I haven't written tests and integrations for the props approach yet, figured it would be better to bounce it off a few people before I go too far down that road. |
I have a use case of needing to have a tooltip appear when user hovers over a disabled button to explain why it is disabled. @boaz0 will this PR allow us to have tooltips on disabled buttons? |
Yes - it seems like fixing our problem. |
The current Tooltip component looks at its child to see if that it is disabled. If so, the tooltip is not displayed. In order to always show a tooltip, you can simply add a div tag. For example:
|
Actually, that's a good idea. |
@jgiardino & @seanforyou23 can the message of the tooltip be dynamic depending on the state of the button (enabled vs. disabled)? M&A UX has a use case of needing a tooltip show up on hover when the button is in a disabled state so we can tell the user why the button had been disabled. Will this issue handle that use case? and @jgiardino would this type of notification be accessible? |
For accessibility it's important to understand how the sighted keyboard experience and screen reader experience compare. Sighted keyboard users use the Tab key to shift focus to each interactive element on the screen. The tooltips are accessible to these users when the element receives focus. When an interactive element is disabled, the disabled element does not receive focus. Therefore, to provide a tooltip for disabled elements, then some additional coding is needed to still present the element as disabled by enable the user to shift focus to the element. The screen reader experience is where it starts to get more complicated. These users have a variety of methods for navigating elements. They can still use the Tab key method described above to shift focus to each interactive element, but unlike sighted users, they also need to access non-interactive elements, like headings and paragraph text. Therefore, they also have other methods of navigating elements. These additional methods are slightly different than the Tab key method, but to elaborate on that, it's also important to understand the two types of content that tooltips could provide:
For screen reader users, the text provided in the tooltip could be associated with the element using Something important to keep in mind is that in JAWS (a very popular screen reader), the So all that said, these are my recommendations for using a tooltips:
If you have specific use cases that you'd like to discuss, I'm happy to review them. |
The workaround we were using for Tooltips to work with disabled buttons does not work anymore. Wrap disabled elements with a span when the these are children of a tooltip. See: patternfly/patternfly-react#1894 Closes cockpit-project#14195
The workaround we were using for Tooltips to work with disabled buttons does not work anymore. Wrap disabled elements with a span when the these are children of a tooltip. See: patternfly/patternfly-react#1894 The remaining PF3 tooltips in non React pages were reworked so that they look like PF4. Closes #14195 Co-authored-by: Garrett LeSage <[email protected]>
We finally nailed down a pattern that we can hopefully use for the remaining components (is |
Since this epic was opened, many of the listed components have implemented a solution for adding tooltips when disabled. |
For a tooltip to be accessible for keyboard-only users, the element with the tooltip needs to be able to receive keyboard focus. Elements like
<button>
that support thedisabled
attribute cannot receive keyboard focus.To continue to render these elements as disabled but enable keyboard focus, the following changes are needed:
disabled
aria-disabled="true"
pf-m-disabled
class or similar is included on the element to apply the disabled styling.tabindex="-1"
We should update all interactive elements to include a prop that allows the consumer to make the element disabled but focusable so the element can be used with the Tooltip component. This prop should apply the appropriate attributes and classes for the given element.
Unless someone has a better suggestion, the prop could be named
isDisabledFocusable
(totally open to suggestions on this one)Components that should be updated are:
We could simplify this list if there are any components here that we think are less likely to be disabled and with a tooltip.
The text was updated successfully, but these errors were encountered: