Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
426593010:
Set fullscreenEnabled to false when fullscreen is not supported. (Closed)
|
Created:
6 years, 4 months ago by Ignacio Solla Modified:
6 years, 4 months ago CC:
blink-reviews, blink-reviews-dom_chromium.org, dglazkov+blink, sof, eae+blinkwatch, rwlbuis, aelias_OOO_until_Jul13, qinmin Base URL:
https://chromium.googlesource.com/chromium/blink.git@master Project:
blink Visibility:
Public. |
DescriptionSet fullscreenEnabled to false when fullscreen is not supported.
According to the latest version of the living standard,
fullscreenEnabled must be false if fullscreen is not supported:
http://fullscreen.spec.whatwg.org/
More details about the latest update to the standard:
https://www.w3.org/Bugs/Public/show_bug.cgi?id=26479
https://github.com/whatwg/fullscreen/commit/d35a070d443632d0071b49318e40092667ac9801
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=179520
Patch Set 1 #
Total comments: 5
Patch Set 2 : Address review comments #
Total comments: 2
Patch Set 3 : Update test #Patch Set 4 : Rebase #Patch Set 5 : Fix mergetool auto updates during rebase #Patch Set 6 : Rebase #Patch Set 7 : Rebase against tot #
Messages
Total messages: 39 (0 generated)
philipj@opera.com: Please review changes in FullscreenElementStack dglazkov@chromium.org: Please review changes in LayoutTests
I have to run, but I'll take a look on Monday. Add falken if you want fullscreen review today.
I have to run, but I'll take a look on Monday. Add falken if you want fullscreen review today.
LGTM % nits. There's also HTMLVideoElement.webkitSupportsFullscreen, but since that doesn't take the allowfullscreen attributes into consideration either I think maybe it's best left alone until it can be removed. https://codereview.chromium.org/426593010/diff/1/LayoutTests/fullscreen/full-... File LayoutTests/fullscreen/full-screen-not-enabled-when-unsupported.html (right): https://codereview.chromium.org/426593010/diff/1/LayoutTests/fullscreen/full-... LayoutTests/fullscreen/full-screen-not-enabled-when-unsupported.html:10: var iframe = document.documentElement.appendChild(document.createElement('iframe')); You don't need an iframe to test this, just testing document.webkitFullscreenEnabled would suffice. https://codereview.chromium.org/426593010/diff/1/Source/core/dom/FullscreenEl... File Source/core/dom/FullscreenElementStack.cpp (right): https://codereview.chromium.org/426593010/diff/1/Source/core/dom/FullscreenEl... Source/core/dom/FullscreenElementStack.cpp:60: static bool fullscreenIsSupported(const Document& document, const Element* element) I think this would be slightly nicer with separate fullscreenIsSupported(Document&) and fullscreenIsSupported(Element&), where the latter uses the former. Less conversion between pointers and references, at least.
And also renamed fullscreenEnabled to fullscreenSupported https://codereview.chromium.org/426593010/diff/1/LayoutTests/fullscreen/full-... File LayoutTests/fullscreen/full-screen-not-enabled-when-unsupported.html (right): https://codereview.chromium.org/426593010/diff/1/LayoutTests/fullscreen/full-... LayoutTests/fullscreen/full-screen-not-enabled-when-unsupported.html:10: var iframe = document.documentElement.appendChild(document.createElement('iframe')); On 2014/08/04 09:24:13, philipj wrote: > You don't need an iframe to test this, just testing > document.webkitFullscreenEnabled would suffice. I need it, 'allowfullscreen' is not a property of document or document.documentElement. It is only a property of iframe: https://developer.mozilla.org/en-US/docs/Web/API/document https://developer.mozilla.org/en-US/docs/Web/API/document.documentElement https://developer.mozilla.org/en/docs/Web/HTML/Element/iframe https://codereview.chromium.org/426593010/diff/1/Source/core/dom/FullscreenEl... File Source/core/dom/FullscreenElementStack.cpp (right): https://codereview.chromium.org/426593010/diff/1/Source/core/dom/FullscreenEl... Source/core/dom/FullscreenElementStack.cpp:60: static bool fullscreenIsSupported(const Document& document, const Element* element) On 2014/08/04 09:24:13, philipj wrote: > I think this would be slightly nicer with separate > fullscreenIsSupported(Document&) and fullscreenIsSupported(Element&), where the > latter uses the former. Less conversion between pointers and references, at > least. Done.
https://codereview.chromium.org/426593010/diff/1/LayoutTests/fullscreen/full-... File LayoutTests/fullscreen/full-screen-not-enabled-when-unsupported.html (right): https://codereview.chromium.org/426593010/diff/1/LayoutTests/fullscreen/full-... LayoutTests/fullscreen/full-screen-not-enabled-when-unsupported.html:10: var iframe = document.documentElement.appendChild(document.createElement('iframe')); On 2014/08/04 11:43:33, Ignacio Solla wrote: > On 2014/08/04 09:24:13, philipj wrote: > > You don't need an iframe to test this, just testing > > document.webkitFullscreenEnabled would suffice. > > I need it, 'allowfullscreen' is not a property of document or > document.documentElement. It is only a property of iframe: > https://developer.mozilla.org/en-US/docs/Web/API/document > https://developer.mozilla.org/en-US/docs/Web/API/document.documentElement > https://developer.mozilla.org/en/docs/Web/HTML/Element/iframe For a top-level document, webkitFullscreenEnabled will be true if the fullscreenSupported setting is true and false otherwise, so just testExpected('document.webkitFullscreenEnabled', false) should suffice and is the most direct way to probe the setting. Also testing the iframe case doesn't hurt, of course.
https://codereview.chromium.org/426593010/diff/20001/LayoutTests/fullscreen/f... File LayoutTests/fullscreen/full-screen-not-enabled-when-unsupported.html (right): https://codereview.chromium.org/426593010/diff/20001/LayoutTests/fullscreen/f... LayoutTests/fullscreen/full-screen-not-enabled-when-unsupported.html:2: <div>This tests that the <code>fullscreenSupported</code> property is false when fullscreen Oops, this should say webkitFullscreenEnabled I guess.
On 2014/08/04 12:18:51, philipj wrote: > https://codereview.chromium.org/426593010/diff/1/LayoutTests/fullscreen/full-... > File LayoutTests/fullscreen/full-screen-not-enabled-when-unsupported.html > (right): > > https://codereview.chromium.org/426593010/diff/1/LayoutTests/fullscreen/full-... > LayoutTests/fullscreen/full-screen-not-enabled-when-unsupported.html:10: var > iframe = document.documentElement.appendChild(document.createElement('iframe')); > On 2014/08/04 11:43:33, Ignacio Solla wrote: > > On 2014/08/04 09:24:13, philipj wrote: > > > You don't need an iframe to test this, just testing > > > document.webkitFullscreenEnabled would suffice. > > > > I need it, 'allowfullscreen' is not a property of document or > > document.documentElement. It is only a property of iframe: > > https://developer.mozilla.org/en-US/docs/Web/API/document > > https://developer.mozilla.org/en-US/docs/Web/API/document.documentElement > > https://developer.mozilla.org/en/docs/Web/HTML/Element/iframe > > For a top-level document, webkitFullscreenEnabled will be true if the > fullscreenSupported setting is true and false otherwise, so just > testExpected('document.webkitFullscreenEnabled', false) should suffice and is > the most direct way to probe the setting. > > Also testing the iframe case doesn't hurt, of course. Oh, I see. Changed now.
https://codereview.chromium.org/426593010/diff/20001/LayoutTests/fullscreen/f... File LayoutTests/fullscreen/full-screen-not-enabled-when-unsupported.html (right): https://codereview.chromium.org/426593010/diff/20001/LayoutTests/fullscreen/f... LayoutTests/fullscreen/full-screen-not-enabled-when-unsupported.html:2: <div>This tests that the <code>fullscreenSupported</code> property is false when fullscreen On 2014/08/04 12:23:01, philipj wrote: > Oops, this should say webkitFullscreenEnabled I guess. Done.
LGTM, I hope this can land ASAP since I have conflicting patches brewing :)
On 2014/08/04 12:35:14, philipj wrote: > LGTM, I hope this can land ASAP since I have conflicting patches brewing :) I'll try my best. As soon as https://codereview.chromium.org/428633004/ lands, I'll send this patch to the cq.
On 2014/08/04 12:36:43, Ignacio Solla wrote: > On 2014/08/04 12:35:14, philipj wrote: > > LGTM, I hope this can land ASAP since I have conflicting patches brewing :) > > I'll try my best. As soon as https://codereview.chromium.org/428633004/ lands, > I'll send this patch to the cq. Sounds good, thanks!
The CQ bit was checked by igsolla@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/igsolla@chromium.org/426593010/40001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: linux_blink_dbg on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/linux_blink_dbg/builds/1...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_blink_dbg on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/linux_blink_dbg/builds/1...)
The CQ bit was checked by igsolla@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/igsolla@chromium.org/426593010/40001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: linux_blink_dbg on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/linux_blink_dbg/builds/1...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_blink_dbg on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/linux_blink_dbg/builds/1...)
The CQ bit was checked by igsolla@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/igsolla@chromium.org/426593010/80001
The CQ bit was unchecked by commit-bot@chromium.org
Failed to apply patch for Source/core/dom/FullscreenElementStack.cpp:
While running patch -p1 --forward --force --no-backup-if-mismatch;
patching file Source/core/dom/FullscreenElementStack.cpp
Hunk #1 FAILED at 33.
Hunk #2 succeeded at 57 (offset 2 lines).
Hunk #3 FAILED at 225.
Hunk #4 succeeded at 391 (offset 6 lines).
2 out of 4 hunks FAILED -- saving rejects to file
Source/core/dom/FullscreenElementStack.cpp.rej
Patch: Source/core/dom/FullscreenElementStack.cpp
Index: Source/core/dom/FullscreenElementStack.cpp
diff --git a/Source/core/dom/FullscreenElementStack.cpp
b/Source/core/dom/FullscreenElementStack.cpp
index
adde493d559368f461fe6c102909fc8af48ea9fc..1e90d92b3cd4f5e9ee6201d0cdcf689628398cae
100644
--- a/Source/core/dom/FullscreenElementStack.cpp
+++ b/Source/core/dom/FullscreenElementStack.cpp
@@ -33,8 +33,10 @@
#include "core/events/Event.h"
#include "core/frame/FrameHost.h"
#include "core/frame/LocalFrame.h"
+#include "core/frame/Settings.h"
#include "core/frame/UseCounter.h"
#include "core/html/HTMLFrameOwnerElement.h"
+#include "core/html/HTMLMediaElement.h"
#include "core/page/Chrome.h"
#include "core/page/ChromeClient.h"
#include "core/rendering/RenderFullScreen.h"
@@ -55,6 +57,20 @@ static bool fullscreenIsAllowedForAllOwners(const Document&
document)
return true;
}
+static bool fullscreenIsSupported(const Document& document)
+{
+ // Fullscreen is supported if there is no previously-established user
preference,
+ // security risk, or platform limitation.
+ return document.settings()->fullscreenSupported();
+}
+
+static bool fullscreenIsSupported(const Document& document, const Element&
element)
+{
+ if (document.settings()->disallowFullscreenForNonMediaElements() &&
!isHTMLMediaElement(element))
+ return false;
+ return fullscreenIsSupported(document);
+}
+
static PassRefPtrWillBeRawPtr<Event> createEvent(const AtomicString& type,
EventTarget& target)
{
RefPtrWillBeRawPtr<Event> event = Event::createBubble(type);
@@ -211,7 +227,9 @@ void
FullscreenElementStack::requestFullScreenForElement(Element& element, Reque
if (!UserGestureIndicator::processingUserGesture())
break;
- // There is a previously-established user preference, security risk, or
platform limitation.
+ // Fullscreen is not supported.
+ if (!fullscreenIsSupported(element.document(), element))
+ break;
// 2. Let doc be element's node document. (i.e. "this")
Document* currentDoc = document();
@@ -371,11 +389,11 @@ void FullscreenElementStack::exitFullscreen()
bool FullscreenElementStack::fullscreenEnabled(Document& document)
{
- // 4. The fullscreenEnabled attribute must return true if the context
object and all ancestor
- // browsing context's documents have their fullscreen enabled flag set, or
false otherwise.
+ // 4. The fullscreenEnabled attribute must return true if the context
object has its
+ // fullscreen enabled flag set and fullscreen is supported, and false
otherwise.
// Top-level browsing contexts are implied to have their allowFullScreen
attribute set.
- return fullscreenIsAllowedForAllOwners(document);
+ return fullscreenIsAllowedForAllOwners(document) &&
fullscreenIsSupported(document);
}
void FullscreenElementStack::willEnterFullScreenForElement(Element* element)
On 2014/08/04 15:37:26, I haz the power (commit-bot) wrote:
> Failed to apply patch for Source/core/dom/FullscreenElementStack.cpp:
> While running patch -p1 --forward --force --no-backup-if-mismatch;
> patching file Source/core/dom/FullscreenElementStack.cpp
> Hunk #1 FAILED at 33.
> Hunk #2 succeeded at 57 (offset 2 lines).
> Hunk #3 FAILED at 225.
> Hunk #4 succeeded at 391 (offset 6 lines).
> 2 out of 4 hunks FAILED -- saving rejects to file
> Source/core/dom/FullscreenElementStack.cpp.rej
>
> Patch: Source/core/dom/FullscreenElementStack.cpp
> Index: Source/core/dom/FullscreenElementStack.cpp
> diff --git a/Source/core/dom/FullscreenElementStack.cpp
> b/Source/core/dom/FullscreenElementStack.cpp
> index
>
adde493d559368f461fe6c102909fc8af48ea9fc..1e90d92b3cd4f5e9ee6201d0cdcf689628398cae
> 100644
> --- a/Source/core/dom/FullscreenElementStack.cpp
> +++ b/Source/core/dom/FullscreenElementStack.cpp
> @@ -33,8 +33,10 @@
> #include "core/events/Event.h"
> #include "core/frame/FrameHost.h"
> #include "core/frame/LocalFrame.h"
> +#include "core/frame/Settings.h"
> #include "core/frame/UseCounter.h"
> #include "core/html/HTMLFrameOwnerElement.h"
> +#include "core/html/HTMLMediaElement.h"
> #include "core/page/Chrome.h"
> #include "core/page/ChromeClient.h"
> #include "core/rendering/RenderFullScreen.h"
> @@ -55,6 +57,20 @@ static bool fullscreenIsAllowedForAllOwners(const Document&
> document)
> return true;
> }
>
> +static bool fullscreenIsSupported(const Document& document)
> +{
> + // Fullscreen is supported if there is no previously-established user
> preference,
> + // security risk, or platform limitation.
> + return document.settings()->fullscreenSupported();
> +}
> +
> +static bool fullscreenIsSupported(const Document& document, const Element&
> element)
> +{
> + if (document.settings()->disallowFullscreenForNonMediaElements() &&
> !isHTMLMediaElement(element))
> + return false;
> + return fullscreenIsSupported(document);
> +}
> +
> static PassRefPtrWillBeRawPtr<Event> createEvent(const AtomicString& type,
> EventTarget& target)
> {
> RefPtrWillBeRawPtr<Event> event = Event::createBubble(type);
> @@ -211,7 +227,9 @@ void
> FullscreenElementStack::requestFullScreenForElement(Element& element, Reque
> if (!UserGestureIndicator::processingUserGesture())
> break;
>
> - // There is a previously-established user preference, security risk,
or
> platform limitation.
> + // Fullscreen is not supported.
> + if (!fullscreenIsSupported(element.document(), element))
> + break;
>
> // 2. Let doc be element's node document. (i.e. "this")
> Document* currentDoc = document();
> @@ -371,11 +389,11 @@ void FullscreenElementStack::exitFullscreen()
>
> bool FullscreenElementStack::fullscreenEnabled(Document& document)
> {
> - // 4. The fullscreenEnabled attribute must return true if the context
> object and all ancestor
> - // browsing context's documents have their fullscreen enabled flag set,
or
> false otherwise.
> + // 4. The fullscreenEnabled attribute must return true if the context
> object has its
> + // fullscreen enabled flag set and fullscreen is supported, and false
> otherwise.
>
> // Top-level browsing contexts are implied to have their allowFullScreen
> attribute set.
> - return fullscreenIsAllowedForAllOwners(document);
> + return fullscreenIsAllowedForAllOwners(document) &&
> fullscreenIsSupported(document);
> }
>
> void FullscreenElementStack::willEnterFullScreenForElement(Element* element)
I'm not yet able to land this. I suspect that I need to wait for a deps roll
including this change to blink: https://codereview.chromium.org/428633004/
You never have to wait for rolls inside of Blink, try simply rebasing the patch.
The CQ bit was checked by igsolla@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/igsolla@chromium.org/426593010/100001
On 2014/08/05 05:30:32, philipj wrote: > You never have to wait for rolls inside of Blink, try simply rebasing the patch. uhmm, I had tried rebasing but the previous change was not there, and commit failed with #26. How do you rebase to submit without waiting for rolls? I have just rebased again, and the previous change is there now, so hopefully it will make through this time.
On 2014/08/05 09:05:08, Ignacio Solla wrote: > On 2014/08/05 05:30:32, philipj wrote: > > You never have to wait for rolls inside of Blink, try simply rebasing the > patch. > > uhmm, I had tried rebasing but the previous change was not there, and commit > failed with #26. How do you rebase to submit without waiting for rolls? You can rebase on the master branch in Blink repository. I forgot that isn't the default setup, because I followed the "fetch blink # Blink at Tip of Tree (latest) - if you are working on Blink instead of or in addition to Chromium" bit in http://www.chromium.org/developers/how-tos/get-the-code Let's hope it works now :)
On 2014/08/05 09:13:05, philipj wrote: > On 2014/08/05 09:05:08, Ignacio Solla wrote: > > On 2014/08/05 05:30:32, philipj wrote: > > > You never have to wait for rolls inside of Blink, try simply rebasing the > > patch. > > > > uhmm, I had tried rebasing but the previous change was not there, and commit > > failed with #26. How do you rebase to submit without waiting for rolls? > > You can rebase on the master branch in Blink repository. I forgot that isn't the > default setup, because I followed the "fetch blink # Blink at Tip of Tree > (latest) - if you are working on Blink instead of or in addition to Chromium" > bit in http://www.chromium.org/developers/how-tos/get-the-code > > Let's hope it works now :) Thanks for the pointers!
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: linux_blink_dbg on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/linux_blink_dbg/builds/1...) linux_blink_rel on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/1...) win_blink_rel on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/20874)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_blink_rel on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/20924)
The CQ bit was checked by igsolla@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/igsolla@chromium.org/426593010/120001
Message was sent while issue was closed.
Change committed as 179520 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
