Extract "fullscreen element ready check" from requestFullscreen()

Extract "fullscreen element ready check" from requestFullscreen()

http://fullscreen.spec.whatwg.org/#fullscreen-element-ready-check

This is a change in behavior for what was previously "a descendant
browsing context's document has a non-empty fullscreen element stack,"
now "element has no ancestor element whose local name is iframe and
namespace is the HTML namespace."

It's now never possible to go fullscreen with a descendant of an iframe,
whereas previously it would be possible as long as that iframe's
document had an empty fullscreen element stack.

Since the difference is very unlikely to affect any real content, the
LegacyFullScreenErrorExemption was dropped for this case. There was
apparently no test coverage for that exemption.

BUG=402376
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=180162

[philipj_slow, 2014-08-12 19:08:44]

PTAL. Since I'm taking small steps the spec comments are now a mix of old and
new, but in the end it'll be awesome. The elementReady() check will also be used
in the async section, per spec.

[falken, 2014-08-13 04:26:57]

I support following the spec, but also wonder if some real content makes
elements in iframes go fullscreen. For example, this person:
http://bugs.jquery.com/ticket/14903
http://christophercurrie.github.io/technology/2014/03/20/internet-explorer-11...

You said it's very unlikely to affect real content. What do you base that on?

https://codereview.chromium.org/462103002/diff/1/LayoutTests/fullscreen/api/e...
File
LayoutTests/fullscreen/api/element-ready-check-fullscreen-element-sibling.html
(right):

https://codereview.chromium.org/462103002/diff/1/LayoutTests/fullscreen/api/e...
LayoutTests/fullscreen/api/element-ready-check-fullscreen-element-sibling.html:17:
trusted_request(b);
Should the onfullscreen* be set before the request?

https://codereview.chromium.org/462103002/diff/1/LayoutTests/fullscreen/api/e...
File LayoutTests/fullscreen/api/element-ready-check-fullscreen-iframe-child.html
(right):

https://codereview.chromium.org/462103002/diff/1/LayoutTests/fullscreen/api/e...
LayoutTests/fullscreen/api/element-ready-check-fullscreen-iframe-child.html:18:
iframe.appendChild(div);
I might not understand iframes well enough. Why do you appendChild to iframe vs
iframe.contentDocument.body? If it's appended to body, can it go fullscreen?

https://codereview.chromium.org/462103002/diff/1/LayoutTests/fullscreen/api/e...
LayoutTests/fullscreen/api/element-ready-check-fullscreen-iframe-child.html:20:
assert_equals(div.ownerDocument, document, "FIXME bla blass");
FIXMEs :)

[philipj_slow, 2014-08-13 07:38:36]

https://codereview.chromium.org/462103002/diff/1/LayoutTests/fullscreen/api/e...
File
LayoutTests/fullscreen/api/element-ready-check-fullscreen-element-sibling.html
(right):

https://codereview.chromium.org/462103002/diff/1/LayoutTests/fullscreen/api/e...
LayoutTests/fullscreen/api/element-ready-check-fullscreen-element-sibling.html:17:
trusted_request(b);
On 2014/08/13 04:26:57, falken wrote:
> Should the onfullscreen* be set before the request?

It can be, but when the event is async I'm in the habit of adding the event
listener as late as possible, to fail in case the event is actually sync. Presto
had bugs like this and I've found them in Gecko too:
https://bugzilla.mozilla.org/show_bug.cgi?id=975270

https://codereview.chromium.org/462103002/diff/1/LayoutTests/fullscreen/api/e...
File LayoutTests/fullscreen/api/element-ready-check-fullscreen-iframe-child.html
(right):

https://codereview.chromium.org/462103002/diff/1/LayoutTests/fullscreen/api/e...
LayoutTests/fullscreen/api/element-ready-check-fullscreen-iframe-child.html:18:
iframe.appendChild(div);
On 2014/08/13 04:26:57, falken wrote:
> I might not understand iframes well enough. Why do you appendChild to iframe
vs
> iframe.contentDocument.body? If it's appended to body, can it go fullscreen?

This is the core of our misunderstanding :) The spec change and this test is
about a child of the iframe element *in the same document*, not anything in the
iframe's document. The test was originally <iframe><div></div></iframe>, but the
HTML parser turns the children of <iframe> into a text node "<div></div>". In
order to trigger the case, you'd need to use an XHTML parser or insert children
using scripts, as I've done. The children won't be visible.

Can I phrase the commit message differently to make this clearer?

https://codereview.chromium.org/462103002/diff/1/LayoutTests/fullscreen/api/e...
LayoutTests/fullscreen/api/element-ready-check-fullscreen-iframe-child.html:20:
assert_equals(div.ownerDocument, document, "FIXME bla blass");
On 2014/08/13 04:26:57, falken wrote:
> FIXMEs :)

Will remove, they're paranoid checks I added while struggling to understand why
my test wasn't working. Turned out that trusted_event was using the wrong
document and using iframe.contentDocument.body as the container fixes that, as
well as manual testing.

[falken, 2014-08-13 07:49:40]

https://codereview.chromium.org/462103002/diff/1/LayoutTests/fullscreen/api/e...
File
LayoutTests/fullscreen/api/element-ready-check-fullscreen-element-sibling.html
(right):

https://codereview.chromium.org/462103002/diff/1/LayoutTests/fullscreen/api/e...
LayoutTests/fullscreen/api/element-ready-check-fullscreen-element-sibling.html:17:
trusted_request(b);
On 2014/08/13 07:38:36, philipj wrote:
> On 2014/08/13 04:26:57, falken wrote:
> > Should the onfullscreen* be set before the request?
> 
> It can be, but when the event is async I'm in the habit of adding the event
> listener as late as possible, to fail in case the event is actually sync.
Presto
> had bugs like this and I've found them in Gecko too:
> https://bugzilla.mozilla.org/show_bug.cgi?id=975270

Ah, interesting! Thanks.

https://codereview.chromium.org/462103002/diff/1/LayoutTests/fullscreen/api/e...
File LayoutTests/fullscreen/api/element-ready-check-fullscreen-iframe-child.html
(right):

https://codereview.chromium.org/462103002/diff/1/LayoutTests/fullscreen/api/e...
LayoutTests/fullscreen/api/element-ready-check-fullscreen-iframe-child.html:18:
iframe.appendChild(div);
On 2014/08/13 07:38:36, philipj wrote:
> On 2014/08/13 04:26:57, falken wrote:
> > I might not understand iframes well enough. Why do you appendChild to iframe
> vs
> > iframe.contentDocument.body? If it's appended to body, can it go fullscreen?
> 
> This is the core of our misunderstanding :) The spec change and this test is
> about a child of the iframe element *in the same document*, not anything in
the
> iframe's document. The test was originally <iframe><div></div></iframe>, but
the
> HTML parser turns the children of <iframe> into a text node "<div></div>". In
> order to trigger the case, you'd need to use an XHTML parser or insert
children
> using scripts, as I've done. The children won't be visible.
> 
> Can I phrase the commit message differently to make this clearer?

Ah, I somewhat suspected that. So wrt the spec text "element has no ancestor
element whose local name is iframe", an element in an iframe's document is
not considered a descendant of the <iframe> element itself? i.e., its root
is the iframe document?

[philipj_slow, 2014-08-13 08:00:15]

fix test

[philipj_slow, 2014-08-13 08:12:14]

https://codereview.chromium.org/462103002/diff/1/LayoutTests/fullscreen/api/e...
File LayoutTests/fullscreen/api/element-ready-check-fullscreen-iframe-child.html
(right):

https://codereview.chromium.org/462103002/diff/1/LayoutTests/fullscreen/api/e...
LayoutTests/fullscreen/api/element-ready-check-fullscreen-iframe-child.html:18:
iframe.appendChild(div);
On 2014/08/13 07:49:40, falken wrote:
> On 2014/08/13 07:38:36, philipj wrote:
> > On 2014/08/13 04:26:57, falken wrote:
> > > I might not understand iframes well enough. Why do you appendChild to
iframe
> > vs
> > > iframe.contentDocument.body? If it's appended to body, can it go
fullscreen?
> > 
> > This is the core of our misunderstanding :) The spec change and this test is
> > about a child of the iframe element *in the same document*, not anything in
> the
> > iframe's document. The test was originally <iframe><div></div></iframe>, but
> the
> > HTML parser turns the children of <iframe> into a text node "<div></div>".
In
> > order to trigger the case, you'd need to use an XHTML parser or insert
> children
> > using scripts, as I've done. The children won't be visible.
> > 
> > Can I phrase the commit message differently to make this clearer?
> 
> Ah, I somewhat suspected that. So wrt the spec text "element has no ancestor
> element whose local name is iframe", an element in an iframe's document is
> not considered a descendant of the <iframe> element itself? i.e., its root
> is the iframe document?

Right, if you click through to the definition of "ancestor" in the spec, you'll
get to the DOM spec where it's clear that it's all talking about DOM trees, not
anything crossing document boundaries. If you have an element in an iframe and
follow parentNode up, you'll end up at the iframe's document.

[falken, 2014-08-13 08:45:15]

LGTM!

https://codereview.chromium.org/462103002/diff/1/LayoutTests/fullscreen/api/e...
File LayoutTests/fullscreen/api/element-ready-check-fullscreen-iframe-child.html
(right):

https://codereview.chromium.org/462103002/diff/1/LayoutTests/fullscreen/api/e...
LayoutTests/fullscreen/api/element-ready-check-fullscreen-iframe-child.html:18:
iframe.appendChild(div);
On 2014/08/13 08:12:14, philipj wrote:
> On 2014/08/13 07:49:40, falken wrote:
> > On 2014/08/13 07:38:36, philipj wrote:
> > > On 2014/08/13 04:26:57, falken wrote:
> > > > I might not understand iframes well enough. Why do you appendChild to
> iframe
> > > vs
> > > > iframe.contentDocument.body? If it's appended to body, can it go
> fullscreen?
> > > 
> > > This is the core of our misunderstanding :) The spec change and this test
is
> > > about a child of the iframe element *in the same document*, not anything
in
> > the
> > > iframe's document. The test was originally <iframe><div></div></iframe>,
but
> > the
> > > HTML parser turns the children of <iframe> into a text node "<div></div>".
> In
> > > order to trigger the case, you'd need to use an XHTML parser or insert
> > children
> > > using scripts, as I've done. The children won't be visible.
> > > 
> > > Can I phrase the commit message differently to make this clearer?
> > 
> > Ah, I somewhat suspected that. So wrt the spec text "element has no ancestor
> > element whose local name is iframe", an element in an iframe's document is
> > not considered a descendant of the <iframe> element itself? i.e., its root
> > is the iframe document?
> 
> Right, if you click through to the definition of "ancestor" in the spec,
you'll
> get to the DOM spec where it's clear that it's all talking about DOM trees,
not
> anything crossing document boundaries. If you have an element in an iframe and
> follow parentNode up, you'll end up at the iframe's document.

OK, I get it now. I actually had clicked through but it wasn't obvious to me.

[philipj_slow, 2014-08-13 08:47:15]

The CQ bit was checked by philipj@opera.com

[commit-bot: I haz the power, 2014-08-13 08:47:51]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/philipj@opera.com/462103002/20001

[commit-bot: I haz the power, 2014-08-13 09:49:34]

Change committed as 180162