Fullscreen videos on mobile Twitter on Android fall out of fullscreen in landscape mode |
||||||||||
Issue descriptionChrome Version: 61.0.3142.0 (Official Build) dev (32-bit) OS: Android 7.1.2 What steps will reproduce the problem? (1) In Chrome Dev on Android (tested on a Pixel phone), navigate to a tweet containing a video, for example: https://twitter.com/xeographics/status/883276868065136640 https://twitter.com/VRScout/status/883009383579271168 (2) Click the full-screen button. (3) Orient the device to landscape mode. What is the expected result? Expect the video to stay in full-screen mode. What happens instead? The browser exits full-screen mode upon the device orientation change. Works correctly in Chrome Stable. Sorry, I haven't had time to bisect. This breaks the Twitter Lite PWA pretty badly so I'm calling this a P1 release blocker. Hopefully it's just a bug and not a policy change.
,
Jul 7 2017
,
Jul 13 2017
,
Jul 13 2017
Note: 1) Good Build: 61.0.3139.6 Bad Build: 61.0.3140.0 Good Commit: 482071 Bad commit: 482072 2) Bisect range: https://chromium.googlesource.com/chromium/src/+log/61.0.3139.6..61.0.3140.0?pretty=fuller&n=10000 3) Please find logs and video @ http://go/chrome-androidlogs1/6/740205 4) Culprit: https://chromium.googlesource.com/chromium/src/+/a9613b5edbee77c0b668743018b495cb50ca0d0c 5) This issue exists on all devices on "twitter lite" videos.
,
Jul 13 2017
johnme@, can you PTAL
,
Jul 13 2017
Strange - I can reproduce on Dev 61.0.3142.0 / Pixel XL / OPR1 if and only if my OS auto-rotation setting is disabled (locked to portrait only). Sometimes it exits fullscreen; other times it actually stays in fullscreen but rotates to portrait fullscreen. Both are bad. But I can't reproduce at all on a developer chrome_public_apk build of tip of tree, which makes this hard to debug. I'll try more builds...
,
Jul 13 2017
Hmm. Sorry about the poor repro instructions. I can confirm that this behaves differently in the Twitter Lite PWA as opposed to simply navigating Chrome Dev to https://mobile.twitter.com/xeographics/status/883276868065136640 . When this was reproducible for me, I was viewing these Tweets from the Twitter Lite icon on my home screen (which was installed from my Chrome Dev instance). Chrome Dev didn't repro the problem. I then removed the Twitter Lite shortcut from my home screen, went to mobile.twitter.com from Chrome Dev, and added it to the home screen again. Now I can't reproduce the original behavior even in Twitter Lite. What state could there be in the WebAPK that would be cleared up by reinstalling it against (theoretically) the same Chrome version?
,
Jul 13 2017
By "removed from homescreen" I assume you mean uninstalled, right? Given that you were previously filing webapk bugs, maybe the new version actually fell back to the old-style add to homescreen flow and it correctly handles screen orientation change events? Can you double check it's the twitter lite webapk now by see if it's in you apps list?
,
Jul 13 2017
Yes, sorry, I meant uninstalled it. It's still the WebAPK version; Twitter Lite's in my apps list again after adding it to the homescreen again from Chrome Dev.
,
Jul 14 2017
Hrmm. No good leads yet. One thought was that maybe twitter changed their web manifest. They specify "portrait" as their orientation and I see code in WebappActivity to lock orientation but I see our update logic should've updated the webapk if the manifest's orientation changed. The code to lock rotation changed in M57 but I dont' see much change since then. John: when launching a video from webappactivity, do we intent to another activity?
,
Jul 14 2017
Actually it does look like John made a few video orientation / rotation locking change so maybe there's something there: https://codereview.chromium.org/2890423003
,
Jul 17 2017
I have Twitter Lite installed as a Web APK and I couldn't reproduce this. I tried with a landscape and portrait video and in both cases, I could fullscreen the video by pressing the buttond and by rotating the phone. I did not try the specific URLs as I'm not sure haw I could launch Twitter Lite on a given URL.
,
Jul 17 2017
Still looking into this, but having trouble reproducing. Using the Twitter Lite PWA apk doesn't seem to have helped. I'll keep trying tomorrow.
,
Jul 18 2017
Reproduced on Chrome Dev 61.0.3142.0 / Nexus 6P / N2G03 in Twitter Lite PWA. Could only reproduce in the standalone PWA (see below), and with OS screen orientation auto-rotate *enabled* (i.e. not locked to portrait). After pressing the fullscreen button (triggering fullscreen orientation lock), rotating the device to either landscape orientation causes the video to exit fullscreen. Since their PWA is locked to portrait (presumably via the manifest), exiting fullscreen results in the screen returning to portrait even though the device is now being held in landscape. I'd accepted the prompt to add it and ran it from the homescreen icon that appeared, but I just checked and it's not listed in All Apps, so it seems this is not the WebAPK version. I also have both the video-fullscreen-orientation-lock and video-rotate-to-fullscreen about:flags manually enabled (that's probably not necessary, but just trying to document my state exactly). And it was a horizontal video.
,
Jul 18 2017
Seems to be because the manifest locks to portrait. When the MediaControlsOrientationLockDelegate unlocks the orientation ScreenOrientationProvider#unlockOrientation actually locks it back to portrait, which kicks us out of fullscreen. I'll work on a fix tomorrow.
,
Jul 20 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/7845aae11a4ffb2ddad2ea82e97c80b9a7cec2e1 commit 7845aae11a4ffb2ddad2ea82e97c80b9a7cec2e1 Author: John Mellor <johnme@chromium.org> Date: Thu Jul 20 13:41:54 2017 [Media controls] Fix fullscreen orientation lock vs manifest lock MediaControlsOrientationLockDelegate was releasing its fullscreen screen orientation lock when the device had been rotated to the match the orientation of the video (and other conditions were met). This was intended to have no immediate effect, and simply allow the user to exit fullscreen by rotating the device away. However it turns out that unlocking actually locks to the "default" orientation, and e.g. for an add-to-homescreen webapp that has set its manifest to portrait orientation, the "default" orientation would be portrait. So unlocking when in landscape would immediately rotate to portrait and exit fullscreen! This patch fixes that - instead of unlocking when the device orientation matches the video orientation, it locks to the "any" orientation which enables accelerometer-based screen orientation auto-rotation (overriding manifest orientations). A full unlock to "default" orientation happens later when fullscreen is exited. (To understand this, it's helpful to know that Chrome doesn't have a stack of orientation locks, it's simply last write wins, with the exception that ScreenOrientationProvider restores the manifest orientation when unlocking == locking to "default"). Bug: 740205 Change-Id: I590b33fa45a1ab6cc44ae3b49f983ae57d41a682 Reviewed-on: https://chromium-review.googlesource.com/577849 Reviewed-by: Mounir Lamouri <mlamouri@chromium.org> Commit-Queue: John Mellor <johnme@chromium.org> Cr-Commit-Position: refs/heads/master@{#488229} [modify] https://crrev.com/7845aae11a4ffb2ddad2ea82e97c80b9a7cec2e1/third_party/WebKit/Source/modules/media_controls/MediaControlsOrientationLockDelegate.cpp [modify] https://crrev.com/7845aae11a4ffb2ddad2ea82e97c80b9a7cec2e1/third_party/WebKit/Source/modules/media_controls/MediaControlsOrientationLockDelegate.h [modify] https://crrev.com/7845aae11a4ffb2ddad2ea82e97c80b9a7cec2e1/third_party/WebKit/Source/modules/media_controls/MediaControlsOrientationLockDelegateTest.cpp
,
Jul 20 2017
Will mark as fixed once I've confirmed this landed before branch point.
,
Jul 26 2017
Verified fix in latest M61 release.
,
Jul 26 2017
Thanks for the fix!
,
Jul 31 2017
Good, the fix made it into M61. Thanks for verifying :)
,
Jul 31 2017
[Auto-generated comment by a script] We noticed that this issue is targeted for M-61; it appears the fix may have landed after branch point, meaning a merge might be required. Please confirm if a merge is required here - if so add Merge-Request-61 label, otherwise remove Merge-TBD label. Thanks.
,
Jul 31 2017
|
||||||||||
►
Sign in to add a comment |
||||||||||
Comment 1 by kbr@chromium.org
, Jul 7 2017