Closed
Bug 999162
Opened 10 years ago
Closed 10 years ago
Calling scrollTo(0, 0) while at the top of a page prevents subsequent panning
Categories
(Core :: Panning and Zooming, defect)
Tracking
()
RESOLVED
FIXED
mozilla31
Tracking | Status | |
---|---|---|
b2g-v2.0 | --- | fixed |
People
(Reporter: kgrandon, Assigned: kats)
References
Details
Attachments
(2 files)
6.91 KB,
application/zip
|
Details | |
1.50 KB,
patch
|
drs
:
review+
|
Details | Diff | Splinter Review |
Calling scrollTo(0,0) works if I scroll down the page a bit, but not if I do this while at the top of a page. Note: this only impacts the first pan action, subsequent pans do not suffer from this.
Comment 1•10 years ago
|
||
A bit confused. If you're already at the top of the page, what do you expect scrollTo(0,0) to do?
Reporter | ||
Comment 2•10 years ago
|
||
It doesn't need to do anything, but the next pan action after calling it should continue to work.
Reporter | ||
Comment 3•10 years ago
|
||
Note that while we may be able to avoid calling it if at the top, we would have to check the scrollTop position which would introduce unnecessary performance cost/extra code. We need to fix it so that we can call scrollTo(0,0) while at the top and have panning working like normal afterward.
Comment 4•10 years ago
|
||
(In reply to Kevin Grandon :kgrandon from comment #2) > It doesn't need to do anything, but the next pan action after calling it > should continue to work. Got it. I changed the subject to clarify - I know I was confused.
Summary: Calling scrollTo(0, 0) while at the top of a page prevents panning → Calling scrollTo(0, 0) while at the top of a page prevents subsequent panning
Assignee | ||
Comment 6•10 years ago
|
||
So it's not actually the scrollTo() call itself that's the problem, it's that clicking on the link also changes the URL because it's a fragment jump, so we get a NotifyLayersUpdated with "aIsFirstPaint" set to true on the next paint. That causes the scroll offset and state to get reset, which cancels out the scroll that the user is in the middle of. If the "click" event is cancelled (i.e. e.preventDefault()) then this issue doesn't appear.
Assignee | ||
Comment 7•10 years ago
|
||
This fixes the problem but may cause regressions. I'll need to test a few scenarios before I request review.
Comment 8•10 years ago
|
||
No-Jun, do you have time to take a look at testing this, with the patches Kats has? Just to accelerate some other work.
Flags: needinfo?(npark)
Assignee | ||
Comment 9•10 years ago
|
||
Ok, it seems to do what I want. Try push: https://tbpl.mozilla.org/?tree=Try&rev=703bd0a36ca8
Assignee | ||
Updated•10 years ago
|
Attachment #8412025 -
Flags: review?(drs+bugzilla)
Comment 10•10 years ago
|
||
Tested by flashing latest gecko with patch on 1.4 build. now the scrolling is smooth when running the scrollTest app (i.e. not scrolling, click the top link, then scroll down). Before applying the patch, it showed jitter since it did not execute the scroll action right after calling scrollTo(0,0).
Flags: needinfo?(npark)
Comment 11•10 years ago
|
||
Comment on attachment 8412025 [details] [diff] [review] Patch LGTM, I'm actually not sure why it wasn't this way to begin with. I didn't test this since you and No-Jun both have.
Attachment #8412025 -
Flags: review?(drs+bugzilla) → review+
Assignee | ||
Updated•10 years ago
|
Keywords: checkin-needed
Comment 12•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/60c2852b75ee
Keywords: checkin-needed
Reporter | ||
Comment 13•10 years ago
|
||
Awesome, thanks guys!
Comment 14•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/60c2852b75ee
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
Reporter | ||
Comment 15•10 years ago
|
||
Landed a follow-up to remove the workaround: https://github.com/mozilla-b2g/gaia/commit/459dc1fb093b26679989b1e8641a8813bfa3a6b1
Reporter | ||
Comment 17•10 years ago
|
||
Mass modify - set status-b2g-v2.0 fixed for fixed bugs under vertical homescreen dependency tree.
status-b2g-v2.0:
--- → fixed
You need to log in
before you can comment on or make changes to this bug.
Description
•