Closed Bug 1304052 Opened 4 years ago Closed 4 years ago

The fullscreen videos are zoomable

Categories

(Firefox for Android :: Toolbar, defect, P1)

48 Branch
All
Android
defect

Tracking

()

VERIFIED FIXED
Firefox 52
Tracking Status
firefox49 --- fix-optional
firefox50 --- verified
firefox51 --- verified
firefox52 --- verified

People

(Reporter: alex.caziuc, Assigned: kats)

References

()

Details

(Keywords: qablocker, regression)

Attachments

(2 files)

Tested using Galaxy Note 5(6.0.1) with Aurora 51(2016-09-20)

STR:
1. Go to goo.gl/xNbxO8 or webmfiles.org/demo-files
2. Play a video -> enter fullscreen
3. Zoom in the video

Actual:
You can zoom the video(see also that the video controls are zoomed)

Expected:
You should not be able to zoom the video


Note: It does not reproduce on youtube videos.
47 Release is good build.
Version: 52 Branch → Trunk
Regression window:

good build:2015-11-30
bad build:2015-12-01

http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=47b49b0d32360fab04b11ff9120970979c426911&tochange=66a6d7ec9534b9d7847b665142fef0dd87623768

This was caused by the APZ feature, since Bug 1206872 - [meta] Enable APZ on fennec on nightly builds is in the pushlog; and the Release builds are affected from Firefox 48, Bug 1206874 - (fennec-aboard-apz) [meta] Let APZ on Fennec ride the trains was fixed in 48
Per comment 1, Change the component.
Component: Audio/Video → Graphics, Panning and Zooming
Hi Kats, could you help investigate this one? 

(This doesn't seem to be a dot release worthy issue for Fx49, more like a fix-optional.)
Flags: needinfo?(bugmail)
I'll take a look.
Assignee: nobody → bugmail
Flags: needinfo?(bugmail)
Version: Trunk → 48 Branch
This used to be handled in JavaPanZoomController, at [1]. I'll try updating ZoomConstraintsClient to disallow zooming if the document is in fullscreen.

[1] https://hg.mozilla.org/releases/mozilla-release/file/0596e83f0658/mobile/android/base/java/org/mozilla/gecko/gfx/JavaPanZoomController.java#l1177
Comment on attachment 8793478 [details]
Bug 1304052 - Disable APZ zooming when the document has any fullscreen state.

https://reviewboard.mozilla.org/r/80210/#review78866

::: layout/base/ZoomConstraintsClient.cpp:218
(Diff revision 1)
>      ViewAs<ScreenPixel>(screenSize, PixelCastJustification::LayoutDeviceIsScreenForBounds));
>  
>    mozilla::layers::ZoomConstraints zoomConstraints =
>      ComputeZoomConstraintsFromViewportInfo(viewportInfo);
>  
> +  if (mDocument->Fullscreen()) {

Might it make sense to handle this in GetViewportInfo()?
Attachment #8793478 - Flags: review?(botond) → review+
(In reply to Botond Ballo [:botond] from comment #7)
> > +  if (mDocument->Fullscreen()) {
> 
> Might it make sense to handle this in GetViewportInfo()?

Hmm, I'd prefer not to. GetViewportInfo is well-defined in that it returns the contents of the meta-viewport tag, nothing more and nothing less. In my mind the fullscreen behaviour is a layer above that.
Pushed by kgupta@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/ad59137b2fee
Disable APZ zooming when the document has any fullscreen state. r=botond
You should nit listen the prefixed fullscreenchange event. Instead, you should listen the unprefixed event in the system group. Listening an event in normal group means content can capture and prevent the native code from getting that event.
https://hg.mozilla.org/mozilla-central/rev/ad59137b2fee
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 52
(In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #10)
> You should nit listen the prefixed fullscreenchange event. Instead, you
> should listen the unprefixed event in the system group. Listening an event
> in normal group means content can capture and prevent the native code from
> getting that event.

Thanks for the heads up. I'll write a followup patch to correct that.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attachment #8793750 - Flags: review?(xidorn+moz)
Priority: -- → P1
Comment on attachment 8793750 [details] [diff] [review]
Part 2 - Update listener

Review of attachment 8793750 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM
Attachment #8793750 - Flags: review?(xidorn+moz) → review+
Pushed by kgupta@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/20395f99a3d6
Use the unprefixed fullscreenchange event, and register the listener in the system group. r=xidorn
https://hg.mozilla.org/mozilla-central/rev/20395f99a3d6
Status: REOPENED → RESOLVED
Closed: 4 years ago4 years ago
Resolution: --- → FIXED
Verified as fixed on Galaxy Note 5(6.0.1) with Nightly 52(2016-09-22)
Comment on attachment 8793478 [details]
Bug 1304052 - Disable APZ zooming when the document has any fullscreen state.

Approval Request Comment
[Feature/regressing bug #]: APZ
[User impact if declined]: When viewing videos in fullscreen, the user can pinch-zoom. The video resizes in response to the pinch-zoom but then snaps back to the normal fullscreen size. The user experience isn't that great.
[Describe test coverage new/current, TreeHerder]: no automated test coverage
[Risks and why]: pretty low risk, small patch that's well understood
[String/UUID change made/needed]: none
Attachment #8793478 - Flags: approval-mozilla-beta?
Attachment #8793478 - Flags: approval-mozilla-aurora?
Attachment #8793750 - Flags: approval-mozilla-beta?
Attachment #8793750 - Flags: approval-mozilla-aurora?
Comment on attachment 8793478 [details]
Bug 1304052 - Disable APZ zooming when the document has any fullscreen state.

Fix was verified on Nightly, Aurora51+, Beta50+
Attachment #8793478 - Flags: approval-mozilla-beta?
Attachment #8793478 - Flags: approval-mozilla-beta+
Attachment #8793478 - Flags: approval-mozilla-aurora?
Attachment #8793478 - Flags: approval-mozilla-aurora+
Attachment #8793750 - Flags: approval-mozilla-beta?
Attachment #8793750 - Flags: approval-mozilla-beta+
Attachment #8793750 - Flags: approval-mozilla-aurora?
Attachment #8793750 - Flags: approval-mozilla-aurora+
Verified as fixed on Galaxy Note 5(6.0.1) with Aurora 51(2016-09-24)
Verified as fixed using:
Device: Moto X (Android 4.4.4)
Build: Firefox for Android 50 beta 2
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.