fix: improve focal points draggable style/perf (#1371)

* fix: improve focal points draggable style/perf * remove unnecessary global * fix all the things * fix comment
nolanlawson · Aug 4, 2019 · d58ab52 · d58ab52
1 parent 00945a3
commit d58ab52
Show file tree

Hide file tree

Showing 10 changed files with 195 additions and 65 deletions.
diff --git a/package.json b/package.json
@@ -46,7 +46,6 @@
     "@babel/core": "^7.5.0",
     "@gamestdio/websocket": "^0.3.2",
     "@webcomponents/custom-elements": "^1.2.4",
-    "@wessberg/pointer-events": "^1.0.9",
     "babel-loader": "^8.0.6",
     "babel-plugin-transform-react-remove-prop-types": "^0.4.24",
     "cheerio": "^1.0.0-rc.2",
@@ -150,7 +149,8 @@
       "CSS",
       "customElements",
       "AbortController",
-      "matchMedia"
+      "matchMedia",
+      "MessageChannel"
     ],
     "ignore": [
       "dist",

diff --git a/src/routes/_components/Draggable.html b/src/routes/_components/Draggable.html
@@ -1,13 +1,13 @@
-<div class="draggable-area {draggableClass}"
-     on:pointermove="onPointerMove(event)"
-     on:pointerleave="onPointerLeave(event)"
+<div class="draggable-area {draggableClassAfterRaf}"
+     on:pointerMove="onPointerMove(event)"
+     on:pointerLeave="onPointerLeave(event)"
+     on:pointerUp="onPointerUp(event)"
      on:click="onClick(event)"
      ref:area
 >
-  <div class="draggable-indicator {indicatorClass}"
-       style={indicatorStyle}
-       on:pointerdown="onPointerDown(event)"
-       on:pointerup="onPointerUp(event)"
+  <div class="draggable-indicator {indicatorClassAfterRaf}"
+       style={indicatorStyleAfterRaf}
+       on:pointerDown="onPointerDown(event)"
        ref:indicator
   >
     <div class="draggable-indicator-inner">
@@ -30,42 +30,85 @@
   }
 </style>
 <script>
-  import { throttleRaf } from '../_utils/throttleRaf'
+  import { observe } from 'svelte-extras'
+  import {
+    throttleRequestAnimationFrame,
+    throttleRequestPostAnimationFrame
+  } from '../_utils/throttleTimers'
+  import { pointerUp, pointerDown, pointerLeave, pointerMove } from '../_utils/pointerEvents'
+
+  // ensure DOM writes only happen once after a rAF
+  const updateIndicatorStyle = throttleRequestAnimationFrame()
+  const updateIndicatorClass = throttleRequestAnimationFrame()
+  const updateDraggableClass = throttleRequestAnimationFrame()
+
+  // ensure DOM reads only happen once after a rPAF
+  const calculateGBCR = throttleRequestPostAnimationFrame()
 
   const clamp = x => Math.max(0, Math.min(1, x))
-  const throttledRaf = throttleRaf()
 
   export default {
+    oncreate () {
+      this.observe('dragging', dragging => {
+        if (dragging) {
+          this.fire('dragStart')
+        } else {
+          this.fire('dragEnd')
+        }
+      }, { init: false })
+      this.observe('indicatorStyle', indicatorStyle => {
+        console.log('Draggable indicatorStyle', indicatorStyle)
+        updateIndicatorStyle(() => {
+          this.set({ indicatorStyleAfterRaf: indicatorStyle })
+        })
+      })
+      this.observe('indicatorClass', indicatorClass => {
+        updateIndicatorClass(() => {
+          this.set(({ indicatorClassAfterRaf: indicatorClass }))
+        })
+      })
+      this.observe('draggableClass', draggableClass => {
+        updateDraggableClass(() => {
+          this.set({ draggableClassAfterRaf: draggableClass })
+        })
+      })
+    },
     data: () => ({
+      dragging: false,
       draggableClass: '',
+      draggableClassAfterRaf: '',
       indicatorClass: '',
+      indicatorClassAfterRaf: '',
       x: 0,
       y: 0,
       indicatorWidth: 0,
-      indicatorHeight: 0
+      indicatorHeight: 0,
+      indicatorStyleAfterRaf: ''
     }),
     computed: {
       indicatorStyle: ({ x, y, indicatorWidth, indicatorHeight }) => (
         `left: calc(${x * 100}% - ${indicatorWidth / 2}px); top: calc(${y * 100}% - ${indicatorHeight / 2}px);`
       )
     },
     methods: {
+      observe,
       onPointerDown (e) {
-        e.preventDefault()
-        e.stopPropagation()
+        console.log('Draggable: onPointerDown')
         const rect = this.refs.indicator.getBoundingClientRect()
+        console.log('Draggable: e.clientX', e.clientX)
+        console.log('Draggable: e.clientY', e.clientY)
         this.set({
           dragging: true,
           dragOffsetX: e.clientX - rect.left,
           dragOffsetY: e.clientY - rect.top
         })
       },
       onPointerMove (e) {
-        if (this.get().dragging) {
-          e.preventDefault()
-          e.stopPropagation()
-          const { indicatorWidth, indicatorHeight, dragOffsetX, dragOffsetY } = this.get()
-          throttledRaf(() => {
+        console.log('Draggable: onPointerMove')
+        const { dragging, indicatorWidth, indicatorHeight, dragOffsetX, dragOffsetY } = this.get()
+        if (dragging) {
+          console.log('Draggable: dragging')
+          calculateGBCR(() => {
             const rect = this.refs.area.getBoundingClientRect()
             const offsetX = dragOffsetX - (indicatorWidth / 2)
             const offsetY = dragOffsetY - (indicatorHeight / 2)
@@ -77,26 +120,32 @@
         }
       },
       onPointerUp (e) {
-        e.preventDefault()
-        e.stopPropagation()
+        console.log('Draggable: onPointerUp')
         this.set({ dragging: false })
       },
       onPointerLeave (e) {
-        e.preventDefault()
-        e.stopPropagation()
+        console.log('Draggable: onPointerLeave')
         this.set({ dragging: false })
       },
       onClick (e) {
+        console.log('Draggable: onClick')
+        console.log('Draggable: target classList', e.target.classList)
+        console.log('Draggable: currentTarget classList', e.currentTarget.classList)
         if (!e.target.classList.contains('draggable-indicator')) {
-          e.preventDefault()
-          e.stopPropagation()
+          console.log('Draggable: onClick handled')
           const rect = this.refs.area.getBoundingClientRect()
           const x = clamp((e.clientX - rect.left) / rect.width)
           const y = clamp((e.clientY - rect.top) / rect.height)
           this.set({ x, y })
           this.fire('change', { x, y })
         }
       }
+    },
+    events: {
+      pointerUp,
+      pointerDown,
+      pointerLeave,
+      pointerMove
     }
   }
 </script>
diff --git a/src/routes/_components/dialog/components/MediaFocalPointDialog.html b/src/routes/_components/dialog/components/MediaFocalPointDialog.html
@@ -25,12 +25,14 @@
         <!-- 52px == 32px icon width + 10px padding -->
         <Draggable
           draggableClass="media-draggable-area-inner"
-          indicatorClass="media-focal-point-indicator {imageLoaded ? '': 'hidden'}"
+          indicatorClass="media-focal-point-indicator {imageLoaded ? '': 'hidden'} {dragging ? 'dragging' : ''}"
           indicatorWidth={52}
           indicatorHeight={52}
           x={indicatorX}
           y={indicatorY}
           on:change="onDraggableChange(event)"
+          on:dragStart="set({dragging: true})"
+          on:dragEnd="set({dragging: false})"
         >
           <SvgIcon
             className="media-focal-point-indicator-svg"
@@ -142,6 +144,14 @@
     display: flex;
   }
 
+  :global(.media-focal-point-indicator:hover) {
+    background: var(--focal-bg-hover);
+  }
+
+  :global(.media-focal-point-indicator.dragging) {
+    background: var(--focal-bg-drag);
+  }
+
   :global(.media-draggable-area-inner) {
     width: 100%;
     height: 100%;
@@ -177,13 +187,17 @@
   import { store } from '../../../_store/store'
   import { get } from '../../../_utils/lodash-lite'
   import { observe } from 'svelte-extras'
-  import debounce from 'lodash-es/debounce'
   import { scheduleIdleTask } from '../../../_utils/scheduleIdleTask'
   import { coordsToPercent, percentToCoords } from '../../../_utils/coordsToPercent'
   import SvgIcon from '../../SvgIcon.html'
   import { intrinsicScale } from '../../../_thirdparty/intrinsic-scale/intrinsicScale'
   import { resize } from '../../../_utils/events'
   import Draggable from '../../Draggable.html'
+  import { throttleScheduleIdleTask } from '../../../_utils/throttleTimers'
+
+  // Updating the focal points in the store causes a lot of computations (extra JS work),
+  // so we really don't want to do it for every drag event.
+  const updateFocalPointsInStore = throttleScheduleIdleTask()
 
   const parseAndValidateFloat = rawText => {
     let float = parseFloat(rawText)
@@ -208,6 +222,7 @@
       Draggable
     },
     data: () => ({
+      dragging: false,
       rawFocusX: '0',
       rawFocusY: '0',
       containerWidth: 0,
@@ -276,16 +291,14 @@
         })
       },
       setupSyncToStore () {
-        const saveStore = debounce(() => scheduleIdleTask(() => this.store.save()), 1000)
-
         const observeAndSync = (rawKey, key) => {
           this.observe(rawKey, rawFocus => {
             const { realm, index, media } = this.get()
             const rawFocusDecimal = parseAndValidateFloat(rawFocus)
             if (media[index][key] !== rawFocusDecimal) {
               media[index][key] = rawFocusDecimal
               this.store.setComposeData(realm, { media })
-              saveStore()
+              scheduleIdleTask(() => this.store.save())
             }
           }, { init: false })
         }
@@ -294,16 +307,16 @@
         observeAndSync('rawFocusY', 'focusY')
       },
       onDraggableChange ({ x, y }) {
-        const saveStore = debounce(() => scheduleIdleTask(() => this.store.save()), 1000)
-
-        scheduleIdleTask(() => {
-          const focusX = percentToCoords(x * 100)
-          const focusY = percentToCoords(100 - (y * 100))
+        updateFocalPointsInStore(() => {
+          const focusX = parseAndValidateFloat(percentToCoords(x * 100))
+          const focusY = parseAndValidateFloat(percentToCoords(100 - (y * 100)))
           const { realm, index, media } = this.get()
-          media[index].focusX = parseAndValidateFloat(focusX)
-          media[index].focusY = parseAndValidateFloat(focusY)
-          this.store.setComposeData(realm, { media })
-          saveStore()
+          if (media[index].focusX !== focusX || media[index].focusY !== focusY) {
+            media[index].focusX = focusX
+            media[index].focusY = focusY
+            this.store.setComposeData(realm, { media })
+            scheduleIdleTask(() => this.store.save())
+          }
         })
       },
       measure () {

diff --git a/src/routes/_utils/asyncPolyfills.js b/src/routes/_utils/asyncPolyfills.js
@@ -13,7 +13,3 @@ export const importIndexedDBGetAllShim = () => import(
 export const importCustomElementsPolyfill = () => import(
   /* webpackChunkName: '$polyfill$-@webcomponents/custom-elements' */ '@webcomponents/custom-elements'
   )
-
-export const importPointerEventsPolyfill = () => import(
-  /* webpackChunkName: '$polyfill$-@wessberg/pointer-events' */ '@wessberg/pointer-events'
-  )
diff --git a/src/routes/_utils/loadPolyfills.js b/src/routes/_utils/loadPolyfills.js
@@ -2,16 +2,14 @@ import {
   importCustomElementsPolyfill,
   importIndexedDBGetAllShim,
   importIntersectionObserver,
-  importRequestIdleCallback,
-  importPointerEventsPolyfill
+  importRequestIdleCallback
 } from './asyncPolyfills'
 
 export function loadPolyfills () {
   return Promise.all([
     typeof IntersectionObserver === 'undefined' && importIntersectionObserver(),
     typeof requestIdleCallback === 'undefined' && importRequestIdleCallback(),
     !IDBObjectStore.prototype.getAll && importIndexedDBGetAllShim(),
-    typeof customElements === 'undefined' && importCustomElementsPolyfill(),
-    typeof PointerEvent === 'undefined' && importPointerEventsPolyfill()
+    typeof customElements === 'undefined' && importCustomElementsPolyfill()
   ])
 }
diff --git a/src/routes/_utils/pointerEvents.js b/src/routes/_utils/pointerEvents.js
@@ -0,0 +1,53 @@
+import { get } from './lodash-lite'
+
+const hasPointerEvents = process.browser && typeof PointerEvent === 'function'
+
+// Epiphany browser reports that it's a touch device even though it's not
+const isTouchDevice = process.browser && 'ontouchstart' in document && !/Epiphany/.test(navigator.userAgent)
+
+let pointerDown
+let pointerUp
+let pointerLeave
+let pointerMove
+
+function createEventListener (event) {
+  return (node, callback) => {
+    const listener = e => {
+      // lightweight polyfill for clientX/clientY in pointer events,
+      // which is slightly different in touch events
+      if (typeof e.clientX !== 'number') {
+        e.clientX = get(e, ['touches', 0, 'clientX'])
+      }
+      if (typeof e.clientY !== 'number') {
+        e.clientY = get(e, ['touches', 0, 'clientY'])
+      }
+      callback(e)
+    }
+
+    node.addEventListener(event, listener)
+    return {
+      destroy () {
+        node.removeEventListener(event, listener)
+      }
+    }
+  }
+}
+
+if (hasPointerEvents) {
+  pointerDown = createEventListener('pointerdown')
+  pointerUp = createEventListener('pointerup')
+  pointerLeave = createEventListener('pointerleave')
+  pointerMove = createEventListener('pointermove')
+} else if (isTouchDevice) {
+  pointerDown = createEventListener('touchstart')
+  pointerUp = createEventListener('touchend')
+  pointerLeave = createEventListener('touchend')
+  pointerMove = createEventListener('touchmove')
+} else {
+  pointerDown = createEventListener('mousedown')
+  pointerUp = createEventListener('mouseup')
+  pointerLeave = createEventListener('mouseleave')
+  pointerMove = createEventListener('mousemove')
+}
+
+export { pointerDown, pointerUp, pointerLeave, pointerMove }
diff --git a/src/routes/_utils/requestPostAnimationFrame.js b/src/routes/_utils/requestPostAnimationFrame.js
@@ -0,0 +1,9 @@
+// modeled after https://github.com/andrewiggins/afterframe
+// see also https://github.com/WICG/requestPostAnimationFrame
+export const requestPostAnimationFrame = cb => {
+  requestAnimationFrame(() => {
+    const channel = new MessageChannel()
+    channel.port1.onmessage = cb
+    channel.port2.postMessage(undefined)
+  })
+}
diff --git a/src/routes/_utils/throttleRaf.js b/src/routes/_utils/throttleRaf.js