csheany Reporter
	Description • 6 years ago

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

	Stefan Deiac
	Comment 4 • 6 years ago

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.

	csheany Reporter
	Comment 5 • 6 years ago

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.

	csheany Reporter
	Comment 6 • 6 years ago

Still present in the latest Nightly.

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

Are there any related bugs?

	Xidorn Quan [:xidorn] UTC+11
	Comment 7 • 6 years ago

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

	csheany Reporter
	Comment 8 • 6 years ago

I believe I tracked down the regression.

The last good build was 2018-08-19

The first bad build was 2018-08-20

	Xidorn Quan [:xidorn] UTC+11
	Comment 9 • 6 years ago

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

	csheany Reporter
	Comment 10 • 6 years ago

Is this the pushlog range?

	Xidorn Quan [:xidorn] UTC+11
	Comment 12 • 6 years ago

Could you paste the link of the pushlog range here?

	csheany Reporter
	Comment 13 • 6 years ago

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

	csheany Reporter
	Comment 14 • 6 years ago

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

	Xidorn Quan [:xidorn] UTC+11
	Comment 15 • 6 years ago

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

	csheany Reporter
	Comment 16 • 6 years ago

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

	csheany Reporter
	Comment 17 • 6 years ago

Did you notice the second link?

	Xidorn Quan [:xidorn] UTC+11
	Comment 18 • 6 years ago

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.

	Botond Ballo [:botond] Assignee
	Comment 19 • 6 years ago

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.

	Kartikaya Gupta (email:kats@mozilla.staktrace.com)
	Comment 20 • 6 years ago

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

	Xidorn Quan [:xidorn] UTC+11
	Comment 21 • 6 years ago

(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

	Xidorn Quan [:xidorn] UTC+11
	Comment 22 • 6 years ago

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.

	Xidorn Quan [:xidorn] UTC+11
	Comment 23 • 6 years ago

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.

	Botond Ballo [:botond] Assignee
	Comment 24 • 6 years ago

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

	Xidorn Quan [:xidorn] UTC+11
	Comment 25 • 6 years ago

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.

	csheany Reporter
	Comment 26 • 6 years ago

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

	Xidorn Quan [:xidorn] UTC+11
	Comment 27 • 6 years ago

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

	Xidorn Quan [:xidorn] UTC+11
	Comment 28 • 6 years ago

Filed bug 1500719 for issue described in comment 26.

	Botond Ballo [:botond] Assignee
	Comment 29 • 6 years ago

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

	csheany Reporter
	Comment 30 • 6 years ago

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

	Botond Ballo [:botond] Assignee
	Comment 31 • 6 years ago

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

	Botond Ballo [:botond] Assignee
	Comment 32 • 6 years ago

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

	Botond Ballo [:botond] Assignee
	Comment 34 • 6 years ago

The previous resolution is restored when exiting fullscreen mode.

Depends on D9442

	Botond Ballo [:botond] Assignee
	Comment 35 • 6 years ago

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

	csheany Reporter
	Comment 36 • 6 years ago

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

	Botond Ballo [:botond] Assignee
	Comment 37 • 6 years ago

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

	csheany Reporter
	Comment 38 • 6 years ago

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?

	Botond Ballo [:botond] Assignee
	Comment 39 • 6 years ago

(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

	csheany Reporter
	Comment 40 • 6 years ago

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.

	Pulsebot
	Comment 41 • 6 years ago

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

	Xidorn Quan [:xidorn] UTC+11
	Comment 42 • 6 years ago

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

	Raul Gurzau (:RaulG)
	Comment 44 • 6 years ago
bugherder

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

	Botond Ballo [:botond] Assignee
	Comment 45 • 6 years ago

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?

	csheany Reporter
	Comment 46 • 6 years ago

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?

	csheany Reporter
	Comment 47 • 6 years ago

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

	csheany Reporter
	Comment 48 • 6 years ago

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

	Botond Ballo [:botond] Assignee
	Comment 50 • 6 years ago

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

	Botond Ballo [:botond] Assignee
	Comment 51 • 6 years ago

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

	csheany Reporter
	Comment 52 • 6 years ago

Highwebmedia.com is an example that is still affected.

	Botond Ballo [:botond] Assignee
	Comment 53 • 6 years ago

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

	csheany Reporter
	Comment 54 • 6 years ago

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

	Botond Ballo [:botond] Assignee
	Comment 55 • 6 years ago

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

	csheany Reporter
	Comment 56 • 6 years ago

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

	Botond Ballo [:botond] Assignee
	Comment 57 • 6 years ago

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

	csheany Reporter
	Comment 58 • 6 years ago

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

What API do you think it is using?

	Botond Ballo [:botond] Assignee
	Comment 59 • 6 years ago

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

	csheany Reporter
	Comment 60 • 6 years ago

Thank you for bearing with me.

The discussion can continue here: Bug 1502566

	Xidorn Quan [:xidorn] UTC+11
	Comment 61 • 6 years ago

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

	Botond Ballo [:botond] Assignee
	Comment 62 • 6 years ago

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:

	Botond Ballo [:botond] Assignee
	Comment 63 • 6 years ago

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

	Ryan VanderMeulen [:RyanVM]
	Comment 64 • 6 years ago

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.

	Ryan VanderMeulen [:RyanVM]
	Comment 65 • 6 years ago
bugherder uplift

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

	Pascal Chevrel:pascalc (PTO until August 21)
	Comment 66 • 6 years ago

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

	Sorina Florean [:sflorean]
	Comment 67 • 6 years ago

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)

	Botond Ballo [:botond] Assignee
	Comment 68 • 6 years ago

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

	Xidorn Quan [:xidorn] UTC+11
	Comment 69 • 6 years ago

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.

	Pascal Chevrel:pascalc (PTO until August 21)
	Comment 70 • 6 years ago

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!

	Botond Ballo [:botond] Assignee
	Comment 71 • 6 years ago

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