Bug 30224 – Fragment scrolls initiated by javascript should not add a history item

• Status: RESOLVED
• Resolution: FIXED
• Component: History

[Patrick]

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.

[Steve Block]

Created attachment 45757 [details]
Patch 1 for Bug 30224

[WebKit Review Bot]

style-queue ran check-webkit-style on attachment 45757 [details] without any errors.

[Adam Barth]

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.

[Steve Block]

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?

[WebKit Review Bot]

style-queue ran check-webkit-style on attachment 45880 [details] without any errors.

[Darin Adler]

What does IE do in this case, by the way?

[Eric Seidel (no email)]

Looks sane to me.  But Adam or Brady are better reviewer choices.

[Steve Block]

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.

[Brady Eidson]

Cleared the review flag based on last comment.

[Patrick]

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.

[Darin Adler]

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.

[Ben Murdoch]

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?

[Darin Adler]

Similar issue, but not same. Probably needs its own bug report.

[Ben Murdoch]

(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.

[Mihai Parparita]

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).

[Mihai Parparita]

(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).