APZ can get out of sync when mixing Promises and scrollIntoView

RESOLVED FIXED in Firefox 34

Status

()

defect
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: squib, Assigned: kats)

Tracking

unspecified
mozilla35
All
Gonk (Firefox OS)
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox33 wontfix, firefox34 fixed, firefox35 fixed, b2g-v2.0 wontfix, b2g-v2.0M wontfix, b2g-v2.1 verified, b2g-v2.2 fixed)

Details

Attachments

(3 attachments)

Reporter

Description

5 years ago
Posted file Test case
Attached is a simple test case that appends a bunch of elements and scrolls to one of them. If you open this page on B2G, scroll up a bit, then reload and try to tap the items, you'll see that the item that gets selected is actually well *above* your finger.

This bug, under slightly different circumstances, was also reported at bug 1068471 (over there, I think the slowness of the Promises makes it so you don't have to reload at all, but you *do* have to use insertBefore somewhere).
refer for more information 1068471
Flags: needinfo?(bugmail.mozilla)
Can repro, will investigate.
Assignee: nobody → bugmail.mozilla
Flags: needinfo?(bugmail.mozilla)
OS: Windows 7 → Gonk (Firefox OS)
Hardware: x86_64 → All
This fixes the problem. In the scenario I reproduced what was happening was that when the page loads, the APZ sends a repaint request at (0,0) while content is doing the scrollIntoView. Content sets the scroll offset to y=520, and so the repaint request fails, resulting in a callback transform of y=-520 getting stored. The APZ gets a NLU call with the y=520 scroll offset update and sends an acknowledgement, but since there's no repaint happening the callback transform remains at -520. Subsequent touch events and clicks are incorrectly transformed by 520. I think clearing the callback transform on the scroll offset update is the right thing to do here, because it means that content and the APZ are back in sync with respect to the scroll offset and any future touch events shouldn't get translated around.
Attachment #8491737 - Flags: review?(botond)
Misc logging stuff
Attachment #8491740 - Flags: review?(botond)
Comment on attachment 8491737 [details] [diff] [review]
Part 1 - Reset callback transform on scroll update acknowledgement

Review of attachment 8491737 [details] [diff] [review]:
-----------------------------------------------------------------

Tricky stuff! It took me a while to convince myself that this is correct in various scenarios, including overflow:hidden frames, but it seems sound.
Attachment #8491737 - Flags: review?(botond) → review+
Attachment #8491740 - Flags: review?(botond) → review+
https://hg.mozilla.org/mozilla-central/rev/60a55c2a7551
https://hg.mozilla.org/mozilla-central/rev/2d94eebed4ed
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
Comment on attachment 8491737 [details] [diff] [review]
Part 1 - Reset callback transform on scroll update acknowledgement

Approval Request Comment
[Feature/regressing bug #]: original async scrolling implementation
[User impact if declined]: in rare cases the device will get stuck in a state where clicks/gestures will get delivered to the wrong content. panning or zooming will clear the bad state
[Describe test coverage new/current, TBPL]: manual testing locally; we don't have a good framework to reproduce this scenario in automated testing
[Risks and why]: low risk, but it would probably benefit from some bake time on master before uplift. if it does have regressions, they would manifest as odd behaviour when content changes the scroll position via JS.
[String/UUID change made/needed]: none
Attachment #8491737 - Flags: approval-mozilla-aurora?
Thanks Karthik :)
Does this impact desktop and mobile as well as B2G or only B2G?
Flags: needinfo?(bugmail.mozilla)
Only B2G and Metro.
Flags: needinfo?(bugmail.mozilla)
Comment on attachment 8491737 [details] [diff] [review]
Part 1 - Reset callback transform on scroll update acknowledgement

Approving this given the 2-3 day bake time, also NI :no-jun to see if there is more testing he could do once this lands on 2.1 or atleast keep an eye on any obvious fallouts that will happen due this landing.
Attachment #8491737 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Flags: needinfo?(npark)
I checked the example attached to this bug, as well as various scroll/select UIs (Contacts, Calendar, Dialer, Settings, Gallery, etc.) but I did not see any issues with it.
Flags: needinfo?(npark)
You need to log in before you can comment on or make changes to this bug.