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

Status: RESOLVED FIXED

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

Description

[Olivier Cahagne]

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

[Olivier Cahagne]

Attachment: Testcase

Comment 2

[HARUNAGA Hirotoshi]

I don't think this is a bug.
http://developer.netscape.com/docs/manuals/js/client/jsref/handlers.htm#1121163
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.
Evangelism.

Comment 3

[Boris Zbarsky [:bzbarsky]]

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

Comment 4

[Olivier Cahagne]

NS4.79 on Win2k works fine with the web site and testcase.

Comment 5

[Ruslan Ismailov]

Patch to bug 180341 don't fix that. It simply move submission from
DoSubmitorReset to DoSubmit, where we could find following code
(nsHTMLFormElement.cpp):

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

[Ruslan Ismailov]

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

[Ruslan Ismailov]

When event generated by JS, and onsubmit return false, we follow to this part of
code.

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

[Ruslan Ismailov]

Attachment: Patch v1.0

Patch, that fixed this bug.

Comment 9

[John Keiser (jkeiser)]

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

[Ruslan Ismailov]

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

[John Keiser (jkeiser)]

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

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

Comment 12

[Alexandru Savulov]

check bug 191036: 

http://bugzilla.mozilla.org/show_bug.cgi?id=191036#c7

i think this two bugs are the same.

Comment 13

[John Keiser (jkeiser)]

Attachment: Patch

This addresses the root cause.	Tested against bug 138957.  <button> has this
problem too, it turns out.

Comment 14

[Olivier Cahagne]

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

Comment 15

[Alexandru Savulov]

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)

Comment 16

[Peter Lubczynski]

Comment on attachment 113834 [details] [diff] [review]
Patch

r=peterl

Comment 17

[Alexandru Savulov]

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

[John Keiser (jkeiser)]

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

Comment 19

[Alexandru Savulov]

excellent! go for it.

Comment 20

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 113834 [details] [diff] [review]
Patch

sr=me, but test well!  ;)

Comment 21

[John Keiser (jkeiser)]

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

Comment 22

[John Keiser (jkeiser)]

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

Comment 23

[John Keiser (jkeiser)]

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

Comment 24

[John Keiser (jkeiser)]

Taking

Comment 25

[Asa Dotzler [:asa]]

Comment on attachment 113834 [details] [diff] [review]
Patch

a=asa (on behalf of drivers) for checkin to 1.4a.

Comment 26

[John Keiser (jkeiser)]

Fix checked in.

Comment 27

[mozbugs]

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

Comment 28

[Olivier Cahagne]

*** Bug 188097 has been marked as a duplicate of this bug. ***