1706814 - aria-owns doesn't work if owned element is added to tree later

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[James Teh [:Jamie]]

STR:

1. Open this test case:
   data:text/html,<div role="group" aria-label="owner" id="owner" aria-owns="owned"></div><button onclick="let owned = document.createElement('div'); owned.id = 'owned'; owned.textContent = 'owned'; owned.setAttribute('aria-label', 'owned'); document.body.append(owned);">Add owned</button>
2. Press the button.
3. Look at the a11y tree in the a11y inspector.
   • Expected: section:"owned" should be inside grouping:"owner".
   • Actual: section:"owned" is outside grouping:"owner".

Comment 1

[James Teh [:Jamie]]

3:52.84 INFO: Last good revision: 4c7772c170a1848c4e57fea0087c351fd2288859 (2018-10-29)
3:52.84 INFO: First bad revision: be32f4014f92d0ab717621997e0d36c9bc1c479b (2018-10-30)
3:52.85 INFO: Pushlog:
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=4c7772c170a1848c4e57fea0087c351fd2288859&tochange=be32f4014f92d0ab717621997e0d36c9bc1c479b

Unfortunately, this is a nightly regression window; these builds are too old to have Taskcluster artifacts. Looking briefly, the only core a11y landing I see is:

Alexander Surkov — Bug 1487311 - accessibility doesn't assosiate ids in shadow DOM, r=jamie, sr=smaug
https://hg.mozilla.org/mozilla-central/rev/052d5b7a515ec057ee9c0cee231ee8b8413bdb3c

That seems plausible, so marking that as the regressing bug for now.

Comment 2

[James Teh [:Jamie]]

Note that this is causing problems for work on pdf.js tagged PDF support.

Comment 3

[Brendan Dahl [:bdahl]]

I think I see the issue, but I'm not sure on the proper way to fix it yet. The above regressing patch changed DocAccessible::AddDependentIDsFor to only insert into mDependentIDsHashes if the dependent id is found. Which in our case the element is not found since we insert it later. Before this patch a provider was inserted into mDependentIDsHash regardless of whether it was found.

I'm not quite sure how to fix this because the new lookup is based on the dependents elements document and id, and since the element doesn't exist yet in our case we don't have a document for a key. We could use the document from the provider element, but I'm not sure if that would work with shadow DOM insertions.

Here's a simple patch that fixes my use case:

diff --git a/accessible/generic/DocAccessible.cpp b/accessible/generic/DocAccessible.cpp
index 7dad619f736ea..75489d4c9cc2e 100644
--- a/accessible/generic/DocAccessible.cpp
+++ b/accessible/generic/DocAccessible.cpp
@@ -1876,14 +1876,13 @@ void DocAccessible::AddDependentIDsFor(LocalAccessible* aRelProvider,
       if (id.IsEmpty()) break;
 
       nsIContent* dependentContent = iter.GetElem(id);
-      if (!dependentContent ||
-          (relAttr == nsGkAtoms::aria_owns &&
+      if ((dependentContent && relAttr == nsGkAtoms::aria_owns &&
            !aRelProvider->IsAcceptableChild(dependentContent))) {
         continue;
       }
 
       AttrRelProviders* providers =
-          GetOrCreateRelProviders(dependentContent->AsElement(), id);
+          GetOrCreateRelProviders(dependentContent ? dependentContent->AsElement() : relProviderEl, id);
       if (providers) {
         AttrRelProvider* provider = new AttrRelProvider(relAttr, relProviderEl);
         if (provider) {

Comment 4

[James Teh [:Jamie]]

Thanks for investigating!

(In reply to Brendan Dahl [:bdahl] (84 regression triage) from comment #3)

We could use the document from the provider element, but I'm not sure if that would work with shadow DOM insertions.

This would need some more thought, but I think using the provider element's document or shadow root is okay because in order for the id association to be valid, the two elements must exist within the same DOM scope. I only looked briefly so far, but it looks like this is what your patch does?

Eitan, would you have some cycles to work on this?

Comment 5

[Eitan Isaacson [:eeejay]]

(In reply to James Teh [:Jamie] from comment #4)

Thanks for investigating!

(In reply to Brendan Dahl [:bdahl] (84 regression triage) from comment #3)

We could use the document from the provider element, but I'm not sure if that would work with shadow DOM insertions.

This would need some more thought, but I think using the provider element's document or shadow root is okay because in order for the id association to be valid, the two elements must exist within the same DOM scope. I only looked briefly so far, but it looks like this is what your patch does?

Yeah. Pretty sure that is true. If an element refers to another's ID, it would need to be in the same document/shadow root scope. So the provider element should be fine. I'll work something out with a test and Brendan's changes above.

Eitan, would you have some cycles to work on this?

Comment 6

[Eitan Isaacson [:eeejay]]

Attachment: Bug 1706814 - Allow adding relation providers before dependend content created. r?Jamie

I entirely removed the acceptable child check because that is done later
when the aria-owns is processed.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/360ce2bd515d
Allow adding relation providers before dependend content created. r=Jamie

Comment 8

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/360ce2bd515d