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

Status: VERIFIED FIXED

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

Description

[Christopher Blizzard (:blizzard)]

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.

Comment 1

[Oliver Klee]

Isn't this a feature?

Comment 2

[Christopher Blizzard (:blizzard)]

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.

Comment 3

[Boris Zbarsky [:bzbarsky]]

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

Comment 4

[Ron Cemer]

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

Comment 5

[Ron Cemer]

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.

Comment 6

[Christopher Blizzard (:blizzard)]

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.

Comment 7

[Boris Zbarsky [:bzbarsky]]

ccing pollmann since he fixed bug 22526

Comment 8

[Jaime Rodriguez, Jr.]

Adding nsbranch+ for PDT nomination, and myself to cc: list.

Comment 9

[rods (gone)]

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.

Comment 10

[rods (gone)]

Attachment: proposed patch

Comment 11

[Ron Cemer]

I have forms that have NO submit button at all (only text input fields), and
it still submits whenever I press [Enter].

Comment 12

[Boris Zbarsky [:bzbarsky]]

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

Comment 13

[timeless]

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.

Comment 14

[rods (gone)]

*** This bug has been marked as a duplicate of 22526 ***

Comment 15

[rods (gone)]

I understand the problem now - reopening

Comment 16

[rods (gone)]

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.

Comment 17

[selmer (gone)]

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?)

Comment 18

[Christopher Blizzard (:blizzard)]

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.

Comment 19

[msanz]

adding PDT for review

Comment 20

[Jaime Rodriguez, Jr.]

We'd like to be able to take this one. What's the ETA for getting this reviewed
and ready for checkin?

Comment 21

[rods (gone)]

Attachment: proposed patch

Comment 22

[rods (gone)]

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.

Comment 23

[rods (gone)]

Attachment: comprehensive testcase

Comment 24

[Ron Cemer]

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.

Comment 25

[kinmoz]

Ron, please file a separate bug about the issue you mentioned.

Comment 26

[kinmoz]

Comment on attachment 51054 [details] [diff] [review]
proposed patch

r/sr=[e-mail]

With the following changes:

  - Add support for BUTTON_SUBMIT

  - In nsFormFrame::GetFirstSubmitButtonAndTxtCnt() change the |if|-|if| logic to |if|-|else if|.

Comment 27

[rods (gone)]

I made the changes.

Comment 28

[Marc Attinasi]

Comment on attachment 51054 [details] [diff] [review]
proposed patch

r=attinasi

Comment 29

[selmer (gone)]

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?

Comment 30

[Jaime Rodriguez, Jr.]

check it in - PDT+

kin - pls open another bug for the highlighting.

Comment 31

[kinmoz]

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.

Comment 32

[rods (gone)]

checked in on branch and tip

Comment 33

[Ron Cemer]

This bug has gone away in build 2001092803.
Thanks a lot for all the hard work!  It is much appreciated!

Comment 34

[Blake Ross]

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.

Comment 35

[Blake Ross]

Also broken is enter in, say, the CC field of a bug page, which submits the form
in IE.

Comment 36

[Blake Ross]

Umm...actually, every case except the fourth and latest are failing for me in
that testcase.

Comment 37

[rods (gone)]

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.

Comment 38

[kinmoz]

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

Comment 39

[kinmoz]

Attachment: Patch (Adds <input type=img> to list of submit types)

Comment 40

[rods (gone)]

r=rods

Comment 41

[kinmoz]

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.

Comment 42

[Jeff Williams]

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

Comment 43

[Akkana Peck]

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.

Comment 44

[Marc Attinasi]

Comment on attachment 51523 [details] [diff] [review]
Patch (Adds <input type=img> to list of submit types)

sr=attinasi
r=rods (from comment)

Comment 45

[Jaime Rodriguez, Jr.]

pls check this in - PD+

Comment 46

[rods (gone)]

fixed

Comment 47

[kinmoz]

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.

Comment 48

[Jon Smirl]

NS_FORM_BUTTON_IMAGE is not defined in any h file. I'm unable to build from cvs 
currently.

Comment 49

[Boris Zbarsky [:bzbarsky]]

Jon, fix for that is in.

Comment 50

[kinmoz]

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.

Comment 51

[Jeff Williams]

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?

Comment 52

[rods (gone)]

Which test(s) fail?

Comment 53

[Jeff Williams]

I'll try to remember to post a complete list tonight. The first and second ones
definitely failed.

Comment 54

[rods (gone)]

I just verfified that with today's tip build on Linux they all work.

Comment 55

[Jeff Williams]

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?

Comment 56

[Jeff Williams]

Bugzilla munged my comment a bit--note that testcase 8 fails.

Comment 57

[rods (gone)]

moving to 0.9.6

Comment 58

[djk]

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

Comment 59

[Boris Zbarsky [:bzbarsky]]

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

Comment 60

[Boris Zbarsky [:bzbarsky]]

OK.  The last build in which testcase #1 works for me is 2001-09-27-08.  In
2001-09-27-21 it fails.

Comment 61

[kinmoz]

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.

Comment 62

[Boris Zbarsky [:bzbarsky]]

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

Comment 63

[Christopher Blizzard (:blizzard)]

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.

Comment 64

[kinmoz]

Attachment: Patch (Fixes OPT Builds)

Comment 65

[kinmoz]

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?

Comment 66

[rods (gone)]

looks good r=rods

Comment 67

[Kevin McCluskey (gone)]

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.

Comment 68

[Jaime Rodriguez, Jr.]

pls check this into the branch - PDT+

Comment 69

[Chris Waterson]

sr=waterson

Comment 70

[Boris Zbarsky [:bzbarsky]]

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

Comment 71

[kinmoz]

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.

Comment 72

[Boris Zbarsky [:bzbarsky]]

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

Comment 73

[Asa Dotzler [:asa]]

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. 

Comment 74

[Christian Reis]

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?

Comment 75

[Jeff Williams]

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.

Comment 76

[Aleksander Adamowski]

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.

Comment 77

[rods (gone)]

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

Comment 78

[kinmoz]

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=[e-mail]

Comment 79

[Vladimir Ermakov]

verifying on windows 98 2001-10-12-04-0.9.4

Comment 80

[Vladimir Ermakov]

verifying on 2001-10-24-05-trunk