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.

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!" :-)

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.

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)

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 :-)

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.

> 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.

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.

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).

Fixed, finally! Thanks for the reviews n' all...

> + "this is wrong, trust me! Loose the prefix on the attribute!"); uber-nit -- "lose", not "loose"....

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

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