Closed Bug 54136 Opened 24 years ago Closed 24 years ago

name, id share namespace for checkboxes with label for=

Categories

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

defect

Tracking

()

VERIFIED FIXED

People

(Reporter: jruderman, Assigned: rods)

Details

(Keywords: html4, relnote, Whiteboard: [rtm++]Fix in hand)

Attachments

(3 files)

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.
Attached file test case
My interpretation of the spec and the comments in this bug, is the label should 
ONLY look for "id" and not "name"
Status: NEW → ASSIGNED
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
Summary: name, id share namespace for checkboxes with label for= → [KNOW FIX]name, id share namespace for checkboxes with label for=
Target Milestone: --- → Future
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?
Keywords: correctness
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)
Keywords: html4, relnote
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(). 
Keywords: rtm
Summary: [KNOW FIX]name, id share namespace for checkboxes with label for= → [FIX]name, id share namespace for checkboxes with label for=
Whiteboard: Fix in hand
Attached patch official patchSplinter Review
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.
Whiteboard: Fix in hand → [rtm+]Fix in hand
Target Milestone: Future → M19
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."


Priority: P3 → P2
Marking [rtm need info] Waiting for review + super-review.
Whiteboard: [rtm+]Fix in hand → [rtm need info]Fix in hand
Marking rtm-.  This does not present large problems.
Whiteboard: [rtm need info]Fix in hand → [rtm-]Fix in hand
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)
OS: Windows 98 → All
Hardware: PC → All
Whiteboard: [rtm-]Fix in hand → [rtm+]Fix in hand
r=jst for the patch, btw.
a=buster.  pdt should approve this one, important standards fix.
rtm++
Whiteboard: [rtm+]Fix in hand → [rtm++]Fix in hand
fixed
Status: ASSIGNED → RESOLVED
Closed: 24 years ago
Resolution: --- → FIXED
Summary: [FIX]name, id share namespace for checkboxes with label for= → name, id share namespace for checkboxes with label for=
Build: 2000-10-08-09 MN6
Platform: Win98
Status: RESOLVED → VERIFIED
bug 55244 for the regression that i think was caused by this patch
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: