Closed Bug 633762 Opened 9 years ago Closed 9 years ago
Content View unexpectedly being scrolled downwards with Smooth Scrolling enabled
STR: * turn on Smooth Scrolling * open given Url Now either use your Mouse Wheel or the "Up" Key (both one Tick) to scroll upwards. Expected: scrolling Upwards Actual: Content View is scrolled downwards to the Content's Edge. Last good nightly: 2011-02-09 First bad nightly: 2011-02-10 Pushlog: http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=fd0817e454fe&tochange=199cb6282554 => likely Bug 629587.
(In reply to comment #0) > => likely Bug 629587. Verified by testing against Hourly Builds: http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=fc119d5000e5&tochange=1f8628fd5547
Assignee: nobody → tnikkel
blocking2.0: ? → final+
Bug 633766 is the same thing?
The anchor we are scrolling to goes from having a display to display: none during load. The ScrollTo call for scrolling to this content happens when it has a display and we set mDestination to ~4000 pixels down the page. Later it becomes display none and after a reflow the whole page is now shorter than 4000 pixels, the reflow callback calls ScrollToImpl(GetScrollPosition()), this gets clamped to the available scroll range, but mDestination is still ~4000 pixels. Before bug 629587 the CurPosAttributeChanged call in the reflow callback would call ScrollTo (which sets mDestination) even if the current scroll position was the same as the dest in CurPosAttributeChanged. So mDestination should probably get updated somewhere else.
Smooth scrolling isn't the default, is it? I really don't think this blocks unless I'm totally misreading it.
blocking2.0: final+ → -
This is probably just one symptom, we shouldn't get into this state in our scrolling code.
blocking2.0: - → final+
roc, why do you think this is a blocker? It's a bug, but it doesn't look like an especially serious bug, not worth holding a ship-date for...
It's a recent regression, and when it happens we're just broken.
in a way that's going to annoy the user.
Per meeting today, .x.
blocking2.0: final+ → .x
Bug 551463 added a call to ScrollToImpl in ReflowFinished which updates the current scroll position. Before that the CurPosAttributeChanged call in ReflowFinished would see the attributes on the scroll bars being different from the current scroll position and so would call ScrollTo. None of this seems to have been the designed behaviour; it seems to be working by accident. I think we should just make the early return in CurPosAttributeChanged only happen if there is an async scroll pending that it would ruin.
Attachment #512951 - Flags: review?(roc)
Comment on attachment 512951 [details] [diff] [review] patch Nice. Can you write a test for this?
Attachment #512951 - Flags: review?(roc) → review+
Comment on attachment 512951 [details] [diff] [review] patch While writing a test for this I found it is not hard at all to reproduce. Smooth scrolling is not required. We should fix this.
Attachment #512951 - Flags: approval2.0?
Attachment #512951 - Flags: approval2.0? → approval2.0+
So you have a test then? :-)
Yes, did you want to see it?
Status: NEW → RESOLVED
Closed: 9 years ago
Component: General → Layout
QA Contact: general → layout
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.