Skip to content
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

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

Open
aleventhal opened this issue Jun 28, 2024 · 14 comments
Open

Comments

@aleventhal
Copy link
Contributor

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
Copy link
Member

scottaohara commented Jun 28, 2024

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
Copy link
Contributor Author

aleventhal commented Jun 28, 2024 via email

@jcsteh
Copy link

jcsteh commented Jun 28, 2024

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
Copy link
Contributor Author

aleventhal commented Jun 29, 2024 via email

@spectranaut
Copy link
Contributor

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

@alice
Copy link

alice commented Jul 11, 2024

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
Copy link
Contributor

cyns commented Jul 17, 2024

@nolanlawson fyi

@keithamus
Copy link
Member

keithamus commented Jul 18, 2024

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
Copy link
Contributor

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

@cookiecrook
Copy link
Contributor

Would this also necessitate a IDL change, like:

[CEReactions] attribute FrozenArray<Element>? ariaOwnsElements;

to

[CEReactions] attribute FrozenArray<PlaceholderName-Local???Element>? ariaOwnsElements;

@jnurthen jnurthen removed the Agenda label Jul 23, 2024
@alice
Copy link

alice commented Aug 6, 2024

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
Copy link
Contributor Author

aleventhal commented Aug 6, 2024 via email

@jcsteh
Copy link

jcsteh commented Aug 7, 2024

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
Copy link

alice commented Aug 7, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

9 participants