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.

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

Merged
merged 8 commits into from
Dec 17, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -139,10 +139,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 @@ -312,25 +308,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
10 changes: 1 addition & 9 deletions packages/grid/test/drag-and-drop.common.js
Original file line number Diff line number Diff line change
Expand Up @@ -1088,8 +1088,6 @@ describe('drag and drop', () => {

describe('draggable grid', () => {
let container;
let items;
let table;

beforeEach(async () => {
container = fixtureSync(`
Expand All @@ -1103,8 +1101,6 @@ describe('drag and drop', () => {
document.body.appendChild(container);
flushGrid(grid);
await nextFrame();
items = grid.shadowRoot.querySelector('#items');
table = grid.shadowRoot.querySelector('#table');
});

async function setGridItems(count) {
Expand All @@ -1121,12 +1117,8 @@ describe('drag and drop', () => {
}

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);
expect(grid.$.scroller.style.display).to.equal('');
}

['5000', '50000'].forEach((count) => {
Expand Down
45 changes: 20 additions & 25 deletions packages/virtual-list/src/vaadin-virtual-list-mixin.js
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,7 @@ export const VirtualListMixin = (superClass) =>

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

/** @protected */
Expand All @@ -98,17 +98,13 @@ export const VirtualListMixin = (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.__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 @@ -178,25 +174,24 @@ 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';
/**
* 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
8 changes: 1 addition & 7 deletions packages/virtual-list/test/drag-and-drop.common.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@ import { hover } from '@vaadin/button/test/visual/helpers.js';
describe('drag and drop', () => {
let virtualList;
let container;
let items;

beforeEach(async () => {
container = fixtureSync(`
Expand All @@ -20,7 +19,6 @@ describe('drag and drop', () => {
};
document.body.appendChild(container);
await nextFrame();
items = virtualList.shadowRoot.querySelector('#items');
});

async function setVirtualListItems(count) {
Expand All @@ -39,12 +37,8 @@ describe('drag and drop', () => {
}

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);
expect(virtualList.$.items.style.display).to.equal('');
}

['5000', '50000'].forEach((count) => {
Expand Down
Loading