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

Status: VERIFIED FIXED

(Core :: Panning and Zooming, defect)

Description

[csheany]

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

Comment 1

[csheany]

Attachment: Screenshot_20180925-095936.jpg

Comment 2

[csheany]

Attachment: Screenshot_20180925-095751.jpg

Comment 4

[Stefan Deiac]

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.

Comment 5

[csheany]

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.

Comment 6

[csheany]

Still present in the latest Nightly.

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

Are there any related bugs?

Comment 7

[Xidorn Quan [:xidorn] UTC+10]

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

Comment 8

[csheany]

I believe I tracked down the regression.

The last good build was 2018-08-19

The first bad build was 2018-08-20

Comment 9

[Xidorn Quan [:xidorn] UTC+10]

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

Comment 10

[csheany]

Is this the pushlog range?

Comment 11

[csheany]

Attachment: Screenshot_20181019-221204.jpg

Comment 12

[Xidorn Quan [:xidorn] UTC+10]

Could you paste the link of the pushlog range here?

Comment 13

[csheany]

https://hg.mozilla.org/mozilla-central/pushloghtml?startdate=3+months+ago&enddate=1+month+ago

Comment 14

[csheany]

https://hg.mozilla.org/mozilla-central/pushloghtml?startdate=63+days+ago&enddate=59+days+ago

Comment 15

[Xidorn Quan [:xidorn] UTC+10]

That regression range doesn't seem to be very useful. I'll try to find the regression, so ni? me for now.

Comment 16

[csheany]

Are you sure? https://bugzilla.mozilla.org/show_bug.cgi?id=1465616 seems to stick out to me.

Comment 17

[csheany]

Did you notice the second link?

Comment 18

[Xidorn Quan [:xidorn] UTC+10]

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.

Comment 19

[Botond Ballo [:botond]]

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.

Comment 20

[Kartikaya Gupta (email:[e-mail])]

https://bugzilla.mozilla.org/show_bug.cgi?id=791192 is the pre-existing bug.

Comment 21

[Xidorn Quan [:xidorn] UTC+10]

(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

Comment 22

[Xidorn Quan [:xidorn] UTC+10]

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.

Comment 23

[Xidorn Quan [:xidorn] UTC+10]

Attachment: fullscreen test page

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.

Comment 24

[Botond Ballo [:botond]]

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

Comment 25

[Xidorn Quan [:xidorn] UTC+10]

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.

Comment 26

[csheany]

On the test page there is a 10 second delay where the url bar gets stuck before dissapearing.

Comment 27

[Xidorn Quan [:xidorn] UTC+10]

That's a separate bug... but yeah. Probably I should file a bug about that.

Comment 28

[Xidorn Quan [:xidorn] UTC+10]

Filed bug 1500719 for issue described in comment 26.

Comment 29

[Botond Ballo [:botond]]

Could someone give an example of a page with video that's zoomable that can be used to test this bug?

Comment 30

[csheany]

https://www.mozilla.org/en-US/firefox/

Comment 31

[Botond Ballo [:botond]]

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

Comment 32

[Botond Ballo [:botond]]

I have tried instead the approach described in comment 22, and it seems to be working well. Patch coming up.

Comment 33

[Botond Ballo [:botond]]

Attachment: Bug 1493976 - Define OutOfFlowDisplayData::ComputeVisibleRectForFrame() out of line. r?kats

Comment 34

[Botond Ballo [:botond]]

Attachment: Bug 1493976 - Reset the resolution to 1 when entering fullscreen mode. r?xidorn,kats

The previous resolution is restored when exiting fullscreen mode.

Depends on D9442

Comment 35

[Botond Ballo [:botond]]

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

Comment 36

[csheany]

Despite the complexity should your original idea be considered in the future?

Comment 37

[Botond Ballo [:botond]]

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

Comment 38

[csheany]

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?

Comment 39

[Botond Ballo [:botond]]

(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

Comment 40

[csheany]

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.

Comment 41

[Pulsebot]

Pushed by [e-mail]:
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

Comment 42

[Xidorn Quan [:xidorn] UTC+10]

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

Comment 44

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/fb10b0a27752
https://hg.mozilla.org/mozilla-central/rev/a4e64a2df8a9

Comment 45

[Botond Ballo [:botond]]

Attachment: Simplification attempt

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?

Comment 46

[csheany]

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?

Comment 47

[csheany]

Ironically when the video is still zoomed in, the page's zoom level is restored.

Comment 48

[csheany]

The behavior is inconsistent using the controls or context menu as well as if the video is playing or paused.

Comment 50

[Botond Ballo [:botond]]

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

Comment 51

[Botond Ballo [:botond]]

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

Comment 52

[csheany]

Highwebmedia.com is an example that is still affected.

Comment 53

[Botond Ballo [:botond]]

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

Comment 54

[csheany]

What would you suggest if it is different than this one?

Comment 55

[Botond Ballo [:botond]]

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

Comment 56

[csheany]

I guess what I meant is that I don't know what it would say.

Comment 57

[Botond Ballo [:botond]]

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

Comment 58

[csheany]

And that is not the same as this. What don't I understand.

What API do you think it is using?

Comment 59

[Botond Ballo [:botond]]

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

Comment 60

[csheany]

Thank you for bearing with me.

The discussion can continue here: Bug 1502566

Comment 61

[Xidorn Quan [:xidorn] UTC+10]

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

Comment 62

[Botond Ballo [:botond]]

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:

Comment 63

[Botond Ballo [:botond]]

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

[Ryan VanderMeulen [:RyanVM]]

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.

Comment 65

[Ryan VanderMeulen [:RyanVM]]

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

Comment 66

[Pascal Chevrel:pascalc]

Botond, could you also request uplift to mozilla-release? We may take it as a ride-along. Thanks.

Comment 67

[Sorina Florean [:sflorean]]

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)

Comment 68

[Botond Ballo [:botond]]

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

Comment 69

[Xidorn Quan [:xidorn] UTC+10]

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.

Comment 70

[Pascal Chevrel:pascalc]

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!

Comment 71

[Botond Ballo [:botond]]

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