633762 - Content View unexpectedly being scrolled downwards with Smooth Scrolling enabled

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[(mostly gone) XtC4UaLL [:xtc4uall]]

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.

Comment 1

[(mostly gone) XtC4UaLL [:xtc4uall]]

(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

Comment 2

[Mats Palmgren (inactive)]

Bug 633766 is the same thing?

Comment 3

[Timothy Nikkel (:tnikkel)]

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.

Comment 4

[Benjamin Smedberg]

Smooth scrolling isn't the default, is it? I really don't think this blocks unless I'm totally misreading it.

Comment 5

[Timothy Nikkel (:tnikkel)]

This is probably just one symptom, we shouldn't get into this state in our scrolling code.

Comment 6

[Benjamin Smedberg]

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...

Comment 7

[Robert O'Callahan (:roc) (email my personal email if necessary)]

It's a recent regression, and when it happens we're just broken.

Comment 8

[Robert O'Callahan (:roc) (email my personal email if necessary)]

in a way that's going to annoy the user.

Comment 9

[Benjamin Smedberg]

Per meeting today, .x.

Comment 10

[Timothy Nikkel (:tnikkel)]

Attachment: patch

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.

Comment 11

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 512951 [details] [diff] [review]
patch

Nice. Can you write a test for this?

Comment 12

[Timothy Nikkel (:tnikkel)]

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.

Comment 13

[Robert O'Callahan (:roc) (email my personal email if necessary)]

So you have a test then? :-)

Comment 14

[Timothy Nikkel (:tnikkel)]

Yes, did you want to see it?

Comment 15

[Robert O'Callahan (:roc) (email my personal email if necessary)]

sure :-)

Comment 16

[Timothy Nikkel (:tnikkel)]

Attachment: test

Comment 17

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Very nice!

Comment 18

[Timothy Nikkel (:tnikkel)]

http://hg.mozilla.org/mozilla-central/rev/ab9118f27d17