xhtml namespace is treated as null namespace in xhtml-elements

RESOLVED FIXED in mozilla1.1alpha

Status

()

Core
XML
RESOLVED FIXED
16 years ago
16 years ago

People

(Reporter: sicking, Assigned: jst)

Tracking

Trunk
mozilla1.1alpha
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [HAVE FIX])

Attachments

(2 attachments)

This bug is a split-off from bug 41983. We have many clients that use 
kNameSpaceID_HTML when accessing attributes and therefore I had to add code to 
all attribute-handling functions so that kNameSpaceID_HTML is translated into 
kNameSpaceID_None before getting/setting/checking the attribute.

This bug is about fixing all those clients and removing the special-handling 
code so that html elements behave properly wrt namespaced attributes.

How important is this for 1.0?
I'd say go for 1.0.1 or 1.1.
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla1.1alpha
(Assignee)

Comment 2

16 years ago
Created attachment 82583 [details] [diff] [review]
Die kNameSpaceID_HTML, die!

Comment 3

16 years ago
Comment on attachment 82583 [details] [diff] [review]
Die kNameSpaceID_HTML, die!


>-          if (NS_CONTENT_ATTR_HAS_VALUE == element->GetAttr(kNameSpaceID_None, nsHTMLAtoms::target, value)) {
>+          if (NS_CONTENT_ATTR_HAS_VALUE == element->GetAttr(kNameSpaceID_None,
>+ nsHTMLAtoms::target, value)) {

Indentation problem here. (file nsHTMLContentSink.cpp)

>@@ -2117,7 +2127,8 @@
> nsHTMLDocument::GetApplets(nsIDOMHTMLCollection** aApplets)
> {
>   if (nsnull == mApplets) {
>-    mApplets = new nsContentList(this, nsHTMLAtoms::applet, kNameSpaceID_Unknown);
>+    mApplets = new nsContentList(this, nsHTMLAtoms::applet,
>+                                 kNameSpaceID_Unknown);

Unknown?

Why are you removing the PI code in this file?
> 
>-  isHTML = IsHTMLNameSpace(nameSpaceID);
>+  PRBool isHTML = nameSpaceID == kNameSpaceID_XHTML;

I'd call this one differently, it it matches XHTML content.

>RCS file: /cvsroot/mozilla/layout/html/forms/src/nsGfxTextControlFrame2.cpp,v

>-  rv = nodeInfoManager->GetNodeInfo(nsHTMLAtoms::div, nsnull, kNameSpaceID_HTML, *getter_AddRefs(nodeInfo));
>+  rv = nodeInfoManager->GetNodeInfo(nsHTMLAtoms::div, nsnull,
>+                                    kNameSpaceID_XHTML,
>+                                    *getter_AddRefs(nodeInfo));

Why XHTML here?

God, that's one nice cleanup!
-#define kNameSpaceID_HTML     3
-#define kNameSpaceID_XLink    4
-#define kNameSpaceID_HTML2    5 // This is not a real namespace
+#define kNameSpaceID_XHTML    3
+#define kNameSpaceID_XLink    5

why change XLink from 4 to 5?


in nsHTMLContentSink.cpp:
-      result = mCSSLoader->LoadStyleLink(aElement, url, aTitle, aMedia, kNa...
+      result = mCSSLoader->LoadStyleLink(aElement, url, aTitle, aMedia,
+                                         kNameSpaceID_Unknown,

same question as fabian but a different place, why Unknown?

in nsHTMLDocument.cpp:
-  if (namespaceID == kNameSpaceID_HTML) {
+  if (namespaceID == kNameSpaceID_XHTML) {
     nsCOMPtr<nsIHTMLContent> htmlContent;
 
     rv = NS_CreateHTMLElement(getter_AddRefs(htmlContent), nodeInfo, PR_FALSE);
     content = do_QueryInterface(htmlContent);
   }
   else {
-    rv = NS_NewXMLElement(getter_AddRefs(content), nodeInfo);
+    nsCOMPtr<nsIElementFactory> elementFactory;
+    mNameSpaceManager->GetElementFactory(namespaceID,
+                                         getter_AddRefs(elementFactory));
+
+    if (elementFactory) {
+      rv = elementFactory->CreateInstanceByTag(nodeInfo,
+                                               getter_AddRefs(content));
+    } else {
+      rv = NS_NewXMLElement(getter_AddRefs(content), nodeInfo);
+    }

Nice change! But don't we register an element-factory for xhtml too, so only 
the |else| part of the |if| should be needed?


-          if (nameSpaceID == kNameSpaceID_None || nameSpaceID == kNameSpaceI...
+          if (nameSpaceID == kNameSpaceID_None ||
+              nameSpaceID == kNameSpaceID_XHTML) {

shouldn't this only be for kNameSpaceID_None? Or is this for attribute-values 
rather then attribute-names? Hmm.. maybe this should be handled in a separate 
bug.


@@ -3375,8 +3375,7 @@
         PRInt32 nameSpaceID;
 #endif
 #ifdef INCLUDE_XUL
-        if (NS_SUCCEEDED(aDocElement->GetNameSpaceID(nameSpaceID)) &&
-            nameSpaceID == nsXULAtoms::nameSpaceID) {
+        if (aDocElement->IsContentOfType(nsIContent::eXUL)) {

you could move the |PRInt32 nameSpaceID;| into the |#ifdef MOZ_SVG| since it's 
no longer used in the xul-code.


-  PRBool isXUL = (nameSpaceID == nsXULAtoms::nameSpaceID);
wow, how did that ever work?


Heikki had to fix some xul files as well but i'm not sure if those are 
fixed/removed by now. I just wanted to make sure that you've looked through xul-
files as well. Look towards the end of attachment 54063 [details] [diff] [review] (from bug 41983)


With this and confirmation that there is no broken markup, r=sicking
oh, and i second Fabians "God, that's one nice cleanup!" :-)
(Assignee)

Comment 6

16 years ago
Fabian's questions:

Why kNameSpaceID_Unknown in nsHTMLDocument::GetApplets()? Well, the old code
used that too, I've now changed it to use kNameSpaceID_None since that's what
all applets will use anyway in a HTML document. Either way, using Unknown also
works since that means match any namespace in a nsContentList.

The PI code was removed from nsXMLContentSink since it wasn't used, it was dead
code.

IsHTML was renamed to isXHTML.

The last question was about why we pass kNameSpaceID_XHTML to GetNodeInfo(), we
do that because we want to create a XHTML element (a "div") further down in that
code.


Sicking's questions:

The XLink change was a goofup on my part, well, kinda... I still changed the
values of some of those constants since I don't want any unused numbers in
there, they're now all used, from -1 to 7.

As for the kNameSpaceID_Unknown that's passed to LoadStyleLink(), I don't know,
I'm not changing that, just re-indenting things there...

Yes, using the element factory for all namespaces works, changed.

Yeah, separate bug should be filed for the nsCSSParser issue.

Nope, can't move nameSpaceID, it needs to be defined before the if-else block
starts (nice #define mess there...).

>-  PRBool isXUL = (nameSpaceID == nsXULAtoms::nameSpaceID);
>wow, how did that ever work?

nsXULAtoms::nameSpaceID is not an atom, it's a PRInt32, so it does work, weird,
but it works.

Found and fixed some html: prefixes on attributes in XUL files, added asserts if
someone tries to do that again...

New patch coming up with the above comments taken into account and some more
cleanup done.
(Assignee)

Comment 7

16 years ago
Created attachment 82715 [details] [diff] [review]
More cleanup, and review comments addressed.
(Assignee)

Comment 8

16 years ago
Heikki, would you sr?
-      nsCOMPtr<nsIDOMEventTarget> node(do_QueryInterface(NS_STATIC_CAST
(nsIContent *, this)));
+      nsCOMPtr<nsIDOMEventTarget> node =
+        do_QueryInterface(NS_STATIC_CAST(nsIContent *, this));

It seems unneccesary to a) keep an nsCOMPtr to |this| and b) do_QI(this). I 
can't see all the code in the diff, but couldn't you just use implicit casting 
without any addref-ing?


+    mApplets = new nsContentList(this, nsHTMLAtoms::applet,
+                                 kNameSpaceID_None);
+    if (!mApplets) {
       return NS_ERROR_OUT_OF_MEMORY;
     }

could use NS_ENSURE_TRUE.


with or without the above, r=sicking. (while i really like the cleanups, please 
don't do any more, it's not a small patch to re-read)
Assignee: sicking → jst
Status: ASSIGNED → NEW
(Assignee)

Comment 10

16 years ago
A static cast won't work there, nsGenericElement doesn't implement
nsIDOMEventTarget directly, it's done though a tearoff, so we need the QI.

Yeah, I could use NS_ENSURE_TRUE() for that, but I don't like using that for OOM
checks (since it's not really an error in any way, and we know it can happen, so
we don't want to assert).

And yes, I agree, and I won't make any more changes to this except for updates
per review feedback :-)
Status: NEW → ASSIGNED
Whiteboard: [HAVE FIX]
oki, sounds good to me
Comment on attachment 82715 [details] [diff] [review]
More cleanup, and review comments addressed.

>Index: content/base/public/nsINameSpaceManager.h
>===================================================================

The order of the id's etc. is really important (the impl. relies on this),
so make sure you are not breaking any assumptions there. I seem to recall
even the values used to be important, so double check that too.

>Index: content/base/src/nsContentList.cpp
>===================================================================
>Index: content/base/src/nsHTMLContentSerializer.cpp
>===================================================================
>Index: content/base/src/nsNameSpaceManager.cpp
>===================================================================
>-static const char kHTMLNameSpaceURI[] = "http://www.w3.org/TR/REC-html40";  // XXX?? "urn:w3-org-ns:HTML"??

Make sure that there are no references to this namespace URI anywhere
in the source tree or www.mozilla.org before removing this! Still, this
is going to break sites that use this (there are some). IE and Opera
still support this I believe. I guess the question I am asking are
you really really sure we want to do this (personally I want to do this,
but...)? 

>Index: content/events/src/nsEventStateManager.cpp
>===================================================================
>Index: content/html/content/src/nsGenericHTMLElement.cpp
>===================================================================
>+  NS_ASSERTION(aNameSpaceID != kNameSpaceID_XHTML,
>+               "Error, attribute on [X]HTML element set with XHTML namespace, "
>+               "this is wrong, trust me! Loose the prefix on the attribute!");
>+

Please make sure that we don't do this in the source tree or www.mozilla.org.
I've fixed several instances of this in the past, so maybe there aren't any
but just in case... Also what happens if somebody does gives us this kind
of document?

>Index: content/html/content/src/nsGenericHTMLElement.h
>===================================================================
>Index: content/html/content/src/nsHTMLAnchorElement.cpp
>===================================================================
>Index: content/html/content/src/nsHTMLAreaElement.cpp
>===================================================================
>Index: content/html/content/src/nsHTMLButtonElement.cpp
>===================================================================
>Index: content/html/content/src/nsHTMLFormElement.cpp
>===================================================================
>Index: content/html/content/src/nsHTMLImageElement.cpp
>===================================================================
>Index: content/html/content/src/nsHTMLInputElement.cpp
>===================================================================
>Index: content/html/content/src/nsHTMLLabelElement.cpp
>===================================================================
>Index: content/html/content/src/nsHTMLLinkElement.cpp
>===================================================================
>Index: content/html/content/src/nsHTMLOptionElement.cpp
>===================================================================
>Index: content/html/content/src/nsHTMLSelectElement.cpp
>===================================================================
>Index: content/html/content/src/nsHTMLTextAreaElement.cpp
>===================================================================
>Index: content/html/content/src/nsHTMLUnknownElement.cpp
>===================================================================
>Index: content/html/document/src/nsHTMLContentSink.cpp
>===================================================================
>Index: content/html/document/src/nsHTMLDocument.cpp
>===================================================================
>-      (NS_CONTENT_ATTR_HAS_VALUE == aContent->GetAttr(kNameSpaceID_HTML, nsHTMLAtoms::href, attr))) {
>-      result = PR_TRUE;
>+    return aContent->HasAttr(kNameSpaceID_None, nsHTMLAtoms::href);

Are these absolutely the same thing? Specifically, what about href=""?
I think there was one like this above that I missed, and more below...

>Index: content/html/document/src/nsHTMLFragmentContentSink.cpp
>===================================================================
>Index: content/html/style/src/nsCSSParser.cpp
>===================================================================
>Index: content/xml/document/src/nsXMLContentSink.cpp
>===================================================================
>+  const PRBool isXHTML = nameSpaceID == kNameSpaceID_XHTML;

I think I'd just get rid of this variable and compare nameSpaceID always
to kNameSpaceID_XHTML directly (provided nameSpaceID does not change,
of course). But this is not a big deal.

>+    if (aIndex != (PRUint32)-1 && NS_SUCCEEDED(result)) {

Please add parenthesis to make the grouping clear.

>Index: content/xml/document/src/nsXMLContentSink.h
>===================================================================
>Index: content/xml/document/src/nsXMLDocument.cpp
>===================================================================
>Index: content/xul/document/src/nsXULContentSink.cpp
>===================================================================
>Index: content/xul/document/src/nsXULDocument.cpp
>===================================================================
>Index: content/xul/templates/src/nsXULContentBuilder.cpp
>===================================================================
>Index: layout/html/base/src/nsFrameManager.cpp
>===================================================================
>Index: layout/html/base/src/nsImageFrame.cpp
>===================================================================
>Index: layout/html/base/src/nsObjectFrame.cpp
>===================================================================
>Index: layout/html/base/src/nsPresShell.cpp
>===================================================================
>Index: layout/html/base/src/nsSpacerFrame.cpp
>===================================================================
>Index: layout/html/forms/src/nsButtonFrameRenderer.cpp
>===================================================================
>Index: layout/html/forms/src/nsButtonFrameRenderer.h
>===================================================================
>Index: layout/html/forms/src/nsFormControlHelper.cpp
>===================================================================
>Index: layout/html/forms/src/nsGfxButtonControlFrame.cpp
>===================================================================
>Index: layout/html/forms/src/nsGfxTextControlFrame2.cpp
>===================================================================
>Index: layout/html/forms/src/nsHTMLButtonControlFrame.cpp
>===================================================================
>Index: layout/html/style/src/nsCSSFrameConstructor.cpp
>===================================================================
>Index: layout/html/tests/TestAttributes.cpp
>===================================================================
>Index: webshell/tests/viewer/nsWebCrawler.cpp
>===================================================================
>Index: xpfe/browser/samples/dexparamdialog.xul
>===================================================================
>Index: xpfe/browser/src/navigator-test1.xul
>===================================================================

So double check some things, answer to the questions and correct the 
minor nits and sr=heikki.
Attachment #82715 - Flags: superreview+
(Assignee)

Comment 13

16 years ago
> The order of the id's etc...

Yes, it is, and it's correct as it is in the patch, I've checked that multiple
times (and I'm not really changing any order here, just removing unused an
incorrect constants)...

> Make sure that there are no references to this namespace URI
(http://www.w3.org/TR/REC-html40)...

Done, there are no references to this namespace URI in the mozilla source, nor
is there any in the commercial source. I have no way to verify that there are no
references to this on www.mozilla.org though (I don't have access to that
repository). And yes, I agree with you, we really do want to kill the support
for this, may it RIP from now 'till eternity.

> Please make sure that we don't do this [use xhtml namespaces on attributes]
> in the source tree or www.mozilla.org.

There were a few cases in the mozilla source where we did this, but those are
fixed by this patch. But again, I can't say for www.mozilla.org. If someone
gives us a document with [X]HTML elements with attributes in any namespace other
than <none> we'll add the attribute, but it won't mean what they thought it'd mean.

> Are these absolutely the same thing? (re: HasAttr() vs. rv == ...HAS_VALUE)

Yes, they're identical, even in the href="" case.

Nits fixed.
(Assignee)

Comment 14

16 years ago
Ok, did some poking around on www.mozilla.org and I found:

  http://www.mozilla.org/xpfe/xulref/bulletin.html
  http://www.mozilla.org/xpfe/xulref/stack.html

Both of which define a html namespace for no good reason (i.e. the prefix is
never used). But nothing else found on www.mozilla.org.
(Assignee)

Comment 15

16 years ago
Ok, so heikki agreed to fix those in the www.mozilla.org repository, so I'm
checking in!
Fixed those two things on www.mozilla.org (will take some time before the
webserver picks up the changes).
(Assignee)

Comment 17

16 years ago
Fixed, finally! Thanks for the reviews n' all...
Status: ASSIGNED → RESOLVED
Last Resolved: 16 years ago
Resolution: --- → FIXED
> +         "this is wrong, trust me! Loose the prefix on the attribute!");

uber-nit -- "lose", not "loose"....
(Assignee)

Comment 19

16 years ago
Thanks bz, typo fixed.
For the record (and if this were ever to go on a branch), it caused regression
bug  148249 (due to a missed change that was needed).
Also seems to have caused bug 176606.

/be
(Assignee)

Comment 22

16 years ago
No, I don't think this changed anything wrt bug 176606.

Updated

16 years ago
QA Contact: petersen → rakeshmishra
You need to log in before you can comment on or make changes to this bug.