492837 - Scroll position changes when something later in page is modified.

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Sylvain Pasche]

This is best reproducible on Mac/Win (happens more rarely on Linux).

Open http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml
Scroll down the page.
Expand one of the section (clicking on the arrow).

Expected: the scroll position doesn't change.
Actual: page scrolls half way to the top.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Which exact build are you using?  This is wfm on mac with a "Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2a1pre) Gecko/20090513 Minefield/3.6a1pre" (rev da613c9fae8c) nightly.

Comment 2

[Boris Zbarsky [:bzbarsky]]

Oh, hmm.  I hadn't scrolled down enough, I guess.  I can reproduce this now.

Comment 3

[Boris Zbarsky [:bzbarsky]]

This is pretty exciting.  We end up with a call to nsScrollPortView::ScrollToImpl with a y coordinate that is much smaller than the position we want to be at.  That coordinate is about 2000px, which is much less than the correct position we want to stay at (about 9000px here).  The call to ScrollToImpl comes from here:

#0  nsScrollPortView::ScrollToImpl
#1  0x134ce2fa in nsScrollPortView::ScrollTo
#2  0x130100b5 in nsGfxScrollFrameInner::ScrollbarChanged
#3  0x130121c0 in nsGfxScrollFrameInner::CurPosAttributeChanged 
#4  0x130159e8 in nsGfxScrollFrameInner::ReflowFinished

In this case, the reflow ended up getting interrupted.  So our geometry is probably just semi-bogus... but I suspect we're doing the scroll because we're clamping the scrollbar position to that geometry, right?  Ideally we wouldn't clamp until reflow completes or something, right?

Comment 4

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

That sounds right. Should we pass into the post-reflow callbacks a flag indicating if the reflow was interrupted?

Comment 5

[Boris Zbarsky [:bzbarsky]]

We could do that, sure.  That might not be too bad.

Do you want to take this bug, roc?  You're a lot more familiar with the scrollframe machinery.  If not, I'll deal with it.

Comment 6

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

I can take it, but I can't work on it until 3.5 code freeze at least.

Comment 7

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

This is actually a bit subtle. If the reflow is interrupted, we don't want to just do nothing and forget about the reflow callback. We want the callback to run later when the reflow has finished properly. Of course the frame might not be reflowed again in subsequent reflows --- but it might. So we need to design the API carefully so the reflow callback fires eventually, but only once.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Can we just ignore the callback if we have one of the frame dirty bits set?

Comment 9

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

Yes, that's probably the best.

Comment 10

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

Attachment: fix

Comment 11

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 381685 [details] [diff] [review]
fix

Looks good.  I haven't thought of a way to write a test yet, sadly.

Comment 12

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

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