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

vaadin · Dec 18, 2024 · d7ac386 · d7ac386
1 parent fd8d4ef
commit d7ac386
Show file tree

Hide file tree

Showing 4 changed files with 72 additions and 154 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
@@ -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 });
     }
 
@@ -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 = '';
         });
       }
     }

diff --git a/packages/grid/test/drag-and-drop.test.js b/packages/grid/test/drag-and-drop.test.js
@@ -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';
@@ -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('');
       });
     });
   });

diff --git a/packages/virtual-list/src/vaadin-virtual-list.js b/packages/virtual-list/src/vaadin-virtual-list.js
@@ -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 */
@@ -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 });
   }
 
   /**
@@ -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 = '';
       });
     }
   }

diff --git a/packages/virtual-list/test/virtual-list.test.js b/packages/virtual-list/test/virtual-list.test.js
@@ -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', () => {
@@ -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) {
@@ -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('');
       });
     });
   });