[CtW] Gmail labels have incorrect y coordinates
Categories
(Core :: Disability Access APIs, defect)
Tracking
()
Tracking | Status | |
---|---|---|
firefox110 | --- | verified |
People
(Reporter: morgan, Assigned: Jamie)
References
Details
(Whiteboard: [ctw-m4])
Attachments
(2 files)
STR:
- Sign in to gmail
- Locate the panel that contains labels like "Inbox, Starred, Sent, Spam" etc.
- Navigate to "Starred" using VO
Expected
The VO cursor bounds the "Starred" text
Actual:
The VO cursor bounds the "Inbox" text
Reporter | ||
Updated•2 years ago
|
Assignee | ||
Updated•2 years ago
|
Assignee | ||
Comment 1•2 years ago
|
||
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:
- There doesn't have to be a transform. The presence of will-change: transform means that nsIFrame::IsTransformed will return true.
- 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.
- 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.
- 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.
- 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. - 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". - 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.
- 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.
Assignee | ||
Comment 2•2 years ago
|
||
- 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.
Assignee | ||
Comment 3•2 years ago
|
||
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.
Assignee | ||
Comment 4•2 years ago
|
||
See the code comments for details.
This is just a memory optimisation; there is no user visible change.
Assignee | ||
Updated•2 years ago
|
Assignee | ||
Comment 5•2 years ago
|
||
(In reply to James Teh [:Jamie] from comment #1)
- 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.
Pushed by jteh@mozilla.com: 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•2 years ago
|
||
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 -
Assignee | ||
Updated•2 years ago
|
Pushed by jteh@mozilla.com: 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•2 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/244d9623daaa
https://hg.mozilla.org/mozilla-central/rev/8b70e7042d56
Updated•2 years ago
|
Comment 10•2 years ago
|
||
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.
Description
•