New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Video-docking: Implement drag and drop #10394

Merged

aghassemi merged 2 commits into ampproject:master from wassgha:video-dock-dragging

Jul 28, 2017

Contributor

wassgha commented Jul 12, 2017 •

edited

Loading

Implements dragging for docked videos.

Changes

Viewport is now an instance variable
Corrected JSDoc on video analytics
Implemented drag & drop for all players (used a mask to capture events on iframe players)
Video placeholder only appears when video docking starts (corrects bug where a dark line would appear over and under the video if the aspect ratio isn't correct)
Changed minimized video's z-index

To-do in next PRs

Add minimized controls
Show the draggable mask since docking starts (so that it absorbs click / mouseover events and hides the video controls)
Call unlisteners for touch and mouse events when dragging is over
Explore switching to position: sticky for cleaner code

Closes #9962, Checks off an item on #4154, Checks off two items on #8088

googlebot added the cla: yes label

wassgha force-pushed the video-dock-dragging branch 2 times, most recently from 2b526b2 to 1b9d238 Compare

July 12, 2017 23:16

wassgha requested a review from aghassemi

July 12, 2017 23:35

wassgha changed the title ~~Video dock dragging~~ Video-docking: Implement drag and drop

wassgha closed this

wassgha force-pushed the video-dock-dragging branch from 1b9d238 to cb4c16a Compare

July 19, 2017 18:39

Contributor

aghassemi commented Jul 19, 2017

@wassgha planning a different PR for this?

Contributor Author

wassgha commented Jul 19, 2017 via email •

edited

Loading

No but I'm rearranging the commits (A lot of conflicts and I didn't wanna do it for every commit). It automatically closed because I rolled back the commits.

wassgha reopened this

Contributor Author

wassgha commented Jul 19, 2017 •

edited

Loading

Ready for review once Travis passes

wassgha force-pushed the video-dock-dragging branch 3 times, most recently from 9e4ec25 to 67bb5ad Compare

July 19, 2017 23:03

Contributor Author

wassgha commented Jul 24, 2017

Updated description

aghassemi suggested changes

View reviewed changes

src/service/video-manager-impl.js Outdated

@@ @@ -894,13 +945,235 @@ class VideoEntry { @@
                   // Update docking state
                   if (this.scrollMap_(DOCK_SCALE, 1) == DOCK_SCALE) {
                     this.dockState_ = DockStates.DOCKED;
+                    this.initializeDragging_();

Contributor

aghassemi Jul 26, 2017

this is measuring, which we don't wanna do during a mutate, needs to go to its own measure block.

src/service/video-manager-impl.js Outdated

@@ @@ -894,13 +945,235 @@ class VideoEntry { @@
                   // Update docking state
                   if (this.scrollMap_(DOCK_SCALE, 1) == DOCK_SCALE) {
                     this.dockState_ = DockStates.DOCKED;
+                    this.initializeDragging_();
+                    this.drag_();

Contributor

aghassemi Jul 26, 2017

mmm, drag_() keeps running every rAF, not great. We need to only run it with touch move.

Contributor Author

wassgha Jul 26, 2017

It needs to run every RAF for the momentum drag to kick in (so that it moves the element even after the user is no longer dragging until the velocity is null).

src/service/video-manager-impl.js Outdated

+                    return;
+                  }
+                  const minimizedRect = this.internalElement_./*OK*/getBoundingClientRect();

Contributor

aghassemi Jul 26, 2017

let's keep drag_ a mutate-only function. Does it need to read this every time? can it by cached by initializeDragging_?

src/service/video-manager-impl.js Outdated

+                  // Re-run on every animation frame
+                  this.vsync_.mutate(() => {
+                    this.drag_();

Contributor

aghassemi Jul 26, 2017

ditto comment above, we should only happen with touch move.

src/service/video-manager-impl.js

+                 * @param {boolean} updateDisplacement
+                 * @private
+                 */
+                mouse_(e, updateDisplacement = false) {

Contributor

aghassemi Jul 26, 2017

related to comments above, we really need to separate measure and mutate and tied to events instead of running every rAF. How about something like:

1- video docked
2- initilaizeDragdrop (in a measure block, cache whatever we need, install handlers, on touchmove call dragtouchmove_
3- dragtouchmove_ does reads new coordinates from e and does a mutate (if we needed to reread anything then split it does both a measure and mutate)

We just wanna make sure we never measure during mutate and vice versa.

wassgha force-pushed the video-dock-dragging branch 2 times, most recently from 7ff67a6 to b738852 Compare

July 27, 2017 01:22

aghassemi approved these changes

View reviewed changes

css/amp.css

               }
               .i-amphtml-dockable-video > video.i-amphtml-dockable-video-minimizing,
               .i-amphtml-dockable-video > iframe.i-amphtml-dockable-video-minimizing {
                 position: fixed;
                 height: auto;
                 overflow: hidden;
-                z-index: 2;
+                z-index: 16;

Contributor

aghassemi Jul 28, 2017

run gulp get-zindex. it will regenerated the Z_INDEX.md file

Contributor Author

wassgha Jul 28, 2017

Done :) Should be ready to merge.

wassgha force-pushed the video-dock-dragging branch from b738852 to 5b2f5e8 Compare

July 28, 2017 18:51

wassgha added 2 commits

July 28, 2017 14:53


          Implemented drag/drop

49a2da8


          Bug fixes, requested changes, z-index change

12430bd

wassgha force-pushed the video-dock-dragging branch from 5b2f5e8 to 12430bd Compare

July 28, 2017 21:53

aghassemi merged commit 6aa61ab into ampproject:master

Contributor

aghassemi commented Jul 28, 2017

merged

wassgha deleted the video-dock-dragging branch

July 28, 2017 22:55

wassgha mentioned this pull request

video dock: gestures #9961

Closed

1 task

ampprojectbot added the PR use: In Beta / Experimental label

ampprojectbot added the PR use: In Stable label

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cla: yes PR use: In Beta / Experimental PR use: In Stable