1806026 - [CtW] Gmail labels have incorrect y coordinates

Status: VERIFIED FIXED

(Core :: Disability Access APIs, defect)

Description

[Morgan Reschenberg [:morgan]]

STR:

1. Sign in to gmail
2. Locate the panel that contains labels like "Inbox, Starred, Sent, Spam" etc.
3. Navigate to "Starred" using VO

Expected
The VO cursor bounds the "Starred" text

Actual:
The VO cursor bounds the "Inbox" text

Comment 1

[James Teh [:Jamie]]

Morgan noted on Slack:

Looked into that y-coordinate bounds issue on gmail. I can reproduce the bug, and it goes away if I remove the “will-change” css property on one of the label’s ancestors (I forget exactly which but there’s only one). Suspect transforms are involved here … but couldn’t find any via the inspector so. Need to dig more.

My curiosity got the better of me and I'm keenly aware we're heading towards holidays, so I did some digging here. Findings:

1. There doesn't have to be a transform. The presence of will-change: transform means that nsIFrame::IsTransformed will return true.
2. That means we cache a transform matrix. In this case, we end up caching the identity matrix; i.e. applying it just returns exactly what we put in.
3. Because transforms are self-relative and we thus remove the parent relative offset before applying the transform, this means we remove the parent relative offset for all the children. In the cache, you can see that each child has an increasing y coordinate in its relative-bounds, but we remove that, resulting in them all being 0.
4. I tried working around that by writing a patch which never caches the identity matrix. That fixes the Gmail issue. Aside from all of the below, I think that makes sense anyway - the identity matrix is effectively redundant.
5. Unfortunately, that doesn't fix the problem fully. If you apply a translation transform to a child of the document, you don't get the identity matrix because there's some default translation to the root frame (probably padding or something).
   data:text/html,<div role="main" style="will-change: transform;"><p>hello</p><p>world
   In this example, we don't get the identity matrix, so my workaround in 4) never applies. The bounds for the "hello" are correct, but the bounds for "world" are wrong.
6. This led me to realise that our handling of translation transforms is just broken across the board. It works fine for the first child, but it breaks for subsequent children. Consider this:
   data:text/html,<div role="main" style="transform: 0 300px;"><p>hello</p><p>world
   Again, the bounds for "hello" are correct, but the bounds for "world" are wrong; "world" gets the same y coordinate as "hello".
7. Even though I realise we're removing the parent relative offset when we apply a transform, I thought we only did that for the node with the transform, not its descendants. I don't yet understand why we seem to be removing the parent relative offset of children.
8. As a side note, if an element with a transform doesn't get an Accessible, we don't handle the transform correctly at all because it's not cached on anything. Consider this:
   data:text/html,<div style="translate: 0 300px;"><p>hello
   In this case, hello should be translated down 300px, but it isn't because the div has no Accessible and thus the transform isn't cached.

Comment 2

[James Teh [:Jamie]]

7. Even though I realise we're removing the parent relative offset when we apply a transform, I thought we only did that for the node with the transform, not its descendants. I don't yet understand why we seem to be removing the parent relative offset of children.

Oh, i see. When applying a transform on an ancestor, we pass in the cumulative bounds, not just the parent relative bounds. But then in ApplyTransform, we move the bounds to 0, 0 to remove the parent relative offset. That completely kills the cumulative bounds for all descendants calculated so far.

Comment 3

[James Teh [:Jamie]]

Attachment: Bug 1806026 part 1: When applying a transform in RemoteAccessible, remove only the parent relative offset, not the cumulative offset calculated so far.

When applying an ancestor transform, ApplyTransform is supplied with the cumulative bounds calculated so far; i.e. from descendants.
When removing the parent relative offset, the code previously set the top left to (0, 0).
This not only removed the parent relative offset, but also the cumulative offset calculated for all descendants!
Instead, we now pass in the cumulative bounds as well as the parent relative bounds.
We only subtract the parent relative bounds.

Comment 4

[James Teh [:Jamie]]

Attachment: Bug 1806026 part 2: Don't include the identity matrix when caching transforms for a11y.

See the code comments for details.
This is just a memory optimisation; there is no user visible change.

Comment 5

[James Teh [:Jamie]]

(In reply to James Teh [:Jamie] from comment #1)

8. As a side note, if an element with a transform doesn't get an Accessible, we don't handle the transform correctly at all because it's not cached on anything.

Filed bug 1806356 for this.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6eaa7e0e5fe4
part 1: When applying a transform in RemoteAccessible, remove only the parent relative offset, not the cumulative offset calculated so far. r=morgan
https://hg.mozilla.org/integration/autoland/rev/e2bbf722e41e
part 2: Don't include the identity matrix when caching transforms for a11y. r=morgan

Comment 7

[Serban Stanca [:SerbanS]]

Backed out for causing mochitests failures in browser_test_simple_transform.js.

• Backout link
• Push with failures
• Failure Log
• Failure line: TEST-UNEXPECTED-FAIL | accessible/tests/browser/bounds/browser_test_simple_transform.js | willChangeTop has no cached transform -

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/244d9623daaa
part 1: When applying a transform in RemoteAccessible, remove only the parent relative offset, not the cumulative offset calculated so far. r=morgan
https://hg.mozilla.org/integration/autoland/rev/8b70e7042d56
part 2: Don't include the identity matrix when caching transforms for a11y. r=morgan

Comment 9

[Sandor Molnar[:smolnar]]

https://hg.mozilla.org/mozilla-central/rev/244d9623daaa
https://hg.mozilla.org/mozilla-central/rev/8b70e7042d56

Comment 10

[Monica Chiorean]

I was able to reproduce the issue on MAC 10.13 using FF build 109.0a1.
Verified as fixed on Mac 10.13 using FF build 110.0b9(20230202190127) and 111.0a1.