567938 - form.submit() broken if called from <input type='submit'>'s onclick handler

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Mathieu Pellerin]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.3a5pre) Gecko/20100524 Minefield/3.7a5pre
Build Identifier: 

The latest minefield fails to submit a form when triggered by the javascript submit() function.

Reproducible: Always

Steps to Reproduce:
1. Open the attached test case
2. Click on the "Submit Query" button

Actual Results:  
Nothing happens.

Expected Results:  
The form should be submitted and page reloaded.

Regression range is within the last 5 days.

I'm surprised this isn't tested in current test suit. :)

Comment 1

[Mathieu Pellerin]

Attachment: test case

Comment 2

[Boris Zbarsky [:bzbarsky]]

Regression range:

http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=145123f19ced&tochange=8d6db7f8fe09

Possibly from bug 563668?  Mounir, can you take a look?

Comment 3

[Mounir Lamouri (:mounir)]

Ok, I'm looking at that.

Comment 4

[Mounir Lamouri (:mounir)]

This is working with <button type='submit'> with <input> and if i call the function from onmousedown or onmouseup. In other words, that's not a failure with |form.submit()| but with |form.submit()| called from the onclick handler of a <input type='submit'>.

I'm still investigating.
And thank you for your report Mathieu :)

Comment 5

[Mounir Lamouri (:mounir)]

Found it, that's a dup of bug 567927

Comment 6

[Mounir Lamouri (:mounir)]

Attachment: Patch v1

This should fix a regression (with a test).

Comment 7

[Mounir Lamouri (:mounir)]

s/a/the/

Comment 8

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 447323 [details] [diff] [review]
Patch v1

r=me, with a few caveats:

1)  Can you also add tests for the other cases fixed by the oldType change?  Separate bug ok.

2)  That test will randomly fail if the about:blank load completes before the <script> has had a chance to execute.  What you want to do is to run your test off the main page onload event (using addLoadEvent), and only set up the iframe's load handler there.

Comment 9

[Mounir Lamouri (:mounir)]

Attachment: Patch v2

Thank you very much for your help on the tests Boris.

Comment 11

[Mounir Lamouri (:mounir)]

Boris, do you think you can review this patch soon ? Otherwise, could you move the review to someone else ? (jst, smaug or sicking maybe)
I suppose this regression must be quite annoying for Minefield users...

Comment 12

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 447335 [details] [diff] [review]
Patch v2

>+++ b/content/html/content/test/test_bug567938-1.html
>+  // Cleaning-up.
>+  for (var tmpChild = form.firstChild; tmpChild; tmpChild = form.firstChild) {
>+    form.removeChild(tmpChild);
>+  }

  form.textContent = "";

And for the other tests too.

>+    is(frames['submit_frame'].location.href,
>+       "http://mochi.test:8888/tests/content/html/content/test/foo",
>+       "The form should have been submitted");

If this is true for submit it will also be true for image, no?  So the real test for image is that onload fires?  This seems to apply to the other tests too.

>+++ b/content/html/content/test/test_bug567938-2.html
>+  element.onclick = function() { form.submit(); element.type='input'; };

s/input/text/ ?

r=bzbarsky

Comment 13

[Mounir Lamouri (:mounir)]

Attachment: Patch v2.1

r=bz

Comment 14

[Mathieu Pellerin]

Any chance we can get this applied to trunk? I'm getting uncomfortable switching to Chromium every time I need to submit info in our database ;)

Comment 15

[Jonathan Kew [:jfkthame]]

Pushed to trunk:
http://hg.mozilla.org/mozilla-central/rev/9e3e79324eb9

Comment 16

[Mathieu Pellerin]

Thanks