Bug 26034 - Loading url with anchor should keep page scrolled to anchor while images load and scripts run
Summary: Loading url with anchor should keep page scrolled to anchor while images load...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-05-26 17:01 PDT by Nate Chapin
Modified: 2009-10-29 10:51 PDT (History)
3 users (show)

See Also:


Attachments
patch (7.81 KB, patch)
2009-05-28 13:02 PDT, Nate Chapin
hyatt: review-
Details | Formatted Diff | Diff
patch2 (8.61 KB, patch)
2009-05-29 13:45 PDT, Nate Chapin
no flags Details | Formatted Diff | Diff
patch3 (8.77 KB, patch)
2009-05-29 14:30 PDT, Nate Chapin
no flags Details | Formatted Diff | Diff
patch4 (8.61 KB, patch)
2009-05-29 16:17 PDT, Nate Chapin
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Nate Chapin 2009-05-26 17:01:20 PDT
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.
Comment 1 Nate Chapin 2009-05-28 13:02:46 PDT
Created attachment 30745 [details]
patch
Comment 2 Dave Hyatt 2009-05-29 13:07:52 PDT
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).
Comment 3 Nate Chapin 2009-05-29 13:45:40 PDT
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.
Comment 4 Dave Hyatt 2009-05-29 14:13:20 PDT
Comment on attachment 30786 [details]
patch2

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

r=me.
Comment 5 Nate Chapin 2009-05-29 14:30:18 PDT
Created attachment 30789 [details]
patch3

Removed the spurious if() and fixed a tab that was off.
Comment 6 Nate Chapin 2009-05-29 16:17:13 PDT
Created attachment 30795 [details]
patch4
Comment 7 Nate Chapin 2009-05-29 16:21:31 PDT
(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.
Comment 8 Dimitri Glazkov (Google) 2009-05-29 20:04:53 PDT
Doh, you forgot to mark it for review :) Nobody will ever find it! :)
Comment 9 Dimitri Glazkov (Google) 2009-05-29 20:05:56 PDT
Comment on attachment 30795 [details]
patch4

Please don't forget about better layout tests. We need pretty, pretty layout tests.
Comment 10 Adam Barth 2009-06-01 00:24:17 PDT
Will land.
Comment 11 Adam Barth 2009-06-01 00:53:20 PDT
Fixed in r44311.
Comment 12 Adam Barth 2009-06-01 01:32:47 PDT
Please verify that the expected result landed in http://trac.webkit.org/changeset/44318 is correct.  The attached patch is missing the expected result.
Comment 13 Nate Chapin 2009-06-01 09:48:00 PDT
I'm going to reopen this bug to handle the additional layout tests that I need to add.
Comment 14 Adam Barth 2009-06-01 23:41:40 PDT
Comment on attachment 30795 [details]
patch4

This patch has already been landed.  Removing from commit queue.
Comment 15 Adam Barth 2009-06-02 00:38:11 PDT
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.
Comment 16 Simon Fraser (smfr) 2009-07-09 16:52:29 PDT
This likely caused bug 27137.