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

Issue 740205 link

Starred by 1 user

Issue metadata

Status: Verified
Owner:
no longer working on chrome
Closed: Jul 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Android
Pri: 2
Type: Bug



Sign in to add a comment

Fullscreen videos on mobile Twitter on Android fall out of fullscreen in landscape mode

Project Member Reported by kbr@chromium.org, Jul 7 2017

Issue description

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

 

Comment 1 by kbr@chromium.org, Jul 7 2017

Labels: ReleaseBlock-Stable M-61

Comment 2 by e...@chromium.org, Jul 7 2017

Owner: foolip@chromium.org
Status: Assigned (was: Untriaged)
Cc: dknandiraju@chromium.org
Labels: triage-te
Owner: mlamouri@chromium.org
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.
Cc: mlamouri@chromium.org
Owner: joh...@chromium.org
johnme@, can you PTAL

Comment 6 by joh...@chromium.org, 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...

Comment 7 by kbr@chromium.org, Jul 13 2017

Components: Mobile>WebAPKs
Labels: -Pri-1 -Needs-Bisect -ReleaseBlock-Stable -Type-Bug-Regression Pri-2 Type-Bug
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?

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?

Comment 9 by kbr@chromium.org, 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.

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?
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
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.
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.
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.
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.
Project Member

Comment 16 by bugdroid1@chromium.org, 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

Labels: ReleaseBlock-Beta
Will mark as fixed once I've confirmed this landed before branch point.
Verified fix in latest M61 release.

Comment 19 by kbr@chromium.org, Jul 26 2017

Thanks for the fix!

Status: Verified (was: Assigned)
Good, the fix made it into M61. Thanks for verifying :)
Labels: Merge-TBD
[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.
Labels: -Merge-TBD

Sign in to add a comment