1412678 - Progressive Web Apps break fullscreen mode, so do Custom Tabs

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Web Apps (PWAs), defect, P2)

Description

[Mark]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.13; rv:58.0) Gecko/20100101 Firefox/58.0
Build ID: 20171027220059

Steps to reproduce:

In Fennec Nightly 2017-10-29 (earlier versions too)
Navigate to www.theguardian.com/uk
Tap the house icon in the toolbar to create a Progressive Web App on your desktop
In Fennec, navigate to https://arstechnica.com/gaming/2017/10/assassins-creed-origins-review-a-living-breathing-ancient-world/
There is a video about half way down. Play the video and then try to enter full screen using the button bottom right of the video.
It works OK
Go to your Android desktop and open the Guardian PWA
Go back to Ars Technica in Fennec and try to play the video in full screen
Full Screen video button doesn't work
Kill the PWA from the Android task manager (square icon then swipe it away)
Full Screen video button now works as expected

I guess the same would be true for any PWA and any video.

The same is true of a Fennec Custom Tab but here to get full screen functionality back you have to kill the whole of Fennec not just the Custom Tab

The same is true for the WebExtension function
     document.documentElement.requestFullscreen();
it does nothing when a Fennec PWA or Fennec Custom Tab is open. I think.

Comment 1

[Oana Horvath [:ohorvath]]

I was able to reproduce this with PWA's, but not with a Gmail custom tab.
Devices:
Samsung Galaxy Note 4 (Android 5.0.1)
Huawei P9 Lite (Android 6.0)
build: Nightly 58 (2017-10-29).

Comment 2

[Mark]

I replicated the broken Custom Tabs behaviour just now on:-
Sony Xperia X Compact (Android 7.1.1)

I used an app called SearchBar Ex and it was a PITA to get it to use a Fennec Custom Tab rather than a Chrome one. In SearchBar Ex I set 'Enable Internal Browser' to ON but it took a lot more fiddling after that. I don't know how I got it to work in the end, much rebooting phone etc.

I am really struggling to get apps to use Fennec Custom Tabs. SearchBar Ex was reluctant to do so. AquaMail always uses Chrome Custom Tabs even though I have Fennec Nightly as my default browser and Fennec Custom Tabs set to ON.

Maybe there is another issue with getting apps to use Fennec Custom Tabs, or maybe I am just an idiot (more than likely). 

If there is anything fancy I need to do, please let me know!

Comment 3

[Oana Horvath [:ohorvath]]

You did nothing wrong, setting Fennec as your default browser and Custom tabs ON is all you need to do. 
However, I've just installed the SearchBar Ex app and even though Chrome was the default browser, it asked me which browser I want to use with it. So you can choose Nightly there as default, but it doesn't open a custom tab, just regular browser tabs. That is until you switch 'Enable Internal Browser' to ON. But, 'Enable Internal Browser' specifies that it will use Chrome custom tabs by default, so it's forcing that instead of using your default browser (Fennec). I don't know about AquaMail, couldn't figure out why it uses only Chrome's custom tabs, but it's probably sort of the same thing.

Comment 4

[Mark]

I did get SearchBar Ex to use Fennec Custom Tabs, it's doing so now [ I can send video if you don't believe me ;) ]. I have no idea how I got it to work but I'm reluctant to fiddle with it while this bug is open, I want to leave it to test any fixes.

You are right about Gmail - I opened a Fennec Custom Tab from Gmail and it did not break Fennec browser fullscreen. But SearchBar Ex still does.

So for me it seems to depend on which app opens the Fennec Custom Tab.

Comment 5

[James Willcox (:snorp) ([e-mail]) (he/him)]

Dylan can you take a look please?

Comment 6

[Dylan Roeh (:droeh) (he/him)]

Attachment: Ensure the window most recently interacted with is active

Right, so this basically comes down to the requirement that a fullscreen request is only honored if it comes from the active window, and we weren't tracking which window was active with GeckoView. The concept of an active window doesn't necessarily make sense on Android, but removing that entirely proved to be a bit much. Consequently, here's a patch that ensures the window most recently interacted with is the active window, which should fix this issue and generally ensure somewhat better behavior in situations with multiple GeckoViews.

Comment 7

[James Willcox (:snorp) ([e-mail]) (he/him)]

Comment on attachment 8932925 [details] [diff] [review]
Ensure the window most recently interacted with is active

Review of attachment 8932925 [details] [diff] [review]:
-----------------------------------------------------------------

::: widget/android/nsWindow.cpp
@@ +496,5 @@
>          PostInputEvent([input, guid, blockId, status] (nsWindow* window) {
> +            // Ensure most recently touched window is active.
> +            nsIWidgetListener* listener = window->GetWidgetListener();
> +            if (listener) {
> +                listener->WindowActivated();

I'm worried that calling this on every input is going to be a perf issue. Can you break these blocks out into their own function and only call WindowActivated if we're not already the active window?

Comment 8

[(inactive) Jim Chen [:jchen] [:darchons]]

Comment on attachment 8932925 [details] [diff] [review]
Ensure the window most recently interacted with is active

Looks like this will fix bug 1419000 as well.

We really should be calling `BringToFront()` though, preferably from inside `nsWindow::UserActivity()`. e.g.,

>  if (FindTopLevel() != nsWindow::TopWindow()) {
>    BringToFront();
>  }

Comment 9

[Dylan Roeh (:droeh) (he/him)]

Attachment: Ensure the window most recently interacted with is active

Updated per feedback.

Comment 10

[James Willcox (:snorp) ([e-mail]) (he/him)]

Comment on attachment 8933323 [details] [diff] [review]
Ensure the window most recently interacted with is active

Review of attachment 8933323 [details] [diff] [review]:
-----------------------------------------------------------------

This is much nicer :)

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ee6f0efd8680
Ensure the window most recently interacted with is the active window. r=snorp

Comment 12

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/ee6f0efd8680

Comment 13

[Ryan VanderMeulen [:RyanVM]]

Please request Beta approval on this when you get a chance.

Comment 14

[Dylan Roeh (:droeh) (he/him)]

Comment on attachment 8933323 [details] [diff] [review]
Ensure the window most recently interacted with is active

Approval Request Comment
[Feature/Bug causing the regression]:
[User impact if declined]: PWAs and custom tabs will interfere with fullscreen in Fennec
[Is this code covered by automated tests?]: No
[Has the fix been verified in Nightly?]: Yes
[Needs manual test from QE? If yes, steps to reproduce]:  No
[List of other uplifts needed for the feature/fix]: None
[Is the change risky?]: Not very
[Why is the change risky/not risky?]: This updates behavior so that the window the user is interacting with is always considered the active window
[String changes made/needed]: None

Comment 15

[Gerry Chang [:gchang]]

Comment on attachment 8933323 [details] [diff] [review]
Ensure the window most recently interacted with is active

Fix a PWA related issue. Beta58+.

Comment 16

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/releases/mozilla-beta/rev/0c07f502b744

Comment 17

[Oana Horvath [:ohorvath]]

Verified on Nightly 59 (2017-12-05).
Devices:
Google Pixel (Android 8.0)
Samsung Galaxy Note 4 (Android 5.0.1)