REGRESSION (r82185): Scroll position not restored on navigation back to a page in the page cache

[Beth Dakin]

Regression from http://trac.webkit.org/changeset/82185/trunk/Source/WebCore

* STEPS TO REPRODUCE
1. Navigate to <http://en.wikipedia.org/wiki/MathML>
2. Scroll down
3. In the same tab, navigate to >about:blank>
4. Choose History > Back

* RESULTS
The Wikipedia page is scrolled to the top.

<rdar://problem/9226652>

[Beth Dakin]

Created attachment 90106 [details]
Patch

[Maciej Stachowiak]

Comment on attachment 90106 [details]
Patch

r=me

[Antonio Gomes]

Comment on attachment 90106 [details]
Patch

Why no tests? :)

It should be very easy to be tested, unless I am missing something ...

[Beth Dakin]

(In reply to comment #3)
> (From update of attachment 90106 [details])
> Why no tests? :)
> 
> It should be very easy to be tested, unless I am missing something ...

You're right, Antonio! I wrote a test that I will commit with the change.

[Antonio Gomes]

> > 
> > It should be very easy to be tested, unless I am missing something ...
> 
> You're right, Antonio! I wrote a test that I will commit with the change.

Perfect

[Beth Dakin]

Committed fix and test with revision 84296.

[WebKit Review Bot]

http://trac.webkit.org/changeset/84296 might have broken Qt Linux Release

[Csaba Osztrogonác]

(In reply to comment #7)
> http://trac.webkit.org/changeset/84296 might have broken Qt Linux Release

new bug for it: https://bugs.webkit.org/show_bug.cgi?id=58977

[Yong Li]

(In reply to comment #7)
> http://trac.webkit.org/changeset/84296 might have broken Qt Linux Release

Probably FrameView::resetScrollbarsAndClearContentsSize() is never called in some cases like when it loads an unreachable URL, then the cached scroll position is always (0, 0) and the real scroll position is lost. I don't think the solution is right. HistoryController and ScrollView shouldn't think that much. Who resets contents size to (0, 0) should be responsible to restore the scroll position when contents size changes back.

[Antonio Gomes]

(In reply to comment #9)
> (In reply to comment #7)
> > http://trac.webkit.org/changeset/84296 might have broken Qt Linux Release
> 
> Probably FrameView::resetScrollbarsAndClearContentsSize() is never called in some cases like when it loads an unreachable URL, then the cached scroll position is always (0, 0) and the real scroll position is lost. I don't think the solution is right. HistoryController and ScrollView shouldn't think that much. Who resets contents size to (0, 0) should be responsible to restore the scroll position when contents size changes back.

Beth, could you reply to Yong's comment, please? It broke something important for us.

[Beth Dakin]

Hi Yong and Antontio,

I discovered that this caused a regression in Safari as well: though my patch made pages in the page cache properly restore scroll position on reload, it regressed pages that do not go into the page cache. This pages no longer restore scroll position. 

I have a fix and a layout test that I will post shortly. Hopefully it fixes the QT test as well.

[Beth Dakin]

Created attachment 90672 [details]
Patch that restores scroll position for cached and non-cached pages

Comment on attachment 90672 [details]
Patch that restores scroll position for cached and non-cached pages

View in context: https://bugs.webkit.org/attachment.cgi?id=90672&action=review

> Source/WebCore/ChangeLog:12
> +        *not* in the page cache. This patch fixed both cached and non-cached pages by 

“fixed” or “fixes”?

[Beth Dakin]

Thanks Dan! Committed fix with revision 84604.

[Nico Weber]

It looks like this still happens for in-page navigations. I filed bug 59877 for this.

[Mihai Parparita]

*** Bug 59877 has been marked as a duplicate of this bug. ***