1674104 - Video is zoomed in when in fullscreen mode

Status: VERIFIED FIXED

(GeckoView :: Media, defect)

Description

[Petru-Mugurel Lingurar [:petru]]

From github: https://github.com/mozilla-mobile/fenix/issues/16252.

Site with issue and any steps to reproduce

1. Go to www.twitch.tv
2. Use desktop site
3. Play any video in fullscreen mode

Expected behavior

Video will play fullscreen.

Actual behavior

Video will play fullscreen but it is zoomed in so you could not see the full video.

Does toggling Tracking Protection fix the issue? (Press the shield icon in the toolbar while on the site to see toggle)

No

Can you reproduce in Chrome (or other non-Mozilla browser)?

No, already tried with Chrome and Kiwi Browser.

Device information

• Android device: Mi 9T Pro MIUI 12
• Fenix version: Nightly (Build #2015772363)

Last version that worked properly is:

• Fenix version: Nightly (Build #2015769099)
  All versions next to it are bugged.

Change performed by the Move to Bugzilla add-on.

Comment 1

[Mira Lobontiu (Android QA)]

I was able to reproduce this issue on Samsung Galaxy Note 8 (Android 9) only on 10/29 Nightly.
Not reproducible on Beta 83.0.0-beta.2, nor on RC 82.1.1.

Comment 2

[Mira Lobontiu (Android QA)]

Attachment: Screenshot_20201029-143907_Firefox Nightly.jpg

Comment 3

[Amedyne Moya [:amoya]]

Toolbar related. Most likely a Fenix/AC problem.

Comment 4

[[ex-Mozilla] Agi Sferro | :agi]

This seems like a duplicate of https://github.com/mozilla-mobile/fenix/issues/8252

Comment 5

[Petru-Mugurel Lingurar [:petru]]

I can reproduce the issue on GVE also.
And not just with twitch.
I'd think #8252 could also be resolved here.

Recording for playing fullscreen videos while in desktop mode, on various websites, in today's GVE: https://drive.google.com/file/d/1IaUl6FOPp0ZFNnOrn4nGNBdH_qpf5Wz_/view?usp=sharing

Comment 6

[Petru-Mugurel Lingurar [:petru]]

https://github.com/mozilla-mobile/fenix/issues/17271 also reproduces on GVE
(even with no desktop mode requested)
https://drive.google.com/file/d/1UwH9X-Tmw66Ok_T1rYY828QiHOoQKUjz

Saw these on a Pixel 3 with Android 11.

Comment 7

[[ex-Mozilla] Agi Sferro | :agi]

(In reply to Petru-Mugurel Lingurar [:petru] from comment #5)

I can reproduce the issue on GVE also.
And not just with twitch.
I'd think #8252 could also be resolved here.

Recording for playing fullscreen videos while in desktop mode, on various websites, in today's GVE: https://drive.google.com/file/d/1IaUl6FOPp0ZFNnOrn4nGNBdH_qpf5Wz_/view?usp=sharing

That's likely because now (Bug 1500644) we support the dynamic toolbar, and we have the same bug that Fenix has. Reverting that patch fixes the problem for me.

Hiro, is this something you can look into? It looks like now that we have the dynamic toolbar in GVE, we don't reset the toolbar state when going to fullscreen so some UI elements appear shifted to where they should be.

Comment 8

[Hiroyuki Ikezoe (:hiro)]

When entering fullscreen, we need to set the dynamic toolbar max height to zero and revert it when leaving from fullscreen state, in this onFullScreen function I suppose.

Comment 9

[Hiroyuki Ikezoe (:hiro)]

Attachment: A patch to fix the geckoview example issue

This is a patch what I commented in comment 8. That being said I am pretty sure this will not fix the original issue at all. And I believe the issue on GeckoView example is a different one, what I am seeing on GeckoView example is there is a white blank area where the toolbar positioned, whereas the issue on Fenix there is a black blank area (and what I don't quite understand is in the screenshot in comment 2 the icon at top left position is layouted on the black blank area).

Also I can't reproduce the original issue on my pixel 3. Presumably there is a race condition between fullscreen and device rotation stuff.

Comment 10

[[ex-Mozilla] Agi Sferro | :agi]

Right, the original issue is unrelated, it just looks the same. (In reply to Hiroyuki Ikezoe (:hiro) from comment #9)

Created attachment 9195301 [details] [diff] [review]
A patch to fix the geckoview example issue

This is a patch what I commented in comment 8. That being said I am pretty sure this will not fix the original issue at all.

Right, the original issue needs to be fixed in Fenix, or do you think the original bug is a bug somewhere in GV/Gecko?

And I believe the issue on GeckoView example is a different one, what I am seeing on GeckoView example is there is a white blank area where the toolbar positioned, whereas the issue on Fenix there is a black blank area

The blank area is black supposedly because Fenix's background is black (while GVE's background color is white)

(and what I don't quite understand is in the screenshot in comment 2 the icon at top left position is layouted on the black blank area).

I think that's because the device is in portrait when the video starts playing (and then the device rotates to landscape)

Also I can't reproduce the original issue on my pixel 3. Presumably there is a race condition between fullscreen and device rotation stuff.

It's not super straightforward to reproduce, I can see it on vimeo pretty reliably (the video controls are off the screen in Fenix Nightly)

Hiro, I think we should still fix the GVE problem, do you want to submit a patch for Comment 8? Otherwise I can

Comment 11

[Hiroyuki Ikezoe (:hiro)]

Oops, I didn't see the recording and thus I did totally misunderstand what the problem is. It looks to me it's same as bug 1502566, which means we might have regressed at some point. We've changed fullscreen stuff for fission, we've also changed zooming stuff for desktop zooming, so maybe those kind of works affected? Or maybe it's an edge case falling back from bug 1502566. CCing Botond.

Anyway it's probably an issue in Gecko.

(I am still wondering this issue is really related to dynamic toolbar?)

(In reply to Agi Sferro | :agi | ni? for questions | ⏰ PST | he/him from comment #10)

Hiro, I think we should still fix the GVE problem, do you want to submit a patch for Comment 8? Otherwise I can

Please do it. Thanks!

Comment 12

[[ex-Mozilla] Agi Sferro | :agi]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #11)

Oops, I didn't see the recording and thus I did totally misunderstand what the problem is. It looks to me it's same as bug 1502566, which means we might have regressed at some point. We've changed fullscreen stuff for fission, we've also changed zooming stuff for desktop zooming, so maybe those kind of works affected? Or maybe it's an edge case falling back from bug 1502566. CCing Botond.

Anyway it's probably an issue in Gecko.

(I am still wondering this issue is really related to dynamic toolbar?)

(In reply to Agi Sferro | :agi | ni? for questions | ⏰ PST | he/him from comment #10)

Hiro, I think we should still fix the GVE problem, do you want to submit a patch for Comment 8? Otherwise I can

Please do it. Thanks!

Yeah it might not be related after all, although it is interesting that it seems to reproduce only with the dynamic toolbar code (in GVE, at least).

Botond, do you have any idea of what might be going on here? (ni because lots of users are complaining about this) an easy way to reproduce this for me is loading https://vimeo.com/76979871 and making the video full screen in Fenix Nightly. With this setup the video controls are off screen.

Comment 13

[[ex-Mozilla] Agi Sferro | :agi]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #11)

(In reply to Agi Sferro | :agi | ni? for questions | ⏰ PST | he/him from comment #10)

Hiro, I think we should still fix the GVE problem, do you want to submit a patch for Comment 8? Otherwise I can

Please do it. Thanks!

Bug 1685179

Comment 14

[Hiroyuki Ikezoe (:hiro)]

From a comment in the change for bug 1502566. https://searchfox.org/mozilla-central/rev/a0ccd492719b1ad2106f6456549be62a76f45acb/dom/base/Document.cpp#14477-14482

 // When entering fullscreen, reset the RCD's resolution to the intrinsic    
 // resolution, otherwise the fullscreen content could be sized larger than  
 // the screen (since fullscreen is implemented using position:fixed and     
 // fixed elements are sized to the layout viewport).

So, now I am mostly sure that this issue is related to dynamic toolbar, with dynamic toolbar the intrinsic resolution is bit smaller than expected one also dynamic toolbar affects position:fixed elements unfortunately.

What I am guessing is setting the dynamic toolbar max height on browser side is too late, we need to set the height before we do run the code for bug 1502566. That's not what I did avoid when I implemented dynamic toolbar because I was thinking that whether browser hides the toolbar or not on fullscreen state is depending on the browser.

Comment 15

[Hiroyuki Ikezoe (:hiro)]

Or we could add a PresShell::SetResolutionAndScaleTo call after a reflow kicked by setting the toolbar max height if we are in fullscreen state, here I mean.

Comment 16

[Hiroyuki Ikezoe (:hiro)]

Okay, I was totally misunderstanding how we make an element fullscreen. I was thinking it is scaled, but actually it's not. Cameron told me on Matrix that it's just fit to the (layout) viewport by specifying width: 100% and height: 100% as a position:fixed element. Then I noticed a reliable STR with GeckoView example on Android emulator;

1. Build GeckoView example with applying attachment 9195301 [details] [diff] [review]
2. Rotate Android emulator to landscape mode (it could be possible that the issue can be reproducible on portrate mode)
3. Open https://vimeo.com/76979871
4. Scroll down a bit to hide the dynamic toolbar completely
5. Click the fullscreen icon in the video

So a problem I can see is we don't reset AsyncCompositionManager::mFixedLayerMargins and don't reset APZCTreeManager::mCompositorFixedLayerMargins either when setting the dynamic toolbar max height to zero. Thus position:fixed elements (in the fullscreen case, it's the fullscreen-ed element) are aligned with the old margin value, that results the blank area.

A right place I can think of to reset them is to call setVerticalClipping(0) in GeckoView::setDynamicToolbarMaxHeight so that it can propagate the value to both of the main thead and the compositor. Of cource this is assuming that browser wants to restore the dynamic toolbar state to the original state, e.g. when the toolbar's max height is changed to 60px from 120px and if the toolbar's height at the moment is 30px, then the toolbar's at-the-moment height will be 60px.

Comment 17

[Hiroyuki Ikezoe (:hiro)]

I was thinking I can write a mochitest for this issue with setting layout.dynamic-toolbar-max-height and using WindowSnapshot.js, but I noticed it will not pass with the way I proposed because when we change layout.dynamic-toolbar-max-height pref value, we don't call setVerticalClipping(0), we can of course invoke it when we change the pref value, but... that is something like a workaround to make the test pass. A right way to make the test run properly is to make GeckoView test runner toolbar dynamic, I filed bug 1685242 for it.

Anyway, I will send a patch to fix this without any tests.

Comment 18

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1674104 - Reset the vertical clipping value to zero whenever we change the dynamic toolbar max height. r?#geckoview-reviewers!,botond!

So that we will not mis-align position:fixed elements with the old stale
clipping value.

Comment 19

[Botond Ballo [:botond]]

Thanks for investigating this, Hiro!

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c1443682bef3
Reset the vertical clipping value to zero whenever we change the dynamic toolbar max height. r=botond

Comment 21

[[ex-Mozilla] Agi Sferro | :agi]

(In reply to Botond Ballo [:botond] from comment #19)

Thanks for investigating this, Hiro!

+1000!

Comment 22

[Cosmin Sabou [:CosminS]]

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

Comment 23

[Ryan VanderMeulen [:RyanVM]]

Is this something we should uplift to Beta for Fenix 85? Please nominate ASAP if so.

Comment 24

[Botond Ballo [:botond]]

Comment on attachment 9195551 [details]
Bug 1674104 - Reset the vertical clipping value to zero whenever we change the dynamic toolbar max height. r?#geckoview-reviewers!,botond!

Beta/Release Uplift Approval Request

• User impact if declined: When viewing a video in fullscreen mode, the video can sometimes be slightly zoomed in (by approximately the ratio between "viewport height with dynamic toolbar shown" and "viewport height with dynamic toolbar hidden") such that part of the video is cut off.
• Is this code covered by automated tests?: No
• Has the fix been verified in Nightly?: No
• Needs manual test from QE?: Yes
• If yes, steps to reproduce: As in comment 0
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): It's a very small change, and based on what code is being changed, I would expect any possible regressions to be limited to the behaviour of the dynamic toolbar (e.g. when it is shown or hidden).
• String changes made/needed: none

Comment 25

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9195551 [details]
Bug 1674104 - Reset the vertical clipping value to zero whenever we change the dynamic toolbar max height. r?#geckoview-reviewers!,botond!

Approved for Fenix 85.0.0-beta.9.

Comment 26

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/dc65d4fdd35d

Comment 27

[Hiroyuki Ikezoe (:hiro)]

Thank you, Botond! (I didn't see any emails this weekend.)

Comment 28

[Andrei Bodea[:andrei]]

Verified as fixed on the latest Nightly build 1/19 and Fenix 85.0.0-beta.9.

Comment 29

[Kevin Brosnan [Ex-Mozilla]]

Release Note Request (optional, but appreciated)
[Why is this notable]: Fixes a easy to hit issue with video fullscreen playback
[Affects Firefox for Android]: only
[Suggested wording]: Fixed: Fullscreen video now correctly matches the video size to screen size. Previously fullscreen videos were displayed larger than the screen size resulting in videos that were cut off.
[Links (documentation, blog post, etc)]: this bug and several dupes in the Fenix GitHub

Comment 30

[Julien Cristau [:jcristau]]

Belatedly added to the Firefox for Android 85.0 release notes.

Comment 31

[Chris Peterson [:cpeterson]]

Moving some media bugs to the new GeckoView::Media component.