Closed Bug 99920 Opened 20 years ago Closed 19 years ago

[FIX]hitting enter in any text field submits form without submit button passed as param

Categories

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

x86
Linux
defect

Tracking

()

VERIFIED FIXED
mozilla0.9.6

People

(Reporter: blizzard, Assigned: rods)

References

()

Details

(Keywords: compat, dataloss, Whiteboard: [PDT+] [FIXED on trunk, 0.9.4 and 0.9.5 branches])

Attachments

(4 files, 1 obsolete file)

This is a 0.9.4 build.  If you hit enter in any text field in bugzilla the form
is entered.  This is a bad regression.  If you have a form with multiple buttons
hitting return in a text field means that the form is submitted with a vague
button press.  It used to be that the only way that hitting return in a text
field would submit a form was if the text field was the only visible UI form
element.  I suspect that this is going to break a lot of web sites.  We should
really fix it.
Isn't this a feature?
No, it isn't.  Which button did you depress to submit that form?  That
information probably isn't passed to the CGI on the other end which means it
could end up breaking a truckload of sites.
> which means it could end up breaking a truckload of sites

Unlikely, since our behavior now matches IE's.  See bug 22526 for discussion, 
reviews, and the like.
This is definitely a bug, and IE does NOT behave this way.
As much as IE is in violation of standards, mimicking IE's behavior is not
desirable anyway.  It would be naive to think that making such a drastic change
in behavior of a long-established browser would by any means be acceptable.  If
Mozilla changes this drastically from release to release, people will simply
turn to IE as the browser of choice.  What a terrible thing to do!!!
You're forgetting a HUGE piece of the puzzle:
What if there is NO submit button?  What if I want to control form submission by
JavaScript using "document.entryform.submit()"?
Pressing [Enter] on an input element should NEVER cause an automatic submit(),
unless that element is of type "SUBMIT".  If a web page wants to submit the form
based on any action, JavaScript can easily be used to do it.  But having the
Enter key submit the form automatically, basically prevents Mozilla from being
able to be used to repetitively enter groups of numbers.  The numeric keypad has
an [Enter] key.  A common method for moving between fields on the form is to
trap the [Enter] key and treat it the same as [Tab].  Now you've broken this. 
And worse yet, if the user presses Enter on a form, there's no way to trap and
cancel the submit operation because the onSubmit="... return false" is now
broken also.
You can use IE and hit enter to submit a form.  However, IE will use the default
button and pass it on as it that button was clicked.  ( How the "default" is
determined is a mystery but that's a question for another date and time. )  So,
when you use the url above the output will look like this in IE:

o test1
  o test
o test2
  o test
o submit1
  o test

This is what it looks like in Mozilla:

o text1
  O test
o text2
  o test

No submit button as part of the form data!  I know for a fact that when I used
to hack on web sites that when there was more than one button I would depend on
the fact that the button press would _always_ be part of the submit data.

Here's a real live example of how this breaks a web site as well:

go to: http://www.ajb.dni.us/

enter a zip code into the zip code field and hit return.  You will get an error:

    * You need to choose one occupational group to search from.

If you press any of the buttons on the form you get somewhere reasonable.  This
breaks this web site.
Keywords: compat, dataloss
Summary: hitting enter in any text field submits form → hitting enter in any text field submits form without submit button passed as param
ccing pollmann since he fixed bug 22526
Adding nsbranch+ for PDT nomination, and myself to cc: list.
Keywords: nsbranch+
It looks like we purposely doing this, mjudge touched the code last, but I don't 
think he was the original author. Why is it doing this on purpose?

Kin do you know?

It appears that 4.x will automatically submit a form as follows:
         <FORM METHOD=GET ACTION="search">
             <A HREF="search">Text <BR>Search:</A>
             <INPUT TYPE=TEXT NAME="string" VALUE="" SIZE=8>
             <INPUT TYPE=SUBMIT VALUE="Find">
         </FORM>
but if I add another text field it stops submitting. That is really strange.
Status: NEW → ASSIGNED
Attached patch proposed patch (obsolete) — Splinter Review
I have forms that have NO submit button at all (only text input fields), and
it still submits whenever I press [Enter].
Rod, see bug 22526 for all the hows and so on.  I believe that this should be
marked a duplicate of 22526, which should be reopened...

Ron: you're excluding people who don't have javascript in their browser (eg
visually impaired users using Lynx and text-to-speech software) from using your
site... 
blizzard is of course correct, both winie5.5 and winie6 send the first button.
not only do they send it, while focus is in an edit field, the button they
intend to send has default attribute explicitly informing the user what will
happen if enter is pressed.

*** This bug has been marked as a duplicate of 22526 ***
Status: ASSIGNED → RESOLVED
Closed: 19 years ago
Resolution: --- → DUPLICATE
I understand the problem now - reopening
Status: RESOLVED → REOPENED
Resolution: DUPLICATE → ---
In IE the submit button that gets included in the submission is the one that has 
focus. It appears it is always the first submit button, it skips reset buttons.

So I guess I can just assume we send the data for the submit btn.
Status: REOPENED → ASSIGNED
The *real* fix seems like it might be pretty involved.  Will it?  If so, should
we take the proposed patch for the current release?  (Since we probably shipped
6.1 with this bug, could we just take this off the list and fix it for the next
release?)
As far as I know you guys didn't ship this bug in 6.1.  Anyway, it's going to
break lots of sites.

rods, make sure that if you do hit return in a text field where it's the only
field that the submit button data is _not_ included.  That's the once case where
it isn't.
adding PDT for review
Whiteboard: Fix in hand → Fix in hand, PDT
We'd like to be able to take this one. What's the ETA for getting this reviewed
and ready for checkin?
Whiteboard: Fix in hand, PDT → Fix in hand, [PDT], [ETA ?]
Attachment #50698 - Attachment is obsolete: true
Attached patch proposed patchSplinter Review
The only difference between the way we are doing it with this patch and IE, is 
when you click in a text field in IE it sets a "default" focus on the first 
submit it finds in the form. At this time we don't have a notion of "default" 
focus for a set of submit buttons. We only have a single focus model.

But when the user press <return> in a text field we are now doing the right 
thing by using the first submit button in the form as the submitter.

This site http://www.ajb.dni.us/ now work properly, as does the deadbeef url 
above.
Attached file comprehensive testcase
Summary: hitting enter in any text field submits form without submit button passed as param → [FIX]hitting enter in any text field submits form without submit button passed as param
Severity: normal → major
Priority: -- → P1
Target Milestone: --- → mozilla0.9.5
Whiteboard: Fix in hand, [PDT], [ETA ?] → Fix in hand, [PDT], [ETA pending s/sr 9/28?]
In IE, you can trap the key event and use it to do other things, then cancel the
event so that Enter can be used for moving between fields, which is MUST in
financial web applications.  In Mozilla 0.9.4, you can't cancel the submit, no
matter what you do.
Ron, please file a separate bug about the issue you mentioned.
Comment on attachment 51054 [details] [diff] [review]
proposed patch

r/sr=kin@netscape.com

With the following changes:

  - Add support for BUTTON_SUBMIT

  - In nsFormFrame::GetFirstSubmitButtonAndTxtCnt() change the |if|-|if| logic to |if|-|else if|.
Attachment #51054 - Flags: superreview+
I made the changes.
Comment on attachment 51054 [details] [diff] [review]
proposed patch

r=attinasi
Attachment #51054 - Flags: review+
Whiteboard: Fix in hand, [PDT], [ETA pending s/sr 9/28?] → ETA:ready now, Have review/super-review, waiting for approval for branch, [PDT]
The patch doesn't seem to address the point about highlighting the submit button
that will be used if the enter key is hit.  Are we already doing that, or are we
ignoring that?
check it in - PDT+

kin - pls open another bug for the highlighting.
Summary: [FIX]hitting enter in any text field submits form without submit button passed as param → [PDT+] [FIX]hitting enter in any text field submits form without submit button passed as param
I filed the default submit button hiliting feature that selmer mentioned as bug
102057. So I guess rod is cleared for landing on the 0.9.4 branch.
checked in on branch and tip
Status: ASSIGNED → RESOLVED
Closed: 19 years ago19 years ago
Resolution: --- → FIXED
This bug has gone away in build 2001092803.
Thanks a lot for all the hard work!  It is much appreciated!
No, this fix was incorrect, as shown by the testcase provided...the second to
last case, which is undoubtedly the most common (user/password combo), fails.  A
real world example is aolmail.aol.com, which won't submit on an enter in each
textbox.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Also broken is enter in, say, the CC field of a bug page, which submits the form
in IE.
Umm...actually, every case except the fourth and latest are failing for me in
that testcase.
What exactly doesn't work? Because it works the same for me on Wind2k and Linux 
and passes all the tests and acts just like IE.
Status: REOPENED → ASSIGNED
In the aolmail.aol.com case, it looks like IE treats an <input type=img> the 
same as a submit button, so I think all we have to do to get it working again is 
modify nsFormFrame::GetFirstSubmitButtonAndTxtCnt to look for the first 
occurrence of a submit button or an <input type=img>.
r=rods
FYI, I can't reproduce the failure to submit from within the Cc field that blake
is seeing in my 09/30/01 Mozilla Win32 debug build ... in fact this comment was
submitted by hitting return in the Cc field of this bug.
On SuSE Linux, I also see the problems that Blake described, except that the
fifth testcase also works. Most of them fail.

It doesn't seem like the new patch would make the testcases work, since the
testcases use <input type="submit"> rather than <input type="img">.
On today's build on linux, the first, second, third, and second-to-last test
cases do not submit (but say they should).  The rest seem to work as they should.
Blocks: 101793
Summary: [PDT+] [FIX]hitting enter in any text field submits form without submit button passed as param → [FIX]hitting enter in any text field submits form without submit button passed as param
Whiteboard: ETA:ready now, Have review/super-review, waiting for approval for branch, [PDT] → [PDT+]ETA:ready now, Have review/super-review, waiting for approval for branch, [PDT]
Comment on attachment 51523 [details] [diff] [review]
Patch (Adds <input type=img> to list of submit types)

sr=attinasi
r=rods (from comment)
Attachment #51523 - Flags: superreview+
Attachment #51523 - Flags: review+
pls check this in - PD+
Whiteboard: [PDT+]ETA:ready now, Have review/super-review, waiting for approval for branch, [PDT] → [PDT+] [ETA:ready now, Have review/super-review, waiting for approval for branch
fixed
Status: ASSIGNED → RESOLVED
Closed: 19 years ago19 years ago
Resolution: --- → FIXED
The linux specific issues blake, jeff, and akk are seeing, are puzzling since 
this code is all XP. I can help look into the issue tomorrow when I have a linux 
build.
NS_FORM_BUTTON_IMAGE is not defined in any h file. I'm unable to build from cvs 
currently.
Jon, fix for that is in.
FYI, the comprehensive test case works just fine in my 10/02/01 RedHat 6.2 Debug
Mozilla build.

I even submitted this bug via a return in the Cc field.
This is still broken for me in 2001100208 on SuSE Linux. Reopening.

Is there any information I can provide that would help isolate this? Blake, are
you still seeing this?
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Which test(s) fail?
Status: REOPENED → ASSIGNED
I'll try to remember to post a complete list tonight. The first and second ones
definitely failed.
I just verfified that with today's tip build on Linux they all work.
This *still* doesn't work properly for me with the 2001100308 trunk build.
Here's the complete results of the testcases:

1: Fail
2: Fail
3: Fail
4: Pass
   - testcase has only one <input> (+ two <input type="radio">s)
5: Pass
   - testcase has only one <input>
6: Pass
   - note that passing this testcase means that it *doesn't* submit
7: Pass   - note that passing this testcase means that it *doesn't* submit8: Fail
9: Pass
   - testcase has only one <input>

So basically I would summarize my results (excluding testcases 6 and 7) as follows:
- If the form contains only one generic <input> field, then it submits when I
press Enter.
- If the form contains two or more generic <input> fields, then the form does
not submit when I press Enter.
Blake, if you're out there, can you test this again? Is anyone else still seeing
this?
Bugzilla munged my comment a bit--note that testcase 8 fails.
moving to 0.9.6
Target Milestone: mozilla0.9.5 → mozilla0.9.6
I see this bug using build 2001100403 on Win98.

Here are the testcase results I got:
1. Fail
2. Fail
3. Fail
4. Pass
5. Pass
6. Pass
7. Pass
8. Fail
9. Pass
OK.  I have a trunk mozilla 2001-10-03-08 build on Linux.  I see the same
results as djk and Jeff:

1,2,3,8     Fail
4,5,6,7,9   Pass

And I _cannot_ submit by hitting enter in the cc field on this form.  The cc
field used to work, by the way....
OK.  The last build in which testcase #1 works for me is 2001-09-27-08.  In
2001-09-27-21 it fails.
This is really strange since all the tests work in both my Win32 and RH 6.2 
Linux Mozilla Trunk debug builds from 10/04/01.

Are any of you guys building mozilla yourselves? It might be helpful to us if 
you could put a printf in nsGfxTextControlFrame2::SubmitAttempt() that tells us 
why it's not being executed.
My debug build passes the testcase.  My optimized build fails.  Oh, joy.

I added the following printf:

nsIFrame* submitBtn = mFormFrame->GetFirstSubmitButtonAndTxtCnt(inputTxtCnt);
fprintf(stderr, "The button: %p\nThe input count: %d\n", submitBtn, inputTxtCnt);

In my opt build I get (first testcase):
The button: (nil)
The input count: 2

In my debug build I get (first testcase):
The button: 0x897a6ac
The input count: 2
Looks like the button-getting code fails in opt builds...
Yeah, in my optimized builds the test cases where the button should be read
doesn't work at all.  They don't submit when you hit return.
Ok, I just attatched a fix ... the problem was due to the fact that a QI call
was done inside an NS_ASSERTION macro.

I'm even submitting my comment with the return key in the Cc field of this bug
with my OPT build.

Can we get r=, sr=, a= and pdt on this?
looks good r=rods
Removed PDT+ so it shows up on PDT's radar.

PDT: Kin's patch is very simple fix, without it form-submisson is broken in
optimized builds.
Whiteboard: [PDT+] [ETA:ready now, Have review/super-review, waiting for approval for branch → [PDT] [ETA:ready now, Have review/super-review, waiting for approval for branch
pls check this into the branch - PDT+

Whiteboard: [PDT] [ETA:ready now, Have review/super-review, waiting for approval for branch → [PDT+] [ETA:ready now, Have review/super-review, waiting for approval for branch
sr=waterson
*** Bug 103572 has been marked as a duplicate of this bug. ***
Checked in OPT build fix to the TRUNK:


    mozilla/layout/html/forms/src/nsFormFrame.cpp  revision 3.175


and the MOZILLA_0_9_4_BRANCH:


    mozilla/layout/html/forms/src/nsFormFrame.cpp  revision 3.165.2.5


Mailing drivers to see if they want this on the MOZILLA_0_9_5_BRANCH.
Whiteboard: [PDT+] [ETA:ready now, Have review/super-review, waiting for approval for branch → [PDT+] [CANDIDATE_095] fixed on trunk and 0.9.4 branch
*** Bug 103643 has been marked as a duplicate of this bug. ***
I still have questions about the correctness of this patch. I don't think  that
hitting enter while typing in a summary at
http://bugzilla.mozilla.org/enter_bug.cgi?product=Browser should submit that
report when I haven't put any comments in the report. IE seems to do the same
thing but I'm not sure we should be emulating IE on this one. If you read the
original description of this bug it was explicitely reporting a problem with
hitting enter in Bugzilla forms causing unwanted submits. It seems strange that
the fix for this bug is to return to that undesired state.
Either way this doesn't need to go into the 0.9.5 branch. 
This feels, looks and clicks wrong. Submitting the form by pressing enter on a
bugzilla summary entry feels to me like tricking the user. Are we _positive_
this is the correct fix?
I happen to like IE's behavior, and I'm glad that Mozilla has started emulating
it. My preference would be to keep the current behavior. The people who filed
dups of this bug would agree with me, I think; also, IIRC, the original bug to
add this feature was popular (if somewhat controversial).

At the very least, though, no matter what, *PLEASE* make sure that Enter submits
the form if there's an <input type="password"> field in the form. I've always
found this feature most useful for signing into Web sites quickly.
Then I think it should be a pref. UI in preferences would be something like:

Form submit button:
* Enter
o Ctrl-Enter
o None

* - being the default.
At the moment it works like IE in debug builds and has for more than a week. 
This patch merely fixes a optimized build issue, I inadvertently put some 
important code inside a NS_ASSERTION
Whiteboard: [PDT+] [CANDIDATE_095] fixed on trunk and 0.9.4 branch → [PDT+] [CANDIDATE_095] [FIXED on trunk and 0.9.4 branch]
Checked in OPT build fix to the MOZILLA_0_9_5_BRANCH:

    mozilla/layout/html/forms/src/nsFormFrame.cpp  revision 3.173.2.1

a=blizzard@mozilla.org
Status: ASSIGNED → RESOLVED
Closed: 19 years ago19 years ago
Resolution: --- → FIXED
Whiteboard: [PDT+] [CANDIDATE_095] [FIXED on trunk and 0.9.4 branch] → [PDT+] [FIXED on trunk, 0.9.4 and 0.9.5 branches]
verifying on windows 98 2001-10-12-04-0.9.4
Keywords: vtrunk
verifying on 2001-10-24-05-trunk
Status: RESOLVED → VERIFIED
Component: HTML: Form Submission → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.