[EuiSelectable] add support for option tooltips #7715
Conversation
- dry out props - ensure tooltip is visible on mount when isFocused - update default tooltip position
|
🗒️ I added VRT snapshots to this PR but the initial state of the tooltip story is equal to the playground one. |
|
Hey Lene! I'm so sorry for not reviewing this in time for Monday's release - I'm leaning towards shipping with just the combobox tooltips for this week and getting selectable tooltips in next week if that's okay! |
Of course, no worries - that's no problem! 🙂 |
src/components/selectable/selectable_list/selectable_list.test.tsx
Outdated
Show resolved
Hide resolved
- my 2c: I don't think they cover anything the tests in `selectable_list_item.test.tsx` don't already cover
…ia-describedby` - the `role="option"` should (I believe) go on the `<li>` element to be semantically correct - we can tweak or manually inherit `aria-describedby` from the tooltip as necessary - this also lets us remove an extra unnecessary `<span>` wrapper around the tooltips content
- fix error on keyboard up/down navigation of tooltip story - `enableFunctionToggleControls` has to only be set on the playground otherwise it throws an error because it's not included - ts `as` cast cleanup
…ponent to a function component - not sure if this is too extra, I found this easier to grok personally. If it feels too much for this PR, we can revert it or pull it out to a separate PR
|
@1Copenut I'll probably tag you in to test the screen reader experience of this tomorrow. It's working great for me on Firefox+VO but buggy for Safari+VO 😭 (it reads the tooltip description after moving away from the item on keyboard arrow navigation press, which is all kinds of weird) |
|
@cee-chen Thanks for the changes! The updates look good to me 👍 |
There was a problem hiding this comment.
Hello team! I tested with five screen reader and browser combinations:
- MacOS + Safari + VO
- MacOS + Firefox + VO
- Win10 + Firefox + NVDA
- Win10 + Chrome + NVDA
- Win10 + Chrome + JAWS
3 of 5 performed perfectly. The tooltips were announced as we expected, after the item name and a brief pause. There were two rough edges:
- JAWS + Chrome announces everything when you make a change like selecting or unselecting an item. So that means it announces the instructions, the item name, the item selection status, and the tooltip. This is the same as before, except the tooltip is added on. I would make mention of this fact in docs that too-long tooltips are a design rough edge and should be considered for in-page instruction.
- Safari + VoiceOver is very inconsistent announcing tooltips. It generally announces the tooltip that was removed from the DOM (IE, the previous item's tooltip). This is not great for the ~7% of screen reader users who make this their primary pairing. I pulled this data from the latest WebAIM Screen Reader Survey.
All in all, I don't have enough reservations to say this needs more work. The fact that Firefox + VO worked as expected has me thinking it's an issue in Safari more than VoiceOver.
|
Arggh. Thank you so much for confirming my findings Trevor, I was seeing that same behavior in Safari+VO as well. Oh actually YOU KNOW WHAT. The baseline EuiSelectable component is doing this all by itself even without tooltips, what the heck. I can repro it in the production EUI docs right now: https://eui.elastic.co/#/forms/selectable
You can see it repeating the old/previous item that was navigated away from before it announces the next item, and it doesn't seem to announce the full current/next item or its state until after. @1Copenut I think we need to file an issue for this separate from this PR, as something funky is definitely going on here even outside of tooltips. I'm going to go ahead and merge this PR for now since the issue is with Safari/VO, but we should probably look into a fix separately 😬 |
|
Preview staging links for this PR:
|
💚 Build Succeeded
History
|
Summary
This is part 2 of #7690 (see part 1 here)
closes #7690
This PR updates EuiSelectable to support the two props
toolTipContentandtoolTipPropsto be passed to EuiSelectable options to display a tooltip on hover/focus.QA
gh pr checkout 7715General checklist
@defaultif default values are missing) and playground togglesChecked Code Sandbox works for any docs examplesIf applicable, added the breaking change issue label (and filled out the breaking change checklist)