Closed Bug 1412678 Opened 2 years ago Closed 2 years ago

Progressive Web Apps break fullscreen mode, so do Custom Tabs

Categories

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

Firefox 57
All
Android
defect

Tracking

()

RESOLVED FIXED
Firefox 59
Tracking Status
fennec + ---
firefox-esr52 --- unaffected
firefox57 --- unaffected
firefox58 --- fixed
firefox59 --- verified

People

(Reporter: mark.paxman99, Assigned: droeh)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 1 obsolete file)

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.
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).
Status: UNCONFIRMED → NEW
tracking-fennec: --- → ?
Ever confirmed: true
OS: Unspecified → Android
Hardware: Unspecified → All
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!
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.
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.
Summary: Progressive Web Apps break fullscreen mode, so do Compact Tabs → Progressive Web Apps break fullscreen mode, so do Custom Tabs
Dylan can you take a look please?
Flags: needinfo?(droeh)
See Also: → 1413128
tracking-fennec: ? → +
Priority: -- → P2
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.
Assignee: nobody → droeh
Flags: needinfo?(droeh)
Attachment #8932925 - Flags: review?(snorp)
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?
Attachment #8932925 - Flags: review?(snorp) → review-
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();
>  }
Updated per feedback.
Attachment #8932925 - Attachment is obsolete: true
Attachment #8933323 - Flags: review?(snorp)
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 :)
Attachment #8933323 - Flags: review?(snorp) → review+
Pushed by droeh@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ee6f0efd8680
Ensure the window most recently interacted with is the active window. r=snorp
https://hg.mozilla.org/mozilla-central/rev/ee6f0efd8680
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 59
Please request Beta approval on this when you get a chance.
Flags: needinfo?(droeh)
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
Flags: needinfo?(droeh)
Attachment #8933323 - Flags: approval-mozilla-beta?
Comment on attachment 8933323 [details] [diff] [review]
Ensure the window most recently interacted with is active

Fix a PWA related issue. Beta58+.
Attachment #8933323 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Verified on Nightly 59 (2017-12-05).
Devices:
Google Pixel (Android 8.0)
Samsung Galaxy Note 4 (Android 5.0.1)
You need to log in before you can comment on or make changes to this bug.