Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: hide scroller completely when dragging large grids (#8351) (CP: 23.5) #8365

Merged
merged 1 commit into from
Dec 18, 2024
Merged

fix: hide scroller completely when dragging large grids (#8351) (CP: 23.5) #8365

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 16 additions & 21 deletions packages/grid/src/vaadin-grid-drag-and-drop-mixin.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -120,10 +120,6 @@ 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 });
}

Expand Down Expand Up @@ -286,25 +282,24 @@ export const DragAndDropMixin = (superClass) =>
}
}

/** @private */
/**
* Webkit-based browsers have issues with generating drag images
* for elements that have children with massive heights. Chromium
* browsers crash, while Safari experiences significant performance
* issues. To mitigate these issues, we hide the scroller element
* when drag starts to remove it from the drag image.
*
* Related issues:
* - https://github.com/vaadin/web-components/issues/7985
* - https://issues.chromium.org/issues/383356871
*
* @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';
if (e.target.contains(this) && this.$.table.scrollHeight > 20000) {
this.$.scroller.style.display = 'none';
requestAnimationFrame(() => {
this.$.items.style.maxHeight = initialItemsMaxHeight;
this.$.table.style.overflow = initialTableOverflow;
this.$.scroller.style.display = '';
});
}
}
Expand Down
75 changes: 19 additions & 56 deletions packages/grid/test/drag-and-drop.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import { expect } from '@esm-bundle/chai';
import { aTimeout, fixtureSync, listenOnce, nextFrame, oneEvent } from '@vaadin/testing-helpers';
import { aTimeout, fire, fixtureSync, listenOnce, nextFrame, oneEvent } from '@vaadin/testing-helpers';
import sinon from 'sinon';
import '../vaadin-grid.js';
import { flushGrid, getBodyCellContent, getFirstCell, getRows } from './helpers.js';
Expand Down Expand Up @@ -873,87 +873,50 @@ describe('drag and drop', () => {

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>
`);
<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();
}

function getState() {
return { itemsMaxHeight: items.style.maxHeight, tableOverflow: table.style.overflow };
}

function getExpectedDragStartState() {
return { itemsMaxHeight: '0px', tableOverflow: 'hidden' };
}

function assertStatesEqual(state1, state2) {
expect(state1.itemsMaxHeight).to.equal(state2.itemsMaxHeight);
expect(state1.tableOverflow).to.equal(state2.tableOverflow);
}

async function getStateDuringDragStart(element) {
let stateDuringDragStart;
element.addEventListener(
'dragstart',
() => {
stateDuringDragStart = getState();
},
{ once: true },
);
element.dispatchEvent(new DragEvent('dragstart'));
await new Promise((resolve) => {
requestAnimationFrame(resolve);
});
return stateDuringDragStart;
}

it('should not change state on dragstart for small grids', async () => {
await setGridItems(10);
const initialState = getState();
const stateDuringDragStart = await getStateDuringDragStart(grid);
assertStatesEqual(stateDuringDragStart, initialState);
const finalState = getState();
assertStatesEqual(finalState, initialState);
fire(grid, 'dragstart');
expect(grid.$.scroller.style.display).to.equal('');
await nextFrame();
expect(grid.$.scroller.style.display).to.equal('');
});

['5000', '50000'].forEach((count) => {
it('should temporarily change state on dragstart for large grids', async () => {
await setGridItems(count);
const initialState = getState();
const stateDuringDragStart = await getStateDuringDragStart(grid);
assertStatesEqual(stateDuringDragStart, getExpectedDragStartState());
const finalState = getState();
assertStatesEqual(finalState, initialState);
fire(grid, 'dragstart');
expect(grid.$.scroller.style.display).to.equal('none');
await nextFrame();
expect(grid.$.scroller.style.display).to.equal('');
});

it('should temporarily change state on dragstart for large grids in draggable containers', async () => {
grid.removeAttribute('draggable');
container.setAttribute('draggable', true);
await setGridItems(count);
const initialState = getState();
const stateDuringDragStart = await getStateDuringDragStart(container);
assertStatesEqual(stateDuringDragStart, getExpectedDragStartState());
const finalState = getState();
assertStatesEqual(finalState, initialState);
fire(container, 'dragstart');
expect(grid.$.scroller.style.display).to.equal('none');
await nextFrame();
expect(grid.$.scroller.style.display).to.equal('');
});
});
});
Expand Down
45 changes: 20 additions & 25 deletions packages/virtual-list/src/vaadin-virtual-list.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -112,7 +112,7 @@ class VirtualList extends ElementMixin(ControllerMixin(ThemableMixin(PolymerElem

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

/** @protected */
Expand All @@ -136,17 +136,13 @@ class VirtualList extends ElementMixin(ControllerMixin(ThemableMixin(PolymerElem
/** @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 });
document.addEventListener('dragstart', this.__onDocumentDragStart, { capture: true });
}

/** @protected */
disconnectedCallback() {
super.disconnectedCallback();
document.removeEventListener('dragstart', this.__onDragStart, { capture: true });
document.removeEventListener('dragstart', this.__onDocumentDragStart, { capture: true });
}

/**
Expand Down Expand Up @@ -200,25 +196,24 @@ class VirtualList extends ElementMixin(ControllerMixin(ThemableMixin(PolymerElem
}
}

/** @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';
/**
* Webkit-based browsers have issues with generating drag images
* for elements that have children with massive heights. Chromium
* browsers crash, while Safari experiences significant performance
* issues. To mitigate these issues, we hide the items container
* when drag starts to remove it from the drag image.
*
* Related issues:
* - https://github.com/vaadin/web-components/issues/7985
* - https://issues.chromium.org/issues/383356871
*
* @private
*/
__onDocumentDragStart(e) {
if (e.target.contains(this) && this.scrollHeight > 20000) {
this.$.items.style.display = 'none';
requestAnimationFrame(() => {
this.$.items.style.maxHeight = initialItemsMaxHeight;
this.style.overflow = initialVirtualListOverflow;
this.$.items.style.display = '';
});
}
}
Expand Down
69 changes: 17 additions & 52 deletions packages/virtual-list/test/virtual-list.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import { expect } from '@esm-bundle/chai';
import { fixtureSync, nextFrame } from '@vaadin/testing-helpers';
import { fire, fixtureSync, nextFrame } from '@vaadin/testing-helpers';
import '../vaadin-virtual-list.js';

describe('virtual-list', () => {
Expand Down Expand Up @@ -147,21 +147,18 @@ describe('virtual-list', () => {
});
describe('drag and drop', () => {
let container;
let items;

beforeEach(async () => {
container = fixtureSync(`
<div style="width: 300px; height: 300px;">
<vaadin-virtual-list draggable="true"></vaadin-virtual-list>
</div>
`);
<div style="width: 300px; height: 300px;">
<vaadin-virtual-list draggable="true"></vaadin-virtual-list>
</div>
`);
list = container.querySelector('vaadin-virtual-list');
list.renderer = (root, _, { item }) => {
root.innerHTML = `<div>${item.label}</div>`;
};
document.body.appendChild(container);
await nextFrame();
items = list.shadowRoot.querySelector('#items');
});

async function setVirtualListItems(count) {
Expand All @@ -171,63 +168,31 @@ describe('virtual-list', () => {
await nextFrame();
}

function getState() {
return { itemsMaxHeight: items.style.maxHeight, listOverflow: list.style.overflow };
}

function getExpectedDragStartState() {
return { itemsMaxHeight: '0px', listOverflow: 'hidden' };
}

function assertStatesEqual(state1, state2) {
expect(state1.itemsMaxHeight).to.equal(state2.itemsMaxHeight);
expect(state1.listOverflow).to.equal(state2.listOverflow);
}

async function getStateDuringDragStart(element) {
let stateDuringDragStart;
element.addEventListener(
'dragstart',
() => {
stateDuringDragStart = getState();
},
{ once: true },
);
element.dispatchEvent(new DragEvent('dragstart'));
await new Promise((resolve) => {
requestAnimationFrame(resolve);
});
return stateDuringDragStart;
}

it('should not change state on dragstart for small virtual lists', async () => {
await setVirtualListItems(10);
const initialState = getState();
const stateDuringDragStart = await getStateDuringDragStart(list);
assertStatesEqual(stateDuringDragStart, initialState);
const finalState = getState();
assertStatesEqual(finalState, initialState);
fire(list, 'dragstart');
expect(list.$.items.style.display).to.equal('');
await nextFrame();
expect(list.$.items.style.display).to.equal('');
});

['5000', '50000'].forEach((count) => {
it('should temporarily change state on dragstart for large virtual lists', async () => {
await setVirtualListItems(count);
const initialState = getState();
const stateDuringDragStart = await getStateDuringDragStart(list);
assertStatesEqual(stateDuringDragStart, getExpectedDragStartState());
const finalState = getState();
assertStatesEqual(finalState, initialState);
fire(list, 'dragstart');
expect(list.$.items.style.display).to.equal('none');
await nextFrame();
expect(list.$.items.style.display).to.equal('');
});

it('should temporarily change state on dragstart for large virtual lists in draggable containers', async () => {
list.removeAttribute('draggable');
container.setAttribute('draggable', true);
await setVirtualListItems(count);
const initialState = getState();
const stateDuringDragStart = await getStateDuringDragStart(container);
assertStatesEqual(stateDuringDragStart, getExpectedDragStartState());
const finalState = getState();
assertStatesEqual(finalState, initialState);
fire(container, 'dragstart');
expect(list.$.items.style.display).to.equal('none');
await nextFrame();
expect(list.$.items.style.display).to.equal('');
});
});
});
Expand Down
Loading