From b603a3e4d548132b8ac04663594908a7ef2e2c69 Mon Sep 17 00:00:00 2001 From: Ugur Saglam <106508695+ugur-vaadin@users.noreply.github.com> Date: Wed, 30 Oct 2024 15:55:41 +0200 Subject: [PATCH] fix: exclude items from drag image for large scrollers (#8028) * fix: exclude items from drag image for large scrollers * refactor: apply for all browsers * refactor: add and remove listener on connected and disconnected * refactor: rename and move into constructor * test: update tests to observe whether browser crashes * fix: set overflow hidden temporarily to hide the scroll bar on drag --- .../src/vaadin-grid-drag-and-drop-mixin.js | 44 +++++++++++++ packages/grid/test/drag-and-drop.common.js | 60 ++++++++++++++++++ .../src/vaadin-virtual-list-mixin.js | 45 ++++++++++++- .../test/drag-and-drop-lit.test.js | 2 + .../test/drag-and-drop-polymer.test.js | 2 + .../virtual-list/test/drag-and-drop.common.js | 63 +++++++++++++++++++ 6 files changed, 215 insertions(+), 1 deletion(-) create mode 100644 packages/virtual-list/test/drag-and-drop-lit.test.js create mode 100644 packages/virtual-list/test/drag-and-drop-polymer.test.js create mode 100644 packages/virtual-list/test/drag-and-drop.common.js diff --git a/packages/grid/src/vaadin-grid-drag-and-drop-mixin.js b/packages/grid/src/vaadin-grid-drag-and-drop-mixin.js index 58f22b0e5e..512d7fec04 100644 --- a/packages/grid/src/vaadin-grid-drag-and-drop-mixin.js +++ b/packages/grid/src/vaadin-grid-drag-and-drop-mixin.js @@ -115,6 +115,11 @@ export const DragAndDropMixin = (superClass) => return ['_dragDropAccessChanged(rowsDraggable, dropMode, dragFilter, dropFilter, loading)']; } + constructor() { + super(); + this.__onDocumentDragStart = this.__onDocumentDragStart.bind(this); + } + /** @protected */ ready() { super.ready(); @@ -131,6 +136,22 @@ export const DragAndDropMixin = (superClass) => }); } + /** @protected */ + connectedCallback() { + super.connectedCallback(); + // Chromium based browsers cannot properly generate drag images for elements + // that have children with massive heights. This workaround prevents crashes + // and performance issues by excluding the items from the drag image. + // https://github.com/vaadin/web-components/issues/7985 + document.addEventListener('dragstart', this.__onDocumentDragStart, { capture: true }); + } + + /** @protected */ + disconnectedCallback() { + super.disconnectedCallback(); + document.removeEventListener('dragstart', this.__onDocumentDragStart, { capture: true }); + } + /** @private */ _onDragStart(e) { if (this.rowsDraggable) { @@ -291,6 +312,29 @@ export const DragAndDropMixin = (superClass) => } } + /** @private */ + __onDocumentDragStart(e) { + // The dragged element can be the element itself or a parent of the element + if (!e.target.contains(this)) { + return; + } + // The threshold value 20000 provides a buffer to both + // - avoid the crash and the performance issues + // - unnecessarily avoid excluding items from the drag image + if (this.$.items.offsetHeight > 20000) { + const initialItemsMaxHeight = this.$.items.style.maxHeight; + const initialTableOverflow = this.$.table.style.overflow; + // Momentarily hides the items until the browser starts generating the + // drag image. + this.$.items.style.maxHeight = '0'; + this.$.table.style.overflow = 'hidden'; + requestAnimationFrame(() => { + this.$.items.style.maxHeight = initialItemsMaxHeight; + this.$.table.style.overflow = initialTableOverflow; + }); + } + } + /** @private */ __dndAutoScroll(clientY) { if (this.__dndAutoScrolling) { diff --git a/packages/grid/test/drag-and-drop.common.js b/packages/grid/test/drag-and-drop.common.js index db0524605a..32e68fe1ec 100644 --- a/packages/grid/test/drag-and-drop.common.js +++ b/packages/grid/test/drag-and-drop.common.js @@ -1,6 +1,8 @@ import { expect } from '@vaadin/chai-plugins'; import { aTimeout, fixtureSync, listenOnce, nextFrame, oneEvent } from '@vaadin/testing-helpers'; +import { resetMouse, sendMouse } from '@web/test-runner-commands'; import sinon from 'sinon'; +import { hover } from '@vaadin/button/test/visual/helpers.js'; import { flushGrid, getBodyCellContent, getFirstCell, getRowBodyCells, getRows } from './helpers.js'; describe('drag and drop', () => { @@ -1083,4 +1085,62 @@ describe('drag and drop', () => { expect(getFirstCell(grid).getAttribute('part')).to.contain('drag-source-row-cell'); }); }); + + describe('draggable grid', () => { + let container; + let items; + let table; + + beforeEach(async () => { + container = fixtureSync(` +
+ + + +
+ `); + grid = container.querySelector('vaadin-grid'); + document.body.appendChild(container); + flushGrid(grid); + await nextFrame(); + items = grid.shadowRoot.querySelector('#items'); + table = grid.shadowRoot.querySelector('#table'); + }); + + async function setGridItems(count) { + grid.items = Array.from({ length: count }, (_, i) => ({ value: `Item ${i + 1}` })); + await nextFrame(); + } + + async function dragElement(element) { + await resetMouse(); + await hover(element); + await sendMouse({ type: 'down' }); + await sendMouse({ type: 'move', position: [100, 100] }); + await sendMouse({ type: 'up' }); + } + + async function assertDragSucceeds(draggedElement) { + // maxHeight and overflow are temporarily updated in the related fix + const initialItemsMaxHeight = items.style.maxHeight; + const initialTableOverflow = table.style.overflow; + await dragElement(draggedElement); + expect(items.style.maxHeight).to.equal(initialItemsMaxHeight); + expect(table.style.overflow).to.equal(initialTableOverflow); + } + + ['5000', '50000'].forEach((count) => { + it(`should not crash when dragging a grid with ${count} items`, async () => { + await setGridItems(count); + await assertDragSucceeds(grid); + }); + + it(`should not crash when dragging a container that has a grid with ${count} items`, async () => { + grid.removeAttribute('draggable'); + container.setAttribute('draggable', true); + await setGridItems(count); + await assertDragSucceeds(container); + }); + }); + }); }); diff --git a/packages/virtual-list/src/vaadin-virtual-list-mixin.js b/packages/virtual-list/src/vaadin-virtual-list-mixin.js index 9c1f067cb1..e5b3d427e2 100644 --- a/packages/virtual-list/src/vaadin-virtual-list-mixin.js +++ b/packages/virtual-list/src/vaadin-virtual-list-mixin.js @@ -63,6 +63,11 @@ export const VirtualListMixin = (superClass) => return this.__virtualizer.lastVisibleIndex; } + constructor() { + super(); + this.__onDragStart = this.__onDragStart.bind(this); + } + /** @protected */ ready() { super.ready(); @@ -75,13 +80,28 @@ export const VirtualListMixin = (superClass) => scrollContainer: this.shadowRoot.querySelector('#items'), reorderElements: true, }); - this.__overflowController = new OverflowController(this); this.addController(this.__overflowController); processTemplates(this); } + /** @protected */ + connectedCallback() { + super.connectedCallback(); + // Chromium based browsers cannot properly generate drag images for elements + // that have children with massive heights. This workaround prevents crashes + // and performance issues by excluding the items from the drag image. + // https://github.com/vaadin/web-components/issues/7985 + document.addEventListener('dragstart', this.__onDragStart, { capture: true }); + } + + /** @protected */ + disconnectedCallback() { + super.disconnectedCallback(); + document.removeEventListener('dragstart', this.__onDragStart, { capture: true }); + } + /** * Scroll to a specific index in the virtual list. * @@ -133,6 +153,29 @@ export const VirtualListMixin = (superClass) => } } + /** @private */ + __onDragStart(e) { + // The dragged element can be the element itself or a parent of the element + if (!e.target.contains(this)) { + return; + } + // The threshold value 20000 provides a buffer to both + // - avoid the crash and the performance issues + // - unnecessarily avoid excluding items from the drag image + if (this.$.items.offsetHeight > 20000) { + const initialItemsMaxHeight = this.$.items.style.maxHeight; + const initialVirtualListOverflow = this.style.overflow; + // Momentarily hides the items until the browser starts generating the + // drag image. + this.$.items.style.maxHeight = '0'; + this.style.overflow = 'hidden'; + requestAnimationFrame(() => { + this.$.items.style.maxHeight = initialItemsMaxHeight; + this.style.overflow = initialVirtualListOverflow; + }); + } + } + /** * Requests an update for the content of the rows. * While performing the update, it invokes the renderer passed in the `renderer` property for each visible row. diff --git a/packages/virtual-list/test/drag-and-drop-lit.test.js b/packages/virtual-list/test/drag-and-drop-lit.test.js new file mode 100644 index 0000000000..80a8560bd2 --- /dev/null +++ b/packages/virtual-list/test/drag-and-drop-lit.test.js @@ -0,0 +1,2 @@ +import '../vaadin-lit-virtual-list.js'; +import './drag-and-drop.common.js'; diff --git a/packages/virtual-list/test/drag-and-drop-polymer.test.js b/packages/virtual-list/test/drag-and-drop-polymer.test.js new file mode 100644 index 0000000000..39e29db299 --- /dev/null +++ b/packages/virtual-list/test/drag-and-drop-polymer.test.js @@ -0,0 +1,2 @@ +import '../vaadin-virtual-list.js'; +import './drag-and-drop.common.js'; diff --git a/packages/virtual-list/test/drag-and-drop.common.js b/packages/virtual-list/test/drag-and-drop.common.js new file mode 100644 index 0000000000..cd844bf877 --- /dev/null +++ b/packages/virtual-list/test/drag-and-drop.common.js @@ -0,0 +1,63 @@ +import { expect } from '@vaadin/chai-plugins'; +import { fixtureSync, nextFrame } from '@vaadin/testing-helpers'; +import { resetMouse, sendMouse } from '@web/test-runner-commands'; +import { hover } from '@vaadin/button/test/visual/helpers.js'; + +describe('drag and drop', () => { + let virtualList; + let container; + let items; + + beforeEach(async () => { + container = fixtureSync(` +
+ +
+ `); + virtualList = container.querySelector('vaadin-virtual-list'); + virtualList.renderer = (root, _, { item }) => { + root.innerHTML = `
${item.label}
`; + }; + document.body.appendChild(container); + await nextFrame(); + items = virtualList.shadowRoot.querySelector('#items'); + }); + + async function setVirtualListItems(count) { + virtualList.items = Array.from({ length: count }).map((_, i) => { + return { label: `Item ${i}` }; + }); + await nextFrame(); + } + + async function dragElement(element) { + await resetMouse(); + await hover(element); + await sendMouse({ type: 'down' }); + await sendMouse({ type: 'move', position: [100, 100] }); + await sendMouse({ type: 'up' }); + } + + async function assertDragSucceeds(draggedElement) { + // maxHeight and overflow are temporarily updated in the related fix + const initialItemsMaxHeight = items.style.maxHeight; + const initialVirtualListOverflow = virtualList.style.overflow; + await dragElement(draggedElement); + expect(items.style.maxHeight).to.equal(initialItemsMaxHeight); + expect(virtualList.style.overflow).to.equal(initialVirtualListOverflow); + } + + ['5000', '50000'].forEach((count) => { + it(`should not crash when dragging a virtual list with ${count} items`, async () => { + await setVirtualListItems(count); + await assertDragSucceeds(virtualList); + }); + + it(`should not crash when dragging a container that has a virtual list with ${count} items`, async () => { + virtualList.removeAttribute('draggable'); + container.setAttribute('draggable', true); + await setVirtualListItems(count); + await assertDragSucceeds(container); + }); + }); +});