fix: prevent infinite loop when measuring virtualizer item height (#5433

)
vaadin · Feb 7, 2023 · dfa8d59 · dfa8d59
1 parent f93fe2e
commit dfa8d59
Show file tree

Hide file tree

Showing 4 changed files with 140 additions and 37 deletions.
diff --git a/packages/component-base/src/iron-list-core.js b/packages/component-base/src/iron-list-core.js
@@ -23,6 +23,7 @@ const DEFAULT_PHYSICAL_COUNT = 3;
  * If something in the scrolling engine needs to be changed
  * for the virtualizer's purposes, override a function
  * in virtualizer-iron-list-adapter.js instead of changing it here.
+ * If a function on this file is no longer needed, the code can be safely deleted.
  *
  * This will allow us to keep the iron-list code here as close to
  * the original as possible.
@@ -560,39 +561,6 @@ export const ironList = {
     return this._virtualStart + (this._physicalCount - this._physicalStart) + pidx;
   },
 
-  /**
-   * Updates the height for a given set of items.
-   *
-   * @param {!Array<number>=} itemSet
-   */
-  _updateMetrics(itemSet) {
-    // Make sure we distributed all the physical items
-    // so we can measure them.
-    flush();
-
-    let newPhysicalSize = 0;
-    let oldPhysicalSize = 0;
-    const prevAvgCount = this._physicalAverageCount;
-    const prevPhysicalAvg = this._physicalAverage;
-
-    // eslint-disable-next-line @typescript-eslint/no-unused-vars
-    this._iterateItems((pidx, vidx) => {
-      oldPhysicalSize += this._physicalSizes[pidx];
-      this._physicalSizes[pidx] = this._physicalItems[pidx].offsetHeight;
-      newPhysicalSize += this._physicalSizes[pidx];
-      this._physicalAverageCount += this._physicalSizes[pidx] ? 1 : 0;
-    }, itemSet);
-
-    this._physicalSize = this._physicalSize + newPhysicalSize - oldPhysicalSize;
-
-    // Update the average if it measured something.
-    if (this._physicalAverageCount !== prevAvgCount) {
-      this._physicalAverage = Math.round(
-        (prevPhysicalAvg * prevAvgCount + newPhysicalSize) / this._physicalAverageCount,
-      );
-    }
-  },
-
   /**
    * Updates the position of the physical items.
    */

diff --git a/packages/component-base/src/virtualizer-iron-list-adapter.js b/packages/component-base/src/virtualizer-iron-list-adapter.js
@@ -141,6 +141,56 @@ export class IronListAdapter {
     });
   }
 
+  /**
+   * Updates the height for a given set of items.
+   *
+   * @param {!Array<number>=} itemSet
+   */
+  _updateMetrics(itemSet) {
+    // Make sure we distributed all the physical items
+    // so we can measure them.
+    flush();
+
+    let newPhysicalSize = 0;
+    let oldPhysicalSize = 0;
+    const prevAvgCount = this._physicalAverageCount;
+    const prevPhysicalAvg = this._physicalAverage;
+
+    // eslint-disable-next-line @typescript-eslint/no-unused-vars
+    this._iterateItems((pidx, vidx) => {
+      oldPhysicalSize += this._physicalSizes[pidx];
+      this._physicalSizes[pidx] = Math.ceil(this.__getBorderBoxHeight(this._physicalItems[pidx]));
+      newPhysicalSize += this._physicalSizes[pidx];
+      this._physicalAverageCount += this._physicalSizes[pidx] ? 1 : 0;
+    }, itemSet);
+
+    this._physicalSize = this._physicalSize + newPhysicalSize - oldPhysicalSize;
+
+    // Update the average if it measured something.
+    if (this._physicalAverageCount !== prevAvgCount) {
+      this._physicalAverage = Math.round(
+        (prevPhysicalAvg * prevAvgCount + newPhysicalSize) / this._physicalAverageCount,
+      );
+    }
+  }
+
+  __getBorderBoxHeight(el) {
+    const style = getComputedStyle(el);
+
+    const itemHeight = parseFloat(style.height) || 0;
+
+    if (style.boxSizing === 'border-box') {
+      return itemHeight;
+    }
+
+    const paddingBottom = parseFloat(style.paddingBottom) || 0;
+    const paddingTop = parseFloat(style.paddingTop) || 0;
+    const borderBottomWidth = parseFloat(style.borderBottomWidth) || 0;
+    const borderTopWidth = parseFloat(style.borderTopWidth) || 0;
+
+    return itemHeight + paddingBottom + paddingTop + borderBottomWidth + borderTopWidth;
+  }
+
   __updateElement(el, index, forceSameIndexUpdates) {
     // Clean up temporary placeholder sizing
     if (el.style.paddingTop) {

diff --git a/packages/component-base/test/virtualizer-item-height.test.js b/packages/component-base/test/virtualizer-item-height.test.js
@@ -96,3 +96,88 @@ describe('virtualizer - item height', () => {
     expect(firstItem.offsetHeight).to.equal(firstItemHeight);
   });
 });
+
+describe('virtualizer - item height - sub-pixel', () => {
+  let elementsContainer;
+  let virtualizer;
+  const ITEM_HEIGHT = 30.25;
+
+  beforeEach(() => {
+    const fixture = fixtureSync(`
+      <div style="height: auto;">
+        <div class="scroller">
+          <div style="min-height: 1px;"></div>
+        </div>
+      </div>
+    `);
+    const scrollTarget = fixture.firstElementChild;
+    const scrollContainer = scrollTarget.firstElementChild;
+    elementsContainer = scrollContainer;
+
+    virtualizer = new Virtualizer({
+      createElements: (count) => Array.from({ length: count }, () => document.createElement('div')),
+      updateElement: (el, index) => {
+        el.style.width = '100%';
+        el.id = `item-${index}`;
+
+        if (el.id !== index) {
+          el.textContent = `item-${index}`;
+          el.style.height = `${ITEM_HEIGHT}px`;
+        }
+      },
+      scrollTarget,
+      scrollContainer,
+    });
+
+    virtualizer.size = 1;
+  });
+
+  it('should take sub-pixel value into account when measuring items height', () => {
+    const containerHeight = elementsContainer.getBoundingClientRect().height;
+    expect(containerHeight).to.equal(Math.ceil(ITEM_HEIGHT));
+  });
+
+  it('should measure height correctly if some transform is applied to virtualizer', () => {
+    elementsContainer.style.transform = 'scale(0.5)';
+
+    virtualizer.scrollToIndex(0);
+
+    let containerHeight = elementsContainer.offsetHeight;
+    expect(containerHeight).to.equal(Math.ceil(ITEM_HEIGHT));
+
+    elementsContainer.style.transform = 'scale(1.5)';
+
+    virtualizer.scrollToIndex(0);
+
+    containerHeight = elementsContainer.offsetHeight;
+    expect(containerHeight).to.equal(Math.ceil(ITEM_HEIGHT));
+  });
+
+  it('should measure item height when box-sizing content-box is used', () => {
+    const firstItem = elementsContainer.querySelector('#item-0');
+
+    firstItem.style.boxSizing = 'content-box';
+    firstItem.style.paddingBottom = '3px';
+    firstItem.style.borderTop = '4px solid red';
+    firstItem.style.borderBottom = '5px solid red';
+
+    virtualizer.scrollToIndex(0);
+
+    const containerHeight = elementsContainer.offsetHeight;
+    expect(containerHeight).to.equal(Math.ceil(ITEM_HEIGHT + 3 + 4 + 5));
+  });
+
+  it('should measure item height when box-sizing border-box is used', () => {
+    const firstItem = elementsContainer.querySelector('#item-0');
+
+    firstItem.style.boxSizing = 'border-box';
+    firstItem.style.paddingBottom = '3px';
+    firstItem.style.borderTop = '4px solid red';
+    firstItem.style.borderBottom = '5px solid red';
+
+    virtualizer.scrollToIndex(0);
+
+    const containerHeight = elementsContainer.offsetHeight;
+    expect(containerHeight).to.equal(Math.ceil(ITEM_HEIGHT));
+  });
+});
diff --git a/packages/component-base/test/virtualizer-unlimited-size.test.js b/packages/component-base/test/virtualizer-unlimited-size.test.js
@@ -179,8 +179,8 @@ describe('unlimited size', () => {
     const item = elementsContainer.querySelector(`#item-${virtualizer.lastVisibleIndex}`);
     const itemRect = item.getBoundingClientRect();
     expect(scrollTarget.getBoundingClientRect().bottom).to.be.within(
-      Math.round(itemRect.top),
-      Math.round(itemRect.bottom),
+      Math.floor(itemRect.top),
+      Math.ceil(itemRect.bottom),
     );
   });
 
@@ -198,8 +198,8 @@ describe('unlimited size', () => {
     const item = elementsContainer.querySelector(`#item-${virtualizer.lastVisibleIndex}`);
     const itemRect = item.getBoundingClientRect();
     expect(scrollTarget.getBoundingClientRect().bottom).to.be.within(
-      Math.round(itemRect.top),
-      Math.round(itemRect.bottom),
+      Math.floor(itemRect.top),
+      Math.ceil(itemRect.bottom),
     );
   });
 });