Last Comment Bug 192170 - onClick doesn't submit with onsubmit='return false'
: onClick doesn't submit with onsubmit='return false'
Status: RESOLVED FIXED
: testcase
Product: Core
Classification: Components
Component: HTML: Form Submission (show other bugs)
: Trunk
: x86 Windows 2000
: -- normal (vote)
: ---
Assigned To: John Keiser (jkeiser)
: Ashish Bhatt
Mentors:
http://www.cisco.com/pcgi-bin/cpn/cpn...
: 183516 188097 191036 199111 199189 (view as bug list)
Depends on: 138957
Blocks:
  Show dependency treegraph
 
Reported: 2003-02-06 14:47 PST by Olivier Cahagne
Modified: 2003-11-07 06:57 PST (History)
14 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Testcase (235 bytes, text/html)
2003-02-06 14:48 PST, Olivier Cahagne
no flags Details
Patch v1.0 (1.00 KB, patch)
2003-02-07 11:41 PST, Ruslan Ismailov
no flags Details | Diff | Review
Patch (2.52 KB, patch)
2003-02-07 13:23 PST, John Keiser (jkeiser)
peterlubczynski-bugs: review+
bzbarsky: superreview+
asa: approval1.4a+
Details | Diff | Review

Description Olivier Cahagne 2003-02-06 14:47:20 PST
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 2003-02-06 14:48:13 PST
Created attachment 113736 [details]
Testcase
Comment 2 HARUNAGA Hirotoshi 2003-02-06 16:39:53 PST
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 [:bz] (Out June 25-July 6) 2003-02-06 23:17:57 PST
Hmm... Bug 180341 was supposed to _fix_ this.  Apparently it did not?
Comment 4 Olivier Cahagne 2003-02-07 00:03:20 PST
NS4.79 on Win2k works fine with the web site and testcase.
Comment 5 Ruslan Ismailov 2003-02-07 01:09:13 PST
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 2003-02-07 06:26:24 PST
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 2003-02-07 11:14:30 PST
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 2003-02-07 11:41:40 PST
Created attachment 113819 [details] [diff] [review]
Patch v1.0

Patch, that fixed this bug.
Comment 9 John Keiser (jkeiser) 2003-02-07 12:32:05 PST
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 2003-02-07 12:42:49 PST
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) 2003-02-07 13:04:35 PST
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 2003-02-07 13:21:54 PST
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) 2003-02-07 13:23:01 PST
Created attachment 113834 [details] [diff] [review]
Patch

This addresses the root cause.	Tested against bug 138957.  <button> has this
problem too, it turns out.
Comment 14 Olivier Cahagne 2003-02-07 13:27:57 PST
*** Bug 191036 has been marked as a duplicate of this bug. ***
Comment 15 Alexandru Savulov 2003-02-07 13:28:57 PST
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 2003-02-07 13:33:00 PST
Comment on attachment 113834 [details] [diff] [review]
Patch

r=peterl
Comment 17 Alexandru Savulov 2003-02-07 13:36:38 PST
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) 2003-02-07 13:46:44 PST
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 2003-02-07 14:02:25 PST
excellent! go for it.
Comment 20 Boris Zbarsky [:bz] (Out June 25-July 6) 2003-02-07 20:49:22 PST
Comment on attachment 113834 [details] [diff] [review]
Patch

sr=me, but test well!  ;)
Comment 21 John Keiser (jkeiser) 2003-03-26 07:32:26 PST
*** Bug 199189 has been marked as a duplicate of this bug. ***
Comment 22 John Keiser (jkeiser) 2003-03-26 07:39:01 PST
*** Bug 183516 has been marked as a duplicate of this bug. ***
Comment 23 John Keiser (jkeiser) 2003-03-26 07:40:15 PST
*** Bug 199111 has been marked as a duplicate of this bug. ***
Comment 24 John Keiser (jkeiser) 2003-03-26 07:59:21 PST
Taking
Comment 25 Asa Dotzler [:asa] 2003-03-26 22:16:52 PST
Comment on attachment 113834 [details] [diff] [review]
Patch

a=asa (on behalf of drivers) for checkin to 1.4a.
Comment 26 John Keiser (jkeiser) 2003-03-27 07:51:22 PST
Fix checked in.
Comment 27 mozbugs 2003-11-07 06:41:39 PST
bug 188097 is a duplicate marked Tech Evangelism. Could someone with appropriate
permission dupe it to this one?
Comment 28 Olivier Cahagne 2003-11-07 06:57:00 PST
*** Bug 188097 has been marked as a duplicate of this bug. ***

Note You need to log in before you can comment on or make changes to this bug.