208744 - [FIX]Any attribute 'id' in an XHTML document acts as an id

Status: RESOLVED FIXED

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

Description

[Hixie (not reading bugmail)]

I was stepping through Gecko in the debugger when I came across:

  NS_IMETHODIMP
  nsHTMLDocument::AttributeChanged(nsIContent* aContent, PRInt32 aNameSpaceID,
                                   nsIAtom* aAttribute, PRInt32 aModType,
                                   nsChangeHint aHint)
  {
    ...
    } else if (aAttribute == nsHTMLAtoms::id &&
               aNameSpaceID == kNameSpaceID_None) {
        ...
        nsresult rv = AddToIdTable(value, aContent);

This is rather wrong, as shown by:

   http://www.hixie.ch/tests/adhoc/xml/dynamic/001.xhtml

Namely, an element in the "" namespace is added an attribute "id" and somehow
getElementById() begins to find it. (CSS selectors, however, correctly do not.)

What is an ID attribute should be much more carefully examined for non-HTML
documents. I would hope that in fact we could use the same code for both XHTML
and generic XML documents.

Comment 1

[Boris Zbarsky [:bzbarsky]]

See nsINodeInfo::GetIDAttributeAtom.

Personally, I would prefer to hoist up GetID from nsIXMLContent to nsIContent
(or something similar to that) and create a GetIDAttributeAtom or something like
that to parallel it.... (which would internally just calle GetIDAttributeAtom
for generic XML and would return an "id" atom for classes that know their ID --
nsXULElement, nsSVGElement, nsGenericHTMLElement, etc).

For that matter, if we do that we could maybe combine all the GetElementById
code from nsHTMLDocument and nsXMLDocument into nsDocument (using proper ID
checking per the XML code and the hashtable per the HTML code).  See the XXX
perf comment on GetElementById in nsXMLDocument.

Comment 2

[Alex Vincent [:WeirdAl]]

I would think this a good idea to fix if we implement Element.setIdAttribute(),
Element.setIdAttributeNS(), and Element.setIdAttributeNode() first.  (Reference
DOM Core 3 Last Call WD for details.)

Personally, I thought Hixie's report was correct behavior, as the DOM developer
otherwise has no way of setting an ID-type attribute before DOM 3 Core is
implemented.

Comment 3

[Hixie (not reading bugmail)]

Sure you can. Just use the same attribute as you would have using raw XML, e.g.
the ID attribute on XHTML elements, or the id attribute set in the internal DTD
subset, or whatever.

Comment 4

[Alex Vincent [:WeirdAl]]

Ah, and your testcase has *no* DTD...

Comment 5

[Hixie (not reading bugmail)]

Indeed. (It doesn't need one, it's XML, the semantics are given by the namespace.)

Comment 6

[Boris Zbarsky [:bzbarsky]]

Attachment: Patch

Comment 7

[Boris Zbarsky [:bzbarsky]]

Taking.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 133396 [details] [diff] [review]
Patch

caillon, I hate to do this to you, but I can't think of anyone except you,
peterv, jst, sicking, and maybe dbaron to review this... and jst/sicking are
pretty laggy recently.	:(

Comment 9

[Boris Zbarsky [:bzbarsky]]

Attachment: Forgot a few files...

Comment 10

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

Comment on attachment 133398 [details] [diff] [review]
Forgot a few files...

+nsIAtom*
+nsGenericHTMLElement::GetIDAttributeName() const

shouldn't the nsIAtom* be
NS_IMETHODIMP_(nsIAtom*)?

Comment 11

[Boris Zbarsky [:bzbarsky]]

Tough to tell.. the NS_IMETHODs in the header become "nsresult" in the cpp in
this file.  I'm not sure whether this is done for a reason, or just because.

Comment 12

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

hm, but your patch does show
 NS_IMETHODIMP_(PRBool)
 nsGenericHTMLElement::HasClass(nsIAtom* aClass, PRBool aCaseSensitive) const
in that file

Comment 13

[Boris Zbarsky [:bzbarsky]]

Yeah, this file has never made sense to me that way.  I'm perfectly happy to do
NS_IMETHODIMP_(nsIAtom*), of course.

Comment 14

[Christopher Aillon (sabbatical, not receiving bugmail)]

Comment on attachment 133398 [details] [diff] [review]
Forgot a few files...

>Index: content/html/content/src/nsGenericHTMLElement.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/content/html/content/src/nsGenericHTMLElement.cpp,v
>retrieving revision 1.443
>diff -p -u -1 -0 -r1.443 nsGenericHTMLElement.cpp
>--- content/html/content/src/nsGenericHTMLElement.cpp	2 Oct 2003 21:29:32 -0000	1.443
>+++ content/html/content/src/nsGenericHTMLElement.cpp	16 Oct 2003 08:57:03 -0000
>@@ -2271,20 +2271,32 @@ nsGenericHTMLElement::GetID(nsIAtom** aR
> 
> nsresult
> nsGenericHTMLElement::GetClasses(nsVoidArray& aArray) const
> {
>   if (nsnull != mAttributes) {
>     return mAttributes->GetClasses(aArray);
>   }
>   return NS_OK;
> }
> 
>+nsIAtom*
>+nsGenericHTMLElement::GetIDAttributeName() const


NS_IMETHODIMP_(nsIAtom*)


>+{
>+  return nsHTMLAtoms::id;
>+}
>+
>+nsIAtom*
>+nsGenericHTMLElement::GetClassAttributeName() const


Ditto.


>+{
>+  return nsHTMLAtoms::kClass;
>+}
>+
> NS_IMETHODIMP_(PRBool)
> nsGenericHTMLElement::HasClass(nsIAtom* aClass, PRBool aCaseSensitive) const
> {
>   if (mAttributes) {
>     return mAttributes->HasClass(aClass, aCaseSensitive);
>   }
>   return PR_FALSE;
> }
> 
> nsresult


>Index: content/html/document/src/nsHTMLDocument.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/content/html/document/src/nsHTMLDocument.cpp,v
>retrieving revision 3.509
>diff -p -u -1 -0 -r3.509 nsHTMLDocument.cpp
>--- content/html/document/src/nsHTMLDocument.cpp	7 Oct 2003 23:21:50 -0000	3.509
>+++ content/html/document/src/nsHTMLDocument.cpp	16 Oct 2003 08:57:12 -0000
>@@ -1443,21 +1443,21 @@ nsHTMLDocument::AttributeWillChange(nsIC
> 
>     aContent->GetTag(getter_AddRefs(tag));
> 
>     if (IsNamedItem(aContent, tag, value)) {
>       nsresult rv = RemoveFromNameTable(value, aContent);
> 
>       if (NS_FAILED(rv)) {
>         return rv;
>       }
>     }
>-  } else if (aAttribute == nsHTMLAtoms::id &&
>+  } else if (aAttribute == aContent->GetIDAttributeName() &&
>              aNameSpaceID == kNameSpaceID_None) {


There probably should be an assert for aAttribute not being null somewhere.


>     nsresult rv = RemoveFromIdTable(aContent);
> 
>     if (NS_FAILED(rv)) {
>       return rv;
>     }
>   }
> 
>   return nsDocument::AttributeWillChange(aContent, aNameSpaceID, aAttribute);
> }
>@@ -1475,25 +1475,27 @@ nsHTMLDocument::AttributeChanged(nsICont
> 
>     aContent->GetTag(getter_AddRefs(tag));
> 
>     if (IsNamedItem(aContent, tag, value)) {
>       nsresult rv = UpdateNameTableEntry(value, aContent);
> 
>       if (NS_FAILED(rv)) {
>         return rv;
>       }
>     }
>-  } else if (aAttribute == nsHTMLAtoms::id &&
>+  } else if (aAttribute == aContent->GetIDAttributeName() &&
>              aNameSpaceID == kNameSpaceID_None) {


Same here about the assert, though it probably doesn't matter since you likely
will hit asserts in GetAttr().


>     nsAutoString value;
> 
>-    aContent->GetAttr(aNameSpaceID, nsHTMLAtoms::id, value);
>+    aContent->GetAttr(aNameSpaceID,
>+                      aContent->GetIDAttributeName(),
>+                      value);
> 
>     if (!value.IsEmpty()) {
>       nsresult rv = AddToIdTable(value, aContent);
> 
>       if (NS_FAILED(rv)) {
>         return rv;
>       }
>     }
>   }
> 

>Index: content/html/style/src/nsCSSStyleSheet.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/content/html/style/src/nsCSSStyleSheet.cpp,v
>retrieving revision 3.281
>diff -p -u -1 -0 -r3.281 nsCSSStyleSheet.cpp
>--- content/html/style/src/nsCSSStyleSheet.cpp	14 Oct 2003 21:06:58 -0000	3.281
>+++ content/html/style/src/nsCSSStyleSheet.cpp	16 Oct 2003 08:57:20 -0000
>@@ -3795,21 +3795,22 @@ static PRBool SelectorMatches(RuleProces
>       } while (attr && result);
>     }
>   }
>   if (result && (aSelector->mIDList || aSelector->mClassList)) {
>     // test for ID & class match
>     result = localFalse;
>     if (data.mStyledContent) {
>       // case sensitivity: bug 93371
>       PRBool isCaseSensitive = data.mCompatMode != eCompatibility_NavQuirks;
>       nsAtomList* IDList = aSelector->mIDList;
>-      if (nsnull == IDList || aAttribute == nsHTMLAtoms::id) {
>+      if (nsnull == IDList ||
>+          aAttribute == data.mContent->GetIDAttributeName()) {

This needs to check that aAttribute is not null.

>         result = PR_TRUE;
>       }
>       else if (nsnull != data.mContentID) {
>         result = PR_TRUE;
>         if (isCaseSensitive) {
>           do {
>             if (localTrue == (IDList->mAtom != data.mContentID)) {
>               result = PR_FALSE;
>               break;
>             }
>@@ -3826,21 +3827,21 @@ static PRBool SelectorMatches(RuleProces
>             if (localTrue !=
>                 id1.Equals(id2, nsCaseInsensitiveCStringComparator())) {
>               result = PR_FALSE;
>               break;
>             }
>             IDList = IDList->mNext;
>           } while (IDList);
>         }
>       }
>       
>-      if (result && aAttribute != nsHTMLAtoms::kClass) {
>+      if (result && aAttribute != data.mContent->GetClassAttributeName()) {


Same as above.


>         nsAtomList* classList = aSelector->mClassList;
>         while (nsnull != classList) {
>           if (localTrue == (!data.mStyledContent->HasClass(classList->mAtom, isCaseSensitive))) {
>             result = PR_FALSE;
>             break;
>           }
>           classList = classList->mNext;
>         }
>       }
>     }



r=caillon with comments dealt with, yadda yadda.  ;-)

Comment 15

[Boris Zbarsky [:bzbarsky]]

Attachment: updated to caillon's comments

Comment 16

[Peter Van der Beken [:peterv]]

Comment on attachment 133558 [details] [diff] [review]
updated to caillon's comments

So do you want to do the GetID change too? And is the comment at
http://lxr.mozilla.org/seamonkey/source/content/base/public/nsIStyledContent.h#
59 still correct?

Comment 17

[Boris Zbarsky [:bzbarsky]]

I was going to do GetID as a part of a separate deCOM patch at some point...
this patch had gotten big enough.

And yes, that comment is still correct for now.

Checked in.

Comment 18

[Malcolm Rowe]

Not sure how relevant this is, but.. the ID-ness of attributes in XML (in the 
absence of a DTD) is part of an open W3C TAG issue, xmlIDSemantics-32.

see http://www.w3.org/2001/tag/ilist#xmlIDSemantics-32
and http://www.w3.org/2001/tag/doc/xmlIDsemantics-32.html

Comment 19

[Boris Zbarsky [:bzbarsky]]

Right.  When they decide on something, we'll look into implementing it...

Far more worrisome is the suggestion in DOM3 Core that elements of the same type
can have different ID attributes.. ;)