Closed
Bug 1276438
Opened 9 years ago
Closed 7 years ago
document.body implemented on the incorrect interface
Categories
(Core :: DOM: Core & HTML, defect)
Core
DOM: Core & HTML
Tracking
()
RESOLVED
FIXED
mozilla60
Tracking | Status | |
---|---|---|
firefox60 | --- | fixed |
People
(Reporter: sephr, Assigned: bzbarsky)
References
(Blocks 1 open bug)
Details
(Keywords: dev-doc-complete, Whiteboard: [tw-dom] btpp-backlog)
Attachments
(7 files)
4.85 KB,
patch
|
nika
:
review+
|
Details | Diff | Splinter Review |
5.15 KB,
patch
|
nika
:
review+
|
Details | Diff | Splinter Review |
4.39 KB,
patch
|
nika
:
review+
|
Details | Diff | Splinter Review |
14.51 KB,
patch
|
nika
:
review+
|
Details | Diff | Splinter Review |
3.48 KB,
patch
|
nika
:
review+
|
Details | Diff | Splinter Review |
19.63 KB,
patch
|
nika
:
review+
|
Details | Diff | Splinter Review |
3.28 KB,
patch
|
nika
:
review+
|
Details | Diff | Splinter Review |
User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/51.0.2704.63 Safari/537.36
Steps to reproduce:
document.body is implemented on the HTMLDocument interface, instead of the Document interface as required by the latest W3C HTML5 spec.
For example, the following code should return true:
(new DOMParser).parseFromString("<html xmlns='http://www.w3.org/1999/xhtml'><body/></html>","application/xhtml+xml").body !== null
Chrome, Edge, IE, Safari, and Opera all correctly implement document.body on the Document interface and return true with that example.
Additionally, Firefox's implementation of document.body (on HTMLDocument) refuses to work with non-HTMLDocument documents. For example, run the following (the result should also be true):
(Object.getOwnPropertyDescriptor(Document.prototype, "body") || Object.getOwnPropertyDescriptor(HTMLDocument.prototype, "body")).get.call((new DOMParser).parseFromString("<html xmlns='http://www.w3.org/1999/xhtml'><body/></html>","application/xhtml+xml")) !== null
Chrome, Edge, IE, Safari, and Opera also all return true for that example.
Actual results:
The examples above return false.
Expected results:
The examples above should both return true. document.body should be moved to the Document interface.
It should function equivalently to this workaround at https://gist.github.com/eligrey/5a600bade889e54f4b7f1cce42165288 :
if (!Object.getOwnPropertyDescriptor(Document.prototype, "body"))
Object.defineProperty(Document.prototype, "body", {
enumerable: true
, configurable: true
, get: function() {
return this.evaluate(
"/*[local-name()='html'][namespace-uri()='http://www.w3.org/1999/xhtml']"
+ "/*[local-name()='body'][namespace-uri()='http://www.w3.org/1999/xhtml']"
, this, null, XPathResult.FIRST_ORDERED_NODE_TYPE, null
).singleNodeValue;
}
, set: function(replacement) {
var body = this.body;
body.parentElement.replaceChild(replacement, body);
}
});
Reporter | ||
Updated•9 years ago
|
Summary: document.body implemented on incorrect interface → document.body implemented on the incorrect interface
Reporter | ||
Comment 1•9 years ago
|
||
Oh sorry, incorrect wording for "actual results".
s/The examples above return false/Firefox throws an error/
Updated•9 years ago
|
Component: Untriaged → DOM
Product: Firefox → Core
Comment 2•9 years ago
|
||
Of course we're not trying to implement W3C HTML5 spec, but sure, WhatWG spec has the same.
However the whole setup for different *Document interfaces is unclear - I mean it is unclear whether the HTML spec is web compatible and/or whether it is something we want to have.
But in this particular case, looks like we could move .body to Document. Need to then make sure the value of .body is calculated the same way in different browsers in all the special case, like
what if the document is an SVG document, or some random XML document etc.
Whiteboard: [tw-dom]
Reporter | ||
Comment 3•9 years ago
|
||
Chrome and Safari implement it equivalently to my workaround, essentially being the XPath 2.0 query "/html:html/html:body". IE and Edge implement it similar to the XPath 2.0 query "//html:body[namespace-uri(root())='http://www.w3.org/1999/xhtml']" (meaning the first <body> that appears anywhere in a document with the root node in the HTML namespace, including when the root node is the body element itself)
For example, the following examples will return `null` in all browsers other than Firefox:
(new DOMParser).parseFromString("<html><body/></html>","application/xml").body // null (correct)
(new DOMParser).parseFromString("<svg xmlns='http://www.w3.org/2000/svg'><foreignObject><html xmlns='http://www.w3.org/1999/xhtml'><body/></html></foreignObject></svg>","application/xhtml+xml").body // null (correct)
I think there are a couple other things like document.head that should also be moved/ported to Document.
Oddly enough, Firefox treats application/xhtml+xml documents as HTMLDocuments when navigated to in the browser (so I can use document.body in a XHTML5 document, but only when navigated to), but treats them as Documents when parsed through DOMParser. Is that a bug or intentional behavior? If so, I'll file a separate issue for that.
Updated•8 years ago
|
Whiteboard: [tw-dom] → [tw-dom] btpp-backlog
Assignee | ||
Updated•7 years ago
|
Assignee | ||
Comment 5•7 years ago
|
||
Attachment #8945687 -
Flags: review?(nika)
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → bzbarsky
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Assignee | ||
Comment 6•7 years ago
|
||
Attachment #8945688 -
Flags: review?(nika)
Assignee | ||
Comment 7•7 years ago
|
||
There are two changes here:
1) We allow setting .body even if the root element is not an <html:html>. This
is what the spec says to do, and what we used to do before the changes in bug
366200. No tests for this yet, pending
https://github.com/whatwg/html/issues/3403 getting resolved.
2) We use GetBody(), not GetBodyElement(), to look for an existing thing to
replace. This matters if there are <frameset>s involved.
Attachment #8945689 -
Flags: review?(nika)
Assignee | ||
Comment 8•7 years ago
|
||
The "body" part of responsexml-document-properties.htm is not really per current
spec text, and fails in every non-Firefox browser, and in Firefox after this
change. https://github.com/w3c/web-platform-tests/issues/2668 tracks this issue
to some extent, but if all browsers are going to align here anyway, we should
just adjust the test and move on.
Attachment #8945690 -
Flags: review?(nika)
Assignee | ||
Comment 9•7 years ago
|
||
Attachment #8945691 -
Flags: review?(nika)
Assignee | ||
Comment 10•7 years ago
|
||
Attachment #8945692 -
Flags: review?(nika)
Assignee | ||
Comment 11•7 years ago
|
||
Attachment #8945693 -
Flags: review?(nika)
Assignee | ||
Comment 12•7 years ago
|
||
> No tests for this yet, pending https://github.com/whatwg/html/issues/3403 getting resolved.
Tests are being added in https://github.com/w3c/web-platform-tests/pull/9231
Updated•7 years ago
|
Attachment #8945687 -
Flags: review?(nika) → review+
Updated•7 years ago
|
Attachment #8945688 -
Flags: review?(nika) → review+
Updated•7 years ago
|
Attachment #8945689 -
Flags: review?(nika) → review+
Updated•7 years ago
|
Attachment #8945690 -
Flags: review?(nika) → review+
Updated•7 years ago
|
Attachment #8945691 -
Flags: review?(nika) → review+
Comment 13•7 years ago
|
||
Comment on attachment 8945692 [details] [diff] [review]
part 6. Stop using nsIDOMHTMLDocument::GetBody
Review of attachment 8945692 [details] [diff] [review]:
-----------------------------------------------------------------
::: toolkit/components/find/nsWebBrowserFind.cpp
@@ +433,5 @@
> +
> + if (doc->IsHTMLOrXHTML()) {
> + Element* body = doc->GetBody();
> + NS_ENSURE_ARG_POINTER(body);
> + NS_ADDREF(*aNode = body->AsDOMNode());
This is kinda gross.
Kinda feels like we should have something like do_AddRef(aNode, body->AsDOMNode()); which is short for *aNode = do_AddRef(body->AsDOMNode()).take();
That's definitely beyond the scope of this patch though ^.^
Attachment #8945692 -
Flags: review?(nika) → review+
Comment 14•7 years ago
|
||
Comment on attachment 8945693 [details] [diff] [review]
part 7. Get rid of nsIDOMHTMLDocument::GetBody
Review of attachment 8945693 [details] [diff] [review]:
-----------------------------------------------------------------
beautiful ^.^
Attachment #8945693 -
Flags: review?(nika) → review+
Assignee | ||
Comment 15•7 years ago
|
||
> This is kinda gross.
I agree. We should drop nsIDOMNode use from nsWebBrowserFind, ideally. ;)
Comment 16•7 years ago
|
||
Pushed by bzbarsky@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e8d6e35f38a7
part 1. Move the implementation of the .body getter from nsHTMLDocument to nsIDocument. r=mystor
https://hg.mozilla.org/integration/mozilla-inbound/rev/7f8a7edd3785
part 2. Move the implementation of the .body setter from nsHTMLDocument to nsIDocument. r=mystor
https://hg.mozilla.org/integration/mozilla-inbound/rev/82958e09a8c6
part 3. Align the .body setter with the spec a bit better. r=mystor
https://hg.mozilla.org/integration/mozilla-inbound/rev/d6b890b102b7
part 4. Move the .body getter from HTMLDocument to Document. r=mystor
https://hg.mozilla.org/integration/mozilla-inbound/rev/ca1495aea976
part 5. Get rid of IsCurrentBodyElement. r=mystor
https://hg.mozilla.org/integration/mozilla-inbound/rev/c5f9de9ad886
part 6. Stop using nsIDOMHTMLDocument::GetBody. r=mystor
https://hg.mozilla.org/integration/mozilla-inbound/rev/8e795b1484eb
part 7. Get rid of nsIDOMHTMLDocument::GetBody. r=mystor
Comment 17•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/e8d6e35f38a7
https://hg.mozilla.org/mozilla-central/rev/7f8a7edd3785
https://hg.mozilla.org/mozilla-central/rev/82958e09a8c6
https://hg.mozilla.org/mozilla-central/rev/d6b890b102b7
https://hg.mozilla.org/mozilla-central/rev/ca1495aea976
https://hg.mozilla.org/mozilla-central/rev/c5f9de9ad886
https://hg.mozilla.org/mozilla-central/rev/8e795b1484eb
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox60:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
Updated•7 years ago
|
Keywords: dev-doc-needed
Comment 18•7 years ago
|
||
I've updated the docs to make it clear the is implemented on Document:
https://developer.mozilla.org/en-US/docs/Web/API/Document#Properties
https://developer.mozilla.org/en-US/docs/Web/API/Document/body#Browser_compatibility
And added a note to the Fx60 rel notes:
https://developer.mozilla.org/en-US/Firefox/Releases/60#DOM
Let me know if that's OK. Thanks!
Flags: needinfo?(bzbarsky)
Keywords: dev-doc-needed → dev-doc-complete
Assignee | ||
Comment 19•7 years ago
|
||
For what it's worth, document.body can return a <frameset>, not just a <body>. The docs at https://developer.mozilla.org/en-US/docs/Web/API/Document#Properties only mention the latter; not sure whether this is on purpose.
I fixed the note on the compat table to be a bit clearer about what behavior actually changed.
Thank you for the documentation updates!
Flags: needinfo?(bzbarsky)
Comment 20•7 years ago
|
||
(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #19)
> For what it's worth, document.body can return a <frameset>, not just a
> <body>. The docs at
> https://developer.mozilla.org/en-US/docs/Web/API/Document#Properties only
> mention the latter; not sure whether this is on purpose.
That's a good point. I've updated it to match the description on the actual property page.
> I fixed the note on the compat table to be a bit clearer about what behavior
> actually changed.
>
> Thank you for the documentation updates!
And thanks for your help, much appreciated.
Updated•6 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•