54136 - name, id share namespace for checkboxes with label for=

Status: VERIFIED FIXED

(Core :: Layout: Form Controls, defect, P2)

Description

[Jesse Ruderman]

Mozilla accepts code like

<input type=checkbox name="a"><label for="a">label</label>.

This code doesn't work on IE, and afaict shouldn't work according to the HTML 4 
spec.  There was some discussion about this in bug 1394, where it was decided 
that the id and name attributes should not share a namespace.

Comment 1

[Jesse Ruderman]

Attachment: test case

Comment 2

[rods (gone)]

My interpretation of the spec and the comments in this bug, is the label should 
ONLY look for "id" and not "name"

Comment 3

[rods (gone)]

nsIHTMLDocument which is implemented by nsHTMLDocument,  needs a new method for 
searching for a piece of content by Id and just Id. The current GetElementById 
checks the Id and then name as searches throught the content tree. 

Therefore, in this example of html:

<input type=checkbox name="e1" id="e2">
<input type=checkbox name="e2" id="e1">
<label for="e1">matches name of first and id of second</label>

the label element finds the wrong "for" content, because the name "e1" will 
match on the first piece of content instead of the second.

One suggestion would be to add a new arg to the MatchName method indicating 
whether it should check name and id. 
Then add a new method to the nsIHTMLDocument interface and nsHTMLDocument impl 
called GetElementByIdOnly

Comment 4

[Jesse Ruderman]

I disagree with futuring this bug.  This bug allows page authors to write 
incorrect HTML that will work on Mozilla but not Internet Explorer.  This is 
exactly the kind of thing that we're frustrated at Microsoft for doing with IE.
Since IE doesn't implement labels perfectly, webpage authors are likely to 
assume "oh, this just doesn't work in IE" and leave the bad HTML lying around.

I haven't seen a whole lot of other Mozilla bugs like this, so I don't know 
what priority they generally get.

Should this bug get the "html4" keyword and/or block bug 7954?

Comment 5

[rods (gone)]

The fix isn't that hard and I know this is one of those bugs that lets you write 
bad html. But they will never let me check this in for this release because it 
is not severe enough. So the best we can do and release note it for now. (As I 
said, the bad part of this is some people will write bad html and not even know 
it)

Comment 6

[rods (gone)]

Captured from mail:

ekrock wrote:

We are violating the DOM1 specification in a very basic way here on one of the 
most-widely used DOM1 methods of all. We can make a case that this is 
high-profile standards compliance and low-risk. 

Rod Spears wrote: 

Hey, I am all for change GetElementById to do just that, I thought we had to 
have the part where it matches on name for some reason. 

The fix then is very simple and low risk. I think the motivation is that without 
the fix it will enable a lot of BAD html/script to be written and then down the 
road when we fix it, it will break a lot of pages. 

Eric K., what are your thoughts, do we stand a chance of getting this through? 

  Here is the diff from my trunk build: 

  Index: nsHTMLDocument.cpp 
  =================================================================== 
  RCS file: /cvsroot/mozilla/layout/html/document/src/nsHTMLDocument.cpp,v 
  retrieving revision 3.286 
  diff -u -r3.286 nsHTMLDocument.cpp 
  --- nsHTMLDocument.cpp  2000/09/22 10:18:05     3.286 
  +++ nsHTMLDocument.cpp  2000/09/29 14:08:03 
  @@ -2325,10 +2325,6 @@ 
         aName.Equals(value)) { 
       return aContent; 
     } 
  -  else if ((NS_CONTENT_ATTR_HAS_VALUE == 
aContent->GetAttribute(kNameSpaceID_HTML, nsHTMLAtoms::name, value)) && 
  -           aName.Equals(value)) { 
  -    return aContent; 
  -  } 

     PRInt32 i, count; 
     aContent->ChildCount(count); 

  Rod 
    

  Johnny Stenback wrote: 

    Rod Spears wrote: 

    > nsIHTMLDocument which is implemented by nsHTMLDocument,  needs a new 
    > method for searching for a piece of content by Id and just Id. The 
    > current GetElementById checks the Id and then name as searches 
    > throught the content tree. 
    > 
    > Therefore, in this example of html: 
    > 
    > <input type=checkbox name="e1" id="e2"> 
    > <input type=checkbox name="e2" id="e1"> 
    > <label for="e1">matches name of first and id of second</label> 
    > 
    > the label element finds the wrong "for" content, because the name "e1" 
    > will match on the first piece of content instead of the second. 
    > 
    > One suggestion would be to add a new arg to the MatchName method 
    > indicating whether it should check name and id. Then add a new method 
    > to the nsIHTMLDocument interface and nsHTMLDocument impl called 
    > GetElementByIdOnly 
    > 
    > What do you think? 
    > 

    I think we should fix this, but I'm not sure we'll be able to convinse 
    PDT that this must go in for rtm. 

    Why would we need a GetElementByIdOnly()? GetElementById() should simply 
    get elements with a matching ID, not a matching NAME. If someone needs a 
    method that returns elements with a certain name, they can use the DOM 
    method GetElementsByName(). 

Comment 7

[rods (gone)]

Attachment: official patch

Comment 8

[rods (gone)]

Attachment: patch file, this should be correct (this time)

Comment 9

[Kevin McCluskey (gone)]

Marking rtm+. Low risk fix. We are violating the DOM 1 spec, opening the door to
people writing script that will not work with future versions of the browser.

Comment 10

[Kevin McCluskey (gone)]

Added ekrock's comment + marking P2.

ekrock wrote:

"We are violating the DOM1 specification in a very basic way here on one of the
most-widely used DOM1 methods of all. We can make a case that this is
high-profile standards compliance and low-risk."

Comment 11

[Kevin McCluskey (gone)]

Marking [rtm need info] Waiting for review + super-review.

Comment 12

[Michael La Guardia]

Marking rtm-.  This does not present large problems.

Comment 13

[Johnny Stenback (:jst)]

PDT: IMO choosing to not fix this bug right now is a bad move that we have the
opportunity to prevent. This bug might not immediately break things or seem bad
enough to be accepted for rtm, but if we don't fix this now we'll allow people
to write scrips that appear to be working in Netscape 6.0 but once we fix this
bug later on, we'll break those scripts. This bug also makes mozilla break on
perfectly valid pages in some cases. The script writers will not know that
they're writing a bad script untill they try the script in a browser without
this bug.

If the fear is that this fix will break something, let the fix go onto the trunk
and let QA and the mozilla community beat on this for a while before we land it
on the branch.

(removing [rtm-] and putting [rtm+] back)

Comment 14

[Johnny Stenback (:jst)]

r=jst for the patch, btw.

Comment 15

[buster]

a=buster.  pdt should approve this one, important standards fix.

Comment 16

[Phil Peterson]

rtm++

Comment 17

[rods (gone)]

fixed

Comment 18

[bsharma]

Build: 2000-10-08-09 MN6
Platform: Win98

Comment 19

[Jesse Ruderman]

bug 55244 for the regression that i think was caused by this patch