1693411 - Elements with visibility hidden remain in accessibility tree

Status: VERIFIED FIXED

(Core :: Disability Access APIs, defect)

Description

[libucha]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 11_2_1) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/88.0.4324.150 Safari/537.36

Steps to reproduce:

I am a google employee that works on the help panel product. I discovered this bug while performing accessibility testing. Since this bug is not reproducible in Chrome and is reproducible with both NVDA and JAWS I am led to believe this is a Firefox issue.

1. Turn on NVDA screenreader
2. Navigate to play.google.com
3. Click on the "Help" button in the top right corner. This will open up the Google Help Panel. System focus should move to the search bar of the help panel.
4. Click on an article in the Popular Resources section of the help panel.
5. An article should be rendered in the help panel. Move to the next line/element with the screenreader.

Actual results:

After completing step 4 NVDA remains on the clicked article element and navigating to the next line will move to the next recommended article in the previous view. The previous view is still navigable via the screenreader even though it has been marked with the visibility: hidden css attribute. This is not reproducible in Chrome and is reproducible in Firefox with the JAWS screenreader.

Expected results:

When the help panel switches views, the system focus moves to the new frame with the article content. NVDA should follow and should no longer be able to access the elements that have had the visibility: hidden css rule applied to them.

Comment 1

[BugBot [:suhaib / :marco/ :calixte]]

The Bugbug bot thinks this bug should belong to the 'Core::Disability Access APIs' component, and is moving the bug to that component. Please revert this change in case you think the bot is wrong.

Comment 2

[James Teh [:Jamie]]

Confirmed. This is pretty nasty.

Normally, visibility: hidden content does get removed from the a11y tree. I don't know why it's breaking in this case. I suspect fallout from bug 686400, but I need to verify that.

Comment 3

[James Teh [:Jamie]]

I tracked it down to visibility: hidden combined with position: fixed. That is, visibility: hidden by itself does prune the a11y tree:

data:text/html,<div id="test">Test</div><button onclick="test.style = 'visibility: hidden;'">Hide</button>

but if you add position: fixed, it doesn't prune the a11y tree:

data:text/html,<div id="test">Test</div><button onclick="test.style = 'visibility: hidden; position: fixed;'">Hide</button>

I don't know why yet.

Comment 4

[James Teh [:Jamie]]

I tested Firefox 69 and 70 and it seems this bug was introduced in 70 (where bug 686400 landed), so I'm fairly sure that's the regressing bug.

Rather than removing Accessibles when they go invisible, we now wait for layout to notify of an insertion when the frame gets reconstructed. Early investigations suggest that when position: fixed is set, we don't get an insertion notification from layout when visibility: hidden is set. I guess maybe the frame isn't reconstructed in that case? But it worked before bug 686400, which suggests it fires a removal, and I've confirmed it still fires a removal (with REMOVE_FOR_RECONSTRUCTION). So why fire a reconstruction removal but not an insertion?

Comment 5

[James Teh [:Jamie]]

Emilio, do you have any top-of-your-head thoughts concerning comment 4? Happy to look into this further myself, but I'd be grateful for any starting thoughts you could offer. Thanks.

Comment 6

[James Teh [:Jamie]]

Another thing that's confusing me: I would have thought that because this is getting visibility: hidden, it should go through the code path here:
https://searchfox.org/mozilla-central/rev/e84baf5c730abd453be5d50dcb0aba1e743bac47/layout/base/RestyleManager.cpp#2567
but instead, it seems to be getting here (but after bug 686400, it doesn't run because the reconstruction flag is set):
https://searchfox.org/mozilla-central/rev/e84baf5c730abd453be5d50dcb0aba1e743bac47/layout/base/nsCSSFrameConstructor.cpp#7428

Comment 7

[Emilio Cobos Álvarez (:emilio)]

So we don't go through the visibility notifications because we have two style changes, one of them which reconstructs the frame. This should also reproduce with other kinds of reframe like this:

data:text/html,<div id="test">Test</div><button onclick="test.style = 'visibility: hidden; display: inline;'">Hide</button>

So something like this should fix it, but might need some tweaks for visibility: visible inside visibility: hidden or such? I don't recall off-hand how we dealt with that in a11y.

diff --git a/accessible/generic/DocAccessible.cpp b/accessible/generic/DocAccessible.cpp
index 5b9c1d1a62a7..3656e9c74575 100644
--- a/accessible/generic/DocAccessible.cpp
+++ b/accessible/generic/DocAccessible.cpp
@@ -1520,6 +1520,12 @@ bool DocAccessible::PruneOrInsertSubtree(nsIContent* aRoot) {
       return false;
     }
 
+    // Frame is invisible, remove it.
+    if (frame && !frame->StyleVisibility()->IsVisible()) {
+      ContentRemoved(aRoot);
+      return false;
+    }
+
     // If it's a XULLabel it was probably reframed because a `value` attribute
     // was added. The accessible creates its text leaf upon construction, so we
     // need to recreate. Remove it, and schedule for reconstruction.

Comment 8

[James Teh [:Jamie]]

Oh, durp. So layout does notify a11y of an insertion; a11y just isn't expecting a visibility: hidden insertion and so doesn't handle it. I'm sorry. 😳

Handling visible inside of hidden or the like could be interesting. A11y currently relies on layout to reinsert visible descendants via that SendA11yNotifications thing. We should be able to handle this via PruneOrInsertSubtree, though.

Comment 9

[James Teh [:Jamie]]

Attachment: Bug 1693411: Make sure an a11y node is removed when visibility: hidden is set but the layout frame is reconstructed; e.g. position: fixed.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/677fa397ffba
Make sure an a11y node is removed when visibility: hidden is set but the layout frame is reconstructed; e.g. position: fixed. r=eeejay

Comment 11

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/677fa397ffba

Comment 12

[Ionuț Goldan [:igoldan]]

Noticed this performance win! 🎉

== Change summary for alert #28902 (as of Wed, 24 Feb 2021 12:00:15 GMT) ==

Improvements:

Ratio	Suite	Test	Platform	Options	Absolute values (old vs new)
15%	twitter	FirstVisualChange	linux64-shippable-qr	cold nocondprof webrender	240.00 -> 203.33

For up to date results, see: https://treeherder.mozilla.org/perfherder/alerts?id=28902

Comment 13

[Vlad Lucaci, Thunderbird QA (:vlucaci)]

Hello ,

Confirming this issue as verified fixed using Win10x64(JAWS 2021, NVDA) and macOS 10.15.7(VoiceOver) with 89.0a1(20210413214314) and 88.0(20210412175251).