Video doesn't exit from full screen mode when we rotate the device from landscape-to-portrait |
||||||||||||||||||||||
Issue descriptionApplication 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
,
Aug 30 2017
,
Aug 30 2017
,
Aug 31 2017
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.
,
Aug 31 2017
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.
,
Aug 31 2017
We are disabling this feature for M61 and will ship in M62, so adjusting milestone to match.
,
Aug 31 2017
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
,
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
,
Sep 1 2017
,
Sep 2 2017
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
,
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
,
Sep 5 2017
,
Sep 5 2017
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
,
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
,
Sep 5 2017
Fixed in Chrome 62.
,
Sep 11 2017
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.
,
Sep 12 2017
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.
,
Sep 13 2017
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
,
Sep 13 2017
,
Sep 13 2017
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).
,
Sep 13 2017
@logs,video,Images:http://go/chrome-androidlogs1/7/760737
,
Sep 13 2017
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).
,
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
,
Sep 14 2017
Requesting approval to merge d17220483696649dd81013562e1f309543b33885 to m62. This simple safe 2-line fix solves the inconsistent UX spotted by QA.
,
Sep 14 2017
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
,
Sep 14 2017
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.
,
Sep 15 2017
Approved for M62 branch 3202.
,
Sep 15 2017
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
,
Sep 15 2017
,
Sep 19 2017
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
,
Sep 29 2017
johnme@ can you retake a look at this bug? The fix does not seem to work.
,
Oct 4 2017
We have only a week before M62 Stable cut. johnme@, please take a look asap.
,
Oct 4 2017
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!
,
Oct 5 2017
|
||||||||||||||||||||||
►
Sign in to add a comment |
||||||||||||||||||||||
Comment 1 by prashanthpola@chromium.org
, Aug 30 2017