Bug 25570 - WebKit incorrectly fires scroll events during the history.{back,forward,go} call when navigating to a reference fragment
Summary: WebKit incorrectly fires scroll events during the history.{back,forward,go} c...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Page Loading (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Darin Fisher (:fishd, Google)
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-05-05 10:37 PDT by Darin Fisher (:fishd, Google)
Modified: 2009-05-05 22:00 PDT (History)
2 users (show)

See Also:

Attachments
demo (746 bytes, text/html)
2009-05-05 10:38 PDT, Darin Fisher (:fishd, Google) 		no flags Details
v1 patch (3.14 KB, patch)
2009-05-05 14:47 PDT, Darin Fisher (:fishd, Google) 		no flags Details | Formatted Diff | Diff
v2 patch: this time with layout test changes (10.58 KB, patch)
2009-05-05 16:58 PDT, Darin Fisher (:fishd, Google) 		darin: review+
Details | Formatted Diff | Diff
Show Obsolete (1) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Darin Fisher (:fishd, Google) 2009-05-05 10:37:27 PDT 
WebKit incorrectly fires scroll events during the history.{back,forward,go} call when navigating to a reference fragment

This is subtle.  None of the other browsers behave this way.  They always dispatch history.{back,forward,go} asynchronously.  I found this bug because Chrome also happens to behave like IE and FF.  Safari seems to be the only browser that behaves differently, so I think it is probably best that it changes to match the others.

See attached demo.
Comment 1 Darin Fisher (:fishd, Google) 2009-05-05 10:38:14 PDT 
Created attachment 30024 [details]
demo
Comment 2 Darin Fisher (:fishd, Google) 2009-05-05 10:47:33 PDT 
The code in FrameLoader::scheduleHistoryNavigation is what I am suggesting should perhaps change.  If the URL we are at and the URL we are navigating to differ only in reference fragment, then the code calls goBackOrForward directly instead of calling scheduleRedirection.  This results in the navigation occurring synchronously, which leads to scroll events being dispatched during the history.{back,forward,go} call.  That in turn means that JavaScript is getting re-entered, which may be quite unexpected by some web applications.
Comment 3 Darin Fisher (:fishd, Google) 2009-05-05 14:28:27 PDT 
It looks like this synchronous call to goBackOrForward was introduced by andersca in r14669: http://trac.webkit.org/changeset/14669/trunk/WebCore/page/Frame.cpp

That was done as part of the fix for bug 6309.
Comment 4 Darin Fisher (:fishd, Google) 2009-05-05 14:47:16 PDT 
Created attachment 30034 [details]
v1 patch

This patch is incomplete since I will need to correct several layout tests to account for this change, but I wanted to post this for review to get feedback on this approach.  Thanks!
Comment 5 Darin Adler 2009-05-05 14:48:56 PDT 
Comment on attachment 30034 [details]
v1 patch

Seems OK to me. Anders?
Comment 6 Darin Fisher (:fishd, Google) 2009-05-05 16:58:18 PDT 
Created attachment 30041 [details]
v2 patch: this time with layout test changes
Comment 7 Darin Fisher (:fishd, Google) 2009-05-05 22:00:35 PDT 
Landed as:  http://trac.webkit.org/changeset/43274