fix(vue): components presence #1943
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
omg it's working |
|
@TylerAPfledderer @iamdin I'd be happy for your review. I hope I didn't miss anything important. Maybe it's that last PR before Ark UI 1.0 for Vue ^^ |
|
@Omikorin any reason why changes in react and solid tests are showing up here? |
Yes, I found flaky or missing tests between those, so I synced them by the rule "do it now while you remember". |
There was a problem hiding this comment.
@Omikorin on initial pass, this is great and I do not see anything missing related to presence.
I only have one tiny nitpick though 😄
|
|
||
| const triggerProps = computed(() => ({ | ||
| ...api.value.triggerProps, | ||
| 'aria-controls': presenceApi.value.isUnmounted |
There was a problem hiding this comment.
I don't think this attribute should disappear after the initial rendering of the dialog content with lazyMount. Once the dialog content is rendered to the DOM, this attribute should stay attached to the trigger, else the purpose of it is lost.
In other words, the attribute only matters when a user navigates to the trigger, and it should be present if the associated content is visible in the DOM.
I also acknowledge that this attribute does not have much support with screen readers at the moment; it will not be acknowledged by any of the major SRs, with only JAWS ensuring the user's ability to jump to the controlled element.
My above comments stand in regards to future-proof.
There was a problem hiding this comment.
Thanks, I've checked the behavior. This attribute disappears only when using lazyMount with unmountOnExit (like in Dialog LazyMount story). Otherwise, it stays in place along with lazy-mounted content.
It's the same situation in React, so let's ask Chris.
@cschroeter, what do you think about a11y case that Tyler brought?
There was a problem hiding this comment.
I just glimpsed over the conversation. lazyMount or unmountOnExit has often a negative impact on a11y. It is a tradeoff for performance. My recommendation is to avoid using lazyMount or unmountOnExit if possible.
I've read a bit about this topic here: w3c/aria#995 and as far as I can tell there is no clear winner if aria-controls should be removed, empty or just point to the hidden element.
Under this assumption please keep the current solution as it passes the Storybook a11y Addon and this axe addon for jest some contributor brought up.
Cheeers
There was a problem hiding this comment.
Thanks mate. I agree with the current solution.
@TylerAPfledderer I'll dismiss the request then and merge PR. I appreciate your review! :)
|
|
||
| const triggerProps = computed(() => ({ | ||
| ...api.value.triggerProps, | ||
| 'aria-controls': presenceApi.value.isUnmounted |
There was a problem hiding this comment.
Same comment as with DialogTrigger
|
|
||
| const triggerProps = computed(() => ({ | ||
| ...api.value.triggerProps, | ||
| 'aria-controls': presenceApi.value.isUnmounted |
There was a problem hiding this comment.
Ditto comment with DialogTrigger
If checked then done/fixed etc
Not working lazy mounting (from tests):
Aria errors (tests):
Presence existing in component (rewritten):
Note: Working Presence is understood as
lazyMountpassed to a component'sRootpart (like in Chakra UIisLazy). There is no reason to prefer props in items (I know there is, although it's a simpler and more intuitive approach).