Closed Bug 1837496 Opened 1 year ago Closed 1 year ago

Reply buttons on gmail messages with suggested responses have incorrect bounds

Categories

(Core :: Disability Access APIs, defect)

defect

Tracking

()

VERIFIED FIXED
117 Branch
Tracking Status
firefox117 --- verified
firefox118 --- verified

People

(Reporter: morgan, Assigned: morgan)

References

(Blocks 2 open bugs)

Details

(Whiteboard: [ctw-23h2])

Attachments

(2 files)

STR:

  1. Open a gmail message with suggested responses
  2. Attempt to navigate to the reply/reply all/forward buttons with VoiceOver

Expected:

The buttons are reachable and have correct bounds

Actual:

The buttons are reachable but their bounds overlay those of the suggested responses, because the suggested responses are inserted dynamically.

You can solve this by forcing a reflow on the page via zoom or some other method. The bounds are correct after reflow. We're missing a cache update here.

This is also an issue on LinkedIn profile buttons -- they insert the number of connections dynamically and so the bounds of the buttons are wrong. I wonder if this is the same event.

Curious. We already push bounds cache updates if an Accessible is re-parented, a frame is reflowed or a frame is repositioned. I wonder where else there could possibly be an update?

Severity: -- → S3

:Jamie this is fixed with your MarkNeedsDisplayItemRebuild notification patch -- I verified.
Do you think it's worth trying to land that? I'm gonna dig a little deeper and see if there are any other notifications we could use, but I spent some time on this on Friday and came up empty handed (there aren't any obvious style changes I'm seeing, for ex. )

Flags: needinfo?(jteh)

I do worry about performance, since that will be fired recursively on all descendants of an element that gets moved, even if their relative bounds haven't changed. If we do land this, we should remove the other notifications, since they would (should?) be redundant.

I guess we could try profiling this on a really large page/reflow to se if it makes a massive difference?

There must be something we're missing here. Perhaps this isn't a style change, but it could be some style on the parent that impacts relative bounds of descendants in such a way that it is not just a simple offset. Are we dealing with a position: sticky here?

Flags: needinfo?(jteh) → needinfo?(mreschenberg)

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

I do worry about performance, since that will be fired recursively on all descendants of an element that gets moved, even if their relative bounds haven't changed. If we do land this, we should remove the other notifications, since they would (should?) be redundant.

I guess we could try profiling this on a really large page/reflow to se if it makes a massive difference?

There must be something we're missing here. Perhaps this isn't a style change, but it could be some style on the parent that impacts relative bounds of descendants in such a way that it is not just a simple offset. Are we dealing with a position: sticky here?

Not that I can see. There are some pseudo elements with position:absolute but they're within the table row, and the entire table row seems to be in the wrong spot. hmmm..

Flags: needinfo?(mreschenberg)
Whiteboard: [ctw-23h2]

FWIW I profiled the display list notification patch on twitter (scrolling for a minute and a half) and the percentage of time spent in BundleFieldsForCache and NotifyOfPossibleBoundsChange is identical to a profile of similar length without the patch (current central)

:Jamie do you think that patch is worth landing, to fix this issue? (given it doesn't seem to be a performance concern?)

Flags: needinfo?(jteh)

You've convinced me. Can you convince Emilio? :)

Flags: needinfo?(jteh)
Attached patch display_list_notif.patch (deleted) — Splinter Review
Assignee: nobody → mreschenberg

:emilio I think you were worried about performance last time Jamie suggested this fix for a hittesting issue we were experiencing. With the profiles above, do you feel like those concerns are addressed or are there other situations you'd like profiled?

patch diff attached above for ref, I can't find the old phab link

:Jamie do you want to submit this, since you're the original patch author or do you want me to upload it to phab ?

Flags: needinfo?(emilio)

Morgan, does this still work if you also remove the notify call in SetPosition? We shouldn't need that one either if we're doing this for display item changes.

Happy for you to submit it, potentially with the SetPosition change. Do we know how to test this?

Flags: needinfo?(emilio)

Sorry didn't mean to clear ni? without commenting.

The main issue is that this causes bounds changes to be notified for things that don't actually change bounds.

E.g. for something that is animating the color you'd get a lot of bounds change notifications, don't you?

That's true, yes. However, that can also happen for reflow and SetPosition (remembering that we cache bounds relative to the parent acc), albeit less often. We cache the relative bounds locally to prevent excessive IPC traffic and parent process updates.

Pushed by mreschenberg@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/a6ca8c0a7fba NotifyOfPossibleBoundsChange when display list changes are observed, instead of during reflow/in SetPosition r=Jamie,emilio
Status: NEW → RESOLVED
Closed: 1 year ago
Resolution: --- → FIXED
Target Milestone: --- → 117 Branch
Flags: qe-verify+

Reproducible on a 2023-07-24 Nightly build on macOS 12 using VoiceOver.
Verified as fixed on Firefox 117.0b6(build ID: 20230810181819) and Nightly 118.0a1(build ID: 20230810094725) on macOS 12.

Status: RESOLVED → VERIFIED
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: