Reply buttons on gmail messages with suggested responses have incorrect bounds
Categories
(Core :: Disability Access APIs, defect)
Tracking
()
People
(Reporter: morgan, Assigned: morgan)
References
(Blocks 2 open bugs)
Details
(Whiteboard: [ctw-23h2])
Attachments
(2 files)
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
text/x-phabricator-request
|
Details |
STR:
- Open a gmail message with suggested responses
- 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.
Comment 1•1 year ago
|
||
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?
Updated•1 year ago
|
Updated•1 year ago
|
Assignee | ||
Comment 2•1 year ago
|
||
: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. )
Comment 3•1 year ago
|
||
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?
Assignee | ||
Comment 4•1 year ago
|
||
(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..
Updated•1 year ago
|
Assignee | ||
Comment 5•1 year ago
|
||
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?)
Assignee | ||
Comment 7•1 year ago
|
||
Assignee | ||
Comment 8•1 year ago
|
||
: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 ?
Comment 9•1 year ago
|
||
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.
Comment 10•1 year ago
|
||
Happy for you to submit it, potentially with the SetPosition change. Do we know how to test this?
Updated•1 year ago
|
Comment 11•1 year ago
|
||
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?
Comment 12•1 year ago
|
||
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.
Assignee | ||
Comment 13•1 year ago
|
||
Comment 14•1 year ago
|
||
Comment 15•1 year ago
|
||
bugherder |
Updated•1 year ago
|
Comment 16•1 year ago
|
||
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.
Description
•