Closed Bug 1487311 Opened 1 year ago Closed 1 year ago

Accessibility doesn't associate ids in shadow DOM

Categories

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

defect

Tracking

()

VERIFIED FIXED
mozilla65
Tracking Status
firefox65 --- verified

People

(Reporter: Jamie, Assigned: surkov)

References

Details

Attachments

(1 file, 2 obsolete files)

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.
Any news on this?
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.
Any news on this.
Flags: needinfo?(surkov.alexander)
Flags: needinfo?(jteh)
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 :)
Flags: needinfo?(surkov.alexander)
Attached patch patch (obsolete) — Splinter Review
r? from jamie, f? from olli for sanity check
Assignee: nobody → surkov.alexander
Flags: needinfo?(jteh)
Attachment #9019653 - Flags: review?(jteh)
Attachment #9019653 - Flags: feedback?(bugs)
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. :)
Attachment #9019653 - Flags: review?(jteh)
Attached patch patch (obsolete) — Splinter Review
Attachment #9019653 - Attachment is obsolete: true
Attachment #9019653 - Flags: feedback?(bugs)
Attachment #9019915 - Flags: review?(jteh)
Attachment #9019915 - Flags: feedback?(bugs)
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?
(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
Attached patch patchSplinter Review
the tests pass
Attachment #9019915 - Attachment is obsolete: true
Attachment #9019915 - Flags: review?(jteh)
Attachment #9019915 - Flags: feedback?(bugs)
Attachment #9019925 - Flags: superreview?(bugs)
Attachment #9019925 - Flags: review?(jteh)
Comment on attachment 9019925 [details] [diff] [review]
patch

Thanks!
Attachment #9019925 - Flags: review?(jteh) → review+
Attachment #9019925 - Flags: superreview?(bugs) → superreview+
(IIRC there are still other shadow DOM + a11y issues)
(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
Pushed by surkov.alexander@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/052d5b7a515e
accessibility doesn't assosiate ids in shadow DOM, r=jamie, sr=smaug
https://hg.mozilla.org/mozilla-central/rev/052d5b7a515e
Status: NEW → RESOLVED
Closed: 1 year ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla65
Duplicate of this bug: 1502379
Flags: qe-verify+

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

Status: RESOLVED → VERIFIED
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.