search

w3c / aria

Accessible Rich Internet Applications (WAI-ARIA)
https://w3c.github.io/aria/
Other
643 stars 124 forks source link

User agents must not support aria-owns across shadow root boundaries #2266

Open aleventhal opened 3 months ago

aleventhal commented 3 months ago

Because aria-owns is notoriously difficult for user agents to implement in a stable and predictable manner, I suggest that aria-owns cannot cross shadow root boundaries.

Pointing into an ancestor shadow root is already something user agents would hopefully not allow, because it would create a cycle. I would also like to make it impossible to point down via ariaOwnsElements, referenceDelegate, or any other cross-shadow root ARIA technique.

This is my recommendation unless someone can come up with a legit use case.

@jcsteh @scottaohara @cookiecrook @alice

scottaohara commented 3 months ago

definitely wondering what a good use case would be for someone to need to aria-owns parts of a custom elopement's shadow dom into the light dom.

it's not like it'd even be a restriction without precedent, with closed root shadow DOM custom elements not allowing styles from the light DOM to modify the styling of the shadow dom parts.

unless someone has a really good use case for this, i don't have a problem with this limitation, especially if it's a lot of dev effort for potentially little gain.

aleventhal commented 3 months ago

Image maps could have the same issue, with referenceTarget.

On Fri, Jun 28, 2024 at 12:38 PM scottaohara @.***> wrote:

definitely wondering what a good use case would be for someone to need to aria-owns parts of a custom elemement's shadow dom into the light dom.

it doesnt' even seem like a restriction that doesn't have precedent, with the fact that people can make it so light dom styles don't impact shadow dom elements.

unless someone has a really good use case for this, i don't have a problem with this limitation, especially if it's a lot of dev effort for potentially little gain.

— Reply to this email directly, view it on GitHub https://github.com/w3c/aria/issues/2266#issuecomment-2197287554, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAKQAZUIYX6G6QYJUFWE3KTZJWGQTAVCNFSM6AAAAABKCBKYLWVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCOJXGI4DONJVGQ . You are receiving this because you authored the thread.Message ID: <w3c/aria/issues/2266/2197287554 @.***>

jcsteh commented 3 months ago

This seems reasonable, though I can't (currently) think of a case where allowing aria-owns to refer into a shadow DOM would cause problems. I do agree that aria-owns is notoriously tricky though.

CC @eeejay because he's working on ARIA element reflection and will likely have a better idea regarding any potential issues here.

aleventhal commented 3 months ago

There were a lot of weird edge cases with aria-owns that perhaps weren't foreseen, at least in our code. It's untrustworthy when combined with other interesting markup.

On Fri, Jun 28, 2024, 7:26 PM James Teh @.***> wrote:

This seems reasonable, though I can't (currently) think of a case where allowing aria-owns to refer into a shadow DOM would cause problems. I do agree that aria-owns is notoriously tricky though.

CC @eeejay https://github.com/eeejay because he's working on ARIA element reflection and will likely have a better idea regarding any potential issues here.

— Reply to this email directly, view it on GitHub https://github.com/w3c/aria/issues/2266#issuecomment-2197766174, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAKQAZRKUMXTC3RSLY5P75TZJXWKHAVCNFSM6AAAAABKCBKYLWVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCOJXG43DMMJXGQ . You are receiving this because you authored the thread.Message ID: @.***>

spectranaut commented 2 months ago

Discussed briefly in triage: https://www.w3.org/2024/07/11-aria-minutes.html#t01

alice commented 2 months ago

I chatted with Aaron about this some time back, and I agree that it makes sense to at least postpone shipping support for ariaOwnsElements across shadow root boundaries, given how many subtle issues there are with aria-owns. If there is demand for it, we would need to make sure there can be sufficient testing for the cross-shadow case.

cyns commented 2 months ago

@nolanlawson fyi

keithamus commented 2 months ago

Given we're setting via ariaOwnsElements I think it would be possible to raise an exception if you assign a cross-root element. This would help us avoid potential "silent failures". Precedent exists for setting non-elements (e.g. try $('button').popoverTargetElement = 1 in your devtools, you'll see TypeError: Failed to set the 'popoverTargetElement' property on 'HTMLButtonElement': Failed to convert value to 'Element'.)

spectranaut commented 2 months ago

Discussed in today's meeting: https://www.w3.org/2024/07/18-aria-minutes.html#t07

cookiecrook commented 2 months ago

Would this also necessitate a IDL change, like:

[CEReactions] attribute FrozenArray<Element>? ariaOwnsElements;

to

[CEReactions] attribute FrozenArray<PlaceholderName-Local???Element>? ariaOwnsElements;
alice commented 1 month ago

Related to this discussion, I have a change ready for submission in Blink which would put ariaOwnsElements behind a separate feature flag from the rest of the ARIA relationship properties.

This would mean we could ship the remaining properties, but delay shipping any version of ariaOwnsElements (shadow-crossing or not) until we can figure out the logistics, and do enough testing to make sure there are no lurking issues with even the light DOM case (since there can be surprising results when you're not using an ID-based attribute).

How would folks feel about that as a plan? Then we can work towards potentially shipping the non-shadow-crossing version and/or the shadow-crossing version without blocking shipping the remaining properties.

aleventhal commented 1 month ago

+1

On Tue, Aug 6, 2024 at 7:40 AM Alice @.***> wrote:

Related to this discussion, I have a change https://chromium-review.googlesource.com/c/chromium/src/+/5752663 ready for submission in Blink which would put ariaOwnsElements behind a separate feature flag from the rest of the ARIA relationship properties.

This would mean we could ship the remaining properties, but delay shipping any version of ariaOwnsElements (shadow-crossing or not) until we can figure out the logistics, and do enough testing to make sure there are no lurking issues with even the light DOM case (since there can be surprising results when you're not using an ID-based attribute).

How would folks feel about that as a plan? Then we can work towards potentially shipping the non-shadow-crossing version and/or the shadow-crossing version without blocking shipping the remaining properties.

— Reply to this email directly, view it on GitHub https://github.com/w3c/aria/issues/2266#issuecomment-2271083787, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAKQAZVJS3EGPYDK7CNWNOLZQCYZVAVCNFSM6AAAAABKCBKYLWVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDENZRGA4DGNZYG4 . You are receiving this because you authored the thread.Message ID: @.***>

jcsteh commented 1 month ago

I think we need to clarify what Chromium is doing here vs the requirements for all browsers. At this stage, we don't have explicit plans to disable or restrict ariaOwnsElements in the Gecko implementation. We can do that, but that would require spec changes, and in particular, there's an open question about whether this would require an IDL change. To be clear, I'm not saying we're not willing to consider this change, but it would require an additional patch for Gecko at this stage, so we do need to clarify exactly what that change should be.

since there can be surprising results when you're not using an ID-based attribute

Are you able to provide an example? That might help us make more informed decisions here.

alice commented 1 month ago

I think we need to clarify what Chromium is doing here vs the requirements for all browsers.

Agreed. I (personally) think it would be fine for Gecko to ship ariaOwnsElements if you're confident in your implementation. That said, I would imagine as part of that work, it would be good to add more WPT tests like those in accessibility/crashtests concerned with aria-owns, using ariaOwnsElements instead.

I think if Blink is going to hold off on shipping ariaOwnsElements, we would just need to be sure to communicate in the spec and on the relevant MDN pages that ariaOwnsElements is not yet available everywhere.

there can be surprising results when you're not using an ID-based attribute

Sorry, I could have been much clearer about this. I don't mean that there should be any difference, as specced, but more that the code in Blink which checks for aria-owns cycles (including when elements previously referred to get added to the page) assumes there will be actual IDs present in some places. Also, all of the existing tests for the crashy aria-owns cases use IDs, so we can't have good confidence yet that the non-ID cases are well handled. We should definitely fix all of that, but given aria-owns is significantly more complex in implementation than the other attributes, I don't want to block shipping those on fixing all the potential edge cases in aria-owns.