Closed Bug 1520455 Opened 10 months ago Closed 10 months ago

Fullscreen elements aren't in view

Categories

(Firefox for Android :: General, defect)

Firefox 66
ARM
Android
defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 66
Tracking Status
firefox64 --- unaffected
firefox65 --- unaffected
firefox66 --- verified

People

(Reporter: csheany, Assigned: hiro)

References

Details

(Keywords: regression)

Attachments

(3 files)

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

Flags: needinfo?(botond)

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.

Flags: needinfo?(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?

Flags: needinfo?(csheany)

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
Status: UNCONFIRMED → NEW
Ever confirmed: true
OS: Unspecified → Android
Hardware: Unspecified → ARM
See Also: → 1520466

Hiro, this might be a regression from bug 1423013.

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.

Flags: needinfo?(botond) → needinfo?(stefan.deiac)

What happens when you enter fullscreen?

Which orientation is the video?

Flags: needinfo?(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.

Flags: needinfo?(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?

Blocks: 1423013
Flags: needinfo?(hikezoe)

(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).

Flags: needinfo?(stefan.deiac)

Sure, I will take a look.

Assignee: nobody → hikezoe
Status: NEW → ASSIGNED
Flags: needinfo?(hikezoe)

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

(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.)

(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.

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

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.

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.

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

See Also: → 1520513
Keywords: regression

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.

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

(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?

Flags: needinfo?(botond)

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. :/)

Flags: needinfo?(botond)

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

Pushed by hikezoe@mozilla.com:
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
Status: ASSIGNED → RESOLVED
Closed: 10 months ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 66

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).

Status: RESOLVED → VERIFIED

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

You need to log in before you can comment on or make changes to this bug.