2022255 - translate: 0 on <body> creates a generic accessible

Status: NEW

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

Description

[Emilio Cobos Álvarez [:emilio]]

In bug 2013773, I tried to add translate: 0 to the body in browser.xhtml. That makes a tiny bit difference in performance in the macOS fullscreen animation with that patch, but it shouldn't otherwise change behavior, since the element is already a containing block for fixed-position descendants (via contain: paint). The relevant rule I added it to, for the purposes of reproducing locally, is:

• https://searchfox.org/firefox-main/rev/021cb4edd621e1f5d60ed62409587d6cf1d02845/browser/themes/shared/browser-shared.css#164-171

However, that caused failures in accessible/tests/browser/mac/browser_app.js. The failures are legit, there's an extra accessibility tree node for the <body> element... They may be harmless I guess, but I wasn't sure whether I should change the expectations or not so for now I worked around it by removing the CSS.

I'm assuming this comes from this check, however we don't recreate the accessibility tree when transform changes (that would be really bad)... So is that correct? Plus the transform in this case was an identity transform...

What's the real intention of that code? Should it check if there's any non-identity transform? But that's also not great as long as we don't recreate the a11y subtree when that changes...

Also, drive-by, but looking at it, that overflow check is wrong, if you have something like overflow: hidden auto;... You want aFrame->StyleDisplay()->IsScrollableOverflow() (or aFrame->IsScrollContainerFrame()?).

Comment 1

[James Teh [:Jamie]]

I'm assuming this comes from this check

I'm fairly sure, yeah.

however we don't recreate the accessibility tree when transform changes (that would be really bad)... So is that correct?

Not entirely. We haven't found a case where a mutation here has been a problem, but we might one day. FWIW, we wouldn't have to recreate the accessibility tree to solve this, but we would need to re-parent, which isn't great for some clients but is much better than re-creation for Gecko.

Plus the transform in this case was an identity transform...

True. I can't quite recall, but my guess is that the thinking was that if something has an identity transform, the likelihood of it becoming a non-identity transform later (e.g. CSS animation) is much higher than if it had no transform at all in the first place.

What's the real intention of that code?

The comment above the code is correct. We need this in order for the cache to be correct, since the transform is stored on the Accessible associated with the element that has the transform.

Strictly speaking, we could disable this for chrome documents, since they don't get a RemoteAccessible cache. We don't want to disable it entirely for the parent process though because we want content documents to be the same between parent and remote for testing purposes if nothing else. That's more of a wallpaper than a real fix, but it's probably fine given that this whole thing is really a bit of a hack to massage transforms into the cache.

Comment 2

[Morgan Reschenberg [:morgan]]

(In reply to Emilio Cobos Álvarez [:emilio] from comment #0)

I'm assuming this comes from this check, however we don't recreate the accessibility tree when transform changes (that would be really bad)... So is that correct? Plus the transform in this case was an identity transform...

What's the real intention of that code? Should it check if there's any non-identity transform? But that's also not great as long as we don't recreate the a11y subtree when that changes...

To add to Jamie's answer above, you're right. We really should exclude force creating + caching + applying the identity transform, I don't think we considered that at the time of writing but it might save us a bit of perf overhead and exclude unnecessary cases like this.

Also, drive-by, but looking at it, that overflow check is wrong, if you have something like overflow: hidden auto;... You want aFrame->StyleDisplay()->IsScrollableOverflow() (or aFrame->IsScrollContainerFrame()?).

Thank you ! I'll update this :)