webui : [ChatFormActionAdd][a11y] fix accessibility issues in add menu trigger and items#22736
webui : [ChatFormActionAdd][a11y] fix accessibility issues in add menu trigger and items#22736vignesh191 wants to merge 3 commits intoggml-org:masterfrom
Conversation
|
Hi @vignesh191, thanks for your contribution! Per our contribution guidelines, the automated PR checker found the following issue(s) that need your attention:
Please note that maintainers reserve the right to make final decisions on PRs. If you believe there is a mistake, please comment below. |
| </DropdownMenu.Item> | ||
| <Tooltip.Trigger tabindex={-1}> | ||
| {#snippet child({ props })} | ||
| <div {...props} class="cursor-default"> |
There was a problem hiding this comment.
This div may look off, but disabled rows ( such as Images / Audio Files) need it.
Disabled rows (DropdownMenu.Item) have pointer-events: none, so the tooltip's hover listeners won't fire there. Wrapping it in a non-focusable <div> catches these pointer events correctly.
tabindex={-1} here overrides Tooltip.Trigger's default tabindex=0 to keep this div out of tabbing, and cursor-default overrides tooltip-trigger's hardcoded cursor-pointer since the row isn't clickable.
| import * as DropdownMenu from '$lib/components/ui/dropdown-menu'; | ||
| import * as Tooltip from '$lib/components/ui/tooltip'; | ||
| import { buttonVariants } from '$lib/components/ui/button'; | ||
| import { cn } from '$lib/components/ui/utils'; |
There was a problem hiding this comment.
we don't need to use cn anymore, as Svelte 5 supports Arrays inside of classes
There was a problem hiding this comment.
I did a little digging, and I think we do need it here. Without it, the "+" button renders as a square (rounded-md):
buttonVariants looks like it includes the rounded-md. We manually added rounded-full here. Without the cn, both of these classses get added and rounded-md takes styling precedence 🤷 . It seems like cn is doing meaningful work here merging the tw classes
allozaur
left a comment
There was a problem hiding this comment.
Also, would be great if u included updated tests in Storybook for this :)
|
Please rebuild the WebUI and it will be fine! |
|
@vignesh191 Please rebase and rebuild the static output and we good to merge |
|
@allozaur @ServeurpersoCom Ah, I'll be away from my dev machine for a few days, I can rebuild next Tuesday. Thank you for the prompt reviews! |
We can also cherry pick and merge on our end. Thanks for a heads up! |
Overview
Hello! I found some more keyboard navigation issues in the webui. These changes are for the
ChatFormActionAddcomponent. These changes fix two a11y issues:Tabmust be pressed three times on the "+" button before moving to the next focusable element.Tabshould only be pressed once to navigate out of the element.Both these issues originate from the same root cause: the
Tooltip.Triggerelement.Tooltip.Triggerrenders a focusable<button>by default. Wrapping it around another focusable element (<Button>,<DropdownMenu.Item>, or anything else) produced nested focusable elements and makes extra tab stops for keyboard nav, and ghost wrappers that disrupt mouse events and break focus. Tabbing inception.I see this pattern being used in other places in the webui repo as well. However, before fixing those, I want to first investigate further and see if a more robust solution can be made, or using the solution in this PR (mentioned below) is the best. For now, this fix is localized for the "+" button only.
Additional information
This can be reproduced in the local Storybook instance (
ChatScreenForm) component. Tab through the "+" button and notice, Tab must be pressed 3 times before moving onto the next focusable element. Click on the "+" button and notice focus is incorrectly placed on a disabled menu item.IMO the correct fix for this issue is to use the child snippet pattern from bits-ui. We can use it to pass Tooltip.Trigger's props directly onto the real underlying element without any wrappers, and prevent Tooltip.Trigger from making its own
<button>element. I actually was inspired to use this pattern from existing code that already uses this pattern: ModelsSelectorDropdown.svelte :) This also (ever so slightly) improves performance since theres one less DOM elementRequirements
bits-uipatterns/usage in this repo and to create the actual diff after I decided that this is the most elegant solution