Closed Bug 324482 Opened 19 years ago Closed 19 years ago

DOM allows construction of document with duplicate ids, different browsers behave differently

Categories

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

Product:
Core
Component:
DOM: Core & HTML
Version:
1.8 Branch
Platform:
PowerPC
macOS
Type:
defect
Priority:
Not set
Severity:
major

Tracking

()

Status:
RESOLVED INVALID

People

(Reporter: steffan, Unassigned)

References

(
URL
)

Details

User-Agent:       Mozilla/5.0 (Macintosh; U; PPC Mac OS X; en) AppleWebKit/417.9 (KHTML, like Gecko) Safari/417.8
Build Identifier: Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.8) Gecko/20051111 Firefox/1.5

On the link above when you dynamically create/clone elements and then alter the name I ran into an issue where you'll see in the source that I create a variable of type integer and set it to 0. In the body of the code I do a counter++ and instead of it being 2,3,4,5 etc it appends the numbers as if they were strings. This bug did not occur in older versions and do not happen in Safari on the Mac. Create several dynamic elements then submit the form. On the response page, lasso (like php) will show you what exactly FF submitted. For example you should see layer_name_0 then layer_name_1 then layer_name_2 but FF makes them layer_name_0 layer_name_01 rather than layer_name_0,layer_name_1,layer_name_2. The JS has been examined and found to be syntactically correct. So, it seems to be an error in FF. I look forward to hearing back on this.

Reproducible: Always

Steps to Reproduce:
1. Go to page.
2. hit the add field button several times and submit
3. check form results to see as mentioned in details.

Actual Results:  
layer_name_0 layer_name_01

Expected Results:  
layer_name_0,layer_name_1,layer_name_2....
The page seems to work ok in Firefox trunk nightly
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20060120 Firefox/1.6a1
Assignee: nobody → general
Severity: critical → major
Component: JavaScript Console → JavaScript Engine
Product: Firefox → Core
QA Contact: javascript.console → general
Version: unspecified → 1.8 Branch
This looks invalid, and the summary is misleading: the script uses +, not ++, and it does not use either operateor on an "integer-type" value:

    function moreFields(loc)
    {
        var newFields = document.getElementById(loc + 'readroot').cloneNode(true);
        newFields.style.display = 'block';
        var newField = newFields.childNodes;
        for (var i=0;i<newField.length;i++)
        {
            var theName = newField[i].name
            if (theName)
                newField[i].name = (theName + counter);


Per the DOM spec, the name property of a text input element has value of type DOMString, which maps to a JS string (see http://www.w3.org/TR/2003/REC-DOM-Level-2-HTML-20030109/html.html#ID-6043025 at least; I'll let others cite the ECMAScript binding that maps DOMString to string).

Per ECMA-262 Edition 3, 11.6.1 step 7, string + anything => string (spec mirrored at http://www.mozilla.org/js/language/E262-3.pdf).

However, I did try Konqueror and see it behaving as the reporter expects.

Oh, I see the problem: our DOM clones nodes but somehow fails to clone their name properties, wrongly setting and getting the clone-parent's original name.  This at least accounts for what I see.  Known bug?

/be
Assignee: general → general
Status: UNCONFIRMED → NEW
Component: JavaScript Engine → DOM
Ever confirmed: true
QA Contact: general → ian
Summary: javascript is treating integers as strings. integer++ = integer.toString() + integer.toString() → DOM cloneNode on parent of text inputs accesses clone-parent's children
Whiteboard: DUPEME
(In reply to comment #2)
> Oh, I see the problem: our DOM clones nodes but somehow fails to clone their
> name properties, wrongly setting and getting the clone-parent's original name.

I meant to write "our DOM clones nodes but somehow fails to clone their children, or their children's name properties" -- or something like that ;-).  This rings a bell, but someone else can find the dup faster than I can.

/be
I'm really not sure what to make of comment 3 and the end of comment 2....

What's ACTUALLY going on here is that the script creates an invalid document -- multiple nodes all have the ID "composereadroot".  So the only question is which of those nodes document.getElementById('composereadroot') returns.  In FF 1.5 it's the node with that ID which was most recently added to the document, which leads to the observed behavior on this testcase when one thinks about it (each time you're cloning the set of nodes that you created the previous time).  In current trunk builds the returned node in this particular testcase will be the one that was first added to the document... more or less.  I have no idea what Safari does; per DOM spec returning a different node every single time the function is called even if the DOM hasn't change would be perfectly fine behavior.

Bug 315770 is what changed the behavior on trunk, in case anyone cares.

In any case, marking invalid; no DOM bug here.
Status: NEW → RESOLVED
Closed: 19 years ago
Resolution: --- → INVALID
Note that I have no idea how to usefully resummarize this bug, so I'm not bothering.  The current summary doesn't make any sense to me...
bz: sorry, tired -- thanks for diagnosing.  Is this a case where we'll find out every other browser including IE does what Firefox 1.0.x did, and 1.5 "broke" a de-facto standard?  I'm not suggesting we cave to standards for invalid documents just yet.  But we've all been burned by real-world compatibility constraints that no spec codifies.

/be
Summary: DOM cloneNode on parent of text inputs accesses clone-parent's children → DOM allows construction of document with duplicate ids, different browsers behave differently
The bad thing here is that cloning a subtree also clones all ids in that subtree. I bet that in most cases authors don't think about this fact. Unfortunatly I don't think there is much we can do because the spec is quite clear on that all attributes should be cloned.

It could be used as an argument to using the 'first inserted' node when there are multiple nodes with the same id though, which is probably what the other UAs are doing.

So I wouldn't be entierly opposed the idea to tweak the getElementById implementation with this usecase in mind.

The counter argument is that using the first node in the tree with a given id gives a more more predictable behaviour.
> Is this a case where we'll find out every other browser including IE does what
> Firefox 1.0.x did,

First of all, Firefox 1.0.x behaves identically to Firefox 1.5 on this testcase, as about 3 minutes of testing shows (which makes sense, since this code did not change in the Gecko 1.8 timeframe).  So I'm not sure where the "This bug did not occur in older versions" part of comment 0 comes from either; I suspect that the code tested on said "older versions" was different.

With that out of the way, the behavior of FF 1.5 (and 1.0.x) is so complicated in this area (it's just in this one specific use case that "last inserted" is used), that I strongly doubt there is interop with other browsers anyway.  For that matter, the behavior of current trunk is fairly complicated too.  There is simply no particular attemt made to return a consistent answer in the multiple-id situation.  Any such attempt would slow down things prohibitively, in my opinion.

If someone (Ian?) wants to test what different browsers do, that's fine.  I can write up documentation on what various Gecko versions do so they have an idea of the sort of insanity to look for (insanity from a user's perspective, of course).


According to bzbarsky@mit.edu this has been changed to invalid. I disagree. Yes, the fact that when the div is cloned that it also cloned the id. I added newFields.id="" to resolve the problem. However this is merely a bandaid because the handle newFields points to the CLONED div and not the original. When the code hits the section 
var newField = newFields.childNodes;
			for (var i=0;i<newField.length;i++)
			{
				var theName = newField[i].name
				if (theName)
					newField[i].name = (theName + counter);
			}
it is modifying them all and NOT solely the cloned node as it should. Now, I see where it can actually have an issue where regerencing the master node var newFields = document.getElementById(loc + 'readroot').cloneNode(true); could cause an issue because there are multiple. Here we have a choice. This is it. Now, when you reference a form element that is a single element you get the element but if you reference a element where there are multiple with the same name they are an array and you get an error saying that the object does not exist. Choice is either support it like other browsers do which is to reference the object 0 or throw an error that the object does not exist (like in a form). This is more in line with the way its handled in other situations. Is this confusing?
Status: RESOLVED → REOPENED
Resolution: INVALID → ---
(In reply to comment #4)
> I'm really not sure what to make of comment 3 and the end of comment 2....
> 
> What's ACTUALLY going on here is that the script creates an invalid document --
> multiple nodes all have the ID "composereadroot".  So the only question is
> which of those nodes document.getElementById('composereadroot') returns.  In FF
> 1.5 it's the node with that ID which was most recently added to the document,
> which leads to the observed behavior on this testcase when one thinks about it
> (each time you're cloning the set of nodes that you created the previous time).
>  In current trunk builds the returned node in this particular testcase will be
> the one that was first added to the document... more or less.  I have no idea
> what Safari does; per DOM spec returning a different node every single time the
> function is called even if the DOM hasn't change would be perfectly fine
> behavior.
> 
> Bug 315770 is what changed the behavior on trunk, in case anyone cares.
> 
> In any case, marking invalid; no DOM bug here.
> 

I am replying to this one after the main reply at the end of the thread but I can tell you what safari and IE are doing. they only reference the 0 element and not the last. that is why it works every time. the 0 element has no number after it in my script and that is why it works. So, is it wrong or right. Yes, I did correct the code to get around it but the fact still remains the same that FF is not inline with others. Good or bad? I dont know but I suggest modifying the behavior to work as I suggested where references as a single object rather than array when they are in fact an array throws an error. If FF did this here it would be have blatantly obvious as the issue. Referencing an array as a single object. Boom. Flaw caught and fixed. What would the suggested fix be?
> because the handle newFields points to the CLONED div and not the original.

That's not relevant, actually.

> Choice is either support it like other browsers do which is to reference the
> object 0

Object 0 where?  getElementById does NOT return an array.

> but the fact still remains the same that FF is not inline with others.

As I said, no two browsers are likely to be "inline" with each other wrt to this behavior.  Depending on any particular behavior in this situation will just get you into a world of trouble.

> but I suggest modifying the behavior to work as I suggested where references
> as a single object rather than array when they are in fact an array throws an
> error.

There is no array.  Throwing an error would just violate the DOM spec.

> What would the suggested fix be?

Not creating documents with duplicated ID attributes if you want behavior to be predictable.

This is still invalid.  I am NOT willing to change this code on the branch in any case, especially given that it's been this way for years.  On trunk we changed it only because of other simplifications in the surrounding code.
Status: REOPENED → RESOLVED
Closed: 19 years ago → 19 years ago
Resolution: --- → INVALID
(In reply to comment #11)
> > because the handle newFields points to the CLONED div and not the original.
> 
> That's not relevant, actually.
> 
How is it not relevant? Shouldnt we be modifying only that which we have a handle on?
> > Choice is either support it like other browsers do which is to reference the
> > object 0
> 
> Object 0 where?  getElementById does NOT return an array.
If you try to get getElementById on several items of the same name then getElementById returns an array. Maybe I am having a brain ****. Sorry.
> > but the fact still remains the same that FF is not inline with others.
Well, other than FF I think the 2 biggest are Safari and IE. It could be :)
> As I said, no two browsers are likely to be "inline" with each other wrt to
> this behavior.  Depending on any particular behavior in this situation will
> just get you into a world of trouble.
> 
> > but I suggest modifying the behavior to work as I suggested where references
> > as a single object rather than array when they are in fact an array throws an
> > error.
> 
> There is no array.  Throwing an error would just violate the DOM spec.
Are not multile items of the same name considered to be an array? So, if every time I cloned an object and have the same id this does not fall into the same issue?
> > What would the suggested fix be?
> 
> Not creating documents with duplicated ID attributes if you want behavior to be
> predictable.
Point taken. 
> This is still invalid.  I am NOT willing to change this code on the branch in
> any case, especially given that it's been this way for years.  On trunk we
> changed it only because of other simplifications in the surrounding code.
I will make some examples of what I mean by referencing arrays as a single object and throwing errors and post it back here later.


> How is it not relevant? Shouldnt we be modifying only that which we have a
> handle on?

Yes.  And you only modify the kids of the newFields node.  The problem is that every time you create newFields you clone the last newFields you looked at.  So if you start out with "x" as the name, the first time through the loop you end up with "x0".  The second time the "Add Field" button is clicked you clone the thing that has "x0" as the name and append a 1, so the name is now "x01".  And so forth.

> If you try to get getElementById on several items of the same name then
> getElementById returns an array. Maybe I am having a brain fart. Sorry.

You're having a brain fart, as far as I can tell. Please see the DOM spec on getElementById.  Not only does it not return an array, but it doesn't look at the _name_ of items.  It looks at the id.

> Are not multile items of the same name considered to be an array?

There are no multiple items of the same name here, so what happens for multiple items of the same name is irrelevant.

And in general, multiple items of the same ID are NOT considered an array.

> I will make some examples of what I mean by referencing arrays as a single
> object and throwing errors and post it back here later.

Please make sure to not confuse DOM 0 methods that treat ID and name as the same thing and sometimes return arrays with the DOM Core method getElementById, which treat the attributes differently (except in IE, of course) and return a single element.  Keep those differences in mind as you make said examples.  ;)
IE supports a variant of .getElementById such that if there are multiple items wiht the same ID it returns an array of these items. Mozilla does not and will not support this variant.
(In reply to comment #14)
> IE supports a variant of .getElementById such that if there are multiple items
> wiht the same ID it returns an array of these items. Mozilla does not and will
> not support this variant.
> 
Safari does as well. I'll test in other browsers just for fun and report back. Please don't get me wrong. I am in no way putting down FF. I love it over the other browsers because of its handling of css and js better than others as well as cool plugins like the web developer etc. I just want to help assure that if something is developed in a whacked out browser that FF will be lenient so ignorant people don't bash FF.

IE also returns items with name="x" for getElementById("x").  I think looking at IE here is just a lost cause.

It sounds to me like the behavior of Safari is a serious bug; someone should file a bug on them.  That said, given the code in this testcase, if that's what Safari does it should be throwing exceptions, not working as comment 0 claimed.
(In reply to comment #13)
> > How is it not relevant? Shouldnt we be modifying only that which we have a
> > handle on?
> 
> Yes.  And you only modify the kids of the newFields node.  The problem is that
> every time you create newFields you clone the last newFields you looked at.  So
> if you start out with "x" as the name, the first time through the loop you end
> up with "x0".  The second time the "Add Field" button is clicked you clone the
> thing that has "x0" as the name and append a 1, so the name is now "x01".  And
> so forth.
This makes sense. I hope you can see how at first glance it looked like it was not adding the counter as a string rather than a integer.
> > If you try to get getElementById on several items of the same name then
> > getElementById returns an array. Maybe I am having a brain ****. Sorry.
> 
> You're having a brain ****, as far as I can tell. Please see the DOM spec on
> getElementById.  Not only does it not return an array, but it doesn't look at
> the _name_ of items.  It looks at the id.
As Ben said, IE does as well as Safari. As far sa ID versus NAME I goofed in my typing. I apologize. I did in fact mean ID. I do know there is a difference.
> > Are not multile items of the same name considered to be an array?
> 
> There are no multiple items of the same name here, so what happens for multiple
> items of the same name is irrelevant.
Isnt this what we were saying earlier that when I was cloning the node repeatedly I was creating multiple nodes with the same ID of composereadroot ?
> And in general, multiple items of the same ID are NOT considered an array.
> 
> > I will make some examples of what I mean by referencing arrays as a single
> > object and throwing errors and post it back here later.
> 
> Please make sure to not confuse DOM 0 methods that treat ID and name as the
> same thing and sometimes return arrays with the DOM Core method getElementById,
> which treat the attributes differently (except in IE, of course) and return a
> single element.  Keep those differences in mind as you make said examples.  ;)
Again, I didnt have the ID and NAME confused. It was my fault in not using the correct terminology. I may however be thinking of name vs id in the form elements as we were thinking. Its not so far off though if you consider that as Ben backed me up on that others are treating getElementById as an array if multiples exist. This may be why the code originally worked in IE and Safari. Either it only referenced the first/original/id[0].


(In reply to comment #17)

> Ben backed me up on that others are treating getElementById as an array if
> multiples exist.

I don't know which version of IE Ben was talking about.  IE on XP SP 2 (and all other browsers I tested) returns a single element.  If there are multiple elements with the same ID, getElementById returns the first one. 


> This may be why the code originally worked in IE and Safari.
> Either it only referenced the first/original/id[0].

It 'worked' because in most browsers getElementById will return the first match if there are multiple elements with the same ID.

Firefox does the same, except that in this particular case, getElementById is returning a reference to the element that was cloned rather than the original.
If all other browsers really return the first element when there are multiple with the same ID then I think we should really try to do that.

Could we once we've gotten a successful getElementById call make search all newly inserted content into the hash to make sure we have the first node in there? Or would that be too slow on DOM mutations?
> If all other browsers really return the first element when there are multiple
> with the same ID then I think we should really try to do that.

If they do.  Someone would need to write some testcases to check for this.

> Could we once we've gotten a successful getElementById call make search all
> newly inserted content into the hash to make sure we have the first node in
> there? Or would that be too slow on DOM mutations?

That would be too slow, I suspect.... Feel free to prove me wrong, though.  ;)
Whiteboard: DUPEME
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.