Closed Bug 492837 Opened 11 years ago Closed 11 years ago

Scroll position changes when something later in page is modified.

Categories

(Core :: Layout, defect)

defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: sylvain.pasche, Assigned: roc)

References

(Blocks 1 open bug)

Details

(Keywords: regression)

Attachments

(1 file)

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.
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.
Oh, hmm.  I hadn't scrolled down enough, I guess.  I can reproduce this now.
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?
That sounds right. Should we pass into the post-reflow callbacks a flag indicating if the reflow was interrupted?
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.
I can take it, but I can't work on it until 3.5 code freeze at least.
Assignee: nobody → roc
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.
Can we just ignore the callback if we have one of the frame dirty bits set?
Yes, that's probably the best.
Attached patch fixSplinter Review
Attachment #381685 - Flags: review?(bzbarsky)
Comment on attachment 381685 [details] [diff] [review]
fix

Looks good.  I haven't thought of a way to write a test yet, sadly.
Attachment #381685 - Flags: review?(bzbarsky) → review+
Whiteboard: [needs landing]
Flags: in-testsuite?
http://hg.mozilla.org/mozilla-central/rev/af7c59f1634a
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Whiteboard: [needs landing]
Depends on: 498593
Blocks: 570090
You need to log in before you can comment on or make changes to this bug.