FullScreen: Handle entering full screen security restrictions
Full screen mode should only be activated when handling a user gesture; Full screen mode should not be allowed within IFRAME elements unless they have been marked as "allowfullscreen".
Created attachment 85610 [details] Patch
Somehow, I lost most of my comments in the WebCore/ChangeLog. I'll update the file and repost the patch.
Created attachment 85611 [details] Patch
Comment on attachment 85611 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=85611&action=review > LayoutTests/fullscreen/full-screen-iframe-allowed.html:22 > + runWithKeyDown(function() { > + > + setTimeout(function() { > + consoleWrite("FAIL - did not enter full screen!"); > + endTest(); > + }, 50); > + > + frame.contentDocument.documentElement.webkitRequestFullScreen(); > + }); This makes it look like the 'webkitfullscreenchange' event is posted synchronously, but the Mozilla spec says it should be asynch: "When a Document enters or leaves the fullscreen state, the user agent must queue a task to dispatch this event." If the event is being posted aynchronously, using a timeout to detect success/failure will lead to problems as it is timing dependent. > Source/WebCore/html/HTMLAttributeNames.in:302 > webkitdirectory > +webkitallowfullscreen Should these be sorted? > Source/WebCore/html/HTMLFrameElementBase.cpp:60 > +#if ENABLE(FULLSCREEN_API) > + , m_allowFullScreen(false) > +#endif See below. > Source/WebCore/html/HTMLFrameElementBase.cpp:154 > +#if ENABLE(FULLSCREEN_API) > + } else if (attr->name() == webkitallowfullscreenAttr) { > + // Boolean argument; the attr is irrelevant. > + m_allowFullScreen = true; > +#endif See below. > Source/WebCore/html/HTMLFrameElementBase.cpp:298 > +bool HTMLFrameElementBase::allowFullScreen() const > +{ > + return m_allowFullScreen; > +} Is it necessary to have the member variable, can't you do something like "return hasAttribute(webkitallowfullscreenAttr)" instead?
(In reply to comment #5) > (From update of attachment 85611 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=85611&action=review > > > LayoutTests/fullscreen/full-screen-iframe-allowed.html:22 > > + runWithKeyDown(function() { > > + > > + setTimeout(function() { > > + consoleWrite("FAIL - did not enter full screen!"); > > + endTest(); > > + }, 50); > > + > > + frame.contentDocument.documentElement.webkitRequestFullScreen(); > > + }); > > This makes it look like the 'webkitfullscreenchange' event is posted synchronously, but the Mozilla spec says it should be asynch: "When a Document enters or leaves the fullscreen state, the user agent must queue a task to dispatch this event." If the event is being posted aynchronously, using a timeout to detect success/failure will lead to problems as it is timing dependent. This is one of the problems with the spec. To determine whether entering full screen failed, one must wait indefinitely, as there is no error event. Unfortunately, I don't see another way to determine the failure mode besides a timeout. But, in the case of DumpRenderTree, we should have pretty dependable, reproducible behavior here. > > > Source/WebCore/html/HTMLAttributeNames.in:302 > > webkitdirectory > > +webkitallowfullscreen > > Should these be sorted? Probably. :) > > Source/WebCore/html/HTMLFrameElementBase.cpp:60 > > +#if ENABLE(FULLSCREEN_API) > > + , m_allowFullScreen(false) > > +#endif > > See below. > > > Source/WebCore/html/HTMLFrameElementBase.cpp:154 > > +#if ENABLE(FULLSCREEN_API) > > + } else if (attr->name() == webkitallowfullscreenAttr) { > > + // Boolean argument; the attr is irrelevant. > > + m_allowFullScreen = true; > > +#endif > > See below. > > > Source/WebCore/html/HTMLFrameElementBase.cpp:298 > > +bool HTMLFrameElementBase::allowFullScreen() const > > +{ > > + return m_allowFullScreen; > > +} > > Is it necessary to have the member variable, can't you do something like "return hasAttribute(webkitallowfullscreenAttr)" instead? Sounds reasonable. Changed.
Created attachment 85644 [details] Patch
(In reply to comment #6) > (In reply to comment #5) > > (From update of attachment 85611 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=85611&action=review > > > > > LayoutTests/fullscreen/full-screen-iframe-allowed.html:22 > > > + runWithKeyDown(function() { > > > + > > > + setTimeout(function() { > > > + consoleWrite("FAIL - did not enter full screen!"); > > > + endTest(); > > > + }, 50); > > > + > > > + frame.contentDocument.documentElement.webkitRequestFullScreen(); > > > + }); > > > > This makes it look like the 'webkitfullscreenchange' event is posted synchronously, but the Mozilla spec says it should be asynch: "When a Document enters or leaves the fullscreen state, the user agent must queue a task to dispatch this event." If the event is being posted aynchronously, using a timeout to detect success/failure will lead to problems as it is timing dependent. > > This is one of the problems with the spec. To determine whether entering full screen failed, one must wait indefinitely, as there is no error event. Unfortunately, I don't see another way to determine the failure mode besides a timeout. > I assume you have, or will, give this feedback to roc? ;-)
(In reply to comment #8) > I assume you have, or will, give this feedback to roc? ;-) Of course. :-D
Committed r81038: <http://trac.webkit.org/changeset/81038>