1513232 - [Android 8] Firefox is zoomed in after returning from "picture in picture mode" to a full screen video

Status: VERIFIED FIXED

(Core :: Panning and Zooming, defect, P1)

Description

[Levente Sacal]

Device(s):
 -  Samsung S9 (Android 8.0.0);


Build(s):
 - Nightly 66.0a1 (2018-12-11);

Steps to reproduce:
 1. Watch any Youtube video in fullscreen
 2. From Fullscreen go into Picture in picture mode
 3. Return back to full screen video from Picture in picture

Expected result:
- Video is not zoomed in and is still in full screen

Actual result:
- Video is zoomed in in full screen
 

Notes:
- Was unable to reproduce the issue on Beta or Release
- https://youtu.be/E1IaSJnM_Ag

Comment 1

[Karl Tomlinson (:karlt)]

Regression, so "P1 Fix in the current release or iteration"
https://github.com/mozilla/bug-handling/blob/master/policy/triage-bugzilla.md#how-do-you-triage

The first 65 beta release on https://archive.mozilla.org/pub/mobile/releases/ is 65.0b4 Last Modified 14-Dec-2018 23:21 (in unspecified timezone), which is after this bug is reported, and so I don't know the status on 65.

Comment 2

[miralobontiu]

Ran a regression using Samsung Galaxy S8 (Android 8):

Last good revision: aeb00b8974a472794d513c5884c38a96397334da
First bad revision: d8064028f0e66760a66105089a0bc1258877b6bf
Pushlog:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=aeb00b8974a472794d513c5884c38a96397334da&tochange=d8064028f0e66760a66105089a0bc1258877b6bf

Comment 3

[Botond Ballo [:botond]]

(In reply to Levente Sacal (OoO 31 Dec - 3 Jan) from comment #0)
>  2. From Fullscreen go into Picture in picture mode

To provide some context here: "Picture in picture mode" is an OS feature in Android 8 [1].

I will try to get my hands on an Android 8 device to repro and investigate.

Meanwhile, if someone (snorp?) knows what type of support Fennec has for "Picture in picture mode" and what entering that mode involves, that would be useful to know.

[1] https://developer.android.com/guide/topics/ui/picture-in-picture

Comment 4

[Karl Tomlinson (:karlt)]

I guess this file is involved in some way, but I'm not sure that is helpful info, sorry.
https://searchfox.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/media/PictureInPictureController.java

ni?petru in case of any ideas.

Comment 5

[Botond Ballo [:botond]]

I was able to get a hold of an Android 8 device and reproduce and investigate the bug.

Here's what's happening:

  - When PiP mode is activated, the browser window is resized to
    a much smaller size. On the device I was testing with, it
    was resized to 576x324 screen pixels, which with a device
    scale of 3 corresponds to 192x108 CSS pixels.

  - Accordingly, GetViewportInfo() computes a layout viewport size
    of 192x108 CSS pixels. However, we inflate this to 200x108 CSS 
    pixels because we have a built-in minimum width of 200px [1].
    Note that this inflation does not preserve the aspect ratio.

  - We then run code that updates the resolution to try to preserve
    the amount of content that fits into the display width [2].
    This tries to set the resolution to 0.96, to try to fit the
    slightly widened content into the display width.

  - However, we don't have enough vertical content at this zoom
    level, so it gets re-clamped to 1.0 here [3]. It's this
    re-clamping that's new as of bug 1509552.

  - When you tap on the small version of the video in PiP mode, 
    it becomes slightly bigger: 853x480 screen pixels, or
    284.33x160 CSS pixels. This time, the corresponding viewport
    size at the original aspect ratio is above our minimum, so
    it becomes the new viewport size. However, since this is 
    again an aspect ratio change relative to the previous state, 
    the code at [2] runs again, this time setting the resolution
    to 1.04.

  - When you tap the video once more to exit PiP mode, the
    resolution of 1.04 persists, making the video be slightly
    zoomed in (and have a small scroll range).

    Note that, from the document's point of view, we never
    leave fullscreen mode throughout this process, so the
    resolution changes associated with entering and exiting
    fullscreen mode (added in bug 1493976) do not come into
    play.

[1] https://searchfox.org/mozilla-central/rev/0ee0b63732d35d16ba22d5a1120622e2e8d58c29/dom/base/nsViewportInfo.h#17
[2] https://searchfox.org/mozilla-central/rev/0ee0b63732d35d16ba22d5a1120622e2e8d58c29/layout/base/MobileViewportManager.cpp#302
[3] https://searchfox.org/mozilla-central/rev/0ee0b63732d35d16ba22d5a1120622e2e8d58c29/layout/base/MobileViewportManager.cpp#323

Comment 6

[Botond Ballo [:botond]]

Attachment: Bug 1513232 - Avoid clamping the layout viewport size to be larger than the display size. r=kats

The clamping is intended to be a safeguard against extreme values in the
meta viewport tag. If the layout viewport is sized to the display size,
avoid the clamping, since the display size could legitimately be small
(e.g. in Android 8's picture-in-picture mode).

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4f844b2ebd4f
Avoid clamping the layout viewport size to be larger than the display size. r=kats

Comment 8

[Daniel Varga [:dvarga]]

https://hg.mozilla.org/mozilla-central/rev/4f844b2ebd4f

Comment 9

[Ryan VanderMeulen [:RyanVM]]

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

Comment 10

[Botond Ballo [:botond]]

Comment on attachment 9034053 [details]
Bug 1513232 - Avoid clamping the layout viewport size to be larger than the display size. r=kats

[Beta/Release Uplift Approval Request]

Feature/Bug causing the regression: Bug 1509552

User impact if declined: On devices running Android 8, fullscreen video can be slightly zoomed in after entering and exiting picture-in-picture mode.

Is this code covered by automated tests?: Yes

Has the fix been verified in Nightly?: Yes

Needs manual test from QE?: No

If yes, steps to reproduce: 

List of other uplifts needed: None

Risk to taking this patch: Low

Why is the change risky/not risky? (and alternatives if risky): Small, targeted fix

Note: test is in bug 1517821.

String changes made/needed:

Comment 11

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9034053 [details]
Bug 1513232 - Avoid clamping the layout viewport size to be larger than the display size. r=kats

[Triage Comment]
Fixes fullscreen video being zoomed in slightly on devices running Android 8 under some circumstances. Approved for 65.0b9.

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/71e27c73bb26

Comment 13

[Stefan Deiac]

Verified as fixed on the latest version of Nightly 66.0a1 (2019-01-07) with Samsung Galaxy S8 (Android 8.0) and OnePlus 5T (Android 8.1.0).
I'll let the qe-verify flag till the verification on beta, thanks.

Comment 14

[Sorina Florean [:sflorean]]

Verified as fixed on latest Beta build, 65.0b9 build 2.
Devices: Samsung Galaxy S8(Android 8.0.0) and Samsung S9 (Android 8.0.0).