1520455 - Fullscreen elements aren't in view

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[csheany]

User Agent: Mozilla/5.0 (Android 7.1.1; Tablet; rv:66.0) Gecko/66.0 Firefox/66.0

Steps to reproduce:

1. Open mozilla.org/en-us/firefox
2. Play a video
3. Enter fullscreen

Actual results:

The video is outside the screen

Expected results:

The video should fit the screen

Comment 1

[csheany]

Attachment: Screenshot_20190116-083214.jpg

Comment 2

[Stefan Deiac]

Hi, I tried to reproduce your issue on the latest version of Nightly 66.0a1 (2019-01-16) without any success.
Devices:

• OnePlus 5T (Android 8.1.0);
• Samsung Galaxy S8 (Android 8.0);
• Sony Xperia Z2 (Android 6.0.1).

Also in additional, I tried by requesting a desktop site to investigate but still without any success. Do you have some add-ons installed or any device settings changed? I'll wait for your update, thanks.

Comment 3

[csheany]

Thank you for looking into.

Was the video in portrait or landscape.

Maybe try rotating and then attempt to scroll?

Can you test with a tablet?

Comment 4

[Stefan Deiac]

Hi, after further investigation I can confirm this issue using:

• Samsung Galaxy Tab S3 (Android 8.0);
• Nexus 9 (Android 6.0.1);
• Sony Xperia Z2 (Android 6.0.1);
• OnePlus 5T (Android 8.1.0);

I ran a regression for this issue due to the small difference between a good build and a bad build and I found this:
-Last good revision: 654c9274673479fc772eef2f8a3002982e931a62
-First bad revision: 895cc0297ce2e47092dc5736cd444a573ba03ed2
-Pushlog:https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=654c9274673479fc772eef2f8a3002982e931a62&tochange=895cc0297ce2e47092dc5736cd444a573ba03ed2

Also, I noticed that the bug 1520466 have the same regression range.

Notes:

• Not reproducible on Beta 65.0b11 and 64.0.2

Comment 5

[Botond Ballo [:botond]]

Hiro, this might be a regression from bug 1423013.

Comment 6

[Botond Ballo [:botond]]

Stefan, could you elaborate on what you did differently between comment 2 and comment 4? My initial attemps to repro the problem are failing too.

Comment 7

[csheany]

What happens when you enter fullscreen?

Which orientation is the video?

Comment 8

[Botond Ballo [:botond]]

(In reply to csheany from comment #7)

What happens when you enter fullscreen?

The video is correctly sized to take up the entire screen.

Which orientation is the video?

I tried entering fullscreen mode in either orientation. Once I'm in fullscreen mode, the video is landscape.

Comment 9

[Botond Ballo [:botond]]

I figured it out: I was testing the mobile site, but only the desktop site has the issue.

Definitely a regression from bug 1423013. Hiro, could you have a look?

Comment 10

[Botond Ballo [:botond]]

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

Definitely a regression from bug 1423013.

Specifically, I think this is a side effect of us expanding the layout viewport without also expanding the rect to which we attach position:fixed elements (basically, bug 1516377).

Comment 11

[Hiroyuki Ikezoe (:hiro)]

Sure, I will take a look.

Comment 12

[Botond Ballo [:botond]]

I think the solution will need to involve changes to these two places [1] [2] (the same places that were changed in this patch of bug 1465616) to size the rect in question to the expanded size.

[1] https://searchfox.org/mozilla-central/rev/bee8cf15c901b9f4b0c074c9977da4bbebc506e3/layout/generic/ViewportFrame.cpp#264
[2] https://searchfox.org/mozilla-central/rev/bee8cf15c901b9f4b0c074c9977da4bbebc506e3/layout/painting/nsDisplayList.cpp#968

Comment 13

[Botond Ballo [:botond]]

(As an aside, this also suggests that bug 1423013 might make bug 1510384 worse on some pages like this one, i.e. the video controls may become even tinier. We may want to consider fixing bug 1510384 as a follow-up.)

Comment 14

[Botond Ballo [:botond]]

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

I think the solution will need to involve changes to these two places [1] [2] (the same places that were changed in this patch of bug 1465616) to size the rect in question to the expanded size.

In addition, to avoid regressing bug 1493976, I expect we need to update MobileViewportManager::ComputeIntrinsicResolution() to use an intrinsic resolution based on the expanded size rather than the ICB size.

Comment 15

[Hiroyuki Ikezoe (:hiro)]

OK, now I think the problem is in nsPresContext::UpdateViewportScrollStylesOverride [1]. We forcibly use overflow:hidden on the fullscreen state, but yeah bug 1423013 made the overflow:hidden region reachable. :/ I don't have any idea how to solve this yet.

[1] https://searchfox.org/mozilla-central/rev/bee8cf15c901b9f4b0c074c9977da4bbebc506e3/layout/base/nsPresContext.cpp#1310

Comment 16

[Hiroyuki Ikezoe (:hiro)]

Early return from ScrollFrameHelper::UpdateMinimumScaleSize() for the case fixed this problem. I will check whether the changes what Botond suggested in comment 12 and comment 13 are needed or not.

Comment 17

[Hiroyuki Ikezoe (:hiro)]

Ok, the changes in comment 12 actually are not necessary to fix this particular issue because, I guess, the fullscreened element is positioned at the top left corner so it shouldn't be affected by the minimum scale size. I will defer the change into bug 1516377. As for the change in comment 13, it seems the issue is fixed without the change, but I think we should do that in this bug.

Comment 18

[Hiroyuki Ikezoe (:hiro)]

After some more thought, now I think bailing out from UpdateMinimumScaleSize is not a right way to fix.

Comment 19

[Hiroyuki Ikezoe (:hiro)]

Now I am sure that the changes Botond suggested is the right way to fix this, but with the changes the video element in question is stretched to the minimum scale. I've confirmed that in the case of fullscreen state keeping the aspect ratio of the minimum scale size as the same as the ICB size, so I am guessing there are/is (a) place(s) to use the aspect ratio of the ICB somewhere but I can't find it yet.

Comment 20

[Botond Ballo [:botond]]

Do we know why the layout viewport's aspect ratio is different from the ICB's? Is it because of bug 1508177?

Comment 21

[Hiroyuki Ikezoe (:hiro)]

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

Do we know why the layout viewport's aspect ratio is different from the ICB's? Is it because of bug 1508177?

Bingo!

I've been trying to track down where it comes from, but I don't have enough experience for debugging on Android. (./mach run --debugger crashes on my environment). Anyway replacing MaxScaleRatio with MinScaleRatio in MobileViewportmanager::ComputeIntrinsicScale fixed the stretched video element.

So, given that to fix bug 1508177 needs more work, we should make the changes Botond suggested with the workaround that is to bail out from UpdateMinimumScaleSize on fullscreen state here in this bug?

Botond, what do you think?

Comment 22

[Hiroyuki Ikezoe (:hiro)]

Botond suggested me on IRC that we can just take the workaround bailing out from UpdateMinimumScaleSize here in this bug.

Now I did succeed in writing a mochitest that fails without the workaround and succeeds with the workaround (I've also confirmed that the test works fine with the changes in comment 12 and comment 13 and replacing MaxScaleRatio with MinScaleRatio in ComputeIntrinsicScale). (but unfortunately try server is closed now. :/)

Comment 23

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1520455 - Don't use the minimum scale size on fullscreen state to avoid the layout viewport gets larger than the visual viewport. r=botond

This is a workaround. To properly fix the issue we need to fix both of bug
1516377 and bug 1508177.

Comment 24

[Hiroyuki Ikezoe (:hiro)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6b4fb533b4bf91127344acee7924a4c56b2feed4

Comment 25

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ada22b635f8d
Don't use the minimum scale size on fullscreen state to avoid the layout viewport gets larger than the visual viewport. r=botond

Comment 26

[Cosmin Sabou [:CosminS]]

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

Comment 27

[miralobontiu]

Verified as fixed on the latest Nightly 66.0a1 (2019-01-25) with HTC 10 (Android 8.0) and Sony Xperia Z2 (Android 6.0.1).

Comment 28

[csheany]

Should the APZ Mini Map be in the upper left hand corner?

Comment 29

[csheany]

Attachment: Screenshot_20190306-023522.jpg