[CTW] Bounds reported incorrectly when some pages are scrolled
Categories
(Core :: Disability Access APIs, defect)
Tracking
()
| Tracking | Status | |
|---|---|---|
| firefox101 | --- | fixed |
People
(Reporter: Jamie, Assigned: Jamie)
References
Details
(Whiteboard: [ctw-m2])
Attachments
(2 files)
STR (CTW enabled):
- Open https://phabricator.services.mozilla.com/D142648
- Focus the document and press the end key to scroll to the bottom of the document.
- Check the bounds for the RemoteAccessible for the "Privacy" link.
- Expected: The y coordinate should be positive and within the bounds of the document.
- Actual: The y coordinate is negative; e.g. -52483.
I am also able to reproduce this on https://www.nvaccess.org/
This works as expected with CTW disabled.
I couldn't come up with a distilled test case for this. For example, this doesn't reproduce it (either with or without the wrapping div):
data:text/html,<body><script>let html = '<div role="group">'; for (let i = 0; i < 500; ++i) { html +=<p>Line ${i}</p>; } html += "</div>"; document.body.innerHTML = html;</script>
|
Assignee | |
Updated•2 years ago
|
|
||
Comment 1•2 years ago
|
||
interesting... 🤔
|
|
Assignee | |
Comment 2•2 years ago
|
||
I think I've worked out what's going on here.
We cache parent relative bounds for each Accessible, as well as the scroll offset for scrollable Accessibles. However, when you scroll, the parent relative bounds for the children of the scrollable Accessible change because they take the scroll offset into account. For example:
- You have a document with a y coordinate of 0 and a height of 1000.
- A child paragraph has a y of 1500. It thus has a parent relative y of 1500.
- If you scroll to the top of the child, the document gets a y scroll offset of 1500.
- Now the parent relative y of the child is 0 because the scroll offset of the document is taken into account when calculating bounds.
- Thus, when we subtract the scroll offset, we get -1500.
I think I'm missing a piece of the equation here, as the numbers don't seem that straightforward in real testing, but I think this is the gist of it.
I suspect this is why scrolling sometimes seemed to work before bug 1756229. If a frame was ever reflowed after scrolling, we would push a cache update with the new coordinates which take scroll offset into account.
The key here is "if a frame was ever reflowed". When you scroll, that doesn't necessarily reflow frames. In the case of no reflow, a11y doesn't get notified of a possible bounds change and we don't push a cache update, so everything works as expected. This is probably why our tests work. On the other hand, if a frame gets reflow during or after the scroll, a11y gets notified of a possible bounds change and we push a cache update with the new bounds... which incorrectly take the scroll offset into account.
Of course, we could revert caching of the scroll offset and just make sure the bounds for all frames are updated whenever a scroll occurs, even if they weren't reflowed. However, the main reason for caching scroll offset instead is that we don't want huge cache pushes every time the user scrolls. I think that's what is happening now: reflows cause cache pushes, so this is both a correctness problem and a performance problem.
What I think we need to do instead is make ParentRelativeBounds act as if there's no scroll offset in the parent frame. That is, in step 4 of the above example, the parent relative y of the child should be 1500 even though the document has a scroll offset of 1500. This way, a11y will still get notified of a possible bounds change, but the bounds will be the same (no cache push) unless the Accessible really did move.
The question is how. Do we just add the scroll offset of the parent in ParentRelativeBounds? nsLayoutUtils::GetAllInFlowRectsUnion doesn't seem to have a flag to alter the way it deals with scroll offset.
|
|
Assignee | |
Comment 3•2 years ago
•
|
||
With my original test case:
data:text/html,<body><script>let html = '<div role="group">'; for (let i = 0; i < 500; ++i) { html += `<p>Line ${i}</p>`; } html += "</div>"; document.body.innerHTML = html;</script>
I couldn't reproduce the problem because the frames don't get reflowed when you scroll. However, with this altered version:
data:text/html,<body onscroll="div.style.display = 'inline'; div.offsetTop; div.style.display = 'block'; div.offsetTop;"><script>let html = '<div id="div" role="group">'; for (let i = 0; i < 500; ++i) { html += `<p>Line ${i}</p>`; } html += "</div>"; document.body.innerHTML = html;</script>```
I can reproduce the problem because scrolling causes the entire div to be reflowed.
|
|
Assignee | |
Updated•2 years ago
|
|
|
Assignee | |
Comment 4•2 years ago
|
||
Ah. Also, the tests are broken. They pass even if the value is thousands off. The reason is that they're currently doing this:
_y >= expectedY - 5 || _y <= expectedY + 5
That needs to be &&, not ||. || means we only impose one constraint, not both.
|
|
Assignee | |
Comment 5•2 years ago
|
||
We use two constraints to ensure the bounds fall within 5 px of the expected values in either direction.
However, the function was using || , so only one constraint was being enforced.
In practice, this meant that we wouldn't fail for any value.
Change this to && to enforce both constraints.
|
|
Assignee | |
Comment 6•2 years ago
|
||
This means that cached bounds stay the same regardless of scroll offset.
The scroll position gets subtracted later when calculating absolute bounds.
Pushed by jteh@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/00e72376c7ce part 1: Fix testBoundsInContent to correctly enforce both low and high constraints. r=morgan https://hg.mozilla.org/integration/autoland/rev/f123ca813cb5 part 2: Add the scroll offset of the parent in ParentRelativeBounds. r=morgan
|
||
Comment 8•2 years ago
|
||
| bugherder | ||
https://hg.mozilla.org/mozilla-central/rev/00e72376c7ce
https://hg.mozilla.org/mozilla-central/rev/f123ca813cb5
|
||
Updated•2 years ago
|
Description
•