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

RESOLVED FIXED

Status

()

Core
HTML: Form Submission
RESOLVED FIXED
14 years ago
14 years ago

People

(Reporter: Olivier Cahagne, Assigned: John Keiser (jkeiser))

Tracking

({testcase})

Trunk
x86
Windows 2000
testcase
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

14 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).
(Reporter)

Comment 1

14 years ago
Created attachment 113736 [details]
Testcase

Comment 2

14 years ago
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.
Hmm... Bug 180341 was supposed to _fix_ this.  Apparently it did not?
(Reporter)

Comment 4

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

Comment 5

14 years ago
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

14 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

14 years ago
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

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

Patch, that fixed this bug.

Updated

14 years ago
Attachment #113819 - Attachment is patch: true
Attachment #113819 - Attachment mime type: text/html → text/plain
(Assignee)

Comment 9

14 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

14 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.
(Assignee)

Comment 11

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

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

Comment 12

14 years ago
check bug 191036: 

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

i think this two bugs are the same.
(Assignee)

Comment 13

14 years ago
Created attachment 113834 [details] [diff] [review]
Patch

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

Comment 14

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

Comment 15

14 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)
(Assignee)

Updated

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

Comment 16

14 years ago
Comment on attachment 113834 [details] [diff] [review]
Patch

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

Comment 17

14 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.
(Assignee)

Comment 18

14 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 ...
(Assignee)

Updated

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

Comment 19

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

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

Comment 21

14 years ago
*** Bug 199189 has been marked as a duplicate of this bug. ***
(Assignee)

Comment 22

14 years ago
*** Bug 183516 has been marked as a duplicate of this bug. ***
(Assignee)

Comment 23

14 years ago
*** Bug 199111 has been marked as a duplicate of this bug. ***
(Assignee)

Updated

14 years ago
Attachment #113834 - Flags: approval1.3?
(Assignee)

Comment 24

14 years ago
Taking
Assignee: form-submission → jkeiser
(Assignee)

Updated

14 years ago
Attachment #113834 - Flags: approval1.3?
(Assignee)

Updated

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

Comment 25

14 years ago
Comment on attachment 113834 [details] [diff] [review]
Patch

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

Comment 26

14 years ago
Fix checked in.
Status: NEW → RESOLVED
Last Resolved: 14 years ago
Resolution: --- → FIXED

Comment 27

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

Comment 28

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