New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 760737 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
no longer working on chrome
Closed: Oct 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Android
Pri: 1
Type: Bug

Blocked on:
issue 761613



Sign in to add a comment

Video doesn't exit from full screen mode when we rotate the device from landscape-to-portrait

Project Member Reported by prashanthpola@chromium.org, Aug 30 2017

Issue description

Application Version (from "Chrome Settings > About Chrome"):61.0.3163.72
Android Build Number (from "Android Settings > About Phone/Tablet"):MMB29K
Device: Samsung Galaxy J7 (SM-J700H)

Steps to reproduce: 
1. Launch Chrome
2. Enable the flag "Rotate-to-fullscreen gesture for videos" from Chrome://flags
3. Visit a page with native controls, e.g. YouTube or(https://mounirlamouri.github.io/sandbox/media/dynamic-controls.html)
4. Start playing the video
5. Rotate the phone from portrait-to-landscape
6. Rotate the phone landscape-to-portrait 

Observed behavior: 
Video is not exiting from Full screen mode

Expected behavior: 
Video should exit from full screen


Frequency:100%


Additional comments: 
**Reproducible in most of the devices eg: Samsung Galaxy J1(SM-J100H)/KTU84P,Samsung Galaxy Ace 3(GT-S7262)/JZO54K


 
Labels: ReleaseBlock-Stable
@logs,Video: http://go/chrome-androidlogs1/7/760737

And will provide bisect info soon.
Cc: mlamouri@chromium.org
Owner: joh...@chromium.org

Comment 3 by aluo@chromium.org, Aug 30 2017

Labels: -Pri-2 Needs-Bisect Pri-1 Type-Bug-Regression
We need this fixed in trunk by Friday at the absolute latest, PTAL ASAP.  Note that if the fix is super complex, we will not launch this feature in M61 and will punt to M62, as opposed to taking a risky patch.  Ping me if you want to discuss.

Comment 5 by joh...@chromium.org, Aug 31 2017

Labels: -Restrict-View-Google -Needs-Bisect
Status: Started (was: Assigned)
Thanks for the report! I investigated, and this occurs on devices that lack the sensors necessary for the Device Orientation API. InertialSensor.DeviceOrientationSensorAndroid shows that there are a lot of such devices. I'll put together a patch to disable the feature on such devices.
Labels: -M-61 M-62
We are disabling this feature for M61 and will ship in M62, so adjusting milestone to match.
Labels: -Type-Bug-Regression Type-Bug
It is working fine on below device:
Samsung Note3(SM-N900)/JSS15J
Samsung Galaxy grand neo plus/KTU84P
Zenfone 2/LRX21V
Redmi Note3 /MMB29M
Samsung Galaxy S5 /MMB29K
Nexus 5x/N2G48E
Samsung Galaxy S8/NRD90M
pixel XL/OPR1.170623.011
MOTO G4 Plus/NPJ95.93-14
HTC U 11 / 7.1.1
Project Member

Comment 8 by bugdroid1@chromium.org, Sep 1 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/48a2cccd53bdd18aca0360695df835ed84d4fec5

commit 48a2cccd53bdd18aca0360695df835ed84d4fec5
Author: John Mellor <johnme@chromium.org>
Date: Fri Sep 01 16:50:22 2017

[Media Controls] Disable rotate-to-fullscreen if no Device Orientation

Some devices don't support Device Orientation events. These are required
in order for MediaControlsOrientationLockDelegate to unlock the screen
orientation after rotate-to-fullscreen enters fullscreen, so without
Device Orientation events you can rotate-to-enter-fullscreen but not
rotate-to-exit-fullscreen.

This is a poor user experience, so this patch disables
rotate-to-fullscreen completely on devices that don't support Device
Orientation events.

Also fixes an uninitialized member variable in DeviceOrientationData.

Bug:  760737 
Change-Id: Ic328cf548eb672eab4f030990f5d01f0a2ebca40
Reviewed-on: https://chromium-review.googlesource.com/645981
Commit-Queue: John Mellor <johnme@chromium.org>
Reviewed-by: Mounir Lamouri <mlamouri@chromium.org>
Cr-Commit-Position: refs/heads/master@{#499219}
[modify] https://crrev.com/48a2cccd53bdd18aca0360695df835ed84d4fec5/third_party/WebKit/Source/modules/device_orientation/DeviceOrientationData.cpp
[modify] https://crrev.com/48a2cccd53bdd18aca0360695df835ed84d4fec5/third_party/WebKit/Source/modules/media_controls/MediaControlsOrientationLockDelegateTest.cpp
[modify] https://crrev.com/48a2cccd53bdd18aca0360695df835ed84d4fec5/third_party/WebKit/Source/modules/media_controls/MediaControlsRotateToFullscreenDelegate.cpp
[modify] https://crrev.com/48a2cccd53bdd18aca0360695df835ed84d4fec5/third_party/WebKit/Source/modules/media_controls/MediaControlsRotateToFullscreenDelegate.h
[modify] https://crrev.com/48a2cccd53bdd18aca0360695df835ed84d4fec5/third_party/WebKit/Source/modules/media_controls/MediaControlsRotateToFullscreenDelegateTest.cpp

Cc: amineer@chromium.org
Labels: Merge-Request-62
Project Member

Comment 10 by sheriffbot@chromium.org, Sep 2 2017

Labels: -Merge-Request-62 Hotlist-Merge-Approved Merge-Approved-62
Your change meets the bar and is auto-approved for M62. Please go ahead and merge the CL to branch 3202 manually. Please contact milestone owner if you have questions.
Owners: amineer@(Android), cmasso@(iOS), bhthompson@(ChromeOS), abdulsyed@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Project Member

Comment 11 by bugdroid1@chromium.org, Sep 4 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/cd76873d94cd26b7f65d5ddd18663406ec92c1a9

commit cd76873d94cd26b7f65d5ddd18663406ec92c1a9
Author: John Mellor <johnme@chromium.org>
Date: Mon Sep 04 17:03:44 2017

[Media Controls] Avoid SECURITY_DCHECK when casting events

Both MediaControlsOrientationLockDelegate and
MediaControlsRotateToFullscreenDelegate would incorrectly call
ToDeviceOrientationEvent on synthetic events created by the webpage
whose type name was "deviceorientation", even if they weren't
instances of DeviceOrientationEvent.

This patch fixes that. It also checks that the events are trusted, to
avoid using deviceorientation values synthesized by the webpage (other
event listeners in these classes don't use values from the event
object, so it's less important for them to be trusted).

Bug: 761613, 760737 
Change-Id: I318d10e3b6dd84277a47b06546fd4c3ebcdb03cb
Reviewed-on: https://chromium-review.googlesource.com/647929
Reviewed-by: Mounir Lamouri <mlamouri@chromium.org>
Commit-Queue: John Mellor <johnme@chromium.org>
Cr-Commit-Position: refs/heads/master@{#499515}
[modify] https://crrev.com/cd76873d94cd26b7f65d5ddd18663406ec92c1a9/third_party/WebKit/Source/modules/media_controls/MediaControlsOrientationLockDelegate.cpp
[modify] https://crrev.com/cd76873d94cd26b7f65d5ddd18663406ec92c1a9/third_party/WebKit/Source/modules/media_controls/MediaControlsRotateToFullscreenDelegate.cpp

Blockedon: 761613
Project Member

Comment 13 by bugdroid1@chromium.org, Sep 5 2017

Labels: -merge-approved-62 merge-merged-3202
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/043bb9379b1d4fa44c9ca8eb6246c59ecc2501a8

commit 043bb9379b1d4fa44c9ca8eb6246c59ecc2501a8
Author: John Mellor <johnme@chromium.org>
Date: Tue Sep 05 17:24:14 2017

[Media Controls] Disable rotate-to-fullscreen if no Device Orientation

Some devices don't support Device Orientation events. These are required
in order for MediaControlsOrientationLockDelegate to unlock the screen
orientation after rotate-to-fullscreen enters fullscreen, so without
Device Orientation events you can rotate-to-enter-fullscreen but not
rotate-to-exit-fullscreen.

This is a poor user experience, so this patch disables
rotate-to-fullscreen completely on devices that don't support Device
Orientation events.

Also fixes an uninitialized member variable in DeviceOrientationData.

TBR=johnme@chromium.org

(cherry picked from commit 48a2cccd53bdd18aca0360695df835ed84d4fec5)

Bug:  760737 
Change-Id: Ic328cf548eb672eab4f030990f5d01f0a2ebca40
Reviewed-on: https://chromium-review.googlesource.com/645981
Commit-Queue: John Mellor <johnme@chromium.org>
Reviewed-by: Mounir Lamouri <mlamouri@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#499219}
Reviewed-on: https://chromium-review.googlesource.com/650727
Reviewed-by: John Mellor <johnme@chromium.org>
Cr-Commit-Position: refs/branch-heads/3202@{#24}
Cr-Branched-From: fa6a5d87adff761bc16afc5498c3f5944c1daa68-refs/heads/master@{#499098}
[modify] https://crrev.com/043bb9379b1d4fa44c9ca8eb6246c59ecc2501a8/third_party/WebKit/Source/modules/device_orientation/DeviceOrientationData.cpp
[modify] https://crrev.com/043bb9379b1d4fa44c9ca8eb6246c59ecc2501a8/third_party/WebKit/Source/modules/media_controls/MediaControlsOrientationLockDelegateTest.cpp
[modify] https://crrev.com/043bb9379b1d4fa44c9ca8eb6246c59ecc2501a8/third_party/WebKit/Source/modules/media_controls/MediaControlsRotateToFullscreenDelegate.cpp
[modify] https://crrev.com/043bb9379b1d4fa44c9ca8eb6246c59ecc2501a8/third_party/WebKit/Source/modules/media_controls/MediaControlsRotateToFullscreenDelegate.h
[modify] https://crrev.com/043bb9379b1d4fa44c9ca8eb6246c59ecc2501a8/third_party/WebKit/Source/modules/media_controls/MediaControlsRotateToFullscreenDelegateTest.cpp

Project Member

Comment 14 by bugdroid1@chromium.org, Sep 5 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/73c7e0387a8920b6916a530f77c848d4ef175590

commit 73c7e0387a8920b6916a530f77c848d4ef175590
Author: John Mellor <johnme@chromium.org>
Date: Tue Sep 05 17:25:42 2017

[Media Controls] Avoid SECURITY_DCHECK when casting events

Both MediaControlsOrientationLockDelegate and
MediaControlsRotateToFullscreenDelegate would incorrectly call
ToDeviceOrientationEvent on synthetic events created by the webpage
whose type name was "deviceorientation", even if they weren't
instances of DeviceOrientationEvent.

This patch fixes that. It also checks that the events are trusted, to
avoid using deviceorientation values synthesized by the webpage (other
event listeners in these classes don't use values from the event
object, so it's less important for them to be trusted).

TBR=johnme@chromium.org

(cherry picked from commit cd76873d94cd26b7f65d5ddd18663406ec92c1a9)

Bug: 761613, 760737 
Change-Id: I318d10e3b6dd84277a47b06546fd4c3ebcdb03cb
Reviewed-on: https://chromium-review.googlesource.com/647929
Reviewed-by: Mounir Lamouri <mlamouri@chromium.org>
Commit-Queue: John Mellor <johnme@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#499515}
Reviewed-on: https://chromium-review.googlesource.com/650728
Reviewed-by: John Mellor <johnme@chromium.org>
Cr-Commit-Position: refs/branch-heads/3202@{#25}
Cr-Branched-From: fa6a5d87adff761bc16afc5498c3f5944c1daa68-refs/heads/master@{#499098}
[modify] https://crrev.com/73c7e0387a8920b6916a530f77c848d4ef175590/third_party/WebKit/Source/modules/media_controls/MediaControlsOrientationLockDelegate.cpp
[modify] https://crrev.com/73c7e0387a8920b6916a530f77c848d4ef175590/third_party/WebKit/Source/modules/media_controls/MediaControlsRotateToFullscreenDelegate.cpp

Status: Fixed (was: Started)
Fixed in Chrome 62.
Status: Assigned (was: Fixed)
We still see the same issue as mentioned in C#0 after disabling the feature in Chrome:62.0.3202.16 Device:Samsung Galaxy Ace 3(GT-S7262)/JZO54K,Samsung Galaxy S3 mini(GT-I8200N)/JDQ39 so far observed in these two devices,As per the test page(https://jsbin.com/sobopem) device orientation API result is null.
Cc: joh...@chromium.org
Owner: prashanthpola@chromium.org
I've retested on Samsung Galaxy J1(SM-J100H)/KTU84P (which doesn't support Device Orientation) and can't reproduce on Chrome Dev 62.0.3202.12

I tested with chrome://flags#video-rotate-to-fullscreen both enabled and disabled, on both YouTube and https://mounirlamouri.github.io/sandbox/media/dynamic-controls.html, and in all 4 of those cases the video does not automatically enter fullscreen when you rotate the device.

> We still see the same issue as mentioned in C#0 after disabling the feature

Can you clarify what "the same issue" means? The intended behavior on devices that don't support the Device Orientation API is that rotating the device neither automatically enters nor automatically exits fullscreen. So if you just means that "Video is not exiting from Full screen mode" still occurs, then that's the expected behaviour, as long as the video doesn't automatically enter full screen mode either.
Yes,in Samsung Galaxy J1(SM-J100H)/KTU84P(Which doesn't support Device orientation) the feature has been disabled and it works as intended but in Devices:Samsung Galaxy Ace 3(GT-S7262)/JZO54K,Samsung Galaxy S3 mini(GT-I8200N)/JDQ39(Which doesn't support Device orientation) after enabling the flag when we rotate the device portrait-to-landscape Video automatically enters Full screen mode and doesn't exist automatically from full screen mode when we rotate the device from landscape-to-portrait
Cc: -joh...@chromium.org
Owner: joh...@chromium.org
I tested Chrome Dev 62.0.3202.19 on a Samsung Galaxy S3 mini (GT-I8190)/JZO54K and the Device Orientation API mostly works on that model.

Specifically, https://jsbin.com/sobopem shows numbers instead of nulls, and the numbers update when moving the phone. Also the line at the center of https://jsbin.com/rijakaf correctly always points upwards, though it's very laggy and sometimes takes 5 or more seconds to swing round until it points upwards.

Hence rotate-to-fullscreen is enabled and successfully enters and exits fullscreen on both YouTube and dynamic-controls.html (albeit with occasional lag when exiting).
@logs,video,Images:http://go/chrome-androidlogs1/7/760737
Thanks - I see from the new screenshots that the Device Orientation API is providing actual events with numbers in them (rather than "null"), it's just that the numbers are always zero. I'll do a patch to also treat Device Orientation values equal to zero as meaning that the API isn't supported by the device (and hence disable rotate-to-fullscreen).
Project Member

Comment 23 by bugdroid1@chromium.org, Sep 14 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/d17220483696649dd81013562e1f309543b33885

commit d17220483696649dd81013562e1f309543b33885
Author: John Mellor <johnme@chromium.org>
Date: Thu Sep 14 19:42:46 2017

[Media Controls] Disable rotate-to-fullscren if zero Device Orientation

Rotate-to-fullscreen was previously disabled on devices that don't
support the Device Orientation API, in
https://chromium-review.googlesource.com/645981
(48a2cccd53bdd18aca0360695df835ed84d4fec5). But it turns out there are
devices that claim to support Device Orientation but then give
device orientation values that are always fixed to zero!

This patch disables rotate-to-fullscreen on those devices too, for
the same reason that after entering fullscreen it wouldn't be possible
to rotate-to-exit-fullscreen since the
MediaControlsOrientationLockDelegate won't be able to read the Device
Orientation, which would give an inconsistent UX.

Bug:  760737 
Change-Id: I79ce15bde124d5663028a3f2538f4cfe522e0517
Reviewed-on: https://chromium-review.googlesource.com/667379
Commit-Queue: John Mellor <johnme@chromium.org>
Commit-Queue: Mounir Lamouri <mlamouri@chromium.org>
Reviewed-by: Mounir Lamouri <mlamouri@chromium.org>
Cr-Commit-Position: refs/heads/master@{#502017}
[modify] https://crrev.com/d17220483696649dd81013562e1f309543b33885/third_party/WebKit/Source/modules/media_controls/MediaControlsRotateToFullscreenDelegate.cpp
[modify] https://crrev.com/d17220483696649dd81013562e1f309543b33885/third_party/WebKit/Source/modules/media_controls/MediaControlsRotateToFullscreenDelegateTest.cpp

Labels: Merge-Request-62
Requesting approval to merge d17220483696649dd81013562e1f309543b33885 to m62. This simple safe 2-line fix solves the inconsistent UX spotted by QA.
Project Member

Comment 25 by sheriffbot@chromium.org, Sep 14 2017

Labels: -Merge-Request-62 Merge-Review-62 Hotlist-Merge-Review
This bug requires manual review: M62 has already been promoted to the beta branch, so this requires manual review
Please contact the milestone owner if you have questions.
Owners: amineer@(Android), cmasso@(iOS), bhthompson@(ChromeOS), abdulsyed@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
The patch only changes behaviour on devices that claim to support Device Orientation but then dispatch bogus events where the values are all zeroes. And it handles them by doing the same thing we're successfully doing for devices that don't support Device Orientation at all, so no new states are introduced.
Labels: -Merge-Review-62 Merge-Approved-62
Approved for M62 branch 3202.
Project Member

Comment 28 by bugdroid1@chromium.org, Sep 15 2017

Labels: -merge-approved-62
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/5c1840c9aa0c2917374f3d177e49a6c764d4a557

commit 5c1840c9aa0c2917374f3d177e49a6c764d4a557
Author: John Mellor <johnme@chromium.org>
Date: Fri Sep 15 17:02:00 2017

[Media Controls] Disable rotate-to-fullscren if zero Device Orientation

Rotate-to-fullscreen was previously disabled on devices that don't
support the Device Orientation API, in
https://chromium-review.googlesource.com/645981
(48a2cccd53bdd18aca0360695df835ed84d4fec5). But it turns out there are
devices that claim to support Device Orientation but then give
device orientation values that are always fixed to zero!

This patch disables rotate-to-fullscreen on those devices too, for
the same reason that after entering fullscreen it wouldn't be possible
to rotate-to-exit-fullscreen since the
MediaControlsOrientationLockDelegate won't be able to read the Device
Orientation, which would give an inconsistent UX.

TBR=johnme@chromium.org

(cherry picked from commit d17220483696649dd81013562e1f309543b33885)

Bug:  760737 
Change-Id: I79ce15bde124d5663028a3f2538f4cfe522e0517
Reviewed-on: https://chromium-review.googlesource.com/667379
Commit-Queue: John Mellor <johnme@chromium.org>
Commit-Queue: Mounir Lamouri <mlamouri@chromium.org>
Reviewed-by: Mounir Lamouri <mlamouri@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#502017}
Reviewed-on: https://chromium-review.googlesource.com/668916
Reviewed-by: John Mellor <johnme@chromium.org>
Cr-Commit-Position: refs/branch-heads/3202@{#252}
Cr-Branched-From: fa6a5d87adff761bc16afc5498c3f5944c1daa68-refs/heads/master@{#499098}
[modify] https://crrev.com/5c1840c9aa0c2917374f3d177e49a6c764d4a557/third_party/WebKit/Source/modules/media_controls/MediaControlsRotateToFullscreenDelegate.cpp
[modify] https://crrev.com/5c1840c9aa0c2917374f3d177e49a6c764d4a557/third_party/WebKit/Source/modules/media_controls/MediaControlsRotateToFullscreenDelegateTest.cpp

Labels: -Hotlist-Merge-Review
Status: Fixed (was: Assigned)
Status: Assigned (was: Fixed)
Issue is still reproducible in Chrome:62.0.3202.28 Device:Samsung Galaxy S3 mini(GT-I8200N)/JDQ39(Which doesn't support Device orientation) after enabling the flag when we rotate the device portrait-to-landscape Video automatically enters Full screen mode and doesn't exist automatically from full screen mode when we rotate the device from landscape-to-portrait.But in Chrome:63.0.3219.0  Device:Samsung Galaxy S3 mini(GT-I8200N)/JDQ39 (Which doesn't support Device orientation) the feature has been disabled and it works as intended
johnme@ can you retake a look at this bug? The fix does not seem to work.
We have only a week before M62 Stable cut.

johnme@, please take a look asap.
Status: Fixed (was: Assigned)
Please re-test with 62.0.3202.29 or above. The patch had not yet landed as of 62.0.3202.28. Thanks again for finding this subtle edge case - would never have realised without you!
Cc: dah...@chromium.org

Sign in to add a comment