Closed Bug 1276438 Opened 8 years ago Closed 6 years ago

document.body implemented on the incorrect interface

Categories

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

Product:
Core
Component:
DOM: Core & HTML
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla60
Tracking Flags:
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)

part 1. Move the implementation of the .body getter from nsHTMLDocument to nsIDocument
4.85 KB, patch
nika
: review+
Details | Diff | Splinter Review
part 2. Move the implementation of the .body setter from nsHTMLDocument to nsIDocument
5.15 KB, patch
nika
: review+
Details | Diff | Splinter Review
part 3. Align the .body setter with the spec a bit better
4.39 KB, patch
nika
: review+
Details | Diff | Splinter Review
part 4. Move the .body getter from HTMLDocument to Document
14.51 KB, patch
nika
: review+
Details | Diff | Splinter Review
part 5. Get rid of IsCurrentBodyElement
3.48 KB, patch
nika
: review+
Details | Diff | Splinter Review
part 6. Stop using nsIDOMHTMLDocument::GetBody
19.63 KB, patch
nika
: review+
Details | Diff | Splinter Review
part 7. Get rid of nsIDOMHTMLDocument::GetBody
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);
	}
});
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)
Keywords: dev-doc-needed → dev-doc-complete
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

Creator:
Created:
Updated:
Size: