✨Implement rotate-to-fullscreen by alanorozco · Pull Request #14655 · ampproject/amphtml

alanorozco · 2018-04-16T21:51:21Z

Extracts everything AutoFullscreen into a separate class.
Renames attr to "rotate-to-fullscreen" for consistency with Chrome.

alanorozco · 2018-04-16T21:53:35Z

build-system/tasks/compile.js

      'third_party/webcomponentsjs/ShadowCSS.js',
-      'third_party/rrule/rrule.js',
-      'third_party/react-dates/bundle.js',
+      'node_modules/intersection-observer/intersection-observer.install.js',


Drive-by: Sorted these.

alanorozco · 2018-04-16T21:53:45Z

build-system/tasks/update-packages.js

+ * @param {string} prefix
+ * @param {string} sufix
+ */
+function patchWithWrapper(originalName, patchedName, prefix, sufix) {


Drive-by: Factored this out.

aghassemi · 2018-04-17T17:48:18Z

build-system/tasks/compile.js

      'third_party/css-escape/css-escape.js',
+      'third_party/d3/**/*.js',
      'third_party/mustache/**/*.js',
+      'third_party/react-dates/bundle.js',


we already have an intersection-observer-polyfill (and I don't think we need direct access to InOb anyway) see comments below.

Removed. Using synchronous method suggested.

aghassemi · 2018-04-17T17:49:00Z

src/service/video-manager-impl.js

+ * interaction.
+ * @private @const {string}
+ */
+const BLESSED = 'i-amphtml-blessed';


I don't think this is needed. PlayingStates.PLAYING_MANUAL should cover what you need.

aghassemi · 2018-04-17T17:51:22Z

src/service/video-manager-impl.js

    /** @private @const */
    this.boundSecondsPlaying_ = () => this.secondsPlaying_();

+    /** @public @const {function():!Rot82xManager} */


Rot82x sound like an RFC standard :) Maybe autoFullscreenManager ?

aghassemi · 2018-04-17T17:51:56Z

src/service/video-manager-impl.js

    this.boundSecondsPlaying_ = () => this.secondsPlaying_();

+    /** @public @const {function():!Rot82xManager} */
+    this.getRot82xManager = once(() => this.createRot82xManager_());


why once ? VideoService itself should not be constructed more than once anyway.

aghassemi · 2018-04-17T17:54:30Z

src/service/video-manager-impl.js

  videoPlayed_() {
    this.isPlaying_ = true;

+    if (!this.hasAutoplay) {


ditto, not needed as PlayingStates.PLAYING_MANUAL covers it.

aghassemi · 2018-04-17T17:56:54Z

src/service/video-manager-impl.js

+   * @param {!../video-interface.VideoInterface} impl
+   * @private
+   */
+  maybeScrollTo_(impl) {


as disccused offline, using existing code from viewer

aghassemi · 2018-04-17T20:31:47Z

src/service/video-manager-impl.js

+    /** @private @const {function()} */
+    const installOrientationObserver =
+        this.installOrientationObserver_.bind(this);
+    this.installOrientationObserver_ = once(installOrientationObserver);


do we need once here? given R82FullscreenManager also shouldn't be instantiated more than once.

aghassemi · 2018-04-17T20:35:36Z

src/service/video-manager-impl.js

+
+  /** @private */
+  onRotation_() {
+    const root = this.ampdoc_.getRootNode();


do, instead of using intersectionObserver, can we just call getIntersectionChangeEntry on video elements and calculate based on that?

If orientation event happens too late ( e.g. after getIntersectionChangeEntry values have been updated based on landscape, then you can do the calculations in a viewer.onScroll event. All getIntersectionChangeEntry values are cached by runtime so there is no perf hit. autoplay does it this way)

aghassemi · 2018-04-17T20:38:51Z

src/service/video-manager-impl.js

+            resolve();
+            return;
+          }
+          this.getViewport_()


would scrolling to center work? Like to avoid the calculations done here to figure out top vs bottom if possible

I'm hesitant.

This scrolls "as little as possible". I find this to be a proxy for matching browser behavior without looking out of place.

I think this logic goes with the spirit of preventing the page from shifting unexpectedly, like AMP is supposed to.

I did change two things:

Simplify this greatly by using getIntersectionChangeEntry.

Scroll to center on exit (feels more natural).

aghassemi · 2018-04-17T20:39:06Z

src/service/video-manager-impl.js

+
+  /** @private @return {!Promise} */
+  onceOrientationChanges_() {
+    const magicNumber = 330;


is gonna need some comment explaining the magic :)

Haven't experimented enough to have a good reason for it. :) Pending.

alanorozco · 2018-04-18T18:31:44Z

@aghassemi

PTAL

Removed polyfill.
Added tests.
Did some unrelated housekeeping around vsync in anticipation. Can move to a separate PR if needed.

aghassemi · 2018-04-19T00:07:04Z

src/service/video-manager-impl.js

  autoplayInteractiveVideoBuilt_() {
    const toggleAnimation = playing => {
-      this.vsync_.mutate(() => {
+      this.getVideoForVsync_().mutateElement(() => {


Thanks for making these changes as well!

1. Extracts everything AutoFullscreen into a separate class. 2. Renames attr to "rotate-to-fullscreen" for consistency with Chrome. 3. Does some unrelated housekeeping around vsync in anticipation.

alanorozco added 3 commits April 13, 2018 11:48

🚮 Remove experimental video docking

29525e4

Remove test

900d527

Merge branch 'master' of https://github.com/ampproject/amphtml into fol

935f2a8

googlebot added the cla: yes label Apr 16, 2018

alanorozco force-pushed the r82x branch from 4fce123 to a9125b0 Compare April 16, 2018 21:52

alanorozco commented Apr 16, 2018

View reviewed changes

Implement rotate-to-fullscreen

ac4721f

alanorozco force-pushed the r82x branch from a9125b0 to ac4721f Compare April 16, 2018 21:55

alanorozco requested a review from aghassemi April 16, 2018 21:56

alanorozco added 6 commits April 17, 2018 10:44

Naming, use viewport method, cleanup

5e6a542

Naming, style

5890329

Fix landscape bug, style

65c1891

Use prop instead of attr

e688b6c

Update comment

79dfd90

Style

01e0371

aghassemi reviewed Apr 17, 2018

View reviewed changes

alanorozco added 2 commits April 17, 2018 17:35

Address comments

238095e

Remove polyfill

722eac1

alanorozco force-pushed the r82x branch from 65c7ae2 to 722eac1 Compare April 18, 2018 00:39

alanorozco and others added 11 commits April 17, 2018 17:41

Revert?

f27b310

Revert?

67017eb

Merge branch 'master' of https://github.com/ampproject/amphtml into r82x

8dbc50b

Lint, housekeeping

ce097dc

Soft revert + type

9faf565

Inline

1702900

Simplify, etc

2e835cd

Cleanup, base test

542e749

Refactor compare

1348f44

Simplify

c63786c

Prioritize elements that are playing

4c424a1

alanorozco added 4 commits April 18, 2018 08:09

Simplify

631c855

Naming, style

2b5ea4c

More tests, make tests work

2128151

Tests, lint

9befeb7

aghassemi approved these changes Apr 19, 2018

View reviewed changes

alanorozco added 2 commits April 19, 2018 11:15

Merge branch 'master' of https://github.com/ampproject/amphtml into r82x

e279e56

Merge branch 'master' of https://github.com/ampproject/amphtml into r82x

491be8d

alanorozco merged commit 47a082d into ampproject:master Apr 19, 2018

alanorozco deleted the r82x branch April 19, 2018 22:20

ericlindley-g mentioned this pull request Apr 23, 2018

[Master feature] Trigger video fullscreen in landscape #14062

Closed

aghassemi mentioned this pull request Apr 30, 2018

Status Update 2018-04-30 #14944

Closed

Conversation

alanorozco commented Apr 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alanorozco Apr 18, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aghassemi Apr 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alanorozco commented Apr 18, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

alanorozco commented Apr 16, 2018 •

edited

Loading

alanorozco Apr 18, 2018 •

edited

Loading

aghassemi Apr 17, 2018 •

edited

Loading

alanorozco commented Apr 18, 2018 •

edited

Loading