1487311 - Accessibility doesn't associate ids in shadow DOM

Status: VERIFIED FIXED

(Core :: Disability Access APIs, defect, P3)

Description

[James Teh [:Jamie]]

STR:
1. Open this test case:
data:text/html,<div id="host"></div><script>shadow = host.attachShadow({mode: 'open'}); shadow.innerHTML = `<div id="label">Label</div><input type="text" aria-labelledby="label">`;</script>
2. Use the Accessibility Inspector to examine the name of the input.
Expected: "label"
Actual: null

Same applies for a <label for>:
shadow.innerHTML = `<label for="input">label</label><input type="text" id="input">`;

In fixing this, we need to make sure that we don't support id association the other way. That is, I don't think we want ARIA properties in an outer document to be able to refer to ids in a shadow DOM.

Comment 1

[Olli Pettay [:smaug][[e-mail]]]

Any news on this?

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

Note, Document and ShadowRoot have their own ID tracking.
https://searchfox.org/mozilla-central/rev/de7676288a78b70d2b9927c79493adbf294faad5/dom/base/DocumentOrShadowRoot.h#219
That doesn't cross shadow boundaries.

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

Any news on this.

Comment 4

[alexander :surkov (:asurkov)]

We should introduce dependent IDs hash per shadow root similar to what we do for DocAccessible::mDependentIDsHash. It could be a map between ShadowRootOrDocument and dependent IDs hashes.

I will try to play with this week, if no one feels hot to pick it up :)

Comment 5

[alexander :surkov (:asurkov)]

Attachment: patch

r? from jamie, f? from olli for sanity check

Comment 6

[James Teh [:Jamie]]

Comment on attachment 9019653 [details] [diff] [review]
patch

Review of attachment 9019653 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks!

::: accessible/generic/DocAccessible-inl.h
@@ +224,5 @@
> +  if (hash) {
> +    AttrRelProviders* providers = hash->Get(aID);
> +    if (providers && providers->Length() == 0) {
> +      hash->Remove(aID);
> +      mDependentIDsHashes.Remove(docOrShadowRoot);

This removes the DependentIDsHashtable for the entire DocumentOrShadowRoot if there are no providers for a given id. But there might still be providers for *other ids*. I think there needs to be an additional check here for the size of hash?

@@ +229,5 @@
> +    }
> +  }
> +}
> +
> +

nit: Extraneous blank line.

::: accessible/generic/DocAccessible.cpp
@@ +120,5 @@
> +  for (auto iter = tmp->mDependentIDsHashes.Iter(); !iter.Done(); iter.Next()) {
> +    auto dependentIDsHash = iter.UserData();
> +    for (auto jter = dependentIDsHash->Iter(); !iter.Done(); iter.Next()) {
> +      AttrRelProviders* providers = jter.UserData();
> +      for (int32_t jdx = providers->Length() - 1; jdx >= 0; jdx--) {

nit: I'd prefer clearer names than iter, jter and jdx. Perhaps depidsIter, provsIter and provIter? At the very least, keeping the original pattern, jdx should be renamed kdx.

::: accessible/generic/DocAccessible.h
@@ +677,5 @@
> +  typedef nsTArray<nsAutoPtr<AttrRelProvider> > AttrRelProviders;
> +  typedef nsClassHashtable<nsStringHashKey, AttrRelProviders> DependentIDsHashtable;
> +
> +  /**
> +   * Reutnrs/creates/removes attribute relation providers associated with

nit: Reutnrs -> Returns

::: accessible/tests/mochitest/relations/test_shadowdom.html
@@ +22,5 @@
> +      // explicit content
> +      let label = iframeDoc.getElementById("label");
> +      let element = iframeDoc.getElementById("element");
> +      //testRelation(label, RELATION_LABEL_FOR, element);
> +      //testRelation(element, RELATION_LABELLED_BY, label);

Why are these tests commented out? I think they're important. :)

Comment 7

[alexander :surkov (:asurkov)]

Attachment: patch

Comment 8

[James Teh [:Jamie]]

Comment on attachment 9019915 [details] [diff] [review]
patch

Review of attachment 9019915 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with concerns answered/addressed.

::: accessible/tests/mochitest/relations/test_shadowdom.html
@@ +20,5 @@
> +      let iframeDoc = document.getElementById("iframe").contentDocument;
> +
> +      // explicit content
> +      let label = iframeDoc.getElementById("label");
> +      let element = iframeDoc.getElementById("element");

Do these currently pass? These ids don't seem to be present in the iframe, unless I'm missing something...

@@ +49,5 @@
> +      iframe.id = "iframe";
> +      iframe.src = `data:text/html,<html>
> +        <body>
> +          <div id='shadowcontainer'></div>
> +          <${sc}>

Sorry, forgot to ask about this in the last review. Is there a reason you can't just do <script> directly here?

@@ +64,5 @@
> +    });
> +  </script>
> +<!--
> +  <div id='label'></div><div id='element' aria-labelledby='label'></div>
> +-->

I guess this should be in the iframe, but not commented out?

Comment 9

[alexander :surkov (:asurkov)]

(In reply to James Teh [:Jamie] from comment #8)
> Comment on attachment 9019915 [details] [diff] [review]
> patch
> 
> Review of attachment 9019915 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=me with concerns answered/addressed.
> 
> ::: accessible/tests/mochitest/relations/test_shadowdom.html
> @@ +20,5 @@
> > +      let iframeDoc = document.getElementById("iframe").contentDocument;
> > +
> > +      // explicit content
> > +      let label = iframeDoc.getElementById("label");
> > +      let element = iframeDoc.getElementById("element");
> 
> Do these currently pass? These ids don't seem to be present in the iframe,
> unless I'm missing something...

> @@ +64,5 @@
> > +    });
> > +  </script>
> > +<!--
> > +  <div id='label'></div><div id='element' aria-labelledby='label'></div>
> > +-->
> 
> I guess this should be in the iframe, but not commented out?

artifacts from debugging, try reports are delaying :) will fix it

> @@ +49,5 @@
> > +      iframe.id = "iframe";
> > +      iframe.src = `data:text/html,<html>
> > +        <body>
> > +          <div id='shadowcontainer'></div>
> > +          <${sc}>
> 
> Sorry, forgot to ask about this in the last review. Is there a reason you
> can't just do <script> directly here?
> 

yes, to not confuse the parser

Comment 10

[alexander :surkov (:asurkov)]

Attachment: patch

the tests pass

Comment 11

[James Teh [:Jamie]]

Comment on attachment 9019925 [details] [diff] [review]
patch

Thanks!

Comment 12

[Olli Pettay [:smaug][[e-mail]]]

(IIRC there are still other shadow DOM + a11y issues)

Comment 13

[alexander :surkov (:asurkov)]

(In reply to Olli Pettay [:smaug] (high review load) from comment #12)
> (IIRC there are still other shadow DOM + a11y issues)

right, but this one is probably the biggest one

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/052d5b7a515e
accessibility doesn't assosiate ids in shadow DOM, r=jamie, sr=smaug

Comment 15

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/052d5b7a515e

Comment 17

[Gabi Cheta [:Gabi] Desktop QA]

Verified as fixed with 65.0b12 on Linux 16.4, Windows 10x64 and MacoS 10.13