Bug 44140 – No back/forward list entry added when submitting a form via an onclick handler inside a frame

• Component: History
• Status: RESOLVED FIXED

[Kevin M. Dean]

Visit the test link and enter some text in the field and click the "Go to Frame 2" link. In the Resulting frame, click the "Go Back to Frame 1 via javascript history" link and you will see it move back to frame 1 as expected and the field still has the text you typed as expected from a back function.

Now click the form Send button. Clicking the "Go Back to Frame 1 via javascript history" link now breaks in one of 2 ways. If the test link was the 1st page you loaded in the browser tab, nothing will happen instead of going back to frame 1 in the IFRAME. If you had a page loaded in the tab prior to loading the test link, it will go back to that page because it doesn't see any other page in the history between the 2. The IFRAME change is not incrementing the history when the form is submitted.

If you open the IFRAME in a New Tab on it's own via the contextual menu, you'll find that submitting the form now increments just fine and you can return to Frame 1 via the link. It's only when it's an IFRAME.

This is broken in the latest Webkit, Safari 5 and Chrome, while it works as expected in Firefox, Internet Explorer and Opera.

[Darth]

Can confirm this issue.

[Mihai Parparita]

As far as I can tell, this only happens when an onclick handler is used to submit the form. Here's two variants on the reported testcase:

http://persistent.info/webkit/test-cases/form-submit-history/container.html has an <input type="submit"> and lets normal form submission happen. That seems to work fine (history length of 2 if the test case is opened in a new tab).

http://persistent.info/webkit/test-cases/form-submit-history/container-onclick.html has an <input type="button"> which submits the form via an onclick handler. That results in a history length of 1 (if the test case is opened in a new tab).

[Mihai Parparita]

This bug appears to be triggered by http://trac.webkit.org/changeset/52033 (which was fixing bug 32383). In RedirectScheduler::scheduleFormSubmission we lock the back-forward list if a submission is triggered by JS.

Brady, is there a reason why this wasn't also restricted to submissions that were not in response to user gestures?

[Mihai Parparita]

Created attachment 69090 [details]
Patch

[Dimitri Glazkov (Google)]

Comment on attachment 69090 [details]
Patch

ok.

[Darth]

So to be clear, you guys are removing the restriction so that JS can trigger submits? Right?

Or is that the restriction is being extended to also apply to submits not triggered via JS?

[Mihai Parparita]

(In reply to comment #6)
> So to be clear, you guys are removing the restriction so that JS can trigger submits? Right?
> 
> Or is that the restriction is being extended to also apply to submits not triggered via JS?

The restriction isn't about triggering submits, it's about submits adding history entries. With this change, we don't suppress the history entry if the form submit is done by a user action (e.g. a click on the submit button). Entirely programatic submits (e.g. calling form.submit() inside a setTimeout) inside iframes will still not generate history entries.

[Darth]

What if it's a programmatic submit like below, even though it is triggered by a user gesture?

<button type='button' onclick='foo();' />

foo() {
...
...
form.submit();
...
}

I would assume this should still keep isUserGesture true, thus making everything false and not locking out the history list.

What if -

foo() {
...
...
setTimeout(t, form.submit());
...
}

Will in this case isUserGesture be true or false?
Technically it was triggered by a user action however due to that setTimeout it becomes entirely programmatic according to you.

Thanks

[Mihai Parparita]

(In reply to comment #8)
> What if it's a programmatic submit like below, even though it is triggered by a user gesture?
<snip>
> I would assume this should still keep isUserGesture true, thus making everything false and not locking out the history list.

Yes. That's what form-submit-in-frame-via-onclick.html in my patch tests.

> Will in this case isUserGesture be true or false?
> Technically it was triggered by a user action however due to that setTimeout it becomes entirely programmatic according to you.

isUserGesture will be false. For better or worse, this is consistent with user gesture checks throughout WebKit (e.g. for popup blocking).

[WebKit Commit Bot]

Comment on attachment 69090 [details]
Patch

Rejecting patch 69090 from commit-queue.

Failed to run "['./WebKitTools/Scripts/webkit-patch', '--status-host=queues.webkit.org', 'build-and-test', '--no-clean', '--no-update', '--test', '--quiet', '--non-interactive']" exit_code: 2
Last 500 characters of output:
uccessful.
Files=14, Tests=304,  1 wallclock secs ( 0.63 cusr +  0.13 csys =  0.76 CPU)
Running build-dumprendertree
Compiling Java tests
make: Nothing to be done for `default'.
Running tests from /Users/abarth/git/webkit-queue/LayoutTests
Testing 21476 test cases.
java/lc3/JSObject/ToObject-001.html -> failed

Exiting early after 1 failures. 17582 tests run.
261.25s total testing time

17581 test cases (99%) succeeded
1 test case (<1%) had incorrect layout
28 test cases (<1%) had stderr output

Full output: http://queues.webkit.org/results/4167003

[Mihai Parparita]

Comment on attachment 69090 [details]
Patch

Looks like flaky tests made the CQ reject this. http/tests/appcache/origin-quota.html failed the first time, java/lc3/JSObject/ToObject-001.html the second.

[WebKit Commit Bot]

Comment on attachment 69090 [details]
Patch

Rejecting patch 69090 from commit-queue.

Failed to run "['./WebKitTools/Scripts/webkit-patch', '--status-host=queues.webkit.org', 'land-attachment', '--force-clean', '--ignore-builders', '--quiet', '--non-interactive', '--parent-command=commit-queue', 69090]" exit_code: 2
Last 500 characters of output:
d by pre-commit hook (exit code 1) with output:

    The following files contain tab characters:

        trunk/LayoutTests/fast/history/resources/form-with-input-submit.html
        trunk/LayoutTests/fast/history/resources/form-with-onclick-submit.html

    Please use spaces instead to indent.
    If you must commit a file with tabs, use svn propset to set the "allow-tabs" property.
 at /usr/local/git/libexec/git-core/git-svn line 572


Failed to run "[u'git', u'svn', u'dcommit']" exit_code: 1

Full output: http://queues.webkit.org/results/4191004

[Mihai Parparita]

Comment on attachment 69090 [details]
Patch

(In reply to comment #12)
> (From update of attachment 69090 [details])
> Rejecting patch 69090 from commit-queue.
> The following files contain tab characters:

Oops, will remove tabs and land by hand.

[Mihai Parparita]

Committed r68609: <http://trac.webkit.org/changeset/68609>

[Darth]

(In reply to comment #9)
> <snip>

Cool thanks.
Any idea what will be the timeline when this get's up taken by a non-dev channel build of Chrome?

And same question for someone at Apple, when will this be up taken by Safari 5?

[Mihai Parparita]

(In reply to comment #15)
> Any idea what will be the timeline when this get's up taken by a non-dev channel build of Chrome?

Chrome 8 is the most likely release this will be in.

[Darth]

Been almost 4 months, does Safari folks plan to upgrade to a newer webkit version or uptake the fix for this bug? :(

[Kevin M. Dean]

Safari 4 was Webkit 531.x

Safari 5 is Webkit 533.x likely jumping over Webkit Nightlies of 532.x

Webkit is currently 534.x...

...so my completely uniformed guess would be Webkit 535.x release in Safari 6 unless they decide to push a 5.5 or a 5.1 release with a lot of updates.