Closed Bug 633762 Opened 9 years ago Closed 9 years ago

Content View unexpectedly being scrolled downwards with Smooth Scrolling enabled

Categories

(Core :: Layout, defect)

x86
Windows XP
defect
Not set

Tracking

()

RESOLVED FIXED
Tracking Status
blocking2.0 --- .x+

People

(Reporter: xtc4uall, Assigned: tnikkel)

References

()

Details

(Keywords: regression)

Attachments

(2 files)

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.
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.
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
Attached patch patchSplinter Review
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?
Yes, did you want to see it?
Attached patch testSplinter Review
http://hg.mozilla.org/mozilla-central/rev/ab9118f27d17
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.