Fix issue where _increasePoolIfNeeded could loop indefinitely. Fixes #…

…559 Ensure the _physicalTop is never recalculated to be below the current scroll position, and make sure cached scroll positions are used in methods which may be called outside of a scroll event handler for internal consistency.
PolymerElements · Oct 10, 2019 · bc7cfd7 · bc7cfd7
1 parent 4eed95c
commit bc7cfd7
Show file tree

Hide file tree

Showing 4 changed files with 112 additions and 6 deletions.
diff --git a/.gitignore b/.gitignore
@@ -1 +1,3 @@
 bower_components
+node_modules
+*.d.ts
diff --git a/iron-list.html b/iron-list.html
@@ -846,8 +846,11 @@
         var idxAdjustment = Math.round(delta / this._physicalAverage) * this._itemsPerRow;
         this._virtualStart = this._virtualStart + idxAdjustment;
         this._physicalStart = this._physicalStart + idxAdjustment;
-        // Estimate new physical offset.
-        this._physicalTop = Math.floor(this._virtualStart / this._itemsPerRow) * this._physicalAverage;
+        // Estimate new physical offset based on the virtual start (but never allow
+        // to be more than the current scrollTop)
+        this._physicalTop = Math.min(
+          Math.floor(this._virtualStart / this._itemsPerRow) *
+            this._physicalAverage, this._scrollPosition);
         this._update();
       } else {
         var reusables = this._getReusables(isScrollingDown);
@@ -882,7 +885,8 @@
       var physicalCount = this._physicalCount;
       var top = this._physicalTop + this._scrollOffset;
       var bottom = this._physicalBottom + this._scrollOffset;
-      var scrollTop = this._scrollTop;
+      // This may be called outside of a scrollHandler, so use last cached position
+      var scrollTop = this._scrollPosition;
       var scrollBottom = this._scrollBottom;
 
       if (fromTop) {
@@ -1412,7 +1416,8 @@
       // Note: the delta can be positive or negative.
       if (deltaHeight !== 0) {
         this._physicalTop = this._physicalTop - deltaHeight;
-        var scrollTop = this._scrollTop;
+        // This may be called outside of a scrollHandler, so use last cached position
+        var scrollTop = this._scrollPosition;
         // juking scroll position during interial scrolling on iOS is no bueno
         if (!IOS_TOUCH_SCROLLING && scrollTop > 0) {
           this._resetScrollPosition(scrollTop - deltaHeight);

diff --git a/test/basic.html b/test/basic.html
@@ -19,6 +19,7 @@
 
   <link rel="import" href="helpers.html">
   <link rel="import" href="x-list.html">
+  <link rel="import" href="odd-sized-list.html">
 </head>
 <body>
 
@@ -28,6 +29,12 @@
     </template>
   </test-fixture>
 
+  <test-fixture id="oddSizedList">
+    <template>
+      <odd-sized-list></odd-sized-list>
+    </template>
+  </test-fixture>
+
 <script>
 
   suite('basic features', function() {
@@ -267,7 +274,7 @@
       document.body.style.height = '10000px';
       Polymer.dom.flush();
 
-      window.addEventListener('scroll', function() {
+      var handler = function() {
         setTimeout(function() {
           if (test == 1) {
             assert.isTrue(scrollerSpy.called, 'should call the scroll handler when the list is rendered 1');
@@ -290,15 +297,45 @@
             assert.isTrue(scrollerSpy.called, 'should call the scroll handler when the list is rendered 2');
             document.body.style.height = '';
             window.scrollTo(0, 0);
+            window.removeEventListener('scroll', handler);
             done();
           }
         });
-      });
+      };
+      window.addEventListener('scroll', handler);
       window.scrollTo(0, 1);
       test = 1;
     });
 
   });
+
+  suite('scrolling through heterogeneously-sized list', function() {
+    var list;
+    setup(function() {
+      list = fixture('oddSizedList').$.list;
+      Polymer.dom.flush();
+    });
+    // This test catches a runaway condition where _increasePoolIfNeeded
+    // could recurse indefinitely due to the physical average becoming
+    // large enough (due to heterogeneously-sized items) to position the
+    // physical top lower than the scroll position, causing _isClientFull
+    // to return true; this 
+    test('Issue #559: scroll heterogeneously sized list', function(done) {
+      list.scrollTop = 2300;
+      requestAnimationFrame(function() {
+        list.scrollTop = 0;
+        list.fire('iron-resize');
+        requestAnimationFrame(function() {
+          // In the current implementation, there should be ~29, but
+          // being generous here to allow for changes in the algorithm;
+          // idea being to catch a runaway situation
+          assert.isAtMost(Polymer.dom(list).children.length, 50);
+          done();
+        });
+      });
+    });
+  });
+
 </script>
 
 </body>

diff --git a/test/odd-sized-list.html b/test/odd-sized-list.html
@@ -0,0 +1,62 @@
+<!--
+@license
+Copyright (c) 2015 The Polymer Project Authors. All rights reserved.
+This code may only be used under the BSD style license found at http://polymer.github.io/LICENSE
+The complete set of authors may be found at http://polymer.github.io/AUTHORS
+The complete set of contributors may be found at http://polymer.github.io/CONTRIBUTORS
+Code distributed by Google as part of the polymer project is also
+subject to an additional IP rights grant found at http://polymer.github.io/PATENTS
+-->
+
+<link rel="import" href="../../polymer/polymer.html">
+<link rel="import" href="../iron-list.html">
+
+<dom-module id="odd-sized-list">
+  <template>
+    <style>
+      :host {
+        display: block;
+        width: 300px;
+        height: 300px;
+        border: 1px solid black;
+      }
+      iron-list {
+        height: 100%;
+      }
+      .item {
+        overflow: hidden;
+        border-bottom: 1px solid gray;
+      }
+    </style>
+    <iron-list id="list" items="{{items}}">
+      <template>
+        <div class="item" style="height: [[item]]px;">Item [[index]] ([[item]]px)</div>
+      </template>
+    </iron-list>
+  </template>
+</dom-module>
+
+<script>
+Polymer({
+  is: 'odd-sized-list',
+  properties: {
+    items: {
+      value: function() {
+        // Some hand-tuned sizes to ensure the physical average
+        // after scrolling back up is such to trigger the bug
+        const items = [];
+        while (items.length < 70) {
+          items.push(30);
+        }
+        while (items.length < 90) {
+          items.push(90);
+        }
+        while (items.length < 5000) {
+          items.push(30);
+        }
+        return items;
+      }
+    }
+  }
+});
+</script>