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

vaadin · Dec 17, 2024 · e9fc991 · e9fc991
1 parent 66f6016
commit e9fc991
Show file tree

Hide file tree

Showing 4 changed files with 38 additions and 62 deletions.
diff --git a/packages/grid/src/vaadin-grid-drag-and-drop-mixin.js b/packages/grid/src/vaadin-grid-drag-and-drop-mixin.js
@@ -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 });
     }
 
@@ -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 = '';
         });
       }
     }

diff --git a/packages/grid/test/drag-and-drop.common.js b/packages/grid/test/drag-and-drop.common.js
@@ -1088,8 +1088,6 @@ describe('drag and drop', () => {
 
   describe('draggable grid', () => {
     let container;
-    let items;
-    let table;
 
     beforeEach(async () => {
       container = fixtureSync(`
@@ -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) {
@@ -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) => {

diff --git a/packages/virtual-list/src/vaadin-virtual-list-mixin.js b/packages/virtual-list/src/vaadin-virtual-list-mixin.js
@@ -65,7 +65,7 @@ export const VirtualListMixin = (superClass) =>
 
     constructor() {
       super();
-      this.__onDragStart = this.__onDragStart.bind(this);
+      this.__onDocumentDragStart = this.__onDocumentDragStart.bind(this);
     }
 
     /** @protected */
@@ -89,17 +89,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 });
     }
 
     /**
@@ -153,25 +149,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 = '';
         });
       }
     }

diff --git a/packages/virtual-list/test/drag-and-drop.common.js b/packages/virtual-list/test/drag-and-drop.common.js
@@ -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(`
@@ -20,7 +19,6 @@ describe('drag and drop', () => {
     };
     document.body.appendChild(container);
     await nextFrame();
-    items = virtualList.shadowRoot.querySelector('#items');
   });
 
   async function setVirtualListItems(count) {
@@ -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) => {