Bug 58638 – Full Screen from within an <iframe> does not cause <iframe> to resize.

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Jer Noble]

The end result is an element which has been resized to occupy the entirety of its <iframe> but not the root document.

[Jer Noble]

Created attachment 89843 [details]
Patch

[Daniel Bates]

Comment on attachment 89843 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=89843&action=review

> Source/WebCore/dom/Document.cpp:4853
> +    Element* element = m_fullScreenElement.get();
> +    while (HTMLFrameOwnerElement* ownerElement = element->document()->ownerElement()) {
> +        if (!ownerElement->isFrameElement())
> +            continue;
> +        
> +        static_cast<HTMLFrameElementBase*>(ownerElement)->setContainsFullScreenElement(false);
> +        element = ownerElement;
> +    }

ewww, duplicate code. This code and the code in Document::webkitWillEnterFullScreenForElement() differs only by the boolean value we pass to setContainsFullScreenElement(). I suggest we extract this code into a common function, maybe called setContainsFullScreenElementOnElementAndItsOwners()? or use a similar idea to RenderObject::setNeedsLayout() and have setContainsFullScreenElement() take an optional markOwners argument.

[Daniel Bates]

(In reply to comment #2)
>[...] or use a similar idea to RenderObject::setNeedsLayout() and have setContainsFullScreenElement() take an optional markOwners argument.

I meant to write "...and have the extracted setContainsFullScreenElement() take an optional markOwners argument." That is, don't add an optional markOwners argument to HTMLFrameElementBase::setContainsFullScreenElement().

[Jer Noble]

(In reply to comment #3)
> (In reply to comment #2)
> >[...] or use a similar idea to RenderObject::setNeedsLayout() and have setContainsFullScreenElement() take an optional markOwners argument.
> 
> I meant to write "...and have the extracted setContainsFullScreenElement() take an optional markOwners argument." That is, don't add an optional markOwners argument to HTMLFrameElementBase::setContainsFullScreenElement().

Let me make sure I understand.  Modify the HTMLFrameElementBase to look like:

    virtual void setContainsFullScreenElement(bool);
    virtual void setContainsFullScreenElement(bool, bool markOwners);

or:

    virtual void setContainsFullScreenElement(bool, bool markOwners = false);

?

[Daniel Bates]

(In reply to comment #4)
> (In reply to comment #3)
> > (In reply to comment #2)
> > >[...] or use a similar idea to RenderObject::setNeedsLayout() and have setContainsFullScreenElement() take an optional markOwners argument.
> > 
> > I meant to write "...and have the extracted setContainsFullScreenElement() take an optional markOwners argument." That is, don't add an optional markOwners argument to HTMLFrameElementBase::setContainsFullScreenElement().
> 
> Let me make sure I understand.  Modify the HTMLFrameElementBase to look like:
> 
>     virtual void setContainsFullScreenElement(bool);
>     virtual void setContainsFullScreenElement(bool, bool markOwners);
> 
> or:
> 
>     virtual void setContainsFullScreenElement(bool, bool markOwners = false);
> 
> ?

You could do this, but this doesn't seem like it would make the call sites cleaner unless you can assume that m_fullScreenElement is always a HTMLFrameElementBase element or an element derived from it.

Instead, I was implying that one idea is to add a static local function to Document that has the form:

enum DoesContainFullScreenElement = {DoesNotContainFullScreenElement, ContainsFullScreenElement};
enum ShouldMarkOwnerElements = {MarkOwnerElements, DoNotMarkOwnerElements};
static void setContainsFullScreenElement(Element*, DoesContainFullScreenElement, ShouldMarkOwnerElements)
{
...
}

Then the call site in Document::webkitWillEnterFullScreenForElement() and Document::webkitDidEnterFullScreenForElement() become:

setContainsFullScreenElement(m_fullScreenElement.get(), ContainsFullScreenElement, MarkOwnerElements);

AND

setContainsFullScreenElement(m_fullScreenElement.get(), DoesNotContainFullScreenElement, MarkOwnerElements);

respectively.

Clearly, the use of enums make this a bit verbose.

[Jer Noble]

(In reply to comment #5)
> You could do this, but this doesn't seem like it would make the call sites cleaner unless you can assume that m_fullScreenElement is always a HTMLFrameElementBase element or an element derived from it.
> 
> Instead, I was implying that one idea is to add a static local function to Document that has the form:
> 
> enum DoesContainFullScreenElement = {DoesNotContainFullScreenElement, ContainsFullScreenElement};
> enum ShouldMarkOwnerElements = {MarkOwnerElements, DoNotMarkOwnerElements};
> static void setContainsFullScreenElement(Element*, DoesContainFullScreenElement, ShouldMarkOwnerElements)

Ah, this makes much more sense.  I'll implement it and submit a new patch.

[Jer Noble]

(In reply to comment #6)
> (In reply to comment #5)
> > You could do this, but this doesn't seem like it would make the call sites cleaner unless you can assume that m_fullScreenElement is always a HTMLFrameElementBase element or an element derived from it.
> > 
> > Instead, I was implying that one idea is to add a static local function to Document that has the form:
> > 
> > enum DoesContainFullScreenElement = {DoesNotContainFullScreenElement, ContainsFullScreenElement};
> > enum ShouldMarkOwnerElements = {MarkOwnerElements, DoNotMarkOwnerElements};
> > static void setContainsFullScreenElement(Element*, DoesContainFullScreenElement, ShouldMarkOwnerElements)
> 
> Ah, this makes much more sense.  I'll implement it and submit a new patch.

Actually, it seems presumptuous to add that enum that will probably never be used.  How about just a:

    static void setContainsFullScreenElementRecursively();

?

[Jer Noble]

(In reply to comment #7)
>     static void setContainsFullScreenElementRecursively();

(with parameters, natch.) ;-)

[Daniel Bates]

(In reply to comment #8)
> (In reply to comment #7)
> >     static void setContainsFullScreenElementRecursively();
> 
> (with parameters, natch.) ;-)

Sounds good.

[Jer Noble]

Created attachment 90243 [details]
Patch

[Daniel Bates]

Comment on attachment 90243 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=90243&action=review

The addition of isFrameElement() and it usage makes this patch also applicable to <frame>s (i.e frames within a frameset document). With the exception of the remarks for WebCore::CSSStyleSelector::SelectorChecker::checkOneSelector() in the change log, the change log text and bug description talk about the HTML IFrame element. I take it this patch is also applicable to frames within a frameset document? If so, as far as I can tell we don't have existing test coverage with respect to an HTML frameset document and this patch doesn't provide a test case for such a document.

> LayoutTests/fullscreen/full-screen-iframe-allowed.html:10
>              consoleWrite("SUCCEED - entered full screen!"); 

I suggest removing this line since you call test() below, which calls consoleWrite() to write an OK or FAIL result.

> Source/WebCore/ChangeLog:1
> [phone]  Jer Noble  <[e-mail]>

This change log is out of date and is missing the entry for the added function setContainsFullScreenElementRecursively().

> Source/WebCore/dom/Document.cpp:4813
> +    } while ((element = element->document()->ownerElement()));

Nit: The parentheses around the expression "element = element->document()->ownerElement()" are unnecessary. I suggest removing them.

> Source/WebCore/dom/Element.h:323
> +    virtual bool isFrameElement() const { return false; }

Note, the name of this method is a bit disingenuous since it's true whenever the element is either a <frame> or an <iframe>. Maybe isFrameOrIFrameElement()? That being said, I don't have a strong preference against the current name.

We should consider using isFrameElement() in more places throughout the code base. It's probably best to do this in a follow up bug.

[Jer Noble]

(In reply to comment #11)
> (From update of attachment 90243 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=90243&action=review
> 
> The addition of isFrameElement() and it usage makes this patch also applicable to <frame>s (i.e frames within a frameset document). With the exception of the remarks for WebCore::CSSStyleSelector::SelectorChecker::checkOneSelector() in the change log, the change log text and bug description talk about the HTML IFrame element. I take it this patch is also applicable to frames within a frameset document? If so, as far as I can tell we don't have existing test coverage with respect to an HTML frameset document and this patch doesn't provide a test case for such a document.

Yes, it should be equally applicable to a frame within a frame set.  I can add some test cases for frame sets; they'll be very simple clones of the iframe cases.

> > LayoutTests/fullscreen/full-screen-iframe-allowed.html:10
> >              consoleWrite("SUCCEED - entered full screen!"); 
> 
> I suggest removing this line since you call test() below, which calls consoleWrite() to write an OK or FAIL result.

Sure.

> > Source/WebCore/ChangeLog:1
> > [phone]  Jer Noble  <[e-mail]>
> 
> This change log is out of date and is missing the entry for the added function setContainsFullScreenElementRecursively().

You're right; I forgot to update the ChangeLog.

> > Source/WebCore/dom/Document.cpp:4813
> > +    } while ((element = element->document()->ownerElement()));
> 
> Nit: The parentheses around the expression "element = element->document()->ownerElement()" are unnecessary. I suggest removing them.

Some compliers will generate warnings when assignments happen within single parentheses.  The double-parens will avoid these warnings.

> > Source/WebCore/dom/Element.h:323
> > +    virtual bool isFrameElement() const { return false; }
> 
> Note, the name of this method is a bit disingenuous since it's true whenever the element is either a <frame> or an <iframe>. Maybe isFrameOrIFrameElement()? That being said, I don't have a strong preference against the current name.

It could also be "isFrameElementBase()", since we're using it to cast to a HTMLFrameElementBase.  Neither HTMLIFrameElement nor HTMLFrameElement are particularly useful classes from an API standpoint, so it's unlikely anyone will want to cast directly to one or the other.

> We should consider using isFrameElement() in more places throughout the code base. It's probably best to do this in a follow up bug.

Indeed.

[Jer Noble]

Created attachment 90306 [details]
Patch

[Daniel Bates]

Comment on attachment 90306 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=90306&action=review

Thanks Jer for the additional test cases. This patch looks sane to me. I'm not as familiar with the changes to WKFullScreenWindowController.mm/Core Animation but they look reasonable. If you have any doubt about this change then feel free to ask someone more familiar with this area of the code to review it.

r=me

> LayoutTests/fullscreen/full-screen-frameset-not-allowed.html:22
> +            consoleWrite("SUCCEED - did not enter full screen!"); 

What you have here is sufficient as is. That being said, could we strengthen this test case by checking that "document.getElementById('frame').contentDocument.width != document.width" here as opposed to just assuming we succeeded on timeout? I mean, outputting success here would be a false positive if the webkitfullscreenchange event isn't dispatched.

Note, if the webkitfullscreenchange event isn't dispatched then we would detect such a regression using the test case LayoutTests/fullscreen/full-screen-frameset-allowed.html (because it only succeeds when the webkitfullscreenchange event is dispatched among other criterion).

> LayoutTests/fullscreen/resources/not-allowed.html:7
> +<p>Test for <a href="http://bugs.webkit.org/show_bug.cgi?id=56264">bug 56264</a>: 
> +Handle entering full screen security restrictions</p>
> +<p>To test manually, click the "Go full screen" button - the page should not enter full screen mode.</p>
> +<div id="console"></div>
> +<script>
> +    top.console = document.getElementById('console');
> +</script>

This is sufficient as is.

If we really wanted to, we could have one file, called frame-test-description-and-console.html, that accepts a query string for the text such that we don't have to have two files, LayoutTests/fullscreen/resources/allowed.html and LayoutTests/fullscreen/resources/not-allowed.html, which are almost identical.

> Source/WebCore/ChangeLog:21
> +        (WebCore::Document::fullScreenIsAllowedForElement): Use the new Element::isFrameElement

Nit: isFrameElement() => isFrameElementBase()

> Source/WebCore/ChangeLog:27
> +        (WebCore::Element::isFrameElement): Added.

Ditto.

> Source/WebCore/ChangeLog:32
> +        (WebCore::HTMLFrameElementBase::isFrameElement): Added.

Ditto.

> Source/WebCore/css/CSSStyleSelector.cpp:2907
>                  // that element. Also, an <iframe>, <object> or <embed> element whose child browsing 

We should update this comment since this logic also applies to <frame>.

[Jer Noble]

(In reply to comment #14)
> (From update of attachment 90306 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=90306&action=review
> 
> Thanks Jer for the additional test cases. This patch looks sane to me. 

Unfortunately, in coming up with the changes you recommend above, it's become clear that entering full screen from within a frameset will not work correctly.  This method relies on resizing frame elements with CSS, and a frame within a frameset cannot be resized that way.

Since the <frameset> and <frame> element are deprecated in HTML5, I don't feel too bad about disallowing full screen from an element within a <frame> element.  I'll update my patch to reflect this.

[Jer Noble]

Created attachment 90751 [details]
Patch

[Jer Noble]

Comment on attachment 90751 [details]
Patch

Gah! webkit-patch upload is totally broken.

[Jer Noble]

Created attachment 90773 [details]
Patch

[Jer Noble]

<rdar://problem/9327313>

[Jer Noble]

Committed r84706: <http://trac.webkit.org/changeset/84706>