Bug 26034 – Loading url with anchor should keep page scrolled to anchor while images load and scripts run

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Nate Chapin]

FrameLoader calls gotoAnchor() in finishedParsing(), which causes WebKit to scroll to an anchor after html has loaded, but before images load and scripts run.  By the time elements have loaded, the original scrolling may no longer be the actual location of the anchor.  Scrolling to the anchor before all elements have loaded makes sense, but ideally we would ensure that the anchor continues to be displayed, even as its position relative to the top of the document might change.

http://code.google.com/p/chromium/issues/detail?id=619 appears to be an example of the image case of this issue.

[Nate Chapin]

Created attachment 30745 [details]
patch

[Dave Hyatt]

Comment on attachment 30745 [details]
patch

Yeah this is really close.  The one thing I think you are missing here is that all scrolls should turn off the lock and not just user scrolls.  For example if the page focuses a text field after the anchor jump, and we scroll to expose that text field, you can't then jump back to the anchor.  If a page sets body.scrollTop back to 0, you can't jump back to the anchor after that, etc.  Basically you need to turn off the lock if any scrolling happens AT ALL (other than the gotoAnchor scroll obviously).

[Nate Chapin]

Created attachment 30786 [details]
patch2

Programmatic scroll now releases the anchor lock.  Note that navigating to an anchor is a type of programmatic scroll, so when gotoAnchor() gets called, the anchor lock gets released in FrameView, then gotoAnchor() sets the anchor lock again before it returns.

[Dave Hyatt]

Comment on attachment 30786 [details]
patch2

No need to check if (lockedToAnchor()) in setWasScrolledByUser.  Everything else looks good.

r=me.

[Nate Chapin]

Created attachment 30789 [details]
patch3

Removed the spurious if() and fixed a tab that was off.

[Nate Chapin]

Created attachment 30795 [details]
patch4

[Nate Chapin]

(In reply to comment #6)
> Created an attachment (id=30795) [review]
> patch4
> 

Added a null check for m_frame->view() in FrameLoader::completed(), was intermittently crashing a few tests.

[Dimitri Glazkov (Google)]

Doh, you forgot to mark it for review :) Nobody will ever find it! :)

[Dimitri Glazkov (Google)]

Comment on attachment 30795 [details]
patch4

Please don't forget about better layout tests. We need pretty, pretty layout tests.

[Adam Barth]

Will land.

[Adam Barth]

Fixed in r44311.

[Adam Barth]

Please verify that the expected result landed in http://trac.webkit.org/changeset/44318 is correct.  The attached patch is missing the expected result.

[Nate Chapin]

I'm going to reopen this bug to handle the additional layout tests that I need to add.

[Adam Barth]

Comment on attachment 30795 [details]
patch4

This patch has already been landed.  Removing from commit queue.

[Adam Barth]

Comment on attachment 30786 [details]
patch2

I'm told if I remove the r+ from this obsolete patch, this will be removed from the commit queue.

[Simon Fraser (smfr)]

This likely caused bug 27137.