csheany Reporter
	Description • 6 years ago

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

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

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

	csheany Reporter
	Comment 2 • 6 years ago

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

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

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

	csheany Reporter
	Comment 4 • 6 years ago

Bug 1476225 applies to the page as well

	csheany Reporter
	Comment 5 • 6 years ago

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.

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

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

	csheany Reporter
	Comment 7 • 6 years ago

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

	csheany Reporter
	Comment 8 • 6 years ago

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

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

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

	csheany Reporter
	Comment 10 • 6 years ago

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

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

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

	csheany Reporter
	Comment 12 • 6 years ago

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.

	csheany Reporter
	Comment 13 • 6 years ago

The last good build was 2018-10-24.

The first bad build was 2018-10-25.

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

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

	csheany Reporter
	Comment 15 • 6 years ago

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

Are you zooming in at all?

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

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

	csheany Reporter
	Comment 17 • 6 years ago

I tested both modes with only portrait causing an issue.

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

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

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

	csheany Reporter
	Comment 19 • 6 years ago

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.

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

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

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

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

	csheany Reporter
	Comment 22 • 6 years ago

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

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

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

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

The attached patch fixes the issue for me locally.

	csheany Reporter
	Comment 26 • 6 years ago

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.

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

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

	Pulsebot
	Comment 28 • 6 years ago

Pushed by bballo@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/518912e01f0c
When entering fullscreen mode, use the intrinsic resolution rather than a resolution of 1. r=kats

	csheany Reporter
	Comment 29 • 6 years ago

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.

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

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

	csheany Reporter
	Comment 31 • 6 years ago

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.

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

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

	csheany Reporter
	Comment 33 • 6 years ago

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?

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

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

	Stefan Hindli [:stefan_hindli]
	Comment 35 • 6 years ago
bugherder

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

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

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

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

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:

	Julien Cristau [:jcristau]
	Comment 38 • 6 years ago

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

	Andreea Pavel [:apavel]
	Comment 39 • 6 years ago
bugherder uplift

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

	miralobontiu
	Comment 40 • 6 years ago

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!