onClick doesn't submit with onsubmit='return false'




16 years ago
15 years ago


(Reporter: wolruf, Assigned: john)



Windows 2000

Firefox Tracking Flags

(Not tracked)




(2 attachments, 1 obsolete attachment)

235 bytes, text/html
2.52 KB, patch
: review+
: superreview+
Details | Diff | Splinter Review


16 years ago
build ID: 2003020508 on Win2k.

Steps to reproduce:
1. Load URL or testcase,
2. Try to submit the first form,
3. Nothing happens.

Works fine with IE6.

No error in JavaScript console, didn't find a dupe (searching 'onclick' in subject).

Comment 1

16 years ago
Created attachment 113736 [details]

Comment 2

16 years ago
I don't think this is a bug.
You can use onSubmit to prevent a form from being submitted; to do so,
put a return statement that returns false in the event handler.

and see Bug 180341.
Hmm... Bug 180341 was supposed to _fix_ this.  Apparently it did not?

Comment 4

16 years ago
NS4.79 on Win2k works fine with the web site and testcase.
Keywords: 4xp

Comment 5

16 years ago
Patch to bug 180341 don't fix that. It simply move submission from
DoSubmitorReset to DoSubmit, where we could find following code

867   if(mDeferSubmission) { 
868     // we are in an event handler, JS submitted so we have to
869     // defer this submission. let's remember it and return
870     // without submitting
871     mPendingSubmission = submission;
872     // ensure reentrancy
873     mIsSubmitting = PR_FALSE;
874     return NS_OK; 
875   } 

if in onsubmit stated return false mDeferSubmission is PR_TRUE.

Comment 6

16 years ago
Source of this bug is here (nsHTMLFormElement.cpp):

744   nsresult rv = nsGenericHTMLContainerElement::HandleDOMEvent(aPresContext,
745                                                               aEvent,
746                                                               aDOMEvent,
747                                                               aFlags,
748                                                               aEventStatus); 
749   if (aEvent->message == NS_FORM_SUBMIT) {
750     // let the form know not to defer subsequent submissions
751     mDeferSubmission = PR_FALSE;
752   }

It is possible, that nsGenericHTMLContainerElement kill current event message,
so we don't apply line 751 to it. I think, that correct if in 749 should be:

if ((aEvent->message == NS_FORM_SUBMIT)||
    mGeneratingSubmit) {

Comment 7

16 years ago
When event generated by JS, and onsubmit return false, we follow to this part of

776       if (aEvent->message == NS_FORM_SUBMIT) {
777         // tell the form to flush a possible pending submission.
778         // the reason is that the script returned false (the event was
779         // not ignored) so if there is a stored submission, it needs to
780         // be submitted immediatelly.
781         FlushPendingSubmission();
782       }

And here we could mention, that mPendingSubmission is not defined for generated
event. So I propose to add following code:

if (mGeneratingSubmit && !mPendingSubmission) {
  nsCOMPtr<nsIFormSubmission> submission;
  BuildSubmission(aPresContext, submission, aEvent);
  mPendingSubmission = submission;

(patch followed)

Comment 8

16 years ago
Created attachment 113819 [details] [diff] [review]
Patch v1.0

Patch, that fixed this bug.


16 years ago
Attachment #113819 - Attachment is patch: true
Attachment #113819 - Attachment mime type: text/html → text/plain

Comment 9

16 years ago
So the problem is that the submission wasn't generated in the first place when
you call .submit()?  Why wasn't it generated?  That's the root of the bug.  We
actually *have* to generate the submission at the time .submit() is called, not
at the time that the submit event is caught, because someone could have changed
the form in between that time and now.

Comment 10

16 years ago
If I understand correctly, submisions not generate on first place, because
nsGenericHTMLContainerElement::HandleDOMEvent change aEvent, if onsubmit have
return false. I could not found correct way to dealt with this case.

Comment 11

16 years ago
The submission is generated correctly (just follow nsHTMLFormElement::Submit()).
 The problem is, when the click event returns ok we immediately call
ForgetPendingSubmission() on the assumption that the form will actually be
submitted, so the submission is lost.  Therefore when onsubmit returns false,
the submission that we generated previously is gone.  We should just not call

This is hairy stuff :)  bug 138957 is where this was originally added.
Depends on: 138957

Comment 12

16 years ago
check bug 191036: 


i think this two bugs are the same.

Comment 13

16 years ago
Created attachment 113834 [details] [diff] [review]

This addresses the root cause.	Tested against bug 138957.  <button> has this
problem too, it turns out.
Attachment #113819 - Attachment is obsolete: true

Comment 14

16 years ago
*** Bug 191036 has been marked as a duplicate of this bug. ***

Comment 15

16 years ago
see, there was tha hook, i knew something will go wrong. do we have code there
that deletes the pending submission if we attempt to create a new one? (man, is
been a while and now i have to push all this j2ee stuff in my head)


16 years ago
Attachment #113834 - Flags: review?(peterl)

Comment 16

16 years ago
Comment on attachment 113834 [details] [diff] [review]

Attachment #113834 - Flags: review?(peterl) → review+

Comment 17

16 years ago
wait, do we still submit the name/value pair for the submitting element if we
don't forget the submission? remember: onClick returns true but the pending
submission does not contain the name/value of the triggering element.

Comment 18

16 years ago
Alex: the form submit code forgets the pending submission.  I've run this on all
the testcases in that new test I uploaded in bug 138957.  We submit the
name/value pair if it returns true, and don't if it returns false, and we submit
the right numbers ...


16 years ago
Attachment #113834 - Flags: superreview?(bzbarsky)

Comment 19

16 years ago
excellent! go for it.
Comment on attachment 113834 [details] [diff] [review]

sr=me, but test well!  ;)
Attachment #113834 - Flags: superreview?(bzbarsky) → superreview+

Comment 21

16 years ago
*** Bug 199189 has been marked as a duplicate of this bug. ***

Comment 22

16 years ago
*** Bug 183516 has been marked as a duplicate of this bug. ***

Comment 23

16 years ago
*** Bug 199111 has been marked as a duplicate of this bug. ***


16 years ago
Attachment #113834 - Flags: approval1.3?

Comment 24

16 years ago
Assignee: form-submission → jkeiser


16 years ago
Attachment #113834 - Flags: approval1.3?


16 years ago
Attachment #113834 - Flags: approval1.4a?

Comment 25

16 years ago
Comment on attachment 113834 [details] [diff] [review]

a=asa (on behalf of drivers) for checkin to 1.4a.
Attachment #113834 - Flags: approval1.4a? → approval1.4a+

Comment 26

16 years ago
Fix checked in.
Last Resolved: 16 years ago
Resolution: --- → FIXED

Comment 27

15 years ago
bug 188097 is a duplicate marked Tech Evangelism. Could someone with appropriate
permission dupe it to this one?

Comment 28

15 years ago
*** Bug 188097 has been marked as a duplicate of this bug. ***
You need to log in before you can comment on or make changes to this bug.