Skip to content

Commit

Permalink
fix: exclude items from drag image for large scrollers (#8028)
Browse files Browse the repository at this point in the history
* 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
  • Loading branch information
ugur-vaadin authored Oct 30, 2024
1 parent 5ec5830 commit 6d4a78d
Show file tree
Hide file tree
Showing 6 changed files with 215 additions and 1 deletion.
44 changes: 44 additions & 0 deletions packages/grid/src/vaadin-grid-drag-and-drop-mixin.js
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand All @@ -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) {
Expand Down Expand Up @@ -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) {
Expand Down
60 changes: 60 additions & 0 deletions packages/grid/test/drag-and-drop.common.js
Original file line number Diff line number Diff line change
@@ -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', () => {
Expand Down Expand Up @@ -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(`
<div style="width: 400px; height: 400px;">
<vaadin-grid draggable="true" style="width: 300px; height: 300px;">
<vaadin-grid-column path="value"></vaadin-grid-column>
</vaadin-grid>
</div>
`);
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);
});
});
});
});
45 changes: 44 additions & 1 deletion packages/virtual-list/src/vaadin-virtual-list-mixin.js
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,11 @@ export const VirtualListMixin = (superClass) =>
return this.__virtualizer.lastVisibleIndex;
}

constructor() {
super();
this.__onDragStart = this.__onDragStart.bind(this);
}

/** @protected */
ready() {
super.ready();
Expand All @@ -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.
*
Expand Down Expand Up @@ -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.
Expand Down
2 changes: 2 additions & 0 deletions packages/virtual-list/test/drag-and-drop-lit.test.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
import '../vaadin-lit-virtual-list.js';
import './drag-and-drop.common.js';
2 changes: 2 additions & 0 deletions packages/virtual-list/test/drag-and-drop-polymer.test.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
import '../vaadin-virtual-list.js';
import './drag-and-drop.common.js';
63 changes: 63 additions & 0 deletions packages/virtual-list/test/drag-and-drop.common.js
Original file line number Diff line number Diff line change
@@ -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(`
<div style="width: 300px; height: 300px;">
<vaadin-virtual-list draggable="true"></vaadin-virtual-list>
</div>
`);
virtualList = container.querySelector('vaadin-virtual-list');
virtualList.renderer = (root, _, { item }) => {
root.innerHTML = `<div>${item.label}</div>`;
};
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);
});
});
});

0 comments on commit 6d4a78d

Please sign in to comment.