278739 - Reloading a page whose length changes causes scroll position to change

Status: RESOLVED FIXED

(Core Graveyard :: History: Global, defect)

Description

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

Testcase and directions to reproduce coming in a minute.

Comment 1

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

Attachment: Testcase (see comment #1)

1. Save the page somewhere, and load it in mozilla.
2. Scroll down so the first set of As is at the very top of the page.
3. Add a new block of As and Bs (I just copied the last four blocks) to the
file, save it.
4. click reload in the browser.

Actual results:
You're now farther down the page than you were.

Expected results:
Stay at the correct scroll location.

Comment 2

[Boris Zbarsky [:bzbarsky]]

Further down the page, or further up the page?  I'd expect us to be at the same
pixel position from the top of the page.

Comment 3

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

(In reply to comment #2)
> Further down the page, or further up the page?  I'd expect us to be at the same
> pixel position from the top of the page.

Down.  I did some cursory debugging and nsGfxScrollFrameInner::RestoreState does
see the correct y-offset, so whatever line 2267 talks about might be causing the
problem.  I started with the short testcase, scrolled down, then lenghthened the
page and hit reload.  2010 was saved in the state and properly restored, but the
page ended up down a couple of lines from where it was before.  Hitting reload
again saved and restored a y-offset of 2670, so between the initial state
restoration and the result getting on-screen, something changed.

Comment 4

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

Attachment: patch

This works.  I have no idea why the code does what it does right now - the
unpatched behavior keeps the top of the scroll thumb at the same position and I
guess puts you at the same percent down the page.  With the patch, you end up
at the same *position* down the page.

Comment 5

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

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

Looks good --- for trunk. I recommend not changing this for 1.8. It's always
been this way until now and there's no reason to take any risks fixing it.

Comment 6

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

(In reply to comment #5)
> (From update of attachment 196463 [details] [diff] [review] [edit])
> Looks good --- for trunk. I recommend not changing this for 1.8. It's always
> been this way until now and there's no reason to take any risks fixing it.
> 

Given how far off trunk is from reaching users and the low risk and high impact for web-forum users, could we take this for 1.8.1?

Comment 7

[Boris Zbarsky [:bzbarsky]]

I wouldn't consider any patch to this code low risk, fwiw.

Comment 8

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

(In reply to comment #7)
> I wouldn't consider any patch to this code low risk, fwiw.
> 

Ok, but it has been on trunk for months without regressions...