Avoid use of window.fullScreen and test for its absence #28862
Conversation
The is_fullscreen helper accidentally depended on both the Gecko-only window.fullScreen and the WebKit-only document.webkitIsFullScreen, and wouldn't have worked in a browser supporting only the standard Fullscreen API. Use document.fullscreen instead, and add a negative test for window.fullScreen.
wpt-pr-bot
requested review from
aliams,
andreastt,
AutomatedTester,
mjzffr,
shs96c,
upsuper and
whimboo
|
|
||
| test(function() { | ||
| assert_false('fullScreen' in window); | ||
| }, 'window.fullScreen must not be supported'); |
There was a problem hiding this comment.
I don't quite understand why we need this test here.
window.fullScreen in Gecko is not a historical implementation of the Fullscreen API. It was dated back way earlier than Fullscreen API, and actually was there before Firefox. It was initially implemented in a bug 20 years ago to allow the browser window to go full screen, and this API probably has been leaked to the web content since then.
So I don't think this test makes sense here, as it's unrelated to the Fullscreen API.
There was a problem hiding this comment.
Do you mean it doesn't return the same value as document.fullscreen? I think https://github.com/mozilla/gecko-dev/blob/2ec947a6853ee7fca5997ff5e45bd45b6afd6218/dom/base/nsGlobalWindowInner.cpp#L3655-L3664 is the implementation but I can't figure out where it ends up really returning a value.
If it's truly uncoupled to the Fullscreen API, then I agree a negative test doesn't belong here.
There was a problem hiding this comment.
It does not return the same value as document.fullscreen.
So basically, when document.fullscreen returns true, window.fullScreen returns true as well, because the browser window is also full screen, but not vice versa. If you use F11 to make the browser window full screen on a website which doesn't use Fullscreen API, then window.fullScreen also returns true while document.fullscreen would be false.
In that sense, they are not completely uncoupled, but they are different things.
There was a problem hiding this comment.
Hmm, I wonder why this difference isn't resulting in failing tests. Or even worse, maybe the WebDriver tests depend on the Firefox behavior for window.fullScreen...
Needs investigation!
There was a problem hiding this comment.
Maybe just because nothing tried to make the browser enter full screen mode without using the Fullscreen API.
There was a problem hiding this comment.
It looks like the tests are for https://w3c.github.io/webdriver/#dfn-fullscreen-window, which is defined in terms of https://fullscreen.spec.whatwg.org/#fullscreen-an-element, fortunately it's not exercising the browser UI fullscreen feature which isn't observable to web content.
|
Bad new, this regresses the webdriver tests in Firefox, for example: My guess is that https://w3c.github.io/webdriver/#dfn-fullscreen-window in Firefox isn't implemented in terms of the Fullscreen API, so that @jgraham what do you think we should do here? Add elaborate comments and depend on |
|
I don't know how to make progress with this, so I'm abandoning it. |
|
I totally missed this one, sorry. I'll look at the gecko implementation. |
Yes, we are setting |
The is_fullscreen helper accidentally depended on both the Gecko-only
window.fullScreen and the WebKit-only document.webkitIsFullScreen, and
wouldn't have worked in a browser supporting only the standard
Fullscreen API.
Use document.fullscreen instead, and add a negative test for
window.fullScreen.