148782 - <img name="submit">, anywhere on page, shadows form.submit()

Status: RESOLVED FIXED

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

Description

[Andre-John Mas]

From Bugzilla Helper:
User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.0rc3)
Gecko/20020523
BuildID:    2002052306

When I try logging into the website specifying user id and password and then hit
go nothing happens. The same page in Netscape 4.75 and IE 5 both take me to the
e-mail index. Since it works in 4.75 this is probably evidence that this is a
general Javascript problem, rather than IE specific Javascript. Looking in the
Javascript console I get the following error:
  document.loginForm.submit is not a function
  http://www1.sympatico.ca/help/local/bell/getemailloginform.html
  Line: 779

Reproducible: Always
Steps to Reproduce:
1. Type in user id and password
2. Hit go
3.

Actual Results:  No action and the following entry in the Javascript log:

  document.loginForm.submit is not a function
  http://www1.sympatico.ca/help/local/bell/getemailloginform.html
  Line: 779

Expected Results:  Taking me to the index of the mail page

Comment 1

[Christopher Hoess (gone)]

->Browser-General (probably DOM0 or Evang, but I'm having trouble getting to the 
JS right now)

Comment 2

[Boris Zbarsky [:bzbarsky]]

<input type="image" name="submit" src="/help/local/bell/images/topnav/go2b.gif" 
alt="go" border="0" align="bottom">

So form.submit is the image object (this is true in IE/Moz/NS4).

Now form.submit() should take that object and execute it as a function, per the 
ECMA spec.  Mozilla does this. IE and NS4 for some reason do not.  So this site 
is relying on a bug in IE and NS4, basically. Fixing this bug would require a 
lot of work and would be a violation of the ECMA specification, so tech evang.

Comment 3

[Boris Zbarsky [:bzbarsky]]

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

Comment 4

[Johnny Stenback (:jst)]

Inputs of type "image" should not be exposed as properties of a form object,
this is a regression (from way way back). Taking bug, and fix coming up...

Comment 5

[Johnny Stenback (:jst)]

Really taking bug...

Comment 6

[Johnny Stenback (:jst)]

Attachment: Proposed fix, and some random cleanup...

Comment 7

[Johnny Stenback (:jst)]

Comment on attachment 86484 [details] [diff] [review]
Proposed fix, and some random cleanup...

Duh, that wasn't the problem at all...

Comment 8

[Johnny Stenback (:jst)]

Ok, so the problem here is that there's an *image* element named submit (i.e.
not an input of type image, rather an <img name="submit">) on the page and
that's what's shadowing form.submit(). This won't be fixable w/o making img
elements be form controls so that we can check if the image belongs to a
particular form or not...

Comment 9

[Phil Schwartau]

This is especially confusing in cases where <img name="submit"> is not
even contained in any <form> element, and yet the user gets an error in 
the JS Console like 

          Error: document.form1.submit is not a function

even though the <img> is not even contained in the form named "form1".

Comment 10

[Phil Schwartau]

Note bug 148248 was marked INVALID:
image named 'submit' inside of form shadows the method form.submit()

What is the goal in the current bug? Is it this:

1. If <img name="submit"> is inside <form name="form1">, then allow
   it to shadow form1.submit(). This differs from IE and NN4.7

2. If <img name="submit"> is not inside <form name="form1">, then
   make sure is does not shadow form1.submit()


Is that right?

Comment 11

[Andre-John Mas]

I have done additional tests and the same page works in Opera on Windows 2000 and iCab on MacOS X. So its just Mozilla that doesn't deal with it.

Comment 12

[wacheo]

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

Comment 13

[Phil Schwartau]

Note: this looks like a duplicate report of bug 148248,
"image named 'submit' inside of form shadows the method form.submit()"

Comment 14

[Phil Schwartau]

Oops, I missed my own comment #10 above, so let me repeat it:

Note bug 148248 was marked INVALID:
image named 'submit' inside of form shadows the method form.submit()

What is the goal in the current bug? Is it this:

1. If <img name="submit"> is inside <form name="form1">, then allow
   it to shadow form1.submit(). This differs from IE and NN4.7

2. If <img name="submit"> is not inside <form name="form1">, then
   make sure is does not shadow form1.submit()


Is that right?

Comment 15

[Kai Lahmann (is there, where MNG is)]

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

Comment 16

[Dac Chartrand]

The code on the offending page has been changed. It is no longer a violation of
the ECMA specification. Keep an eye out for regressions I suppose, but as it
stands, Sympatico Mozilla users can once again check their email via the web.

http://getemail.sympatico.ca/

Someone verify and mark this bug (evanglism) as fixed.

Comment 17

[Adam Becevello]

It seems Sympatico changed the pages again and they don't work again.  Now I get
a black screen.

Comment 18

[Adam Becevello]

Sorry about the spam, but I found out why it wasn't working for me.  I turned
off Javascript for a site and forgot to turn it on again.  It now works like it
used to.

Sorry

Comment 19

[Andre-John Mas]

I confirm that it appears that Sympatico appears to have changed their code and
it now appears work with Netscape build (Mozilla/5.0 (Windows; U; Windows NT
5.0; en-US; rv:1.0rc2) Gecko/20020512 Netscape/7.0b1), though I can't check with
the latest Mozilla 1.1 beta since gecko appears to have some page rendering issues.

Comment 20

[Boris Zbarsky [:bzbarsky]]

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

Comment 21

[Phil Schwartau]

See bug 173777 Comment 3 for the details of that bug's HTML.
There, we have an example of case 2 in Comment #14 above:

2. If <img name="submit"> is not inside <form name="form1">,
   then make sure is does not shadow form1.submit()

Comment 22

[Raymond Penners]

In my attempt to convert a friend from IE to Mozilla, I stumbled onto this bug.
He reported that Mozilla did not work with ChangeSynergy 4.1 from Telelogic
(http://www.telelogic.com/products/synergy/changesynergy/index.cfm), and after
some investigation I encountered an img named "submit".

So, comment #16 ("Someone verify and mark this bug (evanglism) as fixed") is
incorrect, as this problem really does expose a Mozilla bug. 

Comment 23

[Phil Schwartau]

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

Comment 24

[Johnny Stenback (:jst)]

Mass-reassigning bugs to [e-mail]

Comment 25

[Phil Schwartau]

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

Comment 26

[Samir Gehani]

adt: nsbeta1-

Comment 27

[harm]

I can confirm that this bug still exists with 1.4a.

Any news on this bug ?
It prevents me and my collegues from using mozilla at work.

Comment 28

[Johnny Stenback (:jst)]

Attachment: Don't ever resolve form.submit to anything other than HTMLFormElement.submit

This specifically rules out anything named 'submit' from being reflected as
form.submit.

Comment 29

[Christopher Aillon (sabbatical, not receiving bugmail)]

Comment on attachment 121884 [details] [diff] [review]
Don't ever resolve form.submit to anything other than HTMLFormElement.submit

r=caillon if you promise to not delete parent this time!  :-)

Comment 30

[Johnny Stenback (:jst)]

Comment on attachment 121884 [details] [diff] [review]
Don't ever resolve form.submit to anything other than HTMLFormElement.submit

This is a trivial patch that fixes problems that pop up here n' there on the
web. This makes more sites work, and I can't see how this could break any
sites, since those sites wouldn't work in IE either.

I see no reason not to take this for 1.4b.

Comment 31

[(not reading, please use [e-mail] instead)]

Comment on attachment 121884 [details] [diff] [review]
Don't ever resolve form.submit to anything other than HTMLFormElement.submit

a=sspitzer

Comment 32

[Johnny Stenback (:jst)]

Fix checked in. FIXED.

Comment 33

[harm]

Hi,

I reported Bug 173777 (duplicate of this bug)
As a fix was checked in I tried a nightly (ID:2003042808) on Win2K.

The problem does not seem to solved !

While accessing a ChangeSynergy web interface i still got the same errors and
could not access the page.
javascript errors:
Error: window.document.pt_form_load.submit is not a function
Source File:
http://ccmweb.natlab.research.philips.com:8603/servlet/com.continuus.webpt.servlet.PTweb?token=3953443433632716384&ACTION_FLAG=main_buttonbar_form&TEMPLATE_FLAG=ButtonBar&role=User
Line: 563

See attachment of bug 173777 for the sources of the page that gives errors.

Comment 34

[Peter Van der Beken [:peterv]]

Try a newer nightly, I doubt that one has the fix yet.

Comment 35

[Boris Zbarsky [:bzbarsky]]

> I tried a nightly (ID:2003042808)

That's 8 in the morning on Apr 28, 2003.

The comment about the fix being checked in was made at 12:28 in the afternoon of
the same day.  Hence there is no way for the build you were using to have the fix.

Comment 36

[harm]

I'll try again tomorrow.

Note: I downloaded the nightly about 10 hours _after_ the 'fixed' comment was
made (comment #32). Seems the 'nightly'-link at www.mozilla.org does not really
point to the _latest_ nightlies..., right ?

Comment 37

[Boris Zbarsky [:bzbarsky]]

It does.  There are actually two builds a day -- morning and evening. The
evening ones start building around 10pm, usually.  A build takes 1 hour on a
really fast machine; 2 hours on a more reasonable one.  You do the math.  ;)

Comment 38

[harm]

Ok, retried with 2003043008 (Win2K)
Bug 173777 (duplicate of this bug) is fixed !

I wrongly assumed the nightlies where updated every 2 hours (or as long as a
build takes)

Comment 39

[Johnny Stenback (:jst)]

Reopening since this caused bug 204029.

Comment 40

[harm]

adding comments to this bug (accidently added it to 148782)

note: backing out this fix, does not seem to fix bug  bug 204029 at all.
(I can't download the actual covers on http://www.cdcovers.cc/dvd_i.php. tried
with 1.4b/W2K)

note2:  IE seems to handle both the site mentioned in bug 204029 as well as
sites hit by this bug. So the 2 things don't seem to be mutual exclusive....

Comment 41

[Boris Zbarsky [:bzbarsky]]

> So the 2 things don't seem to be mutual exclusive

They are mutually exclusive in any compliant ECMAScript implementation (an
object is either a function or not; it cannot be both).  This is one of the
known ECMA-compliance bugs in IE, unfortunately.

Comment 42

[harm]

Does this mean that the specific case of Bug 173777 is not fixable ?
Bug 173777 is marked as a duplicate of this bug bug it seems (from Comment 10)
that his bug covers more.

Comment 43

[Heikki Toivonen (remove -bugzilla when emailing directly)]

It seems the URL no longer has the problem, can someone with real account
verify? Is this bug fixed, or did they just change the website?

Comment 44

[Andre-John Mas]

They changed the page and it works now. I had notified Sympatico some time back,
and it has been fixed on their web site for a while. As part of this bug I would
like to suggest two things a) we find a site with the same problem as the
original , and list it here, and b) document the on mozilla site how to correct
common JS errors that prevent the page correctly working with browsers other
than IE.

Comment 45

[Bruce Longee]

This is still an issue - you can see the problem at
http://www.gramophone.co.uk/cdreviews.asp

Comment 46

[Andre-John Mas]

I have resolved to e-mailing web sites that have this issue, and 
submitting Sympatico's Get E-mail page as an example of how to do things right,
since Sympatico has now corrected this issue. The URL which I submit is
http://getemail.sympatico.ca  

Comment 47

[harm]

This bug seems to describe 2 different cases (comment #10)
The 2nd case as mentioned in comment 21 isn't unfixable, right ?

if IE can do it, opera can do it, netscape 4.x can do it. Mozilla should be able
too :)   (bug 173777)

Comment 48

[Boris Zbarsky [:bzbarsky]]

Can someone create a testcase for item 2 in comment 10, please?  That should be
a non-issue assuming the <form> is actually being closed....

Comment 49

[harm]

I attached a testcase to bug 173777. (duplicate of this bug)
According to comment #21 that attachement is a testcase of item 2 in comment 10.

Comment 50

[Boris Zbarsky [:bzbarsky]]

Attachment: testcase

That's not a testcase.	That's a huge HTML page with multiple frames and lots
of javascript... A testcase is something _small_.

This testcase does show the problem, though; looking into it.

Comment 51

[Boris Zbarsky [:bzbarsky]]

Attachment: Another testcase

Comment 52

[Boris Zbarsky [:bzbarsky]]

Attachment: Proposed patch

Comment 53

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 127694 [details] [diff] [review]
Proposed patch

So the short story is that the check in nsHTMLDocument is bogus.  Note that
this patch does not fix the first testcase I attached; unfortunately, the
empty-forms-inside-tables hack makes it nontrivial (if possible) to fix it
without breaking pages right and left.

Comment 54

[Johnny Stenback (:jst)]

Comment on attachment 127694 [details] [diff] [review]
Proposed patch

/me thinks this is the wrong patch :-)

Comment 55

[Boris Zbarsky [:bzbarsky]]

Attachment: Real proposed patch

Pay no attention to the man behind the mixed-up patched curtain.

Comment 56

[Christopher Aillon (sabbatical, not receiving bugmail)]

Comment on attachment 127704 [details] [diff] [review]
Real proposed patch

> 
>+/* static */ PRBool
>+nsContentUtils::BelongsInForm(nsIDOMHTMLFormElement *aForm,
>+                              nsIContent *aContent)
>+{

...

>+  if (form == aContent) {
>+    // The list for aForm contains the form itself, forms should not

No run-on statements please?

>+    // be reachable by name in the form namespace, so we return false
>+    // here.
>+
>+    return PR_FALSE;
>+  }
>+

....

>+  PRInt32 count = 0;
>+
>+  form->ChildCount(count);
>+
>+  if (!count) {
>+    // The form is a leaf and aContent wasn't inside any other form so
>+    // we return true
>+
>+    return PR_TRUE;
>+  }
>+
>+  // The form is a container but aContent wasn't inside the form,
>+  // return false
>+
>+  return PR_FALSE;


  return (count != 0);



>Index: content/html/document/src/nsHTMLDocument.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/content/html/document/src/nsHTMLDocument.cpp,v
>retrieving revision 3.498
>diff -p -u -1 -0 -r3.498 nsHTMLDocument.cpp
>--- content/html/document/src/nsHTMLDocument.cpp	11 Jul 2003 21:14:57 -0000	3.498
>+++ content/html/document/src/nsHTMLDocument.cpp	14 Jul 2003 04:54:57 -0000
>@@ -3747,41 +3747,32 @@ nsHTMLDocument::ResolveName(const nsAStr
>       // PR_FALSE to FindNamedItems().
>       FindNamedItems(aName, mRootContent, *entry, PR_FALSE);
>     }
>   }
> 
>   PRUint32 length;
>   list->GetLength(&length);
> 
>   if (length) {
>     if (length == 1) {
>-      // Onle one element in the list, return the list in stead of
>+      // Only one element in the list, return the element instead of
>       // returning the list
> 

...

> 
>       return NS_OK;
>     }
> 
>     if (length > 1) {
>       // The list contains more than one element, return the whole


length here will always be > 1.  Wanna fix that?  r=caillon

Comment 57

[Johnny Stenback (:jst)]

Comment on attachment 127704 [details] [diff] [review]
Real proposed patch

+/* static */ PRBool
+nsContentUtils::BelongsInForm(nsIDOMHTMLFormElement *aForm,
+			       nsIContent *aContent)

Make that comment a C++ style comment.

Do you think it would be worth checking if aContent is before aForm in the
tree, or after or in the next form (if any, which would mean it either belongs
to that form, or doesn't belong to a form at all)? Should be fairly easy to do
with nsIDOM3Node::ComareDocumentPosition().

sr=jst

Comment 58

[Boris Zbarsky [:bzbarsky]]

Attachment: Now with CompareDocumentPosition!

Comment 59

[Christopher Aillon (sabbatical, not receiving bugmail)]

Comment on attachment 127760 [details] [diff] [review]
Now with CompareDocumentPosition!


>-        if (len < 2) {
>-          // After t nsFormContentList is done filtering there's zero
>-          // or one element in the list, return that element, or null
>-          // if there's no element in the list.
>+      if (len < 2) {
>+        // After t nsFormContentList is done filtering there's zero

Sorry to pile a nit on top of another nit of things that aren't your fault...

but while you're here, could you remove the |t| above?


>+        // or one element in the list, return that element, or null
>+        // if there's no element in the list.
> 
>-          nsCOMPtr<nsIDOMNode> node;
>+        nsCOMPtr<nsIDOMNode> node;
> 
>-          fc_list->Item(0, getter_AddRefs(node));
>+        fc_list->Item(0, getter_AddRefs(node));
> 
>-          *aResult = node;
>-          NS_IF_ADDREF(*aResult);
>+        *aResult = node;
>+        NS_IF_ADDREF(*aResult);


And NS_IF_ADDREF(*aResult = node); since you're touching this as well...

r=caillon

Comment 60

[Johnny Stenback (:jst)]

Comment on attachment 127760 [details] [diff] [review]
Now with CompareDocumentPosition!

sr=jst

Comment 61

[Boris Zbarsky [:bzbarsky]]

Checked in.

Comment 62

[harm]

(Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.5b) Gecko/20030715)

I can comment that bug 173777 (dup of this bug) is indeed solved with this fix.
ChangeSynergy (web-based PR tool from Telelogic) works OK now with the nightly.

This shoud be item 2 in comment 10.

Thanks.

Comment 63

[Boris Zbarsky [:bzbarsky]]

Yep, item two in comment 10 is what I was aiming to fix.  Thanks for the
confirmation!

Comment 64

[Bruce Longee]

This is still a problem at http://www.gramophone.co.uk/cdreviews.asp
, with both 1.5 alpha and nightly build 1.5.0.2003081204.

Comment 65

[Boris Zbarsky [:bzbarsky]]

No, that page shows item 1 from comment 10, as far as I can tell.

In any case, that page badly misnests forms and tables, leading to the parser 
having to mangle things a lot to get something renderable.  That's the one case 
when we could indeed continue having this bug, but there isn't much we can do 
about it short of changing how the parser handles this situation (not likely).

Comment 66

[Olivier Cahagne]

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