Zooming in on the page causes fullscreen video to be zoomed in also

VERIFIED FIXED in Firefox 64

Status

()

defect
VERIFIED FIXED
11 months ago
8 months ago

People

(Reporter: csheany, Assigned: botond)

Tracking

(Depends on 1 bug, Blocks 1 bug, {regression})

64 Branch
mozilla65
ARM
Android
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox-esr60 unaffected, firefox63+ wontfix, firefox64+ verified, firefox65+ verified)

Details

Attachments

(7 attachments)

User Agent: Mozilla/5.0 (Android 7.1.1; Tablet; rv:64.0) Gecko/64.0 Firefox/64.0
Build ID: 20180921100234

Steps to reproduce:

1. Open a page with video
2. Zoom in on the page
3. Enter fullscreen


Actual results:

The video is zoomed in and the controls are out of frame. The video can be dragged around but not zoomed out.


Expected results:

The entire video should be visible and stationery
Duplicate of this bug: 1493975
Hi, thanks for your report. I can confirm the issue on the latest version of Nightly (64.0a1 2018-09-25) and beta 63.0b9 with Google Pixel (Android 9) and OnePlus 5T (Android 8.1.0).
In FF Release the video it's not zoomed but I notice that after I doing the steps from this issue, the video controls are zoomed over the video.
Status: UNCONFIRMED → NEW
Ever confirmed: true
OS: Unspecified → Android
Hardware: Unspecified → ARM
Thank you for looking into it.

The isssue isn't so much with the controls as it is the video itself.

The controls are to scale but only a portion of the video can be seen.
Still present in the latest Nightly.

The screenshots aren't edited but before result and after results.

Are there any related bugs?
Flags: needinfo?(xidorn+moz)
I can reproduce this, but I don't have idea what would be the best way to fix it.

This is probably not Android-specific, and it may happen if we start supporting "pinch to zoom" at some point (bug 688990), so it's probably more about how we handle fullscreen when we have zoomed.

My suggestion would be that we should probably render fullscreen elements without APZ applied. I'm not familiar with that area of code, so it's not clear whether it is feasible or not.

Display list of fullscreen elements is built in ViewportFrame::BuildDisplayListForTopLayer[1]. I guess it may be possible to pass in some argument or use some special display item type to escape from zooming somehow?

cc botond and hiro since they should know better on viewport rendering and APZ stuff than me.


[1] https://searchfox.org/mozilla-central/rev/9cb3e241502a2d47e2d5057ca771324a446b6695/layout/generic/ViewportFrame.cpp#138
Component: Audio/Video → Panning and Zooming
Flags: needinfo?(xidorn+moz)
Product: Firefox for Android → Core
Version: Firefox 64 → 64 Branch
I believe I tracked down the regression.

The last good build was 2018-08-19

The first bad build was 2018-08-20
If it's a recent regression, maybe you can try using mozregression to track down to a specific regressing bug? (Not sure how it works for Firefox Android, though...)
Is this the pushlog range?
Could you paste the link of the pushlog range here?
Flags: needinfo?(botond)
That regression range doesn't seem to be very useful. I'll try to find the regression, so ni? me for now.
Flags: needinfo?(botond) → needinfo?(xidorn+moz)
Are you sure? https://bugzilla.mozilla.org/show_bug.cgi?id=1465616 seems to stick out to me.
Did you notice the second link?
So there are actually two bugs:

When the page is zoomed in, the fullscreen video is zoomed as well, and you can even scroll the viewport to see different portion of the video. After using mozregression, it points to this pushlog range: https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=c6e7b65bf8b02a32a6c1d583eb1d79e3116d692d&tochange=bac4139e4ff9b3071e1ce17113ac65ed1d8e8598 which points to bug 1465616 indeed.

Another bug is that, even before that bug, when the page is zoomed in, the control is zoomed in (while the video itself is not) for fullscreen. That's probably a pre-existing one.

Given that you think this is a recent regression, I guess you mean the first bug.
Blocks: 1465616
Flags: needinfo?(xidorn+moz)
Flags: needinfo?(kats)
Flags: needinfo?(botond)
Keywords: regression
Interesting. I wonder if fullscreen is implemented using position:fixed with top:0, bottom:0, left:0, right:0, and as a result of bug 1465616 the fullscreen content is now being sized to the layout viewport, which is larger than the screen if you're zoomed in.
(In reply to Botond Ballo [:botond] from comment #19)
> Interesting. I wonder if fullscreen is implemented using position:fixed with
> top:0, bottom:0, left:0, right:0, and as a result of bug 1465616 the
> fullscreen content is now being sized to the layout viewport, which is
> larger than the screen if you're zoomed in.

Yes, fullscreen is using position:fixed with top/bottom/left/right this way, see ua.css[1]. This is also what the spec says.

It should probably be handled specially somehow, either reset and lock the zooming of root when entering fullscreen, or paint the fullscreen element in an independent non-zoomed layer. (This difference is web-exposed, though, so we may want to check what other browsers actually do.)


[1] https://searchfox.org/mozilla-central/rev/4fc7bfa931189e0718dd70b4c59885829f1d761e/layout/style/res/ua.css#304-320
Flags: needinfo?(xidorn+moz)
It seems Chrome resets the zoom level when entering fullscreen. We can probably do this as well. We can even do better: restore the zoom level after exiting fullscreen.

Note that, Chrome seems to change the viewport setting for the fullscreen element, different from what page's initial viewport setting. It isn't clear what heuristic they use.

nsIDocument::HandlePendingFullscreenRequests and nsIDocument::ExitFullscreenInDocTree are good places to handle that for entering and leaving fullscreen respecitvely.
Flags: needinfo?(xidorn+moz)
This is a simple test page for checking behavior of fullscreen on zooming.

Note that you need to use Firefox Nightly and Chrome Canary, since both have just shipped the unprefixed Fullscreen API. In addition, only Chrome Canary seems to the same rendering model as ours.
I was thinking of a fix along slightly different lines: if we can detect fullscreen mode in these two places [1] [2] (the two places in Layout code where bug 1465616 made a change to use the visual viewport size), we can avoid using the visual viewport size in that case, and get back the behaviour we had before bug 1465616.

[1] https://searchfox.org/mozilla-central/rev/4fc7bfa931189e0718dd70b4c59885829f1d761e/layout/generic/ViewportFrame.cpp#290
[2] https://searchfox.org/mozilla-central/rev/4fc7bfa931189e0718dd70b4c59885829f1d761e/layout/painting/nsDisplayList.h#1647
The behavior before bug 1465616 isn't necessarily the best given bug 791192.

If we can solve them together, I guess it makes sense to just do so, as far as it isn't too much more complicated.
On the test page there is a 10 second delay where the url bar gets stuck before dissapearing.
That's a separate bug... but yeah. Probably I should file a bug about that.
Filed bug 1500719 for issue described in comment 26.
Could someone give an example of a page with video that's zoomable that can be used to test this bug?
The fix described in comment 24 would be a bit more involved than I initially envisioned. For correct rendering, we would need a corresponding check on the compositor side [1], which would mean propagating a "fullscreen" flag through the Layers API.

[1] https://searchfox.org/mozilla-central/rev/fcfb479e6ff63aea017d063faa17877ff750b4e5/gfx/layers/apz/src/AsyncPanZoomController.cpp#3951
I have tried instead the approach described in comment 22, and it seems to be working well. Patch coming up.
Assignee: nobody → botond
Flags: needinfo?(botond)
The previous resolution is restored when exiting fullscreen mode.

Depends on D9442
(The first patch isn't related to the fix, it's just a refactoring I made while trying the other fix approach, and figured I might as well land it.)
Despite the complexity should your original idea be considered in the future?
(In reply to csheany from comment #36)
> Despite the complexity should your original idea be considered in the future?

Xidorn's approach (which is reflected in the patch I posted) fixes both this bug and bug 791192. If it works out, then there is no reason to consider my original idea (which would only fix this bug).
As the reporter obviously I am happy to see it get fixed.

It surprised me that it went unnoticed for so long.

Does it make sense in the context of what was intended?
(In reply to csheany from comment #38)
> It surprised me that it went unnoticed for so long.

This regression is new in 63, which so far has only been on the Nightly and Beta channels. The Firefox for Android user population on those channels is fairly small, so unfortunately regressions can go unnoticed for some time.

> Does it make sense in the context of what was intended?

Not sure if I fully understand the question, but let me try:

The intention of bug 656036 (of which bug 1465616, the regressing bug, was a part) was to change how position:fixed elements are laid out. Instead of being attached to the visual viewport (the screen), they are now attached to the layout viewport (see [1] for some background on these concepts).

This is a deliberate change we made to become more consistent with other mobile browsers like Chrome, which already behave this way.

I was aware that there may be uses of position:fixed in the wild which specifically want the previous behaviour, and which would therefore need to adapt. However, such uses would already be broken with Chrome, so I figured they are likely to be rare. Obviously, I was not aware of this particular use of position:fixed in the implementation of Firefox's fullscreen mode (where Chrome compatibility does not play a role since it's a Firefox-internal use).

[1] http://bokand.github.io/viewport/index.html
I'm proud to be one of the few.

Thank you for your explanation. I have been trying to comprehend what's going on.

I also appreciate your help in solving the problem.

This was definitely a learning experinece.

Initially I didn't think about it being related to zooming as much as entering fullscreen

Now I'm going to have to get used to it not happening.
Pushed by bballo@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/fb10b0a27752
Define OutOfFlowDisplayData::ComputeVisibleRectForFrame() out of line. r=kats
https://hg.mozilla.org/integration/autoland/rev/a4e64a2df8a9
Reset the resolution to 1 when entering fullscreen mode. r=kats,xidorn
> I believe we don't need to worry about non-mobile non-e10s. What simplification did you have in mind in this case?

So, for mobile or e10s, I think one stuff being always true is that the root document is always the root content document if its content document.

That means we can handle do the updating in nsIDocument::HandlePendingFullscreenRequests, specifically, when `handled` is true. We can just get the root document from aDocument, and set the zoom level there. Similarly, we can handle the exiting only in nsIDocument::ExitFullscreenInDocTree. Those two functions are always and only called when the root document enter / leave fullscreen. So as far as a content document wouldn't have a non-content root, these two places should always work.

Alternatively, the logic may be combined with FullscreenRoots::{Add,Remove}, which handles root fullscreen document being added / removed. And we can even store the backup zoom level inside the FullscreenRoots rather than a per-document field, since we really only need it per root fullscreen document not per document.

What do you think?
Flags: needinfo?(botond)
Duplicate of this bug: 791192
https://hg.mozilla.org/mozilla-central/rev/fb10b0a27752
https://hg.mozilla.org/mozilla-central/rev/a4e64a2df8a9
Status: NEW → RESOLVED
Closed: 10 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla65
Here is my attempt to implement the suggested simplification.

It doesn't seem to be working for me (I remain zoomed in during fullscreen). I haven't had a chance to debug it yet, but perhaps there is some obvious way in which I misunderstood the suggestion?
Flags: needinfo?(botond)
In Nightly 65.0 10-25-18 there is a definite improvement.

Is there a reason it works in most cases but not all?

Should the page's zoom level be restored?
Ironically when the video is still zoomed in, the page's zoom level is restored.
The behavior is inconsistent using the controls or context menu as well as if the video is playing or paused.
Duplicate of this bug: 1493323
(In reply to csheany from comment #46)
> Is there a reason it works in most cases but not all?

There could be a bug in the implementation.

If you have steps to trigger a case where it doesn't work, please post them.

> Should the page's zoom level be restored?

Yes, it should.
(In reply to Botond Ballo [:botond] from comment #45)
> Here is my attempt to implement the suggested simplification.
> 
> It doesn't seem to be working for me (I remain zoomed in during fullscreen).
> I haven't had a chance to debug it yet, but perhaps there is some obvious
> way in which I misunderstood the suggestion?

It looks like the reason this is not working, is that |root| in FullscreenRoots::Add() is not the RCD. I guess that makes sense, since it's obtained using nsContentUtils::GetRootDocument(), which gets the root document in the process, and Fennec is not e10s so the root document is a chrome document.

Does that mean this suggestion to use FullscreenRoots is not viable? Or did I misunderstand something?
Flags: needinfo?(xidorn+moz)
Highwebmedia.com is an example that is still affected.
(In reply to csheany from comment #52)
> Highwebmedia.com is an example that is still affected.

The fullscreen functionality on this website does not appear to use the Fullscreen API. The patch in this bug only affects use of the Fullscreen API.

==> Please file a different bug.

(I guess that also gives rise to a better answer to your earlier question, "is there a reason it works in most cases but not all": the patch in this bug is only expected to affect websites that use the Fullscreen API).
What would you suggest if it is different than this one?
(In reply to csheany from comment #54)
> What would you suggest if it is different than this one?

File a new bug -> we'll find a regression range for it (or you could, if you're willing to) -> we'll identify the appropriate next steps based on what the regressing bug is.
I guess what I meant is that I don't know what it would say.
(In reply to csheany from comment #56)
> I guess what I meant is that I don't know what it would say.

What the new bug would say? Assuming I'm understanding the issue correctly, something like this:

===

Steps to reproduce:

  1. Visit [URL]
  2. [Steps to navigate to video]
  3. Zoom the page in
  4. Play the video
  5. Click the fullscreen button in the video controls

Expected results:

  Video is sized to fit the screen

Actual results:

  Video is zoomed so it's larger than the screen

===

with the parts in square brackets filled in with specifics.
And that is not the same as this. What don't I understand.

What API do you think it is using?
(In reply to csheany from comment #58)
> And that is not the same as this. What don't I understand.

It might be a regression from the same bug (bug 1465616), but it may need to be addressed a different way (e.g. with a different fix, or it may be an evangelism issue where it's the site that needs to change). So it makes sense to handle it in a different bug.

> What API do you think it is using?

Perhaps the site is emulating fullscreen mode by applying position:fixed itself. In that case, we'd want to see what the behaviour is with Chrome (since in bug 1465616 the intention was to adopt behaviour that matches Chrome). If Chrome has the same bug, the site will need to change. If not, we need to investigate and understand why the two browsers behave differently.
Thank you for bearing with me.

The discussion can continue here: Bug 1502566
(In reply to Botond Ballo [:botond] from comment #51)
> (In reply to Botond Ballo [:botond] from comment #45)
> > Here is my attempt to implement the suggested simplification.
> > 
> > It doesn't seem to be working for me (I remain zoomed in during fullscreen).
> > I haven't had a chance to debug it yet, but perhaps there is some obvious
> > way in which I misunderstood the suggestion?
> 
> It looks like the reason this is not working, is that |root| in
> FullscreenRoots::Add() is not the RCD. I guess that makes sense, since it's
> obtained using nsContentUtils::GetRootDocument(), which gets the root
> document in the process, and Fennec is not e10s so the root document is a
> chrome document.
> 
> Does that mean this suggestion to use FullscreenRoots is not viable? Or did
> I misunderstand something?

Oh interesting, so mobile still has a non-content top level document... Then I guess we probably cannot do that optimization.
Flags: needinfo?(xidorn+moz)
See Also: → 1502566
Comment on attachment 9019169 [details]
Bug 1493976 - Reset the resolution to 1 when entering fullscreen mode. r?xidorn,kats

[Beta/Release Uplift Approval Request]

Feature/Bug causing the regression: Bug 1465616

User impact if declined: When page is zoomed in, activating fullscreen mode on a video makes the video larger than the screen.
If the user wants the video to fit the screen, they have to zoom out before entering fullscreen mode.

Is this code covered by automated tests?: No

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): Straightforward fix, some potential for unexpected interactions but I'd be comfortable taking it at this early stage in the beta cycle.

String changes made/needed:
Attachment #9019169 - Flags: approval-mozilla-beta?
(In reply to Botond Ballo [:botond] from comment #62)
> Comment on attachment 9019169 [details]
> Bug 1493976 - Reset the resolution to 1 when entering fullscreen mode.
> r?xidorn,kats

Note: request applies to just that one patch. (The other patch is an unrelated refactoring that's not necessary for the fix.)
Comment on attachment 9019169 [details]
Bug 1493976 - Reset the resolution to 1 when entering fullscreen mode. r?xidorn,kats

[Triage Comment]
Fixes broken scaling of videos in fullscreen mode if the page was zoomed beforehand. Approved for 64.0b5.
Attachment #9019169 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Botond, could you also request uplift to mozilla-release? We may take it as a ride-along. Thanks.
Flags: needinfo?(botond)
Verified as fixed on the latest Nightly build (65.0a1) and 64.0b5 with the following devices:
- Nokia 6(Android 7.1.1)
- Google Pixel(Android 9)
- Sony Xperia Z2 (Android 6.0.1)
(In reply to Pascal Chevrel:pascalc from comment #66)
> Botond, could you also request uplift to mozilla-release? We may take it as
> a ride-along. Thanks.

Unlike bug 1493742, I don't have a sufficient level of familiarity with this code to say with confidence that this is low-risk enough to uplift to m-r. Xidorn, what do you think?
Flags: needinfo?(botond) → needinfo?(xidorn+moz)
It's very rare to see uplift to release version, so I don't really have a good mental model about that either.

Although I know the code path of fullscreen, I'm not familiar with the code path of SetResolutionAndScaleTo, and it's totally unclear to me how they may potentially interact with each other, so I'd say I don't have enough confidence that this is low-risk to release either.
Flags: needinfo?(xidorn+moz)
If you are both uncomfortable uplifting this patch, I think it's better the let it ride the trains and mark it as wontfix for 63, thanks!
Depends on: 1502566
See Also: 1502566
(In reply to Pascal Chevrel:pascalc from comment #70)
> If you are both uncomfortable uplifting this patch, I think it's better the
> let it ride the trains and mark it as wontfix for 63, thanks!

(I think we made a good call, as bug 1502566 appears to be a regression from this.)
Status: RESOLVED → VERIFIED
Depends on: 1511137
Depends on: 1516471
Summary: Fullscreen video is zoomed in if the page is → Zooming in on the page causes fullscreen video to be zoomed in also
You need to log in before you can comment on or make changes to this bug.