Bug 30224 - Fragment scrolls initiated by javascript should not add a history item
Summary: Fragment scrolls initiated by javascript should not add a history item
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: History (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Steve Block
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-10-08 11:11 PDT by Patrick
Modified: 2010-08-18 08:31 PDT (History)
10 users (show)

See Also:


Attachments
Patch 1 for Bug 30224 (4.22 KB, patch)
2010-01-03 11:31 PST, Steve Block
abarth: review-
Details | Formatted Diff | Diff
Patch 2 for Bug 30224 (6.61 KB, patch)
2010-01-05 04:33 PST, Steve Block
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Patrick 2009-10-08 11:11:58 PDT
Currently, going to m.cnn.com will trigger a javascript location change to m.cnn.com/#___1__ adding a history item for both m.cnn.com and the fragment scroll. Neither Chrome nor Firefox does this (Chrome happens to use its own history list rather than WebKit's).

I believe the fix should be around FrameLoader.cpp:3484. An additional check for a user initiated click would avoid adding the extra history item.
Comment 1 Steve Block 2010-01-03 11:31:04 PST
Created attachment 45757 [details]
Patch 1 for Bug 30224
Comment 2 WebKit Review Bot 2010-01-03 11:36:15 PST
style-queue ran check-webkit-style on attachment 45757 [details] without any errors.
Comment 3 Adam Barth 2010-01-03 11:54:30 PST
Comment on attachment 45757 [details]
Patch 1 for Bug 30224

Behavioral changes require LayoutTests.  We're especially strict about requiring tests for changes to FrameLoader because FrameLoader is super complicated and poorly tested.
Comment 4 Steve Block 2010-01-05 04:33:10 PST
Created attachment 45880 [details]
Patch 2 for Bug 30224

I've added a test to check that fragment scrolls initiated from script do not cause additions to the back/forward list.

To add a test for the reverse - that fragment scrolls initiated by the user do cause additions to the back/forward list - would require faking a user action. Is this possible with the layout test controller?
Comment 5 WebKit Review Bot 2010-01-05 04:37:47 PST
style-queue ran check-webkit-style on attachment 45880 [details] without any errors.
Comment 6 Darin Adler 2010-01-05 08:32:34 PST
What does IE do in this case, by the way?
Comment 7 Eric Seidel (no email) 2010-01-05 14:21:18 PST
Looks sane to me.  But Adam or Brady are better reviewer choices.
Comment 8 Steve Block 2010-01-07 09:24:06 PST
m.cnn.com no longer requires this fix and the proposed change would cause WebKit to behave differently from FF/Chrome/IE.

Abandoning the change.
Comment 9 Brady Eidson 2010-01-07 13:30:30 PST
Cleared the review flag based on last comment.
Comment 10 Patrick 2010-03-03 11:04:53 PST
This is still an issue. There is a new site: http://app.showtime-app.com/ that redirects to an anchor and adds a history item. Trying going there in Safari and you will not be able to back out of the page.

The patch is not perfect. For a page with both a meta redirect and a javascript redirect, one of the redirects is added.

I would love to hear some ideas about how best to fix the issue.
Comment 11 Darin Adler 2010-03-03 11:06:44 PST
Same question as before. How does this site work in other browsers? What’s the difference from other browsers that makes WebKit-based browsers like Safari fail? The general direction is that we want to get closer to the HTML5 standard and to other browsers, rather than further away. And we want to make changes that are unlikely to break existing sites, even ones that work for different reasons in WebKit-based browsers than in others.
Comment 12 Ben Murdoch 2010-07-02 09:01:03 PDT
I'm seeing a very similar issue to this - a redirect is being added to the history list:

- Change your user agent in Safari to Mobile Safari
- Navigate to Wikipedia.org, click through to an article
- Now try to navigate back - you will be unable to leave Wikipedia as back takes you to a page that performs a redirect.

It seems that Firefox adds a history entry for the page that has the redirect, but seems to skip it for back navigation.

Does anyone have any ideas on how to fix this?
Comment 13 Darin Adler 2010-07-02 12:39:06 PDT
Similar issue, but not same. Probably needs its own bug report.
Comment 14 Ben Murdoch 2010-07-06 02:34:14 PDT
(In reply to comment #13)
> Similar issue, but not same. Probably needs its own bug report.

Filed https://bugs.webkit.org/show_bug.cgi?id=41670.
Comment 15 Mihai Parparita 2010-07-29 09:11:57 PDT
Darin, since you had opinions on how to fix this, you may want to look at the patch on bug 42861, since it fixes this and bug 41670. There's also comments on the bug about how other browsers handle similar scenarios (as best as I can tell, my patch brings WebKit closer to them).
Comment 16 Mihai Parparita 2010-08-18 08:31:38 PDT
(In reply to comment #10)
> This is still an issue. There is a new site: http://app.showtime-app.com/ that redirects to an anchor and adds a history item. Trying going there in Safari and you will not be able to back out of the page.

http://trac.webkit.org/changeset/65340 from bug 42861 fixes this (verified in the most recent nightly build).