No back entry created for navigations created by assigning to document.location Tested using rev 20934 Test case coming up...
Created attachment 14083 [details] test case Load this page and click the "go to next page", which navigates back to itself w/ modifications to the query string to result in a unique URL. Notice that no back entry is created for these navigations!
Doh, unfortunately you will need to download this testcase to reproduce the bug. Bugzilla's query params interfere.
Created attachment 14084 [details] better test case This version of the test case should work when hosted on bugzilla.
This regression was introduced between r20809 (pass) and r20814 (fail).
Probably caused by: http://trac.webkit.org/projects/webkit/changeset/20813
Created attachment 14107 [details] A further simplified attachment document.location doesn't exist, but the document part can safely be ignored. location is an object, not a variable. Normally, an assignment like location=URL should produce an error.
(In addition to comment #6) Okay, I must admit that document.location does exist and location=... is an appropriate alternative to location.href=... All four buttons should work like the first two.
(In reply to comment #7) > (In addition to comment #6) > Okay, I must admit that document.location does exist and location=... is an > appropriate alternative to location.href=... > All four buttons should work like the first two. What do other browsers (like MSIE 6/7 and Firefox 2.0.x) do with this test?
(In reply to comment #8) > > What do other browsers (like MSIE 6/7 and Firefox 2.0.x) do with this test? > Firefox 2 and IE 7 on Vista both redirect to webkit.org for all 4 buttons.
Err....ignore me.
(In reply to comment #8) > What do other browsers (like MSIE 6/7 and Firefox 2.0.x) do with this test? Gecko: http://developer.mozilla.org/en/docs/DOM:document.location http://developer.mozilla.org/en/docs/DOM:window.location (i.e., "you can also assign to this property to load another URL")
<rdar://problem/5153025>
Created attachment 14617 [details] window.open test case I believe this bug also occurs when doing a window.open in an onload handler. The initial navigation is not stored in session history. Note that if you do window.open in an onclick handler (in the attached test case, click on the text), the nav is stored. I believe this is due to the difference in handling of userGestured vs non-userGestured loads (the latter is done as FrameLoadTypeInternal).
Created attachment 15068 [details] simple patch Looks like the problem with setting document.location is just that the userGesture flag wasn't properly tracked.
Comment on attachment 15068 [details] simple patch We need a regression test for this.
Created attachment 15072 [details] patch with regression test Here's the patch above, but with a regression test this time.
Comment on attachment 15072 [details] patch with regression test Good catch, but I don't think this is quite the right fix. A location assignment should add a new history item even if off a timer (at least if it's not within a short time period of the original page load. See modified-testcase.html. In Firefox, it appears that assigning document.location in the normal way *always* creates a new history item. See even-immediate-assignment.html. Given this, I think the right thing is probably to always pass true as the userGesture argument, but you may want to test other similar cases and add them as LayoutTests if you think of other corner cases. Otherwise this patch looks great!
Created attachment 15236 [details] a test that fails with this patch.
Created attachment 15237 [details] another difference between Firefox 2 and this patch
Interesting. Firefox appears to add a history item in every case EXCEPT when document.location is directly assigned in an onload handler. If it occurs through a timeout or user gestures, a history item is added. My guess is that a direct assignment in onload is treated as a client redirect and not added to history. We can get close to this behavior if we just force userGesture = true here (that is, always pass false for lockHistory and true for userGesture). But, in that case, we'd still add a history item for a direct assignment in onload. Unfortunately, if we do that, we end up failing/hitting asserts in several layout tests. I'll have to look at this more.
Actually, I take that back. I'm hitting asserts and various errors when I run the layout tests, but (a) none of the tests fail, and (b) it happens without this patch applied. Maybe something else about my checkout is hosed.
Created attachment 15260 [details] simple patch with slew of tests As Maciej suggested, I went with just forcing userGesture=true. This does introduce a difference with Firefox: doing something like onload="document.location = bla" does not create a back/forward item in Firefox, but will for us. I'm not sure whether that's a problem. It seems like overall this is an improvement (this brings us closer to Firefox in other ways).
Comment on attachment 15260 [details] simple patch with slew of tests Awesome, I love the tests, could you please file a bug on the remaining small difference from Firefox behavior? I do think we want such cases handled as a client redirect. It would also be good to check if inline scripts that don't set a timer and happen before onload have this behavior, I suspect they do. r=me
Comment on attachment 15260 [details] simple patch with slew of tests You're missing a ChangeLog for your WebCore changes, and your layout test results have hard-coded paths which are specific to your machine. I believe navigation tests need to be HTTP tests (http/tests/navigation) so that they will have consistent paths across machines.
Created attachment 15278 [details] fix above patch Same as above patch, but - moved the tests to http/tests/navigation - included changes to WebCore/ChangeLog as bdash suggested.
Comment on attachment 15278 [details] fix above patch + bool userGesture = true; + frame->loader()->scheduleLocationChange(str, activeFrame->loader()->outgoingReferrer(), !userGesture, userGesture); There's no need for the userGesture variable anymore. Can you remove it?
Created attachment 15301 [details] small tweak to above patch Nuked the userGesture var.
Comment on attachment 15301 [details] small tweak to above patch r=me
Landed in r24004.
*** Bug 14147 has been marked as a duplicate of this bug. ***