Closed Bug 1811972 Opened 2 years ago Closed 2 years ago

[CTW] Bounds for Google search results are occasionally wrong

Categories

(Core :: Disability Access APIs, defect)

defect

Tracking

()

RESOLVED FIXED
111 Branch
Tracking Status
firefox111 --- fixed

People

(Reporter: nlapre, Assigned: Jamie)

References

Details

(Whiteboard: [ctw-m5] )

Attachments

(1 file, 1 obsolete file)

With the accessibility cache enabled, open Google, search for a term such as "test search" and examine the results. For some results, mousing over a h3 that defines a search result link will not read the link out with NVDA. Examining further, it appears that the cached bounds of these heading accessibles are incorrect. The bounds appear y-translated down the page. Using NVDA's visual highlighting, I can confirm that mousing over the visually-incorrect location of the bounds does cause NVDA to read out the search result.

The pattern of which results have this problem and which don't is unclear to me, but it seems to start occurring after at least the 2nd result.

Something very strange to note here: when I inspect the properties of these headings with the Dev Tools, the problem immediately rights itself. Similarly, if I press the "about this result" button next to a busted search result, the problem rights itself when that dialog closes. These factors make it really tough to get an easily reproducible example, since I can't get to whatever state the markup is in before the Dev Tools touch it (at which point it rights itself). But it's annoying for users nonetheless.

I tested this on Windows 11 with NVDA.

Note: Originally reported by 4RJames@gmail.com here.

Opening Dev Tools often causes a reflow, which forces a bounds check. This suggests that some property is changing that affects location (but does not normally reflow) and we're not updating the cache when that happens.

I would have suggested that bug 1774705 was the culprit, but the fix for that is in central already, so I guess not.

I wonder if other nodes get inserted into the container before these existing result nodes without causing reflow of those result nodes? I wouldn't have thought that was possible, but maybe it is. If that did happen, we wouldn't update the parent relative bounds for the existing nodes, since they aren't reflowed.

Damn it. It is possible, but I don't understand the exact conditions under which it can occur.
data:text/html,<div id="container"><p>1</p><p id="p2" hidden>2</p><p id="p3">3</p></div><button onclick="p2.hidden = false;">Show</button>
When you press the "Show" button, the bounds for p3 get updated correctly, but the bounds for the button do not.

I would have thought either everything would update or nothing, but p3 does update correctly.

Looks like you've found a good minimal test case, but a potentially interesting note: I've tried debugging this remotely from another computer, in an attempt to not trigger a reflow by opening dev tools on the Nightly instance being debugged. So, the setup is, google search results open on one computer, and dev tools opened on another computer. If I manually click through the DOM Inspector in the debugger computer, the bounds remain incorrect until I expand one specific node in the DOM tree (for those curious, it has id rcnt, which appears consistent across page loads, whatever that means), at which point they right themselves. Can we trigger a reflow even if there's no Dev Tools bar present on the instance being debugged? Maybe so!

I don't know about a reflow, but poking the coordinates of a DOM node (e.g. offsetTop) can "flush" layout. I'm not quite sure how that fits in here.

Severity: -- → S3
Assignee: nobody → jteh
See Also: → 1774705

A frame doesn't have to be reflowed to change its position.
For example, if there is a container c followed by a node outside the container o, inserting a node into c reflows c, but moves o down the page without reflowing o.
In this case, we previously weren't being notified that there was a possible bounds change, which meant we weren't updating the cache.
Now, we get notified about frames moving regardless of reflow.
Since this notification includes changes to CSS left/right/top/bottom, we can also remove the code added in bug 1774705 to explicitly watch for changes to these properties.

Nathan, would you mind testing this patch to see if it fixes the Google search results problem for you? It definitely fixes problems, so we'll want to land it, but if it doesn't address the Google problem, I'll need to move it to a different bug.

Flags: needinfo?(nlapre)

For the record, I verified that in the test case in comment 3, p3 gets reflowed (it has a different frame pointer before vs after the button is clicked), but the button does not get reflowed (it has the same frame pointer before vs after).

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

Nathan, would you mind testing this patch to see if it fixes the Google search results problem for you? It definitely fixes problems, so we'll want to land it, but if it doesn't address the Google problem, I'll need to move it to a different bug.

I just tried out your patch, and can confirm that it fixes the button test case you devised. However, the issue with Google search results still remains, unfortunately. Still, seems like an improvement.

Flags: needinfo?(nlapre)
See Also: → 1812208
See Also: 1774705

Comment on attachment 9313741 [details]
Bug 1811972: Push a cache update if bounds change as a result of a frame moving even though it wasn't reflowed.

Revision D167645 was moved to bug 1812208. Setting attachment 9313741 [details] to obsolete.

Attachment #9313741 - Attachment is obsolete: true

Nathan and I debugged this on Zoom. The issue is that some divs get re-parented, but we don't push a bounds update in that case. Because the divs have been re-parented, the parent relative bounds are now relative to the wrong (previous) parent. I have a working patch, but it needs cleanup and a test.

On further reflection, I don't quite understand how the Accessible is being re-parented without there also being a bounds change notification on the Accessible due to reflow. I can only think of a couple of ways an Accessible can be re-parented, the most likely being that reflow occurred and so PruneOrInsertSubtree was called on the new parent. However, in that case, the whole subtree would have been reflowed, so the bounds should have been updated on the problematic Accessible.

One other way to trigger re-parenting is to set aria-label or similar on an element that doesn't have an Accessible yet:

data:text/html,<p style="height: 300px;">1</p><div><p>2</p></div><button onclick="document.querySelector('div').setAttribute('aria-label', 'div');">Change

When you first load this, there is no Accessible for the div. Pressing the Change button causes an Accessible to be created for the div, resulting in very broken bounds for the "2" paragraph.

However, there are no ARIA properties on the relevant parent in Google Search, so this can't be the trigger.

Aha. Adding a click event listener also might cause re-parenting:

data:text/html,<p style="height: 300px;">1</p><div><p>2</p></div><button onclick="document.querySelector('div').addEventListener('click', function () {});">Change

The parent div in question does have a click event listener, so this is likely it.

When an Accessible is moved, it's possible it is re-parented.
In that case, since our cached bounds are relative to the parent, the bounds are now incorrect.
To fix this, queue a bounds cache update whenever an Accessible is moved.
This will also trigger when an Accessible remains under the same parent.
However, because we cache bounds in LocalAccessible, we won't actually push a cache update unless the bounds really changed.

Pushed by jteh@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/2542d4605462
When an Accessible is moved in the tree, queue a bounds cache update. r=morgan
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → 111 Branch
Regressions: 1813256
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: