468708 - namespaceURI for HTML elements should be http://www.w3.org/1999/xhtml; make localName return lower case

Status: RESOLVED FIXED

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

Description

[Henri Sivonen (:hsivonen)]

Build ID:
Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2a1pre)
Gecko/20081209 Minefield/3.2a1pre

Steps to reproduce:
 1) Load http://hsivonen.iki.fi/test/moz/namespaceURI.html

Expected results:
Expected alert "http://www.w3.org/1999/xhtml".

Actual results:
Alerts "null".

Additional info:
Safari 3.2.1 alerts "http://www.w3.org/1999/xhtml". HTML5 requires exposing the NS URI http://www.w3.org/1999/xhtml.

http://www.whatwg.org/specs/web-apps/current-work/#creating-and-inserting-elements
http://www.whatwg.org/specs/web-apps/current-work/#html-namespace-0
http://www.whatwg.org/specs/web-apps/current-work/#apis-in-html-documents

Comment 1

[Olli Pettay [:smaug][[e-mail]]]

Does html 5 specify what should happen if .createElement("html") is called?
Because per DOM 3 Core that creates an element which has null as its
.namespaceURI

Comment 2

[Henri Sivonen (:hsivonen)]

It's defined under 
http://www.whatwg.org/specs/web-apps/current-work/#apis-in-html-documents

As far as HTML5 goes, DOM 3 Core is viewed as a Java server side spec and 
http://simon.html5.org/specs/web-dom-core is expected to become the spec for DOM Core for JS in browsers.

Comment 4

[Henri Sivonen (:hsivonen)]

Taking in order to get fix this in the HTML5 parsing repo.

Comment 5

[Henri Sivonen (:hsivonen)]

Attachment: Patch for implementing HTML5 "APIs in HTML documents" section

Fix for bug 468692 in the same patch.

Comment 6

[Henri Sivonen (:hsivonen)]

The patch implements the "APIs in HTML documents" section of HTML5: 
http://www.whatwg.org/specs/web-apps/current-work/multipage/dom.html#apis-in-html-documents

The exception is that the patch doesn't address attribute nodes; see
http://www.w3.org/Bugs/Public/show_bug.cgi?id=6689

I think we should land this on the trunk sooner than later and well ahead of the HTML5 parser to isolate this DOM change from the new parsing algorithm.

I believe this change doesn't break compat with existing content, because Safari has been shipping with this behavior for some time already. I think the more significant risk is a namespace comparison mismatch somewhere is Gecko's internals that I missed.

What unit tests should I run or what other testing procedures should I perform before requesting review?

Try server builds:
https://build.mozilla.org/tryserver-builds/2009-03-13_03:[e-mail]-try-fc6790d487b/

Comment 7

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Set review flags then :)

I do agree that we should land this as soon as possible.

It does seem a bit weird though that an elements case sensitivity doesn't depend on intrinsic state, but rather on which document it is in. Since the latter can change.

However I still think we should take this patch for now. We can always change it later if the spec changes. The difference is only in extreme edge cases (moving nodes between XML and HTML documents).

Comment 8

[Henri Sivonen (:hsivonen)]

Comment on attachment 367197 [details] [diff] [review]
Patch for implementing HTML5 "APIs in HTML documents" section

Oops. It turns out there are tons of unit tests that depend on these details. Lots of more work ahead.

Comment 9

[Henri Sivonen (:hsivonen)]

This is going to need cleanup after bug 335998 lands.

Comment 10

[Henri Sivonen (:hsivonen)]

Attachment: A more correct patch

Reftests handled, mochitests still to be dealt with...

Comment 11

[Henri Sivonen (:hsivonen)]

Attachment: Hopefully the right patch if tryserver goes green...

So few changes, so many interactions... There's hope that this patch might be it.

Comment 12

[Henri Sivonen (:hsivonen)]

Attachment: Finally green for tryserver unit tests

Whew. Finally green.

I also started pursuing spec changes for the XPath and XSLT stuff.

Filed bug 487023 for follow-up clean-up.

Comment 13

[Boris Zbarsky [:bzbarsky]]

Some drive-by comments:

1)  Do we really want to use the id of an <iframe> as the window name in HTML if
    the <iframe>'s name is not set?  If so, do we really want to make that
    change as part of this bug?  A followup would make more sense.
2)  Does nsGenericHTMLElement still need a GetLocalName impl?
3)  Does nsGenericHTMLElement still need a GetElementsByTagNameNS impl?
4)  Why do you null-check GetOwnerDoc() in GetBaseURI() (with a comment about
    bug 335998, which seems correct), but not null-check it in some other
    places?
5)  Does nsHTMLDocument still need a GetElementsByTagNameNS impl?
6)  Does nsHTMLDocument still need a CreateElem impl?
6)  In the CSS rule processor, can we just set mIsHTMLContent based on the
    namespace instead of doing the extra virtual function call to IsNodeOfType?

Comment 14

[Boris Zbarsky [:bzbarsky]]

Also, could you file a followup bug to go through and replace some of our IsNodeOfType(eHTML) checks (possibly all of them!) with the cheaper (I think... non-virtual, but more indirection) namespace checks?  Certainly code patterns where we check the tag against one thing and check for eHTML can just be a single nodeinfo check.

Comment 15

[Henri Sivonen (:hsivonen)]

Attachment: Patch addressing issues in comment #13

(In reply to comment #13)
> Some drive-by comments:
> 
> 1)  Do we really want to use the id of an <iframe> as the window name in HTML
> if
>     the <iframe>'s name is not set?  If so, do we really want to make that
>     change as part of this bug?  A followup would make more sense.

I filed bug 488247 as a follow-up. (I think it does make sense to unify HTML and XHTML behavior eventually.)

> 4)  Why do you null-check GetOwnerDoc() in GetBaseURI() (with a comment about
>     bug 335998, which seems correct), but not null-check it in some other
>     places?

I thought the ones I didn't null-check couldn't be reached unless the node was in the document, although I suppose I shouldn't rely on that if the document can indeed go away early. I added null checks.

> 5)  Does nsHTMLDocument still need a GetElementsByTagNameNS impl?

It is necessary to satisfy some linker dependency. The dependency doesn't appear to come from a .h file. I expect it comes from a .idl, but I don't see why the superclass doesn't satisfy the dependency.

> 2)  Does nsGenericHTMLElement still need a GetLocalName impl?
> 3)  Does nsGenericHTMLElement still need a GetElementsByTagNameNS impl?
> 6)  Does nsHTMLDocument still need a CreateElem impl?

No, these are unnecessary. I just thought the were similarly weird as nsHTMLDocument's GetElementsByTagNameNS.

> 6)  In the CSS rule processor, can we just set mIsHTMLContent based on the
>     namespace instead of doing the extra virtual function call to IsNodeOfType?

Fixed. Also simplified the namespace getter in the same source file to simply return the namespace without checking node HTMLness first.

I also filed bug 488249 as a follow-up about cleaning up HTMLness checks in general.

Comment 16

[Boris Zbarsky [:bzbarsky]]

> It is necessary to satisfy some linker dependency. 

Ah, because nsHTMLDocument has NS_DECL_NSIDOMDOCUMENT in the header, so it needs to implement it too.  Given how much of it gets forwarded to nsDocument, especialy with this patch, we should consider removing that and only declaring the few functions we really need to override (the ones dealing with XMLEncoding, etc).  That should be another followup bug.

For the CSSRuleProcessor, it might be good to just do:

  mIsHTMLContent = (mNameSpaceID == kNameSpaceID_XHTML);

In the HTML case it's one less branch; in the non-HTML case it's one less branch evaluation and an extra write; perf-wise I think it's a win.

And you can probably nuke CSSNameSpaceID altogether; that can be a followup bug too, to keep the size of this patch down.

Comment 17

[Henri Sivonen (:hsivonen)]

Attachment: Patch addressing issues in comment #16

(In reply to comment #16)
> > It is necessary to satisfy some linker dependency. 
> 
> Ah, because nsHTMLDocument has NS_DECL_NSIDOMDOCUMENT in the header, so it
> needs to implement it too.  Given how much of it gets forwarded to nsDocument,
> especialy with this patch, we should consider removing that and only declaring
> the few functions we really need to override (the ones dealing with
> XMLEncoding, etc).  That should be another followup bug.

Filed bug 488430.

> For the CSSRuleProcessor, it might be good to just do:
> 
>   mIsHTMLContent = (mNameSpaceID == kNameSpaceID_XHTML);
> 
> In the HTML case it's one less branch; in the non-HTML case it's one less
> branch evaluation and an extra write; perf-wise I think it's a win.

Fixed. (Changed r?, since you've already commented on the patch.)

> And you can probably nuke CSSNameSpaceID altogether; that can be a followup bug
> too, to keep the size of this patch down.

Filed bug 488431.

Comment 18

[Boris Zbarsky [:bzbarsky]]

jonas should still review this, especially since I have no idea whether the XSLT/XPath changes are right or not.  The above really was a drive-by, not a review...

Comment 19

[Henri Sivonen (:hsivonen)]

Comment on attachment 372815 [details] [diff] [review]
Patch addressing issues in comment #16

OK changed the review request back.

Comment 20

[Henri Sivonen (:hsivonen)]

Attachment: Patch taking into account the serializer landing

Comment 21

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Reviewed up to nsGenericHTMLElement.h

The only comment so far is this pattern:

GetOwnerDoc() // XXX clean up after bug 335998 lands
&& !(GetOwnerDoc()->IsCaseSensitive())

Please replace that with something like: IsInHTMLDoc() implemented on nsGenericElement, or maybe even nsIContent. Then change the implementation of that function once bug 335998 lands. That'll make both this and bug 335998 easier to review.

Comment 22

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 375724 [details] [diff] [review]
Patch taking into account the serializer landing

>diff --git a/content/html/content/src/nsHTMLStyleElement.cpp b/content/html/content/src/nsHTMLStyleElement.cpp
>@@ -314,17 +314,18 @@ void
> nsHTMLStyleElement::GetStyleSheetURL(PRBool* aIsInline,
>                                      nsIURI** aURI)
> {
>   *aURI = nsnull;
>   *aIsInline = !HasAttr(kNameSpaceID_None, nsGkAtoms::src);
>   if (*aIsInline) {
>     return;
>   }
>-  if (mNodeInfo->NamespaceEquals(kNameSpaceID_XHTML)) {
>+  if (GetOwnerDoc() && // XXX clean up after bug 335998 lands
>+      !(GetOwnerDoc()->IsCaseSensitive())) {

The '!' looks wrong.



>diff --git a/content/html/document/src/nsHTMLDocument.cpp b/content/html/document/src/nsHTMLDocument.cpp

> nsHTMLDocument::GetElementsByTagNameNS(const nsAString& aNamespaceURI,
>                                        const nsAString& aLocalName,
>                                        nsIDOMNodeList** aReturn)
> {
>-  nsAutoString tmp(aLocalName);
>-
>-  if (!IsXHTML()) {
>-    ToLowerCase(tmp); // HTML elements are lower case internally.
>-  }
>-
>-  return nsDocument::GetElementsByTagNameNS(aNamespaceURI, tmp, aReturn);
>+  return nsDocument::GetElementsByTagNameNS(aNamespaceURI, aLocalName, aReturn);
> }

Is this what the spec says to do?

>diff --git a/layout/style/nsCSSRuleProcessor.cpp b/layout/style/nsCSSRuleProcessor.cpp
> static inline PRInt32
> CSSNameSpaceID(nsIContent *aContent)
> {
>-  return aContent->IsNodeOfType(nsINode::eHTML)
>-           ? kNameSpaceID_XHTML
>-           : aContent->GetNameSpaceID();
>+  return aContent->GetNameSpaceID();
> }

Remove this function entirely.


r=me with that.

The changes look good, however I am worried that you might have missed some places that might need changing. What did you search for to find the current places that needed changing?

Comment 23

[Henri Sivonen (:hsivonen)]

Thank you. I'll update the patch to address your review comments when I return to office.

I looked at http://mxr.mozilla.org/mozilla-central/ident?i=kNameSpaceID_None and unit test results to locate the unobvious instances of namespace checks.

Comment 24

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Probably want to look for GetLocalName/.localName and kNameSpaceID_XHTML as well. Especially the former is likely to find you problematic code.

I'm finding a 'INPUT' and an 'IMG' on a quick scan through
http://mxr.mozilla.org/mozilla-central/search?string=.localName

http://mxr.mozilla.org/mozilla-central/ident?i=GetLocalName
will probably also turn up stuff too

Comment 25

[Henri Sivonen (:hsivonen)]

> GetOwnerDoc() // XXX clean up after bug 335998 lands
> && !(GetOwnerDoc()->IsCaseSensitive())
> 
> Please replace that with something like: IsInHTMLDoc() implemented on
> nsGenericElement, or maybe even nsIContent.

Replaced with IsInHTMLDocument() on nsIContent.

(In reply to comment #22)
> (From update of attachment 375724 [details] [diff] [review])
> >diff --git a/content/html/content/src/nsHTMLStyleElement.cpp b/content/html/content/src/nsHTMLStyleElement.cpp
> >@@ -314,17 +314,18 @@ void
> > nsHTMLStyleElement::GetStyleSheetURL(PRBool* aIsInline,
> >                                      nsIURI** aURI)
> > {
> >   *aURI = nsnull;
> >   *aIsInline = !HasAttr(kNameSpaceID_None, nsGkAtoms::src);
> >   if (*aIsInline) {
> >     return;
> >   }
> >-  if (mNodeInfo->NamespaceEquals(kNameSpaceID_XHTML)) {
> >+  if (GetOwnerDoc() && // XXX clean up after bug 335998 lands
> >+      !(GetOwnerDoc()->IsCaseSensitive())) {
> 
> The '!' looks wrong.

Fixed. (The whole method is bogus, though, but that's another bug.)

> >diff --git a/content/html/document/src/nsHTMLDocument.cpp b/content/html/document/src/nsHTMLDocument.cpp
> 
> > nsHTMLDocument::GetElementsByTagNameNS(const nsAString& aNamespaceURI,
> >                                        const nsAString& aLocalName,
> >                                        nsIDOMNodeList** aReturn)
> > {
> >-  nsAutoString tmp(aLocalName);
> >-
> >-  if (!IsXHTML()) {
> >-    ToLowerCase(tmp); // HTML elements are lower case internally.
> >-  }
> >-
> >-  return nsDocument::GetElementsByTagNameNS(aNamespaceURI, tmp, aReturn);
> >+  return nsDocument::GetElementsByTagNameNS(aNamespaceURI, aLocalName, aReturn);
> > }
> 
> Is this what the spec says to do?

Yes. The *NS methods don't tweak their arguments.

> >diff --git a/layout/style/nsCSSRuleProcessor.cpp b/layout/style/nsCSSRuleProcessor.cpp
> > static inline PRInt32
> > CSSNameSpaceID(nsIContent *aContent)
> > {
> >-  return aContent->IsNodeOfType(nsINode::eHTML)
> >-           ? kNameSpaceID_XHTML
> >-           : aContent->GetNameSpaceID();
> >+  return aContent->GetNameSpaceID();
> > }
> 
> Remove this function entirely.

Removed.

> r=me with that.

Thank you. (Should I set r+ myself on the fixed patch?)

(In reply to comment #24)
> Probably want to look for GetLocalName/.localName and kNameSpaceID_XHTML as
> well. Especially the former is likely to find you problematic code.

I checked these. Found one missed case which I fixed.

I don't see the point of both the node type and the namespace check in 
http://mxr.mozilla.org/mozilla-central/source/layout/base/nsCSSFrameConstructor.cpp#3699

> I'm finding a 'INPUT' and an 'IMG' on a quick scan through
> http://mxr.mozilla.org/mozilla-central/search?string=.localName

Fixed three instances.

It seems that the XHTMLness check in testing/mochitest/MochiKit/DOM.js is bogus, but it doesn't matter, because the only thing affected in DOM.js when _xhtml is true is the call to createElementNS(), which now works the same for HTML, too.(In reply to comment #21)

> http://mxr.mozilla.org/mozilla-central/ident?i=GetLocalName
> will probably also turn up stuff too

I didn't check every case, but the ones I checked seemed harmless.

Comment 26

[Henri Sivonen (:hsivonen)]

Attachment: Patch addressing review comments

Comment 27

[Boris Zbarsky [:bzbarsky]]

> I don't see the point of both the node type and the namespace check

That dates back to when you were allowed to use xbl:extends to create HTML-related frames for random content.  That would affect aNameSpaceID but not the IsNodeOfType check.

I think it's fine to remove one or the other; probably better to remove the virtual function call.

> It seems that the XHTMLness check in testing/mochitest/MochiKit/DOM.js is
> bogus

Well... there's no obviously good way of checking it, is the thing.  I'd be fine with moving that to nodeName or whatever gets uppercased in HTML documents.  I think you're right that just always doing the createElementNS call will be safe after this patch lands, though.

Comment 28

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Could you attach an interdiff patch for review?

Comment 29

[Henri Sivonen (:hsivonen)]

Attachment: Interdiff of the previous and current patch

Here's the interdiff. However, it seems that it some cases, interdiff goofed and shows the changes the wrong way even though the actual patch looks correct. (E.g. in GetEmbeds().)

Comment 30

[Henri Sivonen (:hsivonen)]

(In reply to comment #27)
> > I don't see the point of both the node type and the namespace check
> 
> That dates back to when you were allowed to use xbl:extends to create
> HTML-related frames for random content.  That would affect aNameSpaceID but not
> the IsNodeOfType check.
> 
> I think it's fine to remove one or the other; probably better to remove the
> virtual function call.

This change can be deferred until bug 488249, right?
 
> > It seems that the XHTMLness check in testing/mochitest/MochiKit/DOM.js is
> > bogus
> 
> Well... there's no obviously good way of checking it, is the thing.  I'd be
> fine with moving that to nodeName or whatever gets uppercased in HTML
> documents.  I think you're right that just always doing the createElementNS
> call will be safe after this patch lands, though.

I think it's not necessary to change MochiKit at all here. The check can now be considered relevant to pre-HTML5 browsers (it seems that MochiKit is meant to be more general purpose than being just a Gecko-specific framework) and the overall observable behavior is right either way.

Comment 31

[Henri Sivonen (:hsivonen)]

Attachment: Patch addressing review comments

Oops. The new patch lacked a '!'.

Comment 32

[Henri Sivonen (:hsivonen)]

Attachment: Interdiff of prior review and post review

Comment 33

[Boris Zbarsky [:bzbarsky]]

> This change can be deferred until bug 488249, right?

Absolutely.

> I think it's not necessary to change MochiKit at all here.

Right.  That's fine; as long as the tests don't break, I'm happy.

Comment 34

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 377116 [details] [diff] [review]
Patch addressing review comments

>diff --git a/content/base/public/nsIContent.h b/content/base/public/nsIContent.h
> 
>   /**
>+   * Return true iff this node is in an HTML document (in the HTML5 sense of
>+   * the term, i.e. not in an XHTML/XML document).
>+   */
>+  inline PRBool IsInHTMLDocument() const
>+  {
>+    return GetOwnerDoc() && // XXX clean up after bug 335998 lands
>+           !(GetOwnerDoc()->IsCaseSensitive());
>+  }

Please do something like:
nsIDocument* doc = GetOwnerDoc();
return doc && !doc->IsCaseSensitive();

Even if this is just temporary code.

>diff --git a/content/base/src/nsFrameLoader.cpp b/content/base/src/nsFrameLoader.cpp
>--- a/content/base/src/nsFrameLoader.cpp
>+++ b/content/base/src/nsFrameLoader.cpp
>@@ -812,17 +812,18 @@ nsFrameLoader::EnsureDocShell()
>   NS_ENSURE_TRUE(mDocShell, NS_ERROR_FAILURE);
> 
>   // Get the frame name and tell the docshell about it.
>   nsCOMPtr<nsIDocShellTreeItem> docShellAsItem(do_QueryInterface(mDocShell));
>   NS_ENSURE_TRUE(docShellAsItem, NS_ERROR_FAILURE);
>   nsAutoString frameName;
> 
>   PRInt32 namespaceID = mOwnerContent->GetNameSpaceID();
>-  if (namespaceID == kNameSpaceID_XHTML) {
>+  if (namespaceID == kNameSpaceID_XHTML
>+      && !mOwnerContent->IsInHTMLDocument()) {

Please follow prevailing style and put the '&&' at the end of the line above.

How did you generate the interdiff, it's a bummer its so messed up.

Comment 35

[Henri Sivonen (:hsivonen)]

Attachment: Patch addressing new review comments

(In reply to comment #34)
> Please do something like:
> nsIDocument* doc = GetOwnerDoc();
> return doc && !doc->IsCaseSensitive();
> 
> Even if this is just temporary code.

Fixed. (I thought repeat calls to a const method would get compiled away.)

> Please follow prevailing style and put the '&&' at the end of the line above.

Fixed.

> How did you generate the interdiff, it's a bummer its so messed up.

The command I used was
interdiff ../html-xmlns-serializer-full.diff ../html-xmlns-2009-05-13.diff > ../html-xhtml.interdiff

With interdiff from the patchutils of MacPorts.

Thanks.

Comment 36

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

(In reply to comment #35)
> Created an attachment (id=377358) [details]
> Patch addressing new review comments
> 
> (In reply to comment #34)
> > Please do something like:
> > nsIDocument* doc = GetOwnerDoc();
> > return doc && !doc->IsCaseSensitive();
> > 
> > Even if this is just temporary code.
> 
> Fixed. (I thought repeat calls to a const method would get compiled away.)

Even const methods can have side effects (they just can't change the object they are called on, but they can change anything else. And then there is mutable members too). However given that GetOwnerDoc is all inlined it's possible that it'll get optimized away, but I wouldn't rely on it.

Comment 37

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 377358 [details] [diff] [review]
Patch addressing new review comments

Sorry, missed that you had requested review here. You don't need to re-request review if someone's marked the previous patch r+, and all you've changed is address the review comments that came with that r+.

Anyhow, marking r+ :)

Comment 38

[Peter Van der Beken [:peterv]]

Comment on attachment 377358 [details] [diff] [review]
Patch addressing new review comments

>diff --git a/content/base/public/nsIContent.h b/content/base/public/nsIContent.h

>+  inline PRBool IsInHTMLDocument() const
>+  {
>+    nsIDocument* doc = GetOwnerDoc();
>+    return doc && // XXX clean up after bug 335998 lands
>+           !(doc->IsCaseSensitive());

No need for the brackets.

>diff --git a/content/base/src/nsFrameLoader.cpp b/content/base/src/nsFrameLoader.cpp

>+  if (namespaceID == kNameSpaceID_XHTML &&
>+      !mOwnerContent->IsInHTMLDocument()) {

This can go one one line.

>diff --git a/content/html/document/src/nsHTMLContentSink.cpp b/content/html/document/src/nsHTMLContentSink.cpp

>@@ -578,25 +578,17 @@ NS_NewHTMLElement(nsIContent** aResult, 

>+  NS_ASSERTION(aNodeInfo->NamespaceEquals(kNameSpaceID_XHTML), "Someone is still trying to create HTML elements is no namespace!");

Long line, wrap. The assertion text also doesn't make sense (maybe "Someone is trying to create HTML elements that don't have the XHTML namespace"?).

>diff --git a/content/html/document/src/nsHTMLDocument.cpp b/content/html/document/src/nsHTMLDocument.cpp

> nsHTMLDocument::GetApplets(nsIDOMHTMLCollection** aApplets)

>     mApplets = new nsContentList(this, nsGkAtoms::applet,
>-                                 GetDefaultNamespaceID());
>+                                 kNameSpaceID_XHTML);

Rewrap.

>diff --git a/content/html/document/src/nsHTMLFragmentContentSink.cpp b/content/html/document/src/nsHTMLFragmentContentSink.cpp

>@@ -414,31 +414,31 @@ nsHTMLFragmentContentSink::OpenContainer

>-      nodeInfo = mNodeInfoManager->GetNodeInfo(name, nsnull, kNameSpaceID_None);
>+      nodeInfo = mNodeInfoManager->GetNodeInfo(name, nsnull, kNameSpaceID_XHTML);

Long line, rewrap.

>-      nodeInfo = mNodeInfoManager->GetNodeInfo(name, nsnull, kNameSpaceID_None);
>+      nodeInfo = mNodeInfoManager->GetNodeInfo(name, nsnull, kNameSpaceID_XHTML);

Long line, rewrap.

>diff --git a/content/xslt/src/xpath/txMozillaXPathTreeWalker.cpp b/content/xslt/src/xpath/txMozillaXPathTreeWalker.cpp

>+txXPathNodeUtils::isHTMLElementInHTMLDocument(const txXPathNode& aNode)
>+{
>+  if (!aNode.isContent()) {
>+    return PR_FALSE;
>+  }
>+  nsIContent* content = aNode.Content();
>+  return (content->IsNodeOfType(nsINode::eHTML) &&
>+          content->IsInHTMLDocument());

No need for the outer brackets. This can go on one line.

>diff --git a/content/xslt/src/xpath/txNameTest.cpp b/content/xslt/src/xpath/txNameTest.cpp

>-    if (txXPathNodeUtils::getNamespaceID(aNode) != mNamespace)
>+    if (mNamespace != txXPathNodeUtils::getNamespaceID(aNode) 
>+#ifndef TX_EXE        
>+        && !(mNamespace == kNameSpaceID_None && 
>+             txXPathNodeUtils::isHTMLElementInHTMLDocument(aNode))

Given that this is the only place where we call isHTMLElementInHTMLDocument I'd just inline the code here. Or else inline it in txXPathTreeWalker.h.

>+#endif          

You're adding trailing whitespace on almost all these lines. Please remove.

>+      )

Line it up with the opening bracket.

>diff --git a/content/xslt/src/xslt/txMozillaXMLOutput.cpp b/content/xslt/src/xslt/txMozillaXMLOutput.cpp

>@@ -448,62 +448,60 @@ txMozillaXMLOutput::startElement(nsIAtom

>-        return startElementInternal(nsnull, aLowercaseLocalName,
>-                                    kNameSpaceID_None, kNameSpaceID_XHTML);
>+        return startElementInternal(nsnull, aLowercaseLocalName, kNameSpaceID_XHTML);

Long line, rewrap.

>diff --git a/layout/style/nsCSSRuleProcessor.cpp b/layout/style/nsCSSRuleProcessor.cpp

>@@ -892,26 +892,21 @@ RuleProcessorData::RuleProcessorData(nsP

>+    mNameSpaceID = aContent->GetNameSpaceID();
>+    

Trailing whitespace, please remove.

>@@ -1566,17 +1553,17 @@ static PRBool SelectorMatches(RuleProces

>     else if (nsCSSPseudoClasses::mozIsHTML == pseudoClass->mAtom) {
>       result = data.mIsHTMLContent &&
>-        data.mContent->GetNameSpaceID() == kNameSpaceID_None;
>+        data.mContent->IsInHTMLDocument();

This can go on one line.

Let's hope you caught all the places that needed changes, this could lead to subtle breakage.

Comment 39

[Henri Sivonen (:hsivonen)]

Attachment: Patch addressing sr comments (diffed against more recent trunk)

Many thanks for both r and sr. I've fixed all the sr issues in this patch.

Now waiting for the tree to reopen for post-3.5 development.

Comment 40

[Henri Sivonen (:hsivonen)]

Landed as rev 39f1a74e500b

Comment 41

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

This seems like something that requires dev-doc, since code that worked fine with upper-case localNames (or, less likely, depended on a null namespaceURI) will not work fine in Firefox 3.6 or later.  We will need a document that tells developers how to write code that can handle both worlds, if nothing else, and ideally doesn't require them to manually search out every string that they compare against a localName.

Do we know what the compatibility effects of this are likely to be?  I'm a little spooked right now, tbqh, given how hard it was to even find all the places in our own test suites and source trees that needed updating.

Comment 42

[Henri Sivonen (:hsivonen)]

(In reply to comment #41)
> Do we know what the compatibility effects of this are likely to be? I'm a
> little spooked right now, tbqh, given how hard it was to even find all the
> places in our own test suites and source trees that needed updating.

What needed updating was JS that had gotten run in Firefox only. This change makes Gecko behave like WebKit on these points, so cross-browser code on the Web would already be broken in WebKit. Moreover, IE doesn't have the affected APIs at all.

Thus, Web compat issues seem unlikely. (So far, the Web compat fallout has been small, and aligning with WebKit on those points as well has been the easy fix.) However, extension compat issues are possible when extensions implement some of their UI as an HTML DOM as opposed to a XUL DOM. (This appears to have been the issue with Firebug.)

Comment 43

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Yeah, we definitely need dev-docs here. But I'm with Henri on the compat issues. Mostly because we so far there doesn't seem to be any bugs filed on .namespaceURI or .localName changing values.

But yes, there's definitely a compat risk here, which is why we wanted to land this as soon as possible.

Comment 44

[Henri Sivonen (:hsivonen)]

Added https://developer.mozilla.org/en/Updating_extensions_for_Firefox_3.6 and updated the DevMo DOM reference accordingly.

Comment 45

[Henrik Skupin [:whimboo][⌚️UTC+1]]

I found a regression in Firefox 3.6 which is content related. Mafia Wars on Facebook doesn't work partly anymore since this patch has been landed. See bug 536900.

Comment 46

[Henri Sivonen (:hsivonen)]

Added getElementsByTagNameNS to https://developer.mozilla.org/en/Firefox_3.6_for_developers

Comment 47

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

These pages have been updated:

https://developer.mozilla.org/en/DOM/Element
https://developer.mozilla.org/en/DOM/Node.namespaceURI
https://developer.mozilla.org/en/Updating_extensions_for_Firefox_3.6
https://developer.mozilla.org/en/Firefox_3.6_for_developers