WICG / aom

Accessibility Object Model
http://wicg.github.io/aom/
Other
563 stars 59 forks source link

ARIA element reflection across non-descendant/ancestor shadow roots #192

Open nolanlawson opened 2 years ago

nolanlawson commented 2 years ago

In the current proposed implementation of ARIA element reflection in WebKit, as well as the relevant PR on the spec (https://github.com/whatwg/html/pull/3917) and this WPT test, element reflection only works for elements whose shadow roots are either the same, or in a descendant/ancestor relationship:

If element's explicitly set attr-element is a descendant of any of element's shadow-including ancestors, then return element's explicitly set attr-element. Otherwise, return null.

As discussed previously, though (https://github.com/whatwg/html/issues/4925#issuecomment-692508283), there are use cases where the elements' shadow roots may have sibling (or cousin, aunt/niece, etc.) relationships.

For instance, following the ARIA 1.1 Combobox with Listbox Popup example, an implementation with web components might look like:

<fancy-input>
  #shadow-root
    <input type="text">
</fancy-input>
<fancy-listbox>
  #shadow-root
    <ul role="listbox">
      <fancy-option>
        #shadow-root
          <li role="option">List item</li>
      </fancy-option>
    </ul>
</fancy-listbox>

In this case, the web author may want to set the <li> to be the ariaActiveDescendant of the <input>. However, their shadow roots are not in a descendant-ancestor relationship, so attempting to set the ariaActiveDescendant would be a no-op.

With some tinkering of the DOM hierarchy, it's possible to work around this restriction:

<fancy-input>
  #shadow-root
    <input type="text">
    <fancy-listbox>
      #shadow-root
        <ul role="listbox">
          <fancy-option>
            #shadow-root
              <li role="option">List item</li> <!-- shadow descendant of the <input> -->
          </fancy-option>
        </ul>
    </fancy-listbox>
</fancy-input>

However, this requires the <fancy-listbox> to be inside the <fancy-input>'s shadow root, which increases the scope of <fancy-input> beyond just enhancing the <input>.

Another potential alternative is to use cross-root ARIA delegation to have the <input> delegate its aria-activedescendant to the <fancy-input> host, but AFAICT the current version of that spec doesn't handle element ID ref attributes like aria-activedescendant.

If I understand the previous summary (https://github.com/whatwg/html/issues/5401#issuecomment-693739800), this restriction was put in place to avoid leaking internal component details. If we assume all of the shadow roots are open, though, then I'm not sure that that argument is still as strong. It feels like the restriction makes wiring up relationships across shadow boundaries very tricky, whereas in the light DOM world we don't have any similar restrictions.

Would this group be open to relaxing the restriction on cross-shadow ARIA relationships?

/cc @leobalter @mrego

jcsteh commented 2 years ago

If I understand the previous summary (whatwg/html#5401 (comment)), this restriction was put in place to avoid leaking internal component details. If we assume all of the shadow roots are open, though, then I'm not sure that that argument is still as strong.

First, I don't think we can make that assumption. But more importantly, this is less about security and more about encapsulation. As I understand it, one of the major benefits of shadow DOM is that it encapsulates internal details and prevents any leaking or impact of those details (in either direction) except via specifically defined mechanisms. The whole point of shadow DOM is to isolate implementation details. Element references exposed from shadow DOM via the platform would violate that principle.

It feels like the restriction makes wiring up relationships across shadow boundaries very tricky,

I totally understand that this is tricky and tedious. However:

whereas in the light DOM world we don't have any similar restrictions.

That's fair, but in the light DOM, you also don't have other restrictions (and the benefits that come with those) provided by shadow DOM. If isolating internal details is not a requirement, that suggests you don't need to use shadow DOM.

nolanlawson commented 2 years ago

Hi @jcsteh, thanks for the response. A few thoughts:

Element references exposed from shadow DOM via the platform would violate that principle.

In open shadow DOM, it is already possible today to traverse the entire DOM tree. (E.g. kagekiri implements a querySelectorAll() implementation that ignores shadow boundaries.) It's also possible to bubble events past shadow boundaries with composed: true. Inherited styles also leak into shadow roots. In principle, shadow DOM provides encapsulation, but in practice, the boundaries are porous.

So to me, it seems uncontroversial to allow two open shadow roots to link elements with aria relationships – it doesn't expose any new API surface that wasn't already exposed elsewhere. (Closed shadow DOM is another story, but let's set that aside for a moment.)

I guess for me the question is: "Why is it acceptable for elements in separate shadow roots to be linked with aria relationships, but only if those shadow roots are in a descendant-ancestor relationship (and only in one direction)?" To me, it's not clear what benefit this particular restriction provides.

As for closed shadow roots, it does seem tricky to provide a safe way for elements in separate shadow roots to be linked with aria relationships. But unless I'm missing something, it seems that the important distinction is open vs closed, not ancestor vs descendant (or any other relationship between shadow roots). A closed shadow root containing another closed shadow root should still not have access to internal implementation details (in principle).

If isolating internal details is not a requirement, that suggests you don't need to use shadow DOM.

At Salesforce, we use shadow DOM on our platform to provide encapsulation guarantees. However, we would still like a well-defined path to relax these guarantees to enable certain use cases, such as accessibility.

Drawing on the previous example, the <fancy-input> and <fancy-listbox> are encapsulated from one another (and may have been built by two different teams). However, we would still like a way to build to build a <fancy-combobox> on top of these two primitives (which may be built by a third team). The descendant-ancestor restriction would (AIUI) require <fancy-listbox> to be inside of <fancy-input>'s shadow root, breaking that encapsulation.

jcsteh commented 2 years ago

In principle, shadow DOM provides encapsulation, but in practice, the boundaries are porous.

Porous, yes, but clearly defined with regard to leaking element references. That is, you need to explicitly choose to dive into the shadowRoot to access elements inside it; it doesn't just happen as a side effect of some other API. The problem with ARIA element reflection is that this boundary crossing would not at all be explicit.

I guess for me the question is: "Why is it acceptable for elements in separate shadow roots to be linked with aria relationships, but only if those shadow roots are in a descendant-ancestor relationship (and only in one direction)?" To me, it's not clear what benefit this particular restriction provides.

Initially, the objection raised by Mozilla was that there should never be any shadow boundary crossing at all. I guess this was relaxed as a compromise.

At Salesforce, we use shadow DOM on our platform to provide encapsulation guarantees. However, we would still like a well-defined path to relax these guarantees to enable certain use cases, such as accessibility.

The issue here is "well-defined path to relax these guarantees". If the change you suggest is implemented, there would be no explicit choice to relax them with ARIA element reflection. They would just always be relaxed.

CC @AnneVK.

nolanlawson commented 2 years ago

@jcsteh Thanks again for the feedback and context.

That is, you need to explicitly choose to dive into the shadowRoot to access elements inside it; it doesn't just happen as a side effect of some other API. The problem with ARIA element reflection is that this boundary crossing would not at all be explicit.

In open shadow DOM, with my proposal, you would still need to explicitly traverse into a shadow root (e.g. using element.shadowRoot.querySelector(...)) in order to link two cross-shadow elements via ARIA reflection. IDs are scoped to shadow roots, so ID refs wouldn't suffice.

For closed shadow DOM, the component would need to offer an explicit API to provide access to the element that it wants to be linked. E.g.:

customElements.define('fancy-option', class extends HTMLElement {
  #shadow = null

  constructor() {
    super()
    this.#shadow = this.attachShadow({ mode: 'closed' })
    this.#shadow.innerHTML = '<li role="option"></li>'
  }

  get option() {
    return this.#shadow.querySelector('li')
  }
})

otherElement.ariaActiveDescendantElement = $('fancy-option').option

Otherwise it's impossible for anyone else to access its shadowRoot, and thus to set that element as the aria* property of another element.

jcsteh commented 2 years ago

Once it's linked, though, you can fetch the element from that property. For example, if you set foo.ariaActiveDescendantElement to something inside a shadow DOM, anyone with access to foo can then get that shadow element via foo.ariaActiveDescendantElement, potentially without even knowing they've crossed the shadow boundary. There was a proposal a while ago to avoid exposing the value in this instance, but this isn't ideal for authors because they don't have a good model of what an AT actually sees if the properties sometimes report null when in reality they refer to some shadow element.

mrego commented 2 years ago

There are a bunch of previous discussions quite similar to this one.

See for example https://github.com/whatwg/html/issues/6063 which points to https://github.com/WICG/aom/issues/169. Also this delegation proposal: https://github.com/WICG/webcomponents/issues/917

Or other things not directly related to ARIA properties: https://github.com/whatwg/html/issues/3219 & https://github.com/WICG/webcomponents/issues/179

nolanlawson commented 2 years ago

Once it's linked, though, you can fetch the element from that property. For example, if you set foo.ariaActiveDescendantElement to something inside a shadow DOM, anyone with access to foo can then get that shadow element via foo.ariaActiveDescendantElement, potentially without even knowing they've crossed the shadow boundary.

Thanks, I think I understand the concern now. I'm still not clear on why descendant/ancestor relationships are a special case, but the problem we're trying to solve is clear to me.

annevk commented 2 years ago

As I understand it you can go upwards currently, but not downwards. That preserves encapsulation. As in, if you have a single shadow tree it's fine to link from there to outside of it, but you shouldn't have a link from the outside to inside the shadow tree.

nolanlawson commented 2 years ago

@annevk Thank you! After re-reading the language in Alice's PR to whatwg/html, I think I understand now. The spec says that in this case:

element1.ariaActiveDescendantElement = element2

... element2 must be contained within a shadow ancestor of element1. I had it backwards – I thought it had to be a descendant!

That does preserve the encapsulation you mention (i.e. exposing the same information you could get from walking up the tree, which works even for closed shadow roots). But I think unfortunately it makes the combobox situation worse. I don't see how a web developer could reasonably structure it so that the active listbox option is in a shadow ancestor of the <input>:

<fancy-listbox>
  #shadow-root
    <ul role="listbox">
      <fancy-option>
        #shadow-root
          <li role="option">List item 1</li> <!-- shadow ancestor of the <input> -->
            #shadow-root
              <fancy-input>
                #shadow-root
                  <input type="text">
              </fancy-input>
      </fancy-option>
      <fancy-option>
        #shadow-root
          <li role="option">List item 2</li>
      </fancy-option>
    </ul>
</fancy-listbox>
// This works because listOption1 is in a shadow ancestor of the input
input.ariaActiveDescendantElement = listOption1

As you can see, the <input> would have to be moved around every time the active descendant changes (i.e. the user presses ArrowUp/ArrowDown on the combobox to change the active list option). Unless I'm missing something, a standard combobox could not be implemented in a way that aligns with the ARIA element reflection requirements.

nolanlawson commented 2 years ago

I'm not sure if this has been discussed elsewhere, but just to propose a solution, could we enforce the descendant/ancestor restriction only on closed shadow roots? This would preserve the original intention of the restriction, which is to avoid exposing an element that isn't already exposed.

In other words:

element1.ariaActiveDescendantElement = element2
Relationship between elements Shadow mode Allowed?
element1 and element2 are in the same shadow root Any
Any (sibling, cousin, ancestor, descendant, etc.) All shadow roots separating element1 from element2 are open
element2 is in a shadow-ancestor of element1 Any shadow root separating element1 from element2 is closed
Any other relationship Any shadow root separating element1 from element2 is closed

In the case of open shadow roots, both elements are already exposed. Whereas in the case of closed shadow roots, only ancestors are exposed (by walking up the tree). So this restriction would exactly match the status quo in terms of exposure.

annevk commented 2 years ago

Yeah that has been discussed and dismissed. We want a solution that works for open/closed. I recommend studying the links in @mrego's comment above: https://github.com/WICG/aom/issues/192#issuecomment-1122322990. In particular #169 and https://github.com/WICG/webcomponents/issues/917.

nolanlawson commented 2 years ago

Thanks, I read through the comment threads. In particular I see this comment:

The fact open mode allows a deliberate access isn't an excuse for adding new APIs that break encapsulation.

This is a fair point, but open shadow roots are the default for most web component frameworks that I'm aware of, and it seems odd to force open shadow roots to match closed shadow root semantics moving forward.

https://github.com/WICG/webcomponents/issues/917 talks about cross-root ARIA delegation, which doesn't address my scenario because it's not enough to merely delegate the <input>'s aria-activedescendant to its host – it still wouldn't have access to the list option (<li>), as each list option is inside its own shadow root.

Looking back through previous discussion, Alice sketched out a combobox in this comment. But this example assumes that the list option and listbox share the same shadow root, and that furthermore this shadow root is an ancestor of the <input>. This does not match our own usage pattern at Salesforce. (E.g. the search combobox in our CRM app has separate shadow roots for each list option, and the <input> is a distant cousin of the listbox. This works today only because we are using a shadow DOM polyfill.)

With the current limitation, putting the list options and listbox into the same shadow root, and ensuring that that shadow root is at least an ancestor of the <input> (or its host, with delegation), is certainly the only way to get ARIA reflection for ariaActiveDescendantElement to work properly. However, I think this kind of amounts to a mandate of "use light DOM, not shadow DOM" (i.e. use light DOM as much as possible to avoid crossing shadow boundaries).

annevk commented 2 years ago

I see, sorry you had to explain that again. I think in this case input has access to fancy-listbox so it could set that as ARIA descendant. And that could further delegate that to fancy-option. In principle that would be acceptable and hopefully a future version of that draft can address that.

nolanlawson commented 2 years ago

We discussed this a bit in the AOM meeting today. A high-level summary:

mrego commented 2 years ago
  • Per @mrego, the current implementations in WebKit and Chromium may do this, or they may just make the setter a no-op – needs some investigation.

During my tests on the meeting yesterday I was focusing on the wrong thing, sorry about my misleading comments.

So this is the proposed spec text:

On setting, the IDL attribute must perform the following steps:

  1. Let id be the empty string.

  2. If the given value:

    • has the same root as this element, and
    • has an id attribute, and
    • is the first element in this element's node tree whose ID is the value of that id attribute,

    then set id to the given value's ID.

  3. Set the content attribute's value for this element to id.

  4. Set this element's explicitly set attr-element to the given value.

I was focusing in the 2nd step there, but that's the one that updates the attribute to reflect the relationship. That one is not set in Chromium and WebKit when you do element1.ariaActiveDescendant = element2 (and element2 is not a descendant of a shadow-including ancestor), the ariaActiveDescendant attribute on element1 is set to the empty string.

But still 4th step above happens, and internally the explicitly set attr-element is set, and it points to element2. That's not exposed to the web author, and when you do element1.ariaActiveDescendant as @nolanlawson says you get null. But the question is if that could be exposed to the a11y tree, and if that could be enough.

mrego commented 2 years ago

But still 4th step above happens, and internally the explicitly set attr-element is set, and it points to element2. That's not exposed to the web author, and when you do element1.ariaActiveDescendant as @nolanlawson says you get null. But the question is if that could be exposed to the a11y tree, and if that could be enough.

It looks like this is not a good idea and people don't like it. Even from what I heard this might have been discussed and discarded in the past already. This looked like a kind of hacky workaround anyway, so we should discard this and look for better alternatives related to delegation approaches.

The proposed spec text is explicit about this:

Other parts of this specification, or other specifications using attribute reflection, are generally expected to consult the attr-associated element. The explicitly set attr-element is an internal implementation detail of the attr-associated element, and is not to be used directly.

mrego commented 2 years ago

@nolanlawson should we close this issue and carry on the related work on: https://github.com/leobalter/cross-root-aria-delegation/ and https://github.com/Westbrook/cross-root-aria-reflection/ trying to find a solution for these use cases?

mfreed7 commented 1 year ago

But still 4th step above happens, and internally the explicitly set attr-element is set, and it points to element2. That's not exposed to the web author, and when you do element1.ariaActiveDescendant as @nolanlawson says you get null. But the question is if that could be exposed to the a11y tree, and if that could be enough.

It looks like this is not a good idea and people don't like it. Even from what I heard this might have been discussed and discarded in the past already. This looked like a kind of hacky workaround anyway, so we should discard this and look for better alternatives related to delegation approaches.

I found my way here from https://github.com/WICG/aom/issues/195 (and https://github.com/whatwg/html/issues/8544 and https://github.com/whatwg/html/pull/8932), and I wanted to ask if someone could more explicitly state why this isn't a good solution? I.e. just allow the a11y tree to see/use explicitly set attr-element while attr-associated element still returns null. That would completely solve the encapsulation leakage problem, independent of whether the root was open or closed. And it feels like a much more straightforward solution than alternatives. I think I've read this issue and the related ones carefully, but I could not find the detailed description about why this was bad, other than "bad ergonomics". And by "bad ergonomics", it is simply that once you set an attr-associated element with a shadow-encapsulated value, you don't have a way to know that it "worked". To solve that problem, perhaps we could do something very simple like:

<div id=el1>
  <template shadowrootmode=open><div id=el2></div></template>
</div>
<div id=el3></div>
<script>
el1.ariaActiveDescendantElement === undefined // undefined means no reference has been established yet
const el2 = el1.shadowRoot.firstElementChild;
el1.ariaActiveDescendantElement = el2;
el1.ariaActiveDescendantElement === null // null means a reference has been established, but you can't see it because it's shadow-isolated
el1.ariaActiveDescendantElement = el3;
el1.ariaActiveDescendantElement === el3 // non-shadow-isolated values work as they do today
</script>
nolanlawson commented 1 year ago

I think I've read this issue and the related ones carefully, but I could not find the detailed description about why this was bad, other than "bad ergonomics".

This is what I recall from the AOM meeting, yes. I think someone used the word "gross." :slightly_smiling_face:

el1.ariaActiveDescendantElement === undefined // undefined means no reference has been established yet

Doesn't this violate the way most properties on Elements work? I'm used to seeing undefined for expandos that don't exist on an element at all, and null/-1/""/etc. for properties that exist, but haven't been set yet. E.g.:

const el = document.createElement('div')
el.role // null
el.ariaLabel // null
el.tabIndex // -1
el.title // ''
el.falalalala // undefined

Alternatively, I guess you could figure out that "it worked" by checking the aria-activedescendant attribute? Given https://github.com/whatwg/html/pull/8352, you should be able to do:

el.getAttribute('aria-activedescendant') === null // null means no reference has been established yet
el.ariaActiveDescendantElement = el.shadowRoot.firstElementChild
el.getAttribute('aria-activedescendant') === '' //  empty string means reference established
mfreed7 commented 1 year ago

I think I've read this issue and the related ones carefully, but I could not find the detailed description about why this was bad, other than "bad ergonomics".

This is what I recall from the AOM meeting, yes. I think someone used the word "gross." 🙂

Ok, thanks. It's different, but I'm not sure it's all the way to "gross". I mean https://github.com/whatwg/html/pull/8932 is not exactly gross either but it's getting more and more complicated.

el1.ariaActiveDescendantElement === undefined // undefined means no reference has been established yet

Doesn't this violate the way most properties on Elements work? I'm used to seeing undefined for expandos that don't exist on an element at all, and null/-1/""/etc. for properties that exist, but haven't been set yet. E.g.:

Yeah, this is also "gross". I guess my point was that perhaps we could get around the problem that you can't tell if a reference was established, in some simple way.

Alternatively, I guess you could figure out that "it worked" by checking the aria-activedescendant attribute? Given whatwg/html#8352, you should be able to do:

el.getAttribute('aria-activedescendant') === null // null means no reference has been established yet
el.ariaActiveDescendantElement = el.shadowRoot.firstElementChild
el.getAttribute('aria-activedescendant') === '' //  empty string means reference established

I think this is ok also! It meets the use case, and (other than potential web compat issues) I don't see any use cases it breaks.

alice commented 1 year ago

@mfreed7 I'm curious why you prefer some version of returning null/undefined to #195?

annevk commented 1 year ago

What @mfreed7 proposes reveals the existence of a shadow root as far as I can tell, so that does not seem workable. Apart from web developers having to do a lot of custom bookkeeping, which also seems undesirable. https://github.com/WICG/aom/issues/192#issuecomment-1172568392 still seems like the way forward here.

mfreed7 commented 1 year ago

@mfreed7 I'm curious why you prefer some version of returning null/undefined to #195?

I'm not against #195, per se. I was just poking at this to see if there's a simpler solution. #195 seems more complicated than just tweaking the IDL or content attribute behavior, and it is also limited to just custom elements.

What @mfreed7 proposes reveals the existence of a shadow root as far as I can tell, so that does not seem workable. Apart from web developers having to do a lot of custom bookkeeping, which also seems undesirable. #192 (comment) still seems like the way forward here.

Is the standard that we don't even reveal the presence of a shadow root? I'm unclear why we need to hold that level of encapsulation. The presence of a shadow root is already easily detectable just by observing layout, right?

annevk commented 1 year ago

Yes, that's been a design goal since the beginning.

mfreed7 commented 1 year ago

Yes, that's been a design goal since the beginning.

Hasn't that ship long sailed? This (or many similar techniques) works for all but trivial shadow roots, even if they're closed:

    function hasShadowRoot(maybeHost) {
      const oldChildren = Array.from(maybeHost.childNodes);
      Array.from(oldChildren).forEach(el => el.remove());
      const testBox = document.createElement('div');
      testBox.style.height = '100px'
      maybeHost.appendChild(testBox);
      const oldHeight = maybeHost.style.height;
      maybeHost.style.height = 'min-content';
      const hasShadow = getComputedStyle(maybeHost).height !== "100px";
      maybeHost.replaceChildren(...oldChildren);
      maybeHost.style.height = oldHeight;
      return hasShadow;
    }

I'm sure this was discussed, so perhaps you can point me to the conversation for context. But why is that a design goal?

alice commented 1 year ago

195 seems more complicated than just tweaking the IDL or content attribute behavior, and it is also limited to just custom elements.

Hm, it wasn't my intention that it be limited to custom elements - either I explained it poorly, or there's some implication in my reasoning that I failed to notice.

In terms of complication - I'm doubting myself now, but I figured it would be pretty analogous to this change - either way, computing attr-associated elements needs to change (since attr-associated elements represent the internal list which is exposed to AT), so it would be a change to the getter steps, either to null out elements within shadow roots, or to retarget them.

mfreed7 commented 1 year ago

195 seems more complicated than just tweaking the IDL or content attribute behavior, and it is also limited to just custom elements.

Hm, it wasn't my intention that it be limited to custom elements - either I explained it poorly, or there's some implication in my reasoning that I failed to notice.

Apologies! I didn't read #195 closely, and I (for some reason) assumed it was the driver for https://github.com/whatwg/html/pull/8932. Those are relatively disconnected, and I missed that.

I like the approach of #195, and it indeed seems general and not limited to custom elements. It is more complicated than the comments I've made above, but it seems tractable. Retargeting seems straightforward, and providing an API (like getComposedRanges()) that allows passing in known shadow roots to undo retargeting also seems straightforward.

keithamus commented 1 year ago

WCCG had their spring F2F in which this was discussed. You can read the full notes of the discussion (https://github.com/WICG/webcomponents/issues/978#issuecomment-1516897276) in which this was discussed, heading entitled "ARIA Mixin & Cross Root ARIA" - where this issue was specifically discussed.

In the meeting, present members of WCCG reached a consensus to discuss further in breakout sessions. I'd like to call out that https://github.com/WICG/webcomponents/issues/1005 is the tracking issue for that breakout, in which this will likely be discussed.