150232 - Mozilla doesn't submit the form on clicking the ENTER key when <input type="image"> is used

Status: VERIFIED FIXED

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

Description

[Sachin Arora]

Platform: PC
OS: Windows 2000 Professional SP2
Mozilla Build ID: 2002060704
Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.1a) Gecko/20020607

On http://mail.lycos.com and http://www.mail.com/ when I try to login, I have to
press the <Submit> and <Log-in> buttons respectively by a mouse.  While in
Internet explorer I am able to do it just by pressing the <ENTER> key.

Comment 1

[Boris Zbarsky [:bzbarsky]]

The problem is that image inputs are not in form.elements so we think there is
no submit button.

Comment 2

[Boris Zbarsky [:bzbarsky]]

Attachment: minimal testcase

Comment 3

[Boris Zbarsky [:bzbarsky]]

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

Comment 4

[Boris Zbarsky [:bzbarsky]]

This is a regression from the nsFormFrame removal.

Comment 5

[Mats-Olof Sander]

Attachment: Form with one input field

Works ok if the form only contains one input field

Comment 6

[Vladimir Ermakov]

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

Comment 7

[John Keiser (jkeiser)]

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

Comment 8

[Devin Henderson]

i'm seeing this as well, moz 1.1a (2002061908) on winXP professional. is this
going to be fixed/patched any time soon? in my opinion this is a valuable
function that needs to be implemented.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Devin, it'll be patched as soon as someone produces a patch... it's not marked
as "Assigned", but it has a high priority so the developer in the "assigned to"
field would like to get this fixed but is not actively working on it yet... 
There is also no target milestone, so he does not know when he will get to it.

Comment 10

[John Keiser (jkeiser)]

Assigning, though I can't get to it right away.  It's not going to be terribly easy.

Comment 11

[Jesse Ruderman]

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

Comment 12

[Ralph Alvy]

Just in case it matters, this funtionality is working fine with Mozilla
Milestone 1.0 Windows 32, though it still fails with later builds.

Comment 13

[Patrick]

Perhaps related to or dupe of 104449 and 111689 ?

Comment 14

[Boris Zbarsky [:bzbarsky]]

No.  This is not a dup.  It's a regression introduced by removing nsFormFrame,
which happened a looong time after those bugs you cite were filed.

Comment 15

[John Keiser (jkeiser)]

Attachment: Patch v0.9

This patch should do it but is untested.  Posting to transport the patch
between work and home.

Comment 16

[John Keiser (jkeiser)]

Attachment: Patch 0.9 (-u)

Comment 17

[mg8]

I have the same problem with the page <http://www.gmx.de/>, using Mozilla 1.1a
or 1.1b on Mac OS 9.2.2. Interestingly, it only occurred from the 1.1 alpha
version on. Up to 1.0 final, it never happened.

Comment 18

[John Keiser (jkeiser)]

Attachment: Patch

OK, this patch adds EnumerateAllControls into nsIForm, which creates an
enumerator that can go through all the controls in the form in order,
regardless of whether they are in elements or not.  This includes images.

Switching the ENTER key handling to use this works.  I also switched
WalkFormElements to work with this, which simplified the code a huge amount. 
Not that it's overall simpler (the enumerator is a lot of code), but at least
the complexity is a walled off.

This is tested against several places, including bug 99920 and this bug, and
http://www.johnkeiser.com/cgi-bin/mozform.pl and finally the Bugzilla Acid Test
(uploading this patch).

Comment 19

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

mind -u'ing that patch?

Comment 20

[John Keiser (jkeiser)]

Attachment: Patch -u

Same patch, with -u

Comment 21

[John Keiser (jkeiser)]

Attachment: Patch v1.0.1

The last version of the patch was an inbetween snapshot while some things were
being moved around.  This is der patch.

Comment 22

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 94035 [details] [diff] [review]
Patch -u

>+private:
>+  nsHTMLFormElement* mForm;
>+  PRUint32 mControlIndex;
>+  nsVoidArray mNotInElementsArray;
>+  PRInt32 mNotInElementsInd;

please make mNotInElementsArray an nsISupportsArray to make sure we don't leak
or dangle. mElementsIndex and mNotInElementsIndex might be more clear names.

>+  PRBool hasMoreElements;
>+  nsCOMPtr<nsISupports> controlSupports;
>+  nsCOMPtr<nsIFormControl> control;
>+  while (NS_SUCCEEDED(formControls->HasMoreElements(&hasMoreElements)) &&
>+         hasMoreElements) {
>+    rv = formControls->GetNext(getter_AddRefs(controlSupports));
>+    NS_ENSURE_SUCCESS(rv, rv);
>+    control = do_QueryInterface(controlSupports);
> 
>     // Tell the control to submit its name/value pairs to the submission
>     control->SubmitNamesValues(aFormSubmission, aSubmitElement);
>   }

The general rule is to hold on to stuff as short of a time as possible, so
please move the
>+  nsCOMPtr<nsISupports> controlSupports;
>+  nsCOMPtr<nsIFormControl> control;
to inside the loop

>+NS_IMETHODIMP
>+nsHTMLFormElement::EnumerateAllControls(nsISimpleEnumerator** aEnum)

I think GetControlEnumerator would be a more descriptive name?


>+FillFormArrayEnumFunction(nsHashKey *aKey, void *aData, void* closure)
>+{
>+  nsVoidArray* array = NS_STATIC_CAST(nsVoidArray*, closure);
>+
>+  // Go through the array and insert the element at the first place where
>+  // it is less than the element already in the array
>+  nsIFormControl *data = NS_STATIC_CAST(nsIFormControl*, aData);
>+  nsCOMPtr<nsIDOMNode> dataNode = do_QueryInterface(data);
>+  nsCOMPtr<nsIDOMNode> existingNode;
>+  PRInt32 comparison;
>+  for(PRInt32 i=0; i < array->Count(); i++) {

The most common case is probably that the nodes are order in document order, so
if you search in the reversed order you'll probably save cycles in the common
case.

>@@ -1765,6 +1784,84 @@
> NS_IMETHODIMP
> nsHTMLFormElement::SizeOf(nsISizeOfHandler* aSizer, PRUint32* aResult) const
> {
>+
>+
>+// nsFormControlEnumerator
>+NS_IMPL_ADDREF(nsFormControlEnumerator)
>+NS_IMPL_RELEASE(nsFormControlEnumerator)
>+
>+NS_INTERFACE_MAP_BEGIN(nsFormControlEnumerator)
>+NS_INTERFACE_MAP_ENTRY(nsISimpleEnumerator)
>+NS_INTERFACE_MAP_ENTRY(nsISupports)
>+NS_INTERFACE_MAP_END

Couldn't NS_IMPL_ISUPPORTS1 do here?

>+NS_IMETHODIMP
>+nsFormControlEnumerator::GetNext(nsISupports** aNext)
>+{
>+  nsIFormControl* formControl = nsnull;

This one never seems to get released. Please make it an nsCOMPtr.

>+  // First get the form control in form.elements
>+  PRUint32 len;
>+  mForm->GetElementCount(&len);
>+  if (mControlIndex < len) {
>+    mForm->GetElementAt(mControlIndex, &formControl);
>+  }
>+  // If there are still controls in mNotInElementsArray, determine whether said
>+  // control is before the current control in the array, and if so, choose it
>+  // instead
>+  if (mNotInElementsInd < mNotInElementsArray.Count()) {
>+    // Get the not-in-elements control
>+    nsIFormControl* formControl2 = NS_STATIC_CAST(nsIFormControl*,

Make this too an nsCOMPtr, otherwise you'll leak in all the NS_ENSURE_SUCCESS
below.


with that, r=sicking

Comment 23

[John Keiser (jkeiser)]

Attachment: Patch v2.0

This patch fixes most of sicking's issues, PLUS makes mNotInElements an array
instead of a hashtable for efficiency.	The only problem with it being a
hashtable was a logic error where we were adding things to the hashtable
twice--in AddElementToTable and RemoveElementFromTable.  This needs re-review.

This patch has been tested against image submission, normal submission, adding
and removing image elements from a form, and making sure elements' .form got
cleared when the form went away.  And as always I am uploading this patch with
the patched build.

The one thing I didn't fix from your review, sicking:

> The general rule is to hold on to stuff as short of a time as possible, so
> please move the
> >+  nsCOMPtr<nsISupports> controlSupports;
> >+  nsCOMPtr<nsIFormControl> control;
> to inside the loop

I agree, but I have also heard one should not declare COM pointers inside
loops.	Compilers do funny things and don't always release them.

Comment 24

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 94280 [details] [diff] [review]
Patch v2.0

>-void
>-nsFormControlList::SetForm(nsIDOMHTMLFormElement* aForm)
>+static PRBool PR_CALLBACK
>+FillFormArrayEnumFunction(nsHashKey *aKey, void *aData, void* closure)
> {
>-  mForm = aForm; // WEAK - the form owns me
> }

What does this function do? It doesn't seem to be used


>+NS_IMPL_ISUPPORTS1(nsFormControlEnumerator,nsISimpleEnumerator);

space after ','. Didn't you run this through jst-review.pl? ;-)

>+
>+nsFormControlEnumerator::nsFormControlEnumerator(nsHTMLFormElement* aForm)
>+  : mForm(aForm), mElementsIndex(0), mNotInElementsIndex(0)
>+{
>+  NS_INIT_ISUPPORTS();
>+
>+  // Create the sorted mNotInElementsSorted array
>+  PRInt32 len = aForm->mControls->mNotInElements.Count();
>+  for (PRInt32 indexToAdd=0; indexToAdd < len; indexToAdd++) {

Don't declare indexToAdd inside the |for|

>+    // Ref doesn't need to be strong, don't bother making it so
>+    nsIFormControl* controlToAdd = NS_STATIC_CAST(nsIFormControl*,
>+        aForm->mControls->mNotInElements.ElementAt(indexToAdd));
>+
>+    // Go through the array and insert the element at the first place where
>+    // it is less than the element already in the array
>+    nsCOMPtr<nsIDOMNode> controlToAddNode = do_QueryInterface(controlToAdd);
>+    nsCOMPtr<nsIDOMNode> existingNode;
>+    PRUint32 existingArrayLen = 0;

This should always be equal to indexToAdd, no?

>+    mNotInElementsSorted.Count(&existingArrayLen);
>+    PRBool inserted = PR_FALSE;
>+    if (existingArrayLen > 0) {
>+      PRUint32 i = existingArrayLen;
>+      do {
>+        i--;

Wouldn't a |for| construct be much nicer here?

>+        existingNode = do_QueryElementAt(&mNotInElementsSorted, i);
>+        PRInt32 comparison;
>+        if (NS_FAILED(nsHTMLFormElement::CompareNodes(controlToAddNode,
>+                                                      existingNode,
>+                                                      &comparison))) {
>+          break;
>+        }

>+        if (comparison < 0) {

You need to invert this comparison since you can't look at if the new node has
a position before the last node in the array...

>+          mNotInElementsSorted.InsertElementAt(controlToAdd, i);
>+          inserted = PR_TRUE;
>+          break;
>+        }
>+      } while(i > 0);
>+    }
>+    
>+    // If it wasn't inserted yet, it is greater than everything in the array
>+    // and must be appended.
>+    if (!inserted) {
>+      mNotInElementsSorted.AppendElement(controlToAdd);

Since you're going backwards in the list you should add the node first if all
comparisons fails.

Comment 25

[John Keiser (jkeiser)]

Attachment: Patch v2.1

> Don't declare indexToAdd inside the |for|

What is wrong with declaring indexToAdd inside the for?  We do that sort of
thing all over the place.  It's cleaner.

> Wouldn't a |for| construct be much nicer here?

I tried that first, but there was no sane way I could think of. 
existingArrayLen is a PRUint32; I have to be able to compare it to 0 *before*
it is decremented at the end of the loop (because it will go to -1, or <the
biggest possible positive number>.

> You need to invert this comparison since you can't look at if the new node
has
a position before the last node in the array...

It is strange that that worked (but it did).  It's fixed now.

Comment 26

[Boris Zbarsky [:bzbarsky]]

> What is wrong with declaring indexToAdd inside the for?

It gives it different scope on different compilers.  See
http://www.mozilla.org/hacking/portable-cpp.html#variables_in_for.  Note that
MSVC++ is the same as the HP-UX compiler in this matter...

Comment 27

[John Keiser (jkeiser)]

It still doesn't seem worth it to me, especially in a case like this.

I suspect the portability guide is outdated.  I have checked in code like this:

    for (int i=0; ...) {
      // do something
    }
    for (i=0; ...) {
      // do something else
    }

And didn't break ports.  The scope of i is always outside the for loop.

Comment 28

[Boris Zbarsky [:bzbarsky]]

> The scope of i is always outside the for loop.

In your example, the scope should be _inside_ the for loop per the current C++
standard.  If that code worked on all our compilers, you got ridiculously lucky
(and they're all bending over backwards for you).

Comment 29

[John Keiser (jkeiser)]

Attachment: Patch v2.1.1

Fix trouble with double for().

I really don't think that fixing the other one is worth the pain and ugliness. 
A quick "grep ' for ' content/html/content/src/*.cpp" will show that this is a
pretty normal pattern.

Comment 30

[Johnny Stenback (:jst)]

Comment on attachment 94615 [details] [diff] [review]
Patch v2.1.1

@@ -305,7 +309,7 @@
   // This is needed to properly clean up the bi-directional references
   // (both weak and strong) between the form and its form controls.

-  nsHashtable* mNotInElements; // Holds WEAK references
+  nsAutoVoidArray mNotInElements; // Holds WEAK references

I'd suggest using an nsCheapVoidArray here in stead, an nsAutoVoidArray seems
excessive here (since it contains an internal buffer for holding 8 pointers w/o
allocation).

- In nsFormControlEnumerator::nsFormControlEnumerator():

+    if (indexToAdd > 0) {
+      PRUint32 i = indexToAdd;
+      do {
+	 i--;
...
+      } while (i > 0);
+    }
+

Wouldn't that make more sense as:

+    PRUint32 i = indexToAdd;
+    while (i > 0) {
+      --i;
...
+    }

Other than that, sr=jst

Comment 31

[John Keiser (jkeiser)]

Fix checked in 8/17, forget to mark fixed, sorry

Comment 32

[Vladimir Ermakov]

verifying on build 2002-08-27-04-trunk

Comment 33

[Boris Zbarsky [:bzbarsky]]

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

Comment 34

[Olav Vitters]

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

Comment 35

[Patrick]

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

Comment 36

[Boris Zbarsky [:bzbarsky]]

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

Comment 37

[Eric N. Winn]

Platform: PC
OS: Lycoris Desktop/LX Build 53
Mozilla Build ID:
Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.1: Desktop/LX Amethyst) Gecko/20020826

On http://www.comcast.net/comcast.html I try to login but cannot. This form has
2 text elements and an input type=image for submit. Just below is a Google
search form with a single text element and an input type=image for submit. I
cannot use <ENTER> or clicking the mouse on the "Go" image or any combination
cursor in text field + <ENTER> or focus on image + <ENTER>, etc.

The Google search works. The user login does not.

Trying the minimal test case here does not work. Trying the "Form with one input
field" does work.

Not sure if this is the same bug reappearing or a problem with the Lycoris build.

Other odd note. Going to this page with a logged in Bugzilla account only shows
"Leave as VERIFIED FIXED" but earlier view as an anonymous user also shows
"Reopen bug" and "Mark bug as CLOSED". Both pages show the Commit button.

Comment 38

[Malcolm Scott]

Eric: I suspect that Lycoris has picked up a build without this fix. Try
upgrading to a later version.

Comment 39

[Boris Zbarsky [:bzbarsky]]

From the roadmap:

milestone  start        freeze       branch    ideal release  actual release
1.1     12-Jul-2002  31-Jul-2002  02-Aug-2002  09-Aug-2002    26-Aug-2002

So anything that got fixed after Aug 2 (like this bug) is not in 1.1.

Comment 40

[Christian :Biesinger (don't email me, ping me on IRC)]

>So anything that got fixed after Aug 2 (like this bug) is not in 1.1.

well, some fixes after aug. 2 probably _are_ in 1.1, but only if they got
approval and were checked in on the branch.