Closed Bug 1689936 Opened 3 years ago Closed 3 years ago

Some tab documents completely missing from a11y tree on startup

Categories

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

defect

Tracking

()

RESOLVED FIXED
87 Branch
Tracking Status
firefox-esr78 --- unaffected
firefox85 --- unaffected
firefox86 --- unaffected
firefox87 --- fixed

People

(Reporter: Jamie, Assigned: Jamie)

References

(Regression)

Details

(Keywords: regression)

Attachments

(1 file)

As of last week, when starting Firefox and restoring tabs from the previous session, Accessibles for tabs other than the foreground tab are sometimes completely broken. The property page Accessible is completely missing and the document (or anything in it) can't get focus.

mozregression suggests this is due to bug 493683:

12:19.47 INFO: Last good revision: 37cbc73b07c3e0b8d9441bea2beae71ecdc1dd14
12:19.47 INFO: First bad revision: 2c9004c95a7a7d39c2c1417b66fda37892398263
12:19.47 INFO: Pushlog:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=37cbc73b07c3e0b8d9441bea2beae71ecdc1dd14&tochange=2c9004c95a7a7d39c2c1417b66fda37892398263

I can't fathom how those patches could cause this, but I guess I'll start digging. :)

Set release status flags based on info from the regressing bug 493683

Wow. This is a mind boggler.

  1. We process pending insertions and start handling the insertion of a XULTabAccessible.
  2. BindToParent gets called for this XULTabAccessible.
  3. As part of that, we query the LABEL_FOR relation (to determine whether we need to set the eHasNameDependent flag).
  4. That results in a call to XULTabAccessible::RelationByType, which calls nsIDOMXULRelatedElement::getRelatedElement. Unfortunately, getRelatedElement is implemented in JS.
  5. When we call into JS, other stuff runs, causing the insertion of a tabpanel.
  6. A11y ends up dropping this insertion, probably because it was processing other insertions and mContentInsertions was mutated while it was being iterated.

I see a few potential solutions here:

  1. hack: Change Accessible::BindToParent to avoid checking LABEL_FOR for a XULTabAccessible.
  2. Change Accessible::BindToParent to specifically call RelatedAccIterator for at aria-labelledby, instead of using the LABEL_FOR relation. This doesn't handle other cases of LABEL_FOR and is thus less versatile, but we (probably) don't care about other cases of LABEL_FOR at this stage anyway.
  3. Change XULTabAccessible::RelationByType to do the calculation itself instead of using nsIDOMXULRelatedElement.
  4. Change XUL tabpanels to use aria-labelledby instead of custom C++ code. This could be tricky because the tabs don't have ids. Also, the tabpanels don't seem to have a name right now, despite the relation, but using aria-labelledby would give them a name, which might impact AT.
  5. Remove the LABEL_FOR relation on XULTabAccessible. Might impact AT.
  6. Fix the a11y engine to be more tolerant of insertions queued during an insertion. For example, swap mContentInsertions into a temporary map before processing. That said, running JS during a tree update feels pretty fragile to me.

Eitan, any thoughts/preferences?

Flags: needinfo?(eitan)

Personally, I think I'm leaning towards option 2 at this stage, though it's less clean than I'd like.

Huh. With no logical justification, I can live with options 1, 3 and 6 :)

I think 6 is risky, might have some other kinds of fallout, so not sure if it is the best idea, although it is the most "correct".

I like options 1 and 3 because they limit the fix/hack to XULTabAccessible.

Flags: needinfo?(eitan)
Pushed by jteh@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/02335cb3c1af
Move pending a11y content insertions into a temporary data structure before processing them. r=eeejay
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → 87 Branch
See Also: → 1700708
Has Regression Range: --- → yes
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: