1502566 - Fullscreen video on desktop sites is zoomed in [NSFW website]

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: Audio/Video, defect)

Description

[csheany]

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

Steps to reproduce:

 1. Visit highwebmedia.com
2. Open a link and play the video
3. Click the fullscreen button in the video controls


Actual results:

Video is zoomed so it's larger than the screen 


Expected results:

Video is sized to fit the screen

Comment 1

[Botond Ballo [:botond]]

(In reply to csheany from comment #0)
> 1. Visit highwebmedia.com

As a courtesy to developers/testers who may be investigating this bug in e.g. a crowded office, please annotate NSFW links as such.

Assuming this is a regression, would you be willing to find a regression range for it?

Comment 2

[csheany]

Please forgive the oversight in regard to content. I had hoped to provide a different example.

Thank you for editing the summary accordingly.

The regression may very well stem from the original bug.

However unlike before it doesn't seem to matter if the page is zoomed in or not.

I do notice that the page is zoomed in after exiting full screen

Comment 3

[Sorina Florean [:sflorean]]

Hi, we have the bug 1476225 filed for zoomed in pages (e.g. Amazon) but don't imply fullscreen mode. Based on your latest comment, could you please test bug 1476225 to see if it is the same behavior, so we can check for a regression window? Thanks

Comment 4

[csheany]

Bug 1476225 applies to the page as well

Comment 5

[csheany]

With apz.allow_zooming set to false:

In portrait mode the video is still larger than the screen.

In landscape mode the video is smaller than the screen with the page visible along the bottom and right edges.

Comment 6

[Botond Ballo [:botond]]

apz.allow_zooming=false is not a supported configuration on mobile. The behaviour with that pref off doesn't really tell us anything useful.

(That pref exists mostly to gate zooming-related functionality on desktop builds, where it's not enabled yet.)

Comment 7

[csheany]

If the page is already in landscape fullscreen isn't affected.

Comment 8

[csheany]

Can you clarify what you meant by not supported on mobile?

Comment 9

[Botond Ballo [:botond]]

(In reply to csheany from comment #8)
> Can you clarify what you meant by not supported on mobile?

We have a lot of prefs in about:config. Most of them are meant for development purposes only, and many combinations of these prefs are not supported (meaning, there is no expectation of the browser behaving in a reasonable way with such combination). This is why there is a "This might void your warranty!" warning message in about:config (at least on the desktop version).

In the case of apz.allow_zooming, setting it to false is not supported on mobile, as we intend to always allow zooming on mobile (subject to page-specific restrictions like user-scalable=no in a meta viewport tag).

Comment 10

[csheany]

I mentioned it because there was a clear difference but it sounds like there shouldnt't have been.

Comment 11

[Botond Ballo [:botond]]

(In reply to csheany from comment #10)
> I mentioned it because there was a clear difference but it sounds like there
> shouldnt't have been.

Let me try to put it a different way: when you run Firefox with an unsupported combination of prefs, "anything can happen". The behaviour may very well be different from normal behaviour, and it may very well be wrong; that much is expected.

Comment 12

[csheany]

Thank you explaining. I do appreciate it.

I don't like to mess with too much for that very reason.

I figured it was relevant with APZ being the functionality in question.

I am trying to provide as much information as I can.

Comment 13

[csheany]

The last good build was 2018-10-24.

The first bad build was 2018-10-25.

Comment 14

[Botond Ballo [:botond]]

(In reply to csheany from comment #13)
> The last good build was 2018-10-24.
> 
> The first bad build was 2018-10-25.

Are you sure about this range? I tested the 2018-10-24 nightly, and I can still reproduce the issue.

If you're sure about this range, then we may be trying different STR, or have some difference between our environments.

Comment 15

[csheany]

Pretty sure. I hope environments aren't a factor.

Are you zooming in at all?

Comment 16

[Botond Ballo [:botond]]

(In reply to csheany from comment #15)
> I hope environments aren't a factor.

Tablet vs. phone might be a factor.

Screen orientation might also be a factor. Are you in portrait or landscape orientation when testing this?

> Are you zooming in at all?

I was, though re-reading comment 0 I see zooming isn't part of the STR.

I just tried again without zooming on the 2018-10-24 nightly, and I still see the issue.

Comment 17

[csheany]

I tested both modes with only portrait causing an issue.

Did you try the mobile and desktop version of the site?

Comment 18

[Botond Ballo [:botond]]

(In reply to csheany from comment #17)
> I tested both modes with only portrait causing an issue.
> 
> Did you try the mobile and desktop version of the site?

I was trying the mobile version. With the desktop version, I still see the issue, but only if I zoom in first.

Portrait vs. landscape mode doen't seem to make a difference with the desktop version of the site. With the mobile version, the STR only seem applicable in portrait mode (in landscape mode, the video is the only thing you see and it's automatically fullscreen, without a button to enter/exit fullscreen mode).

Could you clarify which version of the site (mobile vs. desktop) you used to find the regression range in comment 13?

Comment 19

[csheany]

I found the regression with the desktop version.

The patch that fixed Bug 1493976 may have had a negative impact.

I'm also noticing another issue. I can't zoom in with the accessibilty toggle off.

Comment 20

[Botond Ballo [:botond]]

(In reply to csheany from comment #19)
> I found the regression with the desktop version.

Ok, thanks. Re-testing with the desktop version of the site, I can now confirm the regression range you found.

To clarify the STR:

  1. Visit highwebmedia.com
  2. Check menu -> Request desktop site
  3. Do not zoom in
  4. As before: open a link, play video, click fullscreen button

With this STR:

  - In the 2018-10-24 nightly, the video is sized to the screen
  - In the 2018-10-25 nightly, the video is larger that the screen

(Step 2 was the important part missing from the original STR. Step 3 is important for the bug _not_ to occur on the 2018-10-24 build; that was my mistake for misreading the original STR.)

> The patch that fixed Bug 1493976 may have had a negative impact.

Agreed.

Specifically, I think the issue is that "1" is not always the resolution that will get the video to be sized to the screen.

> I'm also noticing another issue. I can't zoom in with the accessibilty
> toggle off.

Different bug please :)

Comment 21

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] from comment #20)
> Specifically, I think the issue is that "1" is not always the resolution
> that will get the video to be sized to the screen.

The resolution we want is the "intrinsic scale": the resolution that makes the layout viewport and visual viewport coincide.

Comment 22

[csheany]

I wasn't thinking about the mobile site as tablets default to desktop. 

I didn't mean to say negative but rather unintended. 

For the accessibility issue (Bug 1503742).

Comment 23

[Botond Ballo [:botond]]

(In reply to csheany from comment #22)
> I wasn't thinking about the mobile site as tablets default to desktop.

Ah, I didn't realize that. My bad all around then.

Anyways, glad we got to the bottom of it :)

Comment 24

[Botond Ballo [:botond]]

Attachment: Bug 1502566 - When entering fullscreen mode, use the intrinsic resolution rather than a resolution of 1. r?kats

Comment 25

[Botond Ballo [:botond]]

The attached patch fixes the issue for me locally.

Comment 26

[csheany]

I also experience similar behavior on the desktop version of YouTube. Had I noticed it in the first place I might have filed with regard to that instance. Would you mind checking that as well?

I also notice the controls change but I suppose you want  me to file a seperate bug for that.

Comment 27

[Botond Ballo [:botond]]

(In reply to csheany from comment #26)
> I also experience similar behavior on the desktop version of YouTube. Had I
> noticed it in the first place I might have filed with regard to that
> instance. Would you mind checking that as well?

Confirmed that the desktop version of YouTube has the same issue, and that this patch fixes it as well.

In general, now that I understand what the bug is, I would expect most desktop sites with video to be affected.

> I also notice the controls change but I suppose you want  me to file a
> seperate bug for that.

Yes please. (When filing the bug, please elaborate on what you mean by "change".)

Comment 28

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/518912e01f0c
When entering fullscreen mode, use the intrinsic resolution rather than a resolution of 1. r=kats

Comment 29

[csheany]

Can you elaborate on the bug?

With regard to the controls, what I am seeing is that when the page the is first opened they are desktop-like and after entering fullscreen they are third party mobile which I guess is expected but after exiting fullscreen they remain third party mobile without the abitlity to enter fullscreen again instead of reverting to destop-like.

Comment 30

[Botond Ballo [:botond]]

(In reply to csheany from comment #29)
> Can you elaborate on the bug?

I'm not sure what you mean.

> With regard to the controls, what I am seeing is that when the page the is
> first opened they are desktop-like and after entering fullscreen they are
> third party mobile which I guess is expected but after exiting fullscreen
> they remain third party mobile without the abitlity to enter fullscreen
> again instead of reverting to destop-like.

Thanks - but please post this information in the new bug, not here.

Comment 31

[csheany]

Why desktop sites were affected and about resolution?

I wanted to see if you could reproduce before doing so. I will also probably wait until this patch lands.

Comment 32

[Botond Ballo [:botond]]

(In reply to csheany from comment #31)
> Why desktop sites were affected and about resolution?

Sure. Continuing from bug 1493976 comment 39:

The way I fixed bug 1493976 is by temporarily changing the resolution (the zoom level in mobile browsers) to a value that would make the layout viewport and visual viewport have the same size. This way, even though the position:fixed element (the fullscreen element) is sized to the layout viewport, that size is now the same as the visual viewport.

The mistake I made in bug 1493976 is thinking that the resolution that makes the two viewports the same size is always "1". This is only true on sites which have a <meta name="viewport" content="..."> tag where the content includes "width=device-width". Mobile sites tend to have such a tag, but desktop sites do not. On desktop sites, the layout viewport gets a default width or 980px, which is wider than many screens, so a different resolution value (typically smaller than 1) is needed to get the two viewports to be the same size.

> I wanted to see if you could reproduce before doing so. I will also probably
> wait until this patch lands.

As you've seen from recent examples, reproducing a bug sometimes requires some back-and-forth discussion to clarify the steps. It keeps things more organized to have such discussion in its own bug.

If the new bug ends up being invalid, or the same issue as another bug, that's fine - we have "RESOLVED INVALID" and "RESOLVED DUPLICATE" for those cases.

(Waiting until this patch lands is fine. Thanks!)

Comment 33

[csheany]

Knowing what you know now would you have approached the previous bug differently?

Does this patch resolve mobile sites as well? Should the other patch get backed out?

Comment 34

[Botond Ballo [:botond]]

(In reply to csheany from comment #33)
> Knowing what you know now would you have approached the previous bug
> differently?

No, I believe the approach taken there is still the correct one. We just had to use the correct resolution.

> Does this patch resolve mobile sites as well?

Yep.

> Should the other patch get backed out?

No, this patch is incremental (meant to apply on top of the other one).

Comment 35

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/518912e01f0c

Comment 36

[Ryan VanderMeulen [:RyanVM]]

Please nominate this for Beta approval when you get a chance.

Comment 37

[Botond Ballo [:botond]]

Comment on attachment 9022258 [details]
Bug 1502566 - When entering fullscreen mode, use the intrinsic resolution rather than a resolution of 1. r?kats

[Beta/Release Uplift Approval Request]

Feature/Bug causing the regression: Bug 1493976

User impact if declined: In mobile Firefox browsers, when entering fullscreen mode on a desktop-mode website, the page contents (e.g. the fullscreen video) is zoomed in so it's larger than the screen. This happens even if you were not zoomed in at the time of entering fullscreen mode.

Is this code covered by automated tests?: No

Has the fix been verified in Nightly?: No

Needs manual test from QE?: Yes

If yes, steps to reproduce: Note, SFW STR are now available:
  1. Load youtube.com in Firefox for Android
  2. Menu -> request desktop site
  3. Change to landscape orientation
      (For me, this step is necessary for the fullscreen button to be responsive
       in the first place. That may be an unrelated bug.)
   4. Press the fullscreen button on the video

List of other uplifts needed: None

Risk to taking this patch: Low

Why is the change risky/not risky? (and alternatives if risky): It's well-understood how bug 1493976 caused this regression, and the fix is straightforward.

String changes made/needed:

Comment 38

[Julien Cristau [:jcristau]]

Comment on attachment 9022258 [details]
Bug 1502566 - When entering fullscreen mode, use the intrinsic resolution rather than a resolution of 1. r?kats

regression fix, in nightly for a few days, approved for 64.0b8

Comment 39

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/releases/mozilla-beta/rev/8219b9f7f288

Comment 40

[Mira Lobontiu (Android QA)]

Verified as fixed in build 64.0b9 and Nightly 65.0a1 (2018-11-12), following the STR from Comment 37.
Removing the qe-verify+ flag.

Device: Sony Xperia Z5 (Android 6.0.1)

Thank you!