[WK2] Enable fullscreen request from UIProcess (i.e. user command)

• Component: WebKit2
• Status: RESOLVED INVALID

[Bruno Abinader (history only)]

While implementing fullscreen/window toggle (F11) in EFL's MiniBrowser, I've noticed there is no way other than using JS to request full screen view. This bug intends to implement a method to allow UIProcess (i.e. a user command flag or keyboard key associated to fullscreen toggle) to request full screen view to WebProcess, which handles the necessary user authorization request (accept/deny). The workflow works like this:

1. UIProcess generates a call to WebFullScreenManagerProxy::requestEnterFullScreen()
2. WebFullScreenManagerProxy::requestEnterFullScreen() sends a message to WebProcess' WebFullScreenManager::requestEnterFullScreen()
3. The latter calls a public function from its own class WebFullScreenManager::enterFullScreenForElement(element), with element being the core page's element document.
4. User authorization modal dialog shows as expected, with accept/deny callbacks correctly toggling fullscreen/window modes.

Besides that, the already-implemented WebProcess' WebFullScreenManager::requestExitFullScreen() has been modified to use exitEnterFullScreen() (which uses InjectedBundle's code), following code design and avoiding issues when fullscreen was called from JS (clicking on 'deny' wasn't actually triggering window mode).

[Bruno Abinader (history only)]

Created attachment 185083 [details]
Patch

Proposed patch

[Jer Noble]

Comment on attachment 185083 [details]
Patch

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

Overall, the addition of requestEnterFullScreen() looks good, but I recommend a r- due to the change to requestExitFullScreen.

> Source/WebKit2/WebProcess/FullScreen/WebFullScreenManager.cpp:147
> +void WebFullScreenManager::requestEnterFullScreen()
> +{
> +    // Default to document.documentElement().
> +    enterFullScreenForElement(m_page->corePage()->mainFrame()->document()->documentElement());
> +}
> +

LGTM.  In the future, clients may want some way to specify which element they'd like to be the full-screen one (instead of documentElement()), but this is good for now.

> Source/WebKit2/WebProcess/FullScreen/WebFullScreenManager.cpp:151
> -    m_element->document()->webkitCancelFullScreen();
> +    exitFullScreenForElement(m_element.get());

By not calling webkitCancelFullScreen(), I believe the contents of m_fullScreenElement and m_fullScreenElementStack won't be modified, which is not desirable.  If you're not getting an exitFullScreenForElement() in response, it's probably because the element wasn't in full screen to begin with, and you'll have to handle that case explicitly in the UIProcess.

[Bruno Abinader (history only)]

Created attachment 185097 [details]
Patch

Removed changes on requestExitFullScreen as reviewed by Jer Noble.

[Jer Noble]

(In reply to comment #3)
> Created an attachment (id=185097) [details]
> Patch
> 
> Removed changes on requestExitFullScreen as reviewed by Jer Noble.

Unofficial r+.

[Eric Seidel (no email)]

I'm not a webkit2 owner, you'll have to deal with them.

[Benjamin Poulain]

Comment on attachment 185097 [details]
Patch

-Not really an explanation for why that is needed.
-The fullscreen element is always the documentElement, that is not what the Fullscreen API is for.
-No test.

[Benjamin Poulain]

(In reply to comment #0)
> While implementing fullscreen/window toggle (F11) in EFL's MiniBrowser, I've noticed there is no way other than using JS to request full screen view. This bug intends to implement a method to allow UIProcess (i.e. a user command flag or keyboard key associated to fullscreen toggle) to request full screen view to WebProcess, which handles the necessary user authorization request (accept/deny). The workflow works like this:

It looks like you are mixing two concepts. The fullscreen API and the browser ability to run fullscreen.

You do not need the Web Fullscreen API to show your browser fullscreen. You can just use your native functions to take the View and put it fullscreen.

By abusing the Web Fullscreen API, you are altering the page state (the fullscreen extensions to the Document element) and you give a way for a page to get out of fullscreen.

[Bruno Abinader (history only)]

(In reply to comment #7)
> (In reply to comment #0)
> > While implementing fullscreen/window toggle (F11) in EFL's MiniBrowser, I've noticed there is no way other than using JS to request full screen view. This bug intends to implement a method to allow UIProcess (i.e. a user command flag or keyboard key associated to fullscreen toggle) to request full screen view to WebProcess, which handles the necessary user authorization request (accept/deny). The workflow works like this:
> 
> It looks like you are mixing two concepts. The fullscreen API and the browser ability to run fullscreen.
> 
> You do not need the Web Fullscreen API to show your browser fullscreen. You can just use your native functions to take the View and put it fullscreen.

I understand one can simply call the platform's native API to enter fullscreen mode, however, as Jer mentioned, the CSS ComputedStyles does not get updated when this occurs. Indeed the current implementation is buggy in a sense that the Document fullscreen elements stack is not updated, this is what I'm planning to fix on the following patch.

> 
> By abusing the Web Fullscreen API, you are altering the page state (the fullscreen extensions to the Document element) and you give a way for a page to get out of fullscreen.

Isn't the page state supposed to be changed after we toggle fullscreen mode (i.e. activate CSS :fullscreen pseudo-class)?

[Bruno Abinader (history only)]

Created attachment 185135 [details]
Patch

Now using Document::requestFullScreenForElement for proper fullscreen element stack treatment.

[Benjamin Poulain]

Comment on attachment 185135 [details]
Patch

Not addressed my concerns.
No tests.

[Bruno Abinader (history only)]

(In reply to comment #10)
> (From update of attachment 185135 [details])
> Not addressed my concerns.
> No tests.

OK, let me try to clarify:

Currently on Google Chrome, for instance, if you press the F11 key to go fullscreen mode, the document element is not notified, thus document.webkitIsFullScreen returns 'false' (you can verify this by going fullscreen on this example page - http://johndyer.name/lab/fullscreenapi/ ). My idea is to take advantage of the FullScreen API internals to provide proper style updates. By using only native API to provide fullscreen mode, I've noticed that at least for EFL platform, the app gets unexpected behavior when switching back to window mode.

As you mentioned, yes, the document element is always the fullscreen element, this is why Document::requestEnterFullScreenForElement() sets documentElement() if 'element' parameter is invalid.

I'm working on providing tests for this addition, but most important now is to have your opinion on the idea, or even suggest another approach other than using only native API. If this is still not desired, we can of course abort this patch submission.

[Thiago Marcos P. Santos]

(In reply to comment #11)
> (In reply to comment #10)
> > (From update of attachment 185135 [details] [details])
> > Not addressed my concerns.
> > No tests.
> 
> OK, let me try to clarify:
> 
> Currently on Google Chrome, for instance, if you press the F11 key to go fullscreen mode, the document element is not notified, thus document.webkitIsFullScreen returns 'false' (you can verify this by going fullscreen on this example page - http://johndyer.name/lab/fullscreenapi/ ). My idea is to take advantage of the FullScreen API internals to provide proper style updates. By using only native API to provide fullscreen mode, I've noticed that at least for EFL platform, the app gets unexpected behavior when switching back to window mode.
> 
> As you mentioned, yes, the document element is always the fullscreen element, this is why Document::requestEnterFullScreenForElement() sets documentElement() if 'element' parameter is invalid.
> 
> I'm working on providing tests for this addition, but most important now is to have your opinion on the idea, or even suggest another approach other than using only native API. If this is still not desired, we can of course abort this patch submission.

Let's think about this from a WebApp standpoint: the WebApp won't go to fullscreen unless it wants to go fullscreen. Going further and comparing this use case with a native app, there is no way of the window manager forcing your, let's say, IRC client, to go fullscreen.

If think the current behavior is correct: if the browser goes fullscreen, it is a resize from the WebApp perspective. Think your browser as a window manager: the WebApp might ask it to go fullscreen, but not the other way around.

[Thiago Marcos P. Santos]

To complement: I'm myself a user of Chromium fullscreen sometimes (specially on netbooks). I would find it very very weird if I start to see websites changing their appearance because of that.

[Bruno Abinader (history only)]

(In reply to comment #12)
> Let's think about this from a WebApp standpoint: the WebApp won't go to fullscreen unless it wants to go fullscreen. Going further and comparing this use case with a native app, there is no way of the window manager forcing your, let's say, IRC client, to go fullscreen.
> 
> If think the current behavior is correct: if the browser goes fullscreen, it is a resize from the WebApp perspective. Think your browser as a window manager: the WebApp might ask it to go fullscreen, but not the other way around.

Fair enough, as we discussed on IRC it is not up to the browser to decide for the WebApp to apply its fullscreen styles or not. However, from the debugging point of view, would it still have a chance as a debugging setting one can set up to force fullscreen style to be shown? Otherwise I agree we can skip this change.

[Benjamin Poulain]

> Fair enough, as we discussed on IRC it is not up to the browser to decide for the WebApp to apply its fullscreen styles or not. 

Like Thiago, I think this particular approach is not going to fly. Web Fullscreen API is a weird little spec with special use cases and I would be cautious abusing it.

BUT, I think you have an interesting idea that a webpage may be interested in knowing if it is going fullscreen. If you have solid use cases, that is worth raising the issue to the W3C.

> However, from the debugging point of view, would it still have a chance as a debugging setting one can set up to force fullscreen style to be shown? Otherwise I agree we can skip this change.

Any code has a cost. I don't think is worth is. It is already easy to debug fullscreen APIs.

[Bruno Abinader (history only)]

(In reply to comment #15)
> > However, from the debugging point of view, would it still have a chance as a debugging setting one can set up to force fullscreen style to be shown? Otherwise I agree we can skip this change.
> 
> Any code has a cost. I don't think is worth is. It is already easy to debug fullscreen APIs.

Indeed, a couple JS calls can do the job. Thanks for the insightful discussion!