Closed Bug 1706814 Opened 4 years ago Closed 3 years ago

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

Categories

(Core :: Disability Access APIs, defect)

defect

Tracking

()

RESOLVED FIXED
90 Branch
Tracking Status
firefox-esr78 --- wontfix
firefox88 --- wontfix
firefox89 --- wontfix
firefox90 --- fixed

People

(Reporter: Jamie, Assigned: eeejay)

References

(Blocks 1 open bug, Regression)

Details

(Keywords: regression)

Attachments

(1 file)

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".

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.

Keywords: regression
Regressed by: 1487311
Has Regression Range: --- → yes

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

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) {

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?

Flags: needinfo?(eitan)

(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?

Flags: needinfo?(eitan)

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

Assignee: nobody → eitan
Status: NEW → ASSIGNED
Pushed by eisaacson@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/360ce2bd515d Allow adding relation providers before dependend content created. r=Jamie
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → 90 Branch
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: