Closed Bug 1276438 Opened 9 years ago Closed 7 years ago

document.body implemented on the incorrect interface

Categories

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

defect
Not set
normal

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)

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); } });
Summary: document.body implemented on incorrect interface → document.body implemented on the incorrect interface
Oh sorry, incorrect wording for "actual results". s/The examples above return false/Firefox throws an error/
Component: Untriaged → DOM
Product: Firefox → Core
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]
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.
Depends on: 897815
Whiteboard: [tw-dom] → [tw-dom] btpp-backlog
Blocks: 897815
No longer depends on: 897815
Assignee: nobody → bzbarsky
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
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)
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)
Blocks: 1418076
> 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
Attachment #8945687 - Flags: review?(nika) → review+
Attachment #8945688 - Flags: review?(nika) → review+
Attachment #8945689 - Flags: review?(nika) → review+
Attachment #8945690 - Flags: review?(nika) → review+
Attachment #8945691 - Flags: review?(nika) → review+
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 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+
> This is kinda gross. I agree. We should drop nsIDOMNode use from nsWebBrowserFind, ideally. ;)
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
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)
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)
(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.
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: