From c93fb237d87b9b9e0250339898f7a1bf805c3087 Mon Sep 17 00:00:00 2001 From: Alex Reardon Date: Mon, 2 Jul 2018 15:24:33 +1000 Subject: [PATCH 1/2] avoiding dom reads more agressively --- src/state/middleware/max-scroll-updater.js | 66 +++++++++++++++++----- 1 file changed, 51 insertions(+), 15 deletions(-) diff --git a/src/state/middleware/max-scroll-updater.js b/src/state/middleware/max-scroll-updater.js index 79d378a0e4..25c466fd13 100644 --- a/src/state/middleware/max-scroll-updater.js +++ b/src/state/middleware/max-scroll-updater.js @@ -1,14 +1,14 @@ // @flow import invariant from 'tiny-invariant'; import type { Position } from 'css-box-model'; -import type { State, Viewport } from '../../types'; +import type { State, Viewport, DraggableLocation } from '../../types'; import type { Action, Store } from '../store-types'; import getMaxScroll from '../get-max-scroll'; import { isEqual } from '../position'; import { updateViewportMaxScroll } from '../action-creators'; import isMovementAllowed from '../is-movement-allowed'; -const shouldCheckMaxScroll = (action: Action): boolean => +const shouldCheckOnAction = (action: Action): boolean => action.type === 'MOVE' || action.type === 'MOVE_UP' || action.type === 'MOVE_RIGHT' || @@ -16,17 +16,48 @@ const shouldCheckMaxScroll = (action: Action): boolean => action.type === 'MOVE_LEFT' || action.type === 'MOVE_BY_WINDOW_SCROLL'; -const getNewMaxScroll = (state: State, action: Action): ?Position => { - if (!isMovementAllowed(state)) { +// optimisation: body size can only change when the destination has changed +const hasDroppableOverChanged = ( + previous: ?DraggableLocation, + current: ?DraggableLocation, +): boolean => { + // no previous - if there is a next return true + if (!previous) { + return Boolean(current); + } + + // no current - if there is a previous return true + if (!current) { + return Boolean(previous); + } + + return previous.droppableId !== current.droppableId; +}; + +const getNewMaxScroll = ( + previous: State, + current: State, + action: Action, +): ?Position => { + if (!shouldCheckOnAction(action)) { return null; } - if (!shouldCheckMaxScroll(action)) { + if (!isMovementAllowed(previous) || !isMovementAllowed(current)) { + return null; + } + + if ( + !hasDroppableOverChanged( + previous.impact.destination, + current.impact.destination, + ) + ) { return null; } // check to see if the viewport max scroll has changed - const viewport: Viewport = state.viewport; + const viewport: Viewport = current.viewport; const doc: ?HTMLElement = document.documentElement; invariant(doc, 'Could not find document.documentElement'); @@ -48,14 +79,19 @@ const getNewMaxScroll = (state: State, action: Action): ?Position => { return maxScroll; }; -export default (store: Store) => (next: Action => mixed) => ( - action: Action, -): mixed => { - const maxScroll: ?Position = getNewMaxScroll(store.getState(), action); - // max scroll has changed - updating before action - if (maxScroll) { - next(updateViewportMaxScroll(maxScroll)); - } +export default (store: Store) => { + let previous: State = store.getState(); + + return (next: Action => mixed) => (action: Action): mixed => { + const current: State = store.getState(); + const maxScroll: ?Position = getNewMaxScroll(previous, current, action); + previous = current; + + // max scroll has changed - updating before action + if (maxScroll) { + next(updateViewportMaxScroll(maxScroll)); + } - next(action); + next(action); + }; }; From e1342623c7b57a54e92dbb653ba33c1a7180fb4e Mon Sep 17 00:00:00 2001 From: Alex Reardon Date: Mon, 2 Jul 2018 15:36:13 +1000 Subject: [PATCH 2/2] less DOM reads while dragging --- src/state/middleware/max-scroll-updater.js | 27 ++--- .../middleware/max-scroll-updater.spec.js | 105 +++++++++++------- 2 files changed, 76 insertions(+), 56 deletions(-) diff --git a/src/state/middleware/max-scroll-updater.js b/src/state/middleware/max-scroll-updater.js index 25c466fd13..58a37edd7f 100644 --- a/src/state/middleware/max-scroll-updater.js +++ b/src/state/middleware/max-scroll-updater.js @@ -79,19 +79,16 @@ const getNewMaxScroll = ( return maxScroll; }; -export default (store: Store) => { - let previous: State = store.getState(); - - return (next: Action => mixed) => (action: Action): mixed => { - const current: State = store.getState(); - const maxScroll: ?Position = getNewMaxScroll(previous, current, action); - previous = current; - - // max scroll has changed - updating before action - if (maxScroll) { - next(updateViewportMaxScroll(maxScroll)); - } - - next(action); - }; +export default (store: Store) => (next: Action => mixed) => ( + action: Action, +): mixed => { + const previous: State = store.getState(); + next(action); + const current: State = store.getState(); + const maxScroll: ?Position = getNewMaxScroll(previous, current, action); + + // max scroll has changed - updating before action + if (maxScroll) { + next(updateViewportMaxScroll(maxScroll)); + } }; diff --git a/test/unit/state/middleware/max-scroll-updater.spec.js b/test/unit/state/middleware/max-scroll-updater.spec.js index 0300322923..6a96870151 100644 --- a/test/unit/state/middleware/max-scroll-updater.spec.js +++ b/test/unit/state/middleware/max-scroll-updater.spec.js @@ -9,6 +9,7 @@ import { updateViewportMaxScroll, initialPublish, moveDown, + moveRight, } from '../../../../src/state/action-creators'; import type { Store } from '../../../../src/state/store-types'; import type { Viewport } from '../../../../src/types'; @@ -39,59 +40,81 @@ afterAll(() => { doc.scrollWidth = originalWidth; }); -it('should not update the max viewport scroll if no drag is occurring', () => { +describe('not dragging', () => { + it('should not update the max viewport scroll if no drag is occurring', () => { + const mock = jest.fn(); + const store: Store = createStore(middleware, passThrough(mock)); + + doc.scrollHeight = scrollHeight + 10; + doc.scrollWidth = scrollWidth + 10; + + store.dispatch(prepare()); + + expect(mock).toHaveBeenCalledTimes(1); + expect(mock).toHaveBeenCalledWith(prepare()); + }); +}); + +it('should update if the max scroll position has changed and the destination has changed', () => { const mock = jest.fn(); const store: Store = createStore(middleware, passThrough(mock)); + // now dragging + store.dispatch(prepare()); + store.dispatch(initialPublish(initialPublishArgs)); + expect(store.getState().isDragging).toBe(true); + mock.mockClear(); + + // change in scroll size doc.scrollHeight = scrollHeight + 10; doc.scrollWidth = scrollWidth + 10; - store.dispatch(prepare()); - - expect(mock).toHaveBeenCalledTimes(1); - expect(mock).toHaveBeenCalledWith(prepare()); + const expected: Position = getMaxScroll({ + height: viewport.frame.height, + width: viewport.frame.width, + scrollHeight: scrollHeight + 10, + scrollWidth: scrollWidth + 10, + }); + // changing droppable + store.dispatch(moveRight()); + expect(mock).toHaveBeenCalledTimes(2); + expect(mock).toHaveBeenCalledWith(moveRight()); + expect(mock).toHaveBeenCalledWith(updateViewportMaxScroll(expected)); }); -describe('during a drag', () => { - it('should not update the max viewport scroll if the max scroll position has not changed', () => { - const mock = jest.fn(); - const store: Store = createStore(middleware, passThrough(mock)); +it('should not update if the max scroll position has not changed and destination has', () => { + const mock = jest.fn(); + const store: Store = createStore(middleware, passThrough(mock)); - // now dragging - store.dispatch(prepare()); - store.dispatch(initialPublish(initialPublishArgs)); - expect(store.getState().isDragging).toBe(true); - mock.mockClear(); + // now dragging + store.dispatch(prepare()); + store.dispatch(initialPublish(initialPublishArgs)); + expect(store.getState().isDragging).toBe(true); + mock.mockClear(); - // no change in scroll size - store.dispatch(moveDown()); - expect(mock).toHaveBeenCalledWith(moveDown()); - expect(mock).toHaveBeenCalledTimes(1); - }); + // no change in scroll size but there is a change in destination + store.dispatch(moveRight()); + expect(mock).toHaveBeenCalledWith(moveRight()); + expect(mock).toHaveBeenCalledTimes(1); +}); - it('should update the max viewport scroll if the max scroll position has changed', () => { - const mock = jest.fn(); - const store: Store = createStore(middleware, passThrough(mock)); +it('should not update if the destination has not changed (even if the scroll size has changed)', () => { + // the scroll size should not change in response to a drag if the destination has not changed + const mock = jest.fn(); + const store: Store = createStore(middleware, passThrough(mock)); - // now dragging - store.dispatch(prepare()); - store.dispatch(initialPublish(initialPublishArgs)); - expect(store.getState().isDragging).toBe(true); - mock.mockClear(); + // now dragging + store.dispatch(prepare()); + store.dispatch(initialPublish(initialPublishArgs)); + expect(store.getState().isDragging).toBe(true); + mock.mockClear(); - // change in scroll size - doc.scrollHeight = scrollHeight + 10; - doc.scrollWidth = scrollWidth + 10; + // change in scroll size + doc.scrollHeight = scrollHeight + 10; + doc.scrollWidth = scrollWidth + 10; - const expected: Position = getMaxScroll({ - height: viewport.frame.height, - width: viewport.frame.width, - scrollHeight: scrollHeight + 10, - scrollWidth: scrollWidth + 10, - }); - store.dispatch(moveDown()); - expect(mock).toHaveBeenCalledTimes(2); - expect(mock).toHaveBeenCalledWith(moveDown()); - expect(mock).toHaveBeenCalledWith(updateViewportMaxScroll(expected)); - }); + // not changing droppable + store.dispatch(moveDown()); + expect(mock).toHaveBeenCalledTimes(1); + expect(mock).toHaveBeenCalledWith(moveDown()); });