Bug 159490

Summary: Align Document.body setter with the HTML specification
Product: WebKit Reporter: Chris Dumez <cdumez>
Component: DOMAssignee: Chris Dumez <cdumez>
Status: RESOLVED FIXED    
Severity: Normal CC: cdumez, commit-queue, darin, esprehn+autocc, kangil.han, rniwa
Priority: P2 Keywords: WebExposed
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
URL: https://html.spec.whatwg.org/multipage/dom.html#dom-document-body
See Also: https://bugs.webkit.org/show_bug.cgi?id=159488
Bug Depends on:    
Bug Blocks: 119115    
Attachments:
Description Flags
Patch
none
Patch
none
Patch none

Description Chris Dumez 2016-07-06 14:08:03 PDT
Document.body setter should be accept a frameset element as input:
- https://html.spec.whatwg.org/multipage/dom.html#dom-document-body

We currently throw an exception when given a frameset element.

Both Firefox and Chrome match the HTML specification.
Comment 1 Chris Dumez 2016-07-06 15:10:21 PDT
Created attachment 282954 [details]
Patch
Comment 2 Chris Dumez 2016-07-06 15:18:00 PDT
Created attachment 282955 [details]
Patch
Comment 3 Alex Christensen 2016-07-06 17:10:35 PDT
Comment on attachment 282955 [details]
Patch

It seems like you should make newBody a Ref or keep the null check.
Comment 4 Chris Dumez 2016-07-06 18:31:13 PDT
(In reply to comment #3)
> Comment on attachment 282955 [details]
> Patch
> 
> It seems like you should make newBody a Ref or keep the null check.

Why?

if (!is<HTMLBodyElement>(newBody.get()) && !is<HTMLFrameSetElement>(newBody.get()))
Will be true if newBody is null and therefore we will throw a HIERARCHY_REQUEST_ERR as expected.

We cannot easily take a Ref<> because this function is used by the bindings and the type is nullable in the IDL. If we updated the type in the bindings to be non-nullable then we would no longer match the spec and we would throw a TypeError instead of a HIERARCHY_REQUEST_ERR.
Comment 5 Chris Dumez 2016-07-06 19:00:22 PDT
Created attachment 282981 [details]
Patch
Comment 6 Chris Dumez 2016-07-06 19:01:09 PDT
(In reply to comment #4)
> (In reply to comment #3)
> > Comment on attachment 282955 [details]
> > Patch
> > 
> > It seems like you should make newBody a Ref or keep the null check.
> 
> Why?
> 
> if (!is<HTMLBodyElement>(newBody.get()) &&
> !is<HTMLFrameSetElement>(newBody.get()))
> Will be true if newBody is null and therefore we will throw a
> HIERARCHY_REQUEST_ERR as expected.
> 
> We cannot easily take a Ref<> because this function is used by the bindings
> and the type is nullable in the IDL. If we updated the type in the bindings
> to be non-nullable then we would no longer match the spec and we would throw
> a TypeError instead of a HIERARCHY_REQUEST_ERR.

I added a test case to cover "document.body = null" to confirm that it still throws a HIERARCHY_REQUEST_ERR.
Comment 7 Alex Christensen 2016-07-06 20:54:06 PDT
Comment on attachment 282981 [details]
Patch

Sure enough.  I should be more familiar with is<>.  r=me
That spec is very specific, and now our code reads like the spec.
Comment 8 WebKit Commit Bot 2016-07-06 21:15:05 PDT
Comment on attachment 282981 [details]
Patch

Clearing flags on attachment: 282981

Committed r202893: <http://trac.webkit.org/changeset/202893>
Comment 9 WebKit Commit Bot 2016-07-06 21:15:10 PDT
All reviewed patches have been landed.  Closing bug.