Automatic fullscreen videos on device roation #10108

wassgha · 2017-06-23T07:19:10Z

Switching to landscape when watching a video is a strong indicator that the user wants to expand the video to fullscreen. This feature allows AMP pages to automatically put videos into fullscreen mode when an orientation change is detected.

Changes

Added auto-fullscreen attribute to the video interface
Implemented an orientation change listener
Implemented in/out of fullscreen on manually playing videos when the device rotation changes to landscape
Added fullscreenEnter and fullscreenExit to the video interface and implemented it for all players (amp-video and amp-dailymotion have full support on iOS, other video players don't for now). Opened Implement iOS-compatible fullscreenEnter and fullscreenExit on video players that support it #10561 to check compatibility and implement any internal fullscreen API.

Closes #5380 , Checks-off an item on #4154 , Related-to #10561 , #10660

aghassemi

Overall I think we can move ahead with this feature. I suggest:
1- Run through the requested changed in this PR
2- Add integration tests and do some manual testing specially to make sure:
A) orientation change when AMP doc is invisible (i.e. not the current item in the Viewer) is side-effect free.
B) ensure if video was already made fullscreen by user, this code is side-effect free.
C) ensure when we take the video to fullscreen automatically (specially iframed ones), the fullscreen UI control works as expected (e.g. user can exit fullscreen manually and that does not mess up the code logic here)
3- Validator and documentation PR when we are happy with the feature.

aghassemi · 2017-06-26T17:04:34Z

src/video-interface.js

+  /**
+   * auto-fullscreen
+   *
+   * NOTE: experimental


don't think we need to keep this experimental.

aghassemi · 2017-06-26T17:07:58Z

src/video-interface.js

+   * auto-fullscreen
+   *
+   * NOTE: experimental
+   * If enabled, this automatically expands the currently visible video to


currently visible and playing.

Let's discuss with @spacedino on whether we need to exclude autoplaying from this behaviour or not (I don't see a reason to exclude autoplay from this feature)

aghassemi · 2017-06-26T17:09:59Z

src/service/video-manager-impl.js

+   * @private
+   */
+  maybeInstallOrientationObserver_(entry) {
+    // TODO(@wassgha): Remove this later. For now, the orientation observer


let's replace the TODO with a comment that we only needs this when auto-fullscreen is enabled. Can't think of any other cases we may need this observer.

aghassemi · 2017-06-26T17:16:48Z

src/service/video-manager-impl.js

+    // Chrome apparently considers 'orientationchange' to be an untrusted
+    // event, while 'change' on screen.orientation is considered a user
+    // interaction
+    if (screen.orientation !== undefined) {


note sue if screen is available in all browsers (e.g. IE11) also we want to get the screen from the current win not the global screen so let's do:

const screen = this.ampdoc_.win.screen; if (screen && screen.orientation)

aghassemi · 2017-06-26T17:17:59Z

src/service/video-manager-impl.js

+   */
+  orientationChanged_() {
+
+    const vidElement = this.video.element.querySelector('iframe, video');


add TODO to refactor since video docking is using this as well.

aghassemi · 2017-06-26T17:20:31Z

src/service/video-manager-impl.js

+    const angle = this.ampdoc_.win.orientation !== undefined ?
+                  this.ampdoc_.win.orientation : screen.orientation.angle;
+
+    if (this.isVisible_


reverse and return early (before the angel calculation). Also make sure you bail out if !viewerForDoc(this.ampdoc_).isVisible()

aghassemi · 2017-06-26T17:21:59Z

src/service/video-manager-impl.js

+    && (Math.abs(angle) == 270 || Math.abs(angle) == 90)) {
+
+      // Determine which implementation of requestFullScreen to use
+      const requestFullScreen = vidElement.webkitEnterFullscreen


let's refactor to two new methods on entry: enterFullscreen_() and exitFullscreen_()

aghassemi · 2017-06-26T17:26:49Z

src/service/video-manager-impl.js

+      || vidElement.webkitEnterFullscreen || vidElement.msRequestFullscreen
+      || vidElement.mozRequestFullScreen;
+
+      requestFullScreen.call(vidElement);


try catch these calls since we don't wanna log when request is denied, if it fails, we should not set this.isFullscreen_ = true;

aghassemi · 2017-06-26T17:28:45Z

src/service/video-manager-impl.js

+
+    if (this.isVisible_
+    && this.getPlayingState() == PlayingStates.PLAYING_MANUAL
+    && (Math.abs(angle) == 270 || Math.abs(angle) == 90)) {


what happens if user manually goes to fullscreen and then rotates in the following cases?
1- Chrome amp-video
2- Chrome amp-youtube
3- iOS amp-video
4- iOS amp-youtube

I am assuming (hoping) fullscreen request will be denied.

Behaves correctly (manually tested)

aghassemi · 2017-06-26T17:30:59Z

src/service/video-manager-impl.js

+      requestFullScreen.call(vidElement);
+      this.isFullscreen_ = true;
+
+    } else if (this.isFullscreen_) {


this.isFullscreen_ is misleading, it is more like this.isFullscreenWithAutoFullscreen_

wassgha · 2017-06-30T16:27:06Z

@aghassemi There's an issue i'm not sure how to fix: When you're in portrait and the video is in view then you rotate the video sometimes gets out of view and therefore doesn't go to fullscreen (since we only measure whether it is visible or not after the orientation change...)

wassgha · 2017-07-18T03:33:31Z

@aghassemi Would we want to add a enterFullscreen() method to the video interface to get this working on iOS with iframe based video players?

aghassemi · 2017-07-18T19:18:11Z

@wassgha definitely, not hopeful that any 3P has support for this (including YT) but at minimum we can make it work for IMA

wassgha · 2017-07-20T22:10:32Z

Ready for review :)

aghassemi

looks good, few nits

aghassemi · 2017-07-24T18:10:13Z

extensions/amp-ima-video/0.1/amp-ima-video.js

+   * @override
+   */
+  fullscreenEnter() {
+    // TODO(@aghassemi) Make internal <video> element go fullscreen instead


Do TODO(aghassemi, #10597)

aghassemi · 2017-07-24T18:12:28Z

src/dom.js

+/**
+ * Replacement for `Element.requestFullscreen()` method.
+ * https://developer.mozilla.org/en-US/docs/Web/API/Element/requestFullscreen
+ * @param {Element} element


{!Element} (same below)

aghassemi · 2017-07-24T18:19:36Z

src/video-interface.js

+   * http://caniuse.com/#feat=screen-orientation
+   * and http://caniuse.com/#feat=fullscreen
+   */
+  AUTOFULLSCREEN: 'auto-fullscreen',


what was the conclusion from design review? fullscreen-on-landscape or auto-fullscreen? (I am fine with either)

I think the consensus was to launch as default and add no-fullscreen-on-landscape but i'm not sure about that..

I am not a big fan of binary attributes. Let's do:
fullscreen-on-landscape=(never|always).
If fullscreen-on-landscape is specified without a value, then it equals always
If no fullscreen-on-landscape is specified, for initial launch, it means never, we will have to do an experiment and ramp up slowly where not having fullscreen-on-landscape starts meaning always

aghassemi · 2017-07-24T18:22:25Z

src/service/video-manager-impl.js

+   * @private
+   */
+  orientationChanged_(isLandscape) {
+    // Put the video in/out of fullscreen depending on orientation


return early if !viewerForDoc(this.ampdoc_).isVisible()

Actually from what I've seen, when the video is fullscreen, viewerForDoc(this.ampdoc_).isVisible() always returns false. Is that the normal behavior or is it a bug?

interesting, it is technically correct. Can we ensure at least we don't go to fullscreen on rotation if viewerForDoc(this.ampdoc_).isVisible() is false?

aghassemi · 2017-07-24T18:25:47Z

src/video-interface.js

+   * http://caniuse.com/#feat=screen-orientation
+   * and http://caniuse.com/#feat=fullscreen
+   */
+  AUTOFULLSCREEN: 'auto-fullscreen',


let's also add the validation rules and documentation for all the components that now support this in a separate PR (which would effectively be the launch PR, so let's CC Eric and Rudy on that one)

Do we want to switch to attribute lists on that PR or leave that for later?

yeah would be a good time to do attribute list for video-interface

aghassemi

Couple more requests and then good to merge!

aghassemi · 2017-07-26T17:19:03Z

extensions/amp-dailymotion/0.1/amp-dailymotion.js

@@ -218,6 +227,9 @@ class AmpDailymotion extends AMP.BaseElement {
      case DailymotionEvents.STARTED_BUFFERING:
        this.startedBufferingResolver_(true);
        break;
+      case DailymotionEvents.FULLSCREEN_CHANGE:


nice! they thought of everything.

aghassemi · 2017-07-26T17:26:42Z

src/service/video-manager-impl.js

+   * @private
+   */
+  orientationChanged_(isLandscape) {
+    // Put the video in/out of fullscreen depending on screen orientation


return early if this.loaded_ is false. What can happen is orientation change occurring before the layoutCallback of a video player is called in which case their iframe is null. Now at the moment we can't get to that case because of the this.getPlayingState() == PlayingStates.PLAYING_MANUAL check but I like to keep the code safe since later supporting AUTOPLAY can surface the race condition.

aghassemi · 2017-07-26T17:29:19Z

test/manual/amp-video-auto-fullscreen.amp.html

+<body>
+  <h1>amp-video</h1>
+
+  <amp-video


just add the sample to the end of the existing amp-video.amp.html (and youtube, brid, ima, daily motion)

Needs validation though...

right.. then let's move this to test/manual and create an issue to add examples + ABE samples when launched.

aghassemi · 2017-07-26T17:32:39Z

src/service/video-manager-impl.js

+        VideoAttributes.FULLSCREEN_ON_LANDSCAPE
+    );
+
+    this.hasFullscreenOnLandscape = fsOnLandscapeAttr !== undefined


undefined check not needed (getAttribute actually returns null). Also inconsistent == and === (just use == compiler takes care of it).

…+ bug fixes and iOS compatibility

…ome and iOS

aghassemi

LGTM! Thanks!

googlebot added the cla: yes label Jun 23, 2017

wassgha requested a review from aghassemi June 23, 2017 07:30

aghassemi suggested changes Jun 26, 2017

View reviewed changes

wassgha force-pushed the landscape-fullscreen branch from 052cc14 to 30e88f0 Compare June 30, 2017 15:48

wassgha force-pushed the landscape-fullscreen branch 2 times, most recently from 60f7370 to 03791d2 Compare July 5, 2017 07:18

wassgha force-pushed the landscape-fullscreen branch 7 times, most recently from 4ea6e7f to 26ceeb4 Compare July 15, 2017 06:37

wassgha force-pushed the landscape-fullscreen branch from 26ceeb4 to 424abc4 Compare July 20, 2017 16:45

wassgha mentioned this pull request Jul 20, 2017

Implement iOS-compatible fullscreenEnter and fullscreenExit on video players that support it #10561

Closed

wassgha force-pushed the landscape-fullscreen branch from 8662284 to 99411a1 Compare July 20, 2017 22:10

wassgha force-pushed the landscape-fullscreen branch 2 times, most recently from a87213d to 212b88c Compare July 21, 2017 17:31

aghassemi reviewed Jul 24, 2017

View reviewed changes

wassgha force-pushed the landscape-fullscreen branch 7 times, most recently from 94f885f to 03b3c1e Compare July 26, 2017 02:46

aghassemi reviewed Jul 26, 2017

View reviewed changes

wassgha added 4 commits July 26, 2017 18:11

Started working on the rotate to fullscreen

dbc7053

Implemented enterFullscreen and exitFullscreen for all video players …

2349f02

…+ bug fixes and iOS compatibility

Requested changes for auto-fullscreen

0a568d3

Added isFullscreen method to the video interface, fixes bugs with Chr…

d8478d4

…ome and iOS

wassgha force-pushed the landscape-fullscreen branch from 03b3c1e to d5cc3ee Compare July 27, 2017 01:16

Requested changes

6f76e18

wassgha force-pushed the landscape-fullscreen branch from d5cc3ee to 6f76e18 Compare July 27, 2017 03:44

aghassemi approved these changes Jul 27, 2017

View reviewed changes

aghassemi merged commit 202cb26 into ampproject:master Jul 27, 2017

aghassemi mentioned this pull request Jul 31, 2017

Implement the video-interface for amp-brightcove #7795

Closed

4 tasks

ampprojectbot added the PR use: In Beta / Experimental label Aug 2, 2017

ampprojectbot added the PR use: In Stable label Aug 10, 2017

Automatic fullscreen videos on device roation #10108

Automatic fullscreen videos on device roation #10108

Conversation

wassgha commented Jun 23, 2017 • edited Loading

Changes

aghassemi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wassgha commented Jun 30, 2017

wassgha commented Jul 18, 2017

aghassemi commented Jul 18, 2017

wassgha commented Jul 20, 2017

aghassemi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aghassemi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aghassemi left a comment

Choose a reason for hiding this comment

wassgha commented Jun 23, 2017 •

edited

Loading