1638713 - Don't recreate entire subtree when CSS visibility on ancestor is changed

Status: NEW

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

Description

[James Teh [:Jamie]]

In bug 686400, most unnecessary a11y re-creations were eliminated. However, CSS visibility still causes re-creation.

STR:

1. Open this test case:
   data:text/html,<div id="content" style="visibility: visible;"><p>Maybe visible</p><p style="visibility: visible;">Always visible</p></div><button onClick="content.style.visibility = (content.style.visibility == 'visible') ? 'hidden' : 'visible';">Toggle visibility</button>
2. Get the unique id of the "Always visible" paragraph.
3. Press the Toggle visibility button.
   • Expected: The "Always visible" paragraph should have the same unique id, even though it has been re-parented to the document.
   • Actual: The "Always visible" paragraph has a different unique id.

When CSS visibility on an ancestor gets changed to hidden, we throw away and recreate the entire a11y subtree. I think this happens here:
https://searchfox.org/mozilla-central/rev/09b8072a543c145de2dc9bb76eddddd4a6c09adc/layout/base/RestyleManager.cpp#2572
I guess this would need to be changed so that it uses some other notification and a11y would need to remove the now invisible div without removing its descendants. That latter part is the tricky bit. I don't think a11y has any way of doing this right now. Even PruneOrInsertSubtree ends up calling ContentRemoved for things it wants to remove, which removes the entire subtree.

The other thing I can't figure out is when we want visibility: hidden elements to be in the a11y tree with the invisible state. In most cases, we don't render them in the a11y tree at all, yet we have specific code to expose the invisible state for visibility: hidden, suggesting there are cases where we want this.

Impact: This causes problems for NVDA browse mode when CSS visibility of an ancestor containing much of the content changes. For example, this page toggles CSS visibility on one of its main content areas (#cont-area) every few seconds. This content area contains tables, and if the user is trying to navigate in such a table, navigation fails whenever visibility is toggled on the ancestor.
Originally reported as NVDA issue: https://github.com/nvaccess/nvda/issues/9476

Comment 1

[James Teh [:Jamie]]

Note that this issue does not occur in Chrome. The paragraph maintains its id, even though it gets re-parented. As such, the NVDA browse mode problem does not occur there.

Comment 3

[Frédéric Wang (:fredw)]

(In reply to James Teh [:Jamie] (away returning 22 Jan) from bug 1772042 comment #0)

STR:

1. Open this test case:
   data:text/html,<div id="a" title="a"><p id="b" style="visibility: visible;">p</p><button onclick="a.style.visibility = 'hidden';">hide a
2. Get the Accessible for "p".
3. Press the "hide a" button.
4. Get the Accessible for "p".
   • Expected: The Accessibles should be the same (no re-creation).
   • Actual: The Accessible is different to that retrieved in step 2 (re-creation).

Bug 686400 prevented layout frame reconstruction from re-creating Accessibles. However, we still re-create in the CSS visibility case.

This occurs because layout explicitly removes visibility: hidden elements from the a11y tree. A11y still needs to be notified about these changes, but we should use our PruneOrInsertSubtree code to figure out what to do with them, rather than removing them unconditionally.

We can probably fix this by having RestyleManager call a11y's ContentRangeInserted notification rather than ContentRemoved. The naming is weird, though.

(In reply to James Teh [:Jamie] (away returning 22 Jan) from bug 1772042 comment #1)

Blah. Making the change in RestyleManager doesn't help because PruneOrInsertSubtree can't actually handle removing an ancestor while still keeping descendants. It calls ContentRemoved itself, and even though we then walk the subtree to reinsert nodes, they're already detached at that point (no longer in mNodeToAccessibleMap because of UncacheChildrenInSubtree), so nothing gets reused.

We'd need to find some way to fix this in PruneOrInsertSubtree first.

So do you think it would be possible to introduce a version of ContentRemoved that does not call UncacheChildrenInSubtree ; so that the nodes can are re-used when reparented, and we clear the cache afterwards for the nodes that were not reparented?

Comment 4

[Frédéric Wang (:fredw)]

Attachment: WIP: Bug 1638713 - Use ContentRemoved(nsIContent*) for XUL labels

Comment 5

[Frédéric Wang (:fredw)]

Attachment: WIP: Bug 1638713 - Move some PruneOrInsertSubtree steps before content removal

Comment 6

[Frédéric Wang (:fredw)]

Attachment: WIP: Bug 1638713 - Factor out per-node decisions for PruneOrInsertSubtree

Comment 7

[Frédéric Wang (:fredw)]

Attachment: WIP: Bug 1638713 - Make PruneOrInsertSubtree able to reuse accessible descendants.

Comment 8

[James Teh [:Jamie]]

(In reply to Frédéric Wang (:fredw) from comment #3)

So do you think it would be possible to introduce a version of ContentRemoved that does not call UncacheChildrenInSubtree ; so that the nodes can are re-used when reparented, and we clear the cache afterwards for the nodes that were not reparented?

Sorry I didn't get to answering this. Honestly, I'm not entirely certain how we need to fix this and I need to do some digging to figure it out. I see that in your WIP (abandoned) patches, you removed the Accessible from the root to prevent UncacheChildrenInSubtree from removing them from the map. That seems like it could be a good approach, but I'd need to look into whether this could cause any other problems.

At this point, while there is one use case where this causes real problems, I haven't seen any others reported. Given that this will probably take a bit of time to get right and the high risk of regressions, I don't think it makes sense to spend time on this right now given other priorities.

Comment 9

[Frédéric Wang (:fredw)]

@James: Thanks. The other issue that led me to this was content-visibility: hidden nodes, but at the end it turned out that the issue was different and didn't need this to be fixed.