diff --git a/.changeset/fix-sort-animation-crash.md b/.changeset/fix-sort-animation-crash.md new file mode 100644 index 00000000..6ca9a30d --- /dev/null +++ b/.changeset/fix-sort-animation-crash.md @@ -0,0 +1,5 @@ +--- +'@shopify/draggable': patch +--- + +Fix unhandled TypeError in SortAnimation when sorting into empty containers or across containers with out-of-bounds element indices. diff --git a/src/Plugins/SortAnimation/SortAnimation.js b/src/Plugins/SortAnimation/SortAnimation.js index a65a5f85..b50f5ea0 100644 --- a/src/Plugins/SortAnimation/SortAnimation.js +++ b/src/Plugins/SortAnimation/SortAnimation.js @@ -83,8 +83,11 @@ export default class SortAnimation extends AbstractPlugin { * @param {SortableSortEvent} sortableEvent * @private */ - [onSortableSort]({dragEvent}) { - const {sourceContainer} = dragEvent; + [onSortableSort]({dragEvent} = {}) { + const {sourceContainer} = dragEvent || {}; + if (!sourceContainer) { + return; + } const elements = this.draggable.getDraggableElementsForContainer(sourceContainer); this.lastElements = Array.from(elements).map((el) => { @@ -102,7 +105,11 @@ export default class SortAnimation extends AbstractPlugin { * @private */ [onSortableSorted]({oldIndex, newIndex}) { - if (oldIndex === newIndex) { + if ( + oldIndex === newIndex || + !this.lastElements || + this.lastElements.length === 0 + ) { return; } @@ -123,10 +130,16 @@ export default class SortAnimation extends AbstractPlugin { for (let i = start; i <= end; i++) { const from = this.lastElements[i]; const to = this.lastElements[i + num]; - effectedElements.push({from, to}); + if (from && to) { + effectedElements.push({from, to}); + } } cancelAnimationFrame(this.lastAnimationFrame); + if (effectedElements.length === 0) { + return; + } + // Can be done in a separate frame this.lastAnimationFrame = requestAnimationFrame(() => { effectedElements.forEach((element) => animate(element, this.options)); @@ -144,7 +157,11 @@ export default class SortAnimation extends AbstractPlugin { * @param {String} options.easingFunction * @private */ -function animate({from, to}, {duration, easingFunction}) { +function animate({from, to} = {}, {duration, easingFunction}) { + if (!from || !to || !from.domEl || !to.domEl) { + return; + } + const domEl = from.domEl; const x = from.offsetLeft - to.offsetLeft; const y = from.offsetTop - to.offsetTop; @@ -165,6 +182,10 @@ function animate({from, to}, {duration, easingFunction}) { * @private */ function resetElementOnTransitionEnd(event) { + if (!event.target || !event.target.style) { + return; + } + event.target.style.transition = ''; event.target.style.pointerEvents = ''; event.target.removeEventListener( diff --git a/src/Plugins/SortAnimation/tests/SortAnimation.test.ts b/src/Plugins/SortAnimation/tests/SortAnimation.test.ts new file mode 100644 index 00000000..da791ab6 --- /dev/null +++ b/src/Plugins/SortAnimation/tests/SortAnimation.test.ts @@ -0,0 +1,237 @@ +import {createSandbox, waitForRequestAnimationFrame, DRAG_DELAY} from 'helper'; +import {FixMeAny} from 'shared/types'; + +/* eslint-disable @typescript-eslint/ban-ts-comment */ +// @ts-ignore +import Sortable from '../../../Sortable'; +// @ts-ignore +import SortAnimation, {defaultOptions} from '../SortAnimation'; +/* eslint-enable @typescript-eslint/ban-ts-comment */ + +const sampleMarkup = ` + + +`; + +describe('SortAnimation', () => { + let sandbox: HTMLElement; + let containers: HTMLElement[]; + let sortable: FixMeAny; + let firstItem: HTMLElement; + let secondItem: HTMLElement; + let thirdItem: HTMLElement; + let emptyContainer: HTMLElement; + + beforeEach(() => { + sandbox = createSandbox(sampleMarkup); + containers = Array.from(sandbox.querySelectorAll('.Container')); + firstItem = sandbox.querySelector('.Item--1') as HTMLElement; + secondItem = sandbox.querySelector('.Item--2') as HTMLElement; + thirdItem = sandbox.querySelector('.Item--3') as HTMLElement; + emptyContainer = sandbox.querySelector('.Container--empty') as HTMLElement; + + sortable = new Sortable(containers, { + draggable: '.Item', + delay: DRAG_DELAY, + plugins: [SortAnimation], + }); + }); + + afterEach(() => { + sortable.destroy(); + sandbox.remove(); + }); + + it('initializes with default options', () => { + const plugin = sortable.plugins.find( + (pluginInstance: FixMeAny) => pluginInstance instanceof SortAnimation, + ); + expect(plugin).toBeDefined(); + expect(plugin.options).toMatchObject(defaultOptions); + }); + + it('accepts custom options passed through Sortable', () => { + sortable.destroy(); + + sortable = new Sortable(containers, { + draggable: '.Item', + delay: DRAG_DELAY, + plugins: [SortAnimation], + sortAnimation: { + duration: 350, + easingFunction: 'linear', + }, + }); + + const plugin = sortable.plugins.find( + (pluginInstance: FixMeAny) => pluginInstance instanceof SortAnimation, + ); + expect(plugin.options).toMatchObject({ + duration: 350, + easingFunction: 'linear', + }); + }); + + it('attaches and detaches listeners on Sortable', () => { + const plugin = sortable.plugins.find( + (pluginInstance: FixMeAny) => pluginInstance instanceof SortAnimation, + ); + + plugin.detach(); + expect(() => { + sortable.trigger({ + type: 'sortable:sort', + dragEvent: {sourceContainer: containers[0]}, + }); + sortable.trigger({ + type: 'sortable:sorted', + oldIndex: 0, + newIndex: 1, + dragEvent: {source: firstItem, over: secondItem}, + }); + }).not.toThrow(); + + plugin.attach(); + }); + + it('records lastElements positions on sortable:sort', () => { + const plugin = sortable.plugins.find( + (pluginInstance: FixMeAny) => pluginInstance instanceof SortAnimation, + ); + + sortable.trigger({ + type: 'sortable:sort', + dragEvent: {sourceContainer: containers[0]}, + }); + + expect(plugin.lastElements).toHaveLength(3); + expect(plugin.lastElements[0].domEl).toBe(firstItem); + }); + + it('animates elements during sortable:sorted when moved down', () => { + sortable.trigger({ + type: 'sortable:sort', + dragEvent: {sourceContainer: containers[0]}, + }); + + sortable.trigger({ + type: 'sortable:sorted', + oldIndex: 0, + newIndex: 1, + dragEvent: {source: firstItem, over: secondItem}, + }); + + // 1st frame: applies initial transform and pointer-events: none + waitForRequestAnimationFrame(); + + expect(secondItem.style.pointerEvents).toBe('none'); + + // 2nd frame: applies transition + waitForRequestAnimationFrame(); + + expect(secondItem.style.transition).toContain('transform'); + }); + + it('animates elements during sortable:sorted when moved up', () => { + sortable.trigger({ + type: 'sortable:sort', + dragEvent: {sourceContainer: containers[0]}, + }); + + sortable.trigger({ + type: 'sortable:sorted', + oldIndex: 2, + newIndex: 0, + dragEvent: {source: thirdItem, over: firstItem}, + }); + + waitForRequestAnimationFrame(); + + expect(firstItem.style.pointerEvents).toBe('none'); + + waitForRequestAnimationFrame(); + + expect(firstItem.style.transition).toContain('transform'); + }); + + it('does not throw when sortable:sorted is triggered without prior sort (empty lastElements)', () => { + expect(() => { + sortable.trigger({ + type: 'sortable:sorted', + oldIndex: 0, + newIndex: 1, + dragEvent: { + source: firstItem, + over: undefined, + overContainer: emptyContainer, + }, + }); + + waitForRequestAnimationFrame(); + }).not.toThrow(); + }); + + it('does not throw when indices are out of bounds or elements are undefined', () => { + const plugin = sortable.plugins.find( + (pluginInstance: FixMeAny) => pluginInstance instanceof SortAnimation, + ); + + sortable.trigger({ + type: 'sortable:sort', + dragEvent: {sourceContainer: containers[0]}, + }); + + expect(() => { + // oldIndex and newIndex beyond recorded elements length + sortable.trigger({ + type: 'sortable:sorted', + oldIndex: 10, + newIndex: 20, + dragEvent: {source: firstItem, over: secondItem}, + }); + + waitForRequestAnimationFrame(); + }).not.toThrow(); + + expect(() => { + // sortable:sort with invalid/missing dragEvent + plugin.draggable.trigger({ + type: 'sortable:sort', + dragEvent: undefined, + }); + }).not.toThrow(); + }); + + it('cleans up styles on transitionend event', () => { + sortable.trigger({ + type: 'sortable:sort', + dragEvent: {sourceContainer: containers[0]}, + }); + + sortable.trigger({ + type: 'sortable:sorted', + oldIndex: 0, + newIndex: 1, + dragEvent: {source: firstItem, over: secondItem}, + }); + + waitForRequestAnimationFrame(); + waitForRequestAnimationFrame(); + + const transitionEndEvent = new Event('transitionend', {bubbles: true}); + secondItem.dispatchEvent(transitionEndEvent); + + expect(secondItem.style.transition).toBe(''); + expect(secondItem.style.pointerEvents).toBe(''); + + // Trigger transitionend with null/non-element target without throwing + const customEvent = new CustomEvent('transitionend'); + Object.defineProperty(customEvent, 'target', {value: null}); + window.dispatchEvent(customEvent); + }); +});