1264323 - Adjust namespaceURI for createElement() when using on DOMParser.parseFromString("application/xhtml+xml")

Status: RESOLVED FIXED

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

Description

[Arkadiusz Michalski (Spirit)]

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:45.0) Gecko/20100101 Firefox/45.0
Build ID: 20160407164938

Steps to reproduce:

After DOM fix:https://github.com/whatwg/dom/commit/c8ae9cbd46ca3175e07b7f205ebe375805013119
this testcase should product "http://www.w3.org/1999/xhtml" (not null)

console.log(new DOMParser().parseFromString("<root/>", "application/xhtml+xml").createElement("x").namespaceURI);

Comment 1

[John Dai[:jdai]]

Attachment: Bug 1264323 - Remove redundant trailing spaces.

Comment 2

[John Dai[:jdai]]

Attachment: Bug 1264323 - Set namespaceID when DOMParser.parseFromString("application/xhtml+xml").

Try result:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=d4e9e518dc9c12fe112c789dde4b35a71097b651

Comment 3

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

Comment on attachment 8760135 [details] [diff] [review]
Bug 1264323 - Set namespaceID when DOMParser.parseFromString("application/xhtml+xml").

The spec change is more generic, making createElement behavior depend on contentType always. Could we move setting defaultNamespaceID to SetContentTypeInternal?
See https://dom.spec.whatwg.org/#dom-document-createelement
But, I think the spec is buggy. See the example
http://mozilla.pettay.fi/moztests/imagedoc.html

So, I would be ok to try out option where contentType used the way the spec says unless the document is HTMLDocument (IsHTMLOrXHTML()).

Comment 4

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

So I commented in https://www.w3.org/Bugs/Public/show_bug.cgi?id=19431.

Comment 5

[John Dai[:jdai]]

Attachment: Bug 1264323 - Remove redundant trailing spaces. r=smaug

Comment 6

[John Dai[:jdai]]

Attachment: Bug 1264323 - Set namespaceID when DOMParser.parseFromString(application/xhtml+xml). v2

Address comment 3. I moved setting defaultNamespaceID to SetContentTypeInternal.
Hi Olli, may I have your review? Thank you.

Comment 7

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

Comment on attachment 8760651 [details] [diff] [review]
Bug 1264323 - Set namespaceID when DOMParser.parseFromString(application/xhtml+xml). v2

># HG changeset patch
># User John Dai <[e-mail]>
>Bug 1264323 - Set namespaceID when DOMParser.parseFromString(application/xhtml+xml). r=smaug
>
>
>diff --git a/dom/base/DOMParser.cpp b/dom/base/DOMParser.cpp
>index fd907e0..adcaf7f 100644
>--- a/dom/base/DOMParser.cpp
>+++ b/dom/base/DOMParser.cpp
>@@ -85,17 +85,16 @@ DOMParser::ParseFromString(const nsAString& str,
> 
>   nsresult rv;
> 
>   if (!nsCRT::strcmp(contentType, "text/html")) {
>     nsCOMPtr<nsIDOMDocument> domDocument;
>     rv = SetUpDocument(DocumentFlavorHTML, getter_AddRefs(domDocument));
>     NS_ENSURE_SUCCESS(rv, rv);
>     nsCOMPtr<nsIDocument> document = do_QueryInterface(domDocument);
>-
>     // Keep the XULXBL state, base URL and principal setting in sync with the
>     // XML case
> 
>     if (nsContentUtils::IsSystemPrincipal(mOriginalPrincipal)) {
>       document->ForceEnableXULXBL();
>     }
> 
>     // Make sure to give this document the right base URI
>@@ -258,16 +257,20 @@ DOMParser::ParseFromStream(nsIInputStream *stream,
>   nsCOMPtr<nsIStreamListener> listener;
> 
>   // Have to pass false for reset here, else the reset will remove
>   // our event listener.  Should that listener addition move to later
>   // than this call?  Then we wouldn't need to mess around with
>   // SetPrincipal, etc, probably!
>   nsCOMPtr<nsIDocument> document(do_QueryInterface(domDocument));
>   if (!document) return NS_ERROR_FAILURE;
>+  if (document->IsHTMLOrXHTML() &&
>+      !nsCRT::strcmp(contentType, "application/xhtml+xml")) {
>+    document->SetContentType(NS_LITERAL_STRING("application/xhtml+xml"));
>+  }
Why do we need to do this change?
new DOMParser().parseFromString("<root/>", "application/xhtml+xml") already creates a document (XMLDocument to be precise) which has its content type "application/xhtml+xml"


> nsIDocument::SetContentTypeInternal(const nsACString& aType)
> {
>   mCachedEncoder = nullptr;
>   mContentType = aType;
>+  mDefaultElementType = kNameSpaceID_None;
>+
>+  if (aType.EqualsLiteral("text/html") ||
>+      aType.EqualsLiteral("application/xhtml+xml")) {
>+    mDefaultElementType = kNameSpaceID_XHTML;
>+  }

So I was thinking that perhaps we could do here something like
if (!document->IsHTMLOrXHTML() && mDefaultElementType == kNameSpaceID_None
    (aType.EqualsLiteral("text/html") ||
     aType.EqualsLiteral("application/xhtml+xml")) {
  mDefaultElementType = kNameSpaceID_XHTML;
}



I did file also a spec bug yesterday https://github.com/whatwg/dom/issues/264

Comment 8

[John Dai[:jdai]]

(In reply to Olli Pettay [:smaug] (high review load, please consider other reviewers) from comment #7)
> Comment on attachment 8760651 [details] [diff] [review]
> Bug 1264323 - Set namespaceID when
> DOMParser.parseFromString(application/xhtml+xml). v2
> 
> ># HG changeset patch
> ># User John Dai <[e-mail]>
> >Bug 1264323 - Set namespaceID when DOMParser.parseFromString(application/xhtml+xml). r=smaug
> >
> >
> >diff --git a/dom/base/DOMParser.cpp b/dom/base/DOMParser.cpp
> >index fd907e0..adcaf7f 100644
> >--- a/dom/base/DOMParser.cpp
> >+++ b/dom/base/DOMParser.cpp
> >@@ -85,17 +85,16 @@ DOMParser::ParseFromString(const nsAString& str,
> > 
> >   nsresult rv;
> > 
> >   if (!nsCRT::strcmp(contentType, "text/html")) {
> >     nsCOMPtr<nsIDOMDocument> domDocument;
> >     rv = SetUpDocument(DocumentFlavorHTML, getter_AddRefs(domDocument));
> >     NS_ENSURE_SUCCESS(rv, rv);
> >     nsCOMPtr<nsIDocument> document = do_QueryInterface(domDocument);
> >-
> >     // Keep the XULXBL state, base URL and principal setting in sync with the
> >     // XML case
> > 
> >     if (nsContentUtils::IsSystemPrincipal(mOriginalPrincipal)) {
> >       document->ForceEnableXULXBL();
> >     }
> > 
> >     // Make sure to give this document the right base URI
> >@@ -258,16 +257,20 @@ DOMParser::ParseFromStream(nsIInputStream *stream,
> >   nsCOMPtr<nsIStreamListener> listener;
> > 
> >   // Have to pass false for reset here, else the reset will remove
> >   // our event listener.  Should that listener addition move to later
> >   // than this call?  Then we wouldn't need to mess around with
> >   // SetPrincipal, etc, probably!
> >   nsCOMPtr<nsIDocument> document(do_QueryInterface(domDocument));
> >   if (!document) return NS_ERROR_FAILURE;
> >+  if (document->IsHTMLOrXHTML() &&
> >+      !nsCRT::strcmp(contentType, "application/xhtml+xml")) {
> >+    document->SetContentType(NS_LITERAL_STRING("application/xhtml+xml"));
> >+  }
> Why do we need to do this change?
> new DOMParser().parseFromString("<root/>", "application/xhtml+xml") already
> creates a document (XMLDocument to be precise) which has its content type
> "application/xhtml+xml"
> 

Because we set default value to "application/xml" when we new XMLDocument(). [1][2]

[1] http://mxr.mozilla.org/mozilla-central/source/dom/xml/XMLDocument.h#24
[2] http://mxr.mozilla.org/mozilla-central/source/dom/xml/XMLDocument.cpp#182 
> 
> > nsIDocument::SetContentTypeInternal(const nsACString& aType)
> > {
> >   mCachedEncoder = nullptr;
> >   mContentType = aType;
> >+  mDefaultElementType = kNameSpaceID_None;
> >+
> >+  if (aType.EqualsLiteral("text/html") ||
> >+      aType.EqualsLiteral("application/xhtml+xml")) {
> >+    mDefaultElementType = kNameSpaceID_XHTML;
> >+  }
> 
> So I was thinking that perhaps we could do here something like
> if (!document->IsHTMLOrXHTML() && mDefaultElementType == kNameSpaceID_None
>     (aType.EqualsLiteral("text/html") ||
>      aType.EqualsLiteral("application/xhtml+xml")) {
>   mDefaultElementType = kNameSpaceID_XHTML;
> }
> 
Will do.
> 
> 
> I did file also a spec bug yesterday https://github.com/whatwg/dom/issues/264

Comment 9

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

(In reply to John Dai[:jdai] from comment #8)
> > Why do we need to do this change?
> > new DOMParser().parseFromString("<root/>", "application/xhtml+xml") already
> > creates a document (XMLDocument to be precise) which has its content type
> > "application/xhtml+xml"
> > 
> 
> Because we set default value to "application/xml" when we new XMLDocument().
But then elsewhere we update content type, since (new DOMParser().parseFromString("<root/>", "application/xhtml+xml")).contentType gives the expected content type.

Comment 10

[John Dai[:jdai]]

Attachment: Bug 1264323 - Remove redundant trailing spaces. r=smaug

Comment 11

[John Dai[:jdai]]

(In reply to Olli Pettay [:smaug] (high review load, please consider other reviewers) from comment #9)
> (In reply to John Dai[:jdai] from comment #8)
> > > Why do we need to do this change?
> > > new DOMParser().parseFromString("<root/>", "application/xhtml+xml") already
> > > creates a document (XMLDocument to be precise) which has its content type
> > > "application/xhtml+xml"
> > > 
> > 
> > Because we set default value to "application/xml" when we new XMLDocument().
> But then elsewhere we update content type, since (new
> DOMParser().parseFromString("<root/>", "application/xhtml+xml")).contentType
> gives the expected content type.

Thanks for reminding me of this.

Comment 12

[John Dai[:jdai]]

Attachment: Bug 1264323 - Set namespace when DOMParser.parseFromString(application/xhtml+xml). r=smaug

Aligned with dom spec createElement() step 6[1], after the spec bug[2] was fixed.
Hi Olli, may I have your review? Thank you.

[1] https://dom.spec.whatwg.org/#dom-document-createelement 
[2] https://github.com/whatwg/dom/issues/264

Comment 13

[John Dai[:jdai]]

Comment on attachment 8766739 [details] [diff] [review]
Bug 1264323 - Set namespace when DOMParser.parseFromString(application/xhtml+xml). r=smaug

Add reviewer name.

Comment 14

[John Dai[:jdai]]

Try result:https://treeherder.mozilla.org/#/jobs?repo=try&revision=cec790ef7a46e4391a987eb324a057445b31ea958

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/38eebb8512cf
Remove redundant trailing spaces. r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/b97c6b960d85
Set namespace when DOMParser.parseFromString(application/xhtml+xml). r=smaug

Comment 16

[Carsten Book [:Tomcat]]

had to back this out in https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=9ef5260af8f32989b2a5e1d0bd9a4f9103b566c6 - because with landing of this checkin we had timeouts like https://treeherder.mozilla.org/logviewer.html#?job_id=31056460&repo=mozilla-inbound 

could you take a look at this and if its not related to your push, please set checkin-needed again. 

Sorry for the inconvenience

Comment 17

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9d17de23a08c
Backed out changeset b97c6b960d85 
https://hg.mozilla.org/integration/mozilla-inbound/rev/fa394e9ab716
Backed out changeset 38eebb8512cf

Comment 18

[John Dai[:jdai]]

Looks like its not related to my patch, set checkin-needed again.
Try result: https://treeherder.mozilla.org/#/jobs?repo=try&revision=2be2c10c281cf0ca172c1fa82fcc46f2e8f3f9b2

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0f80aa661618
Remove redundant trailing spaces. r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/09256a911e9c
Set namespace when DOMParser.parseFromString(application/xhtml+xml). r=smaug

Comment 20

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/0f80aa661618
https://hg.mozilla.org/mozilla-central/rev/09256a911e9c