Fix virtual list on main (take 3) (#5899)

rewrite to tsx
rewrite to hook
fix broken virtualization (it didn't work)
removed focus element retention (cannot get it to work)
fixes #4679
closes #5307
see also
https://github.com/transloadit/uppy/pull/5255#issuecomment-2205924942

closes #5890

.changeset/pretty-zebras-chew.md

@@ -0,0 +1,5 @@
+---
+"@uppy/provider-views": patch
+---
+
+VirtualList now virtualises rows in the file list, as was intented. This means better performance when scrolling thousands for files.

packages/@uppy/provider-views/src/Browser.tsx

@@ -76,6 +76,7 @@ function Browser<M extends Meta, B extends Body>(props: BrowserProps<M, B>) {
 
   const renderItem = (item: PartialTreeFile | PartialTreeFolderNode) => (
     <Item
+      key={item.id}
       viewType={viewType}
       toggleCheckbox={(event: Event) => {
         event.stopPropagation()
@@ -93,16 +94,16 @@ function Browser<M extends Meta, B extends Body>(props: BrowserProps<M, B>) {
     />
   )
 
+  // todo remove virtuallist option and always use virtual list
   if (virtualList) {
     return (
       <div className="uppy-ProviderBrowser-body">
-        <ul className="uppy-ProviderBrowser-list">
+        <VirtualList
-          <VirtualList
+          className="uppy-ProviderBrowser-list"
-            data={displayedPartialTree}
+          data={displayedPartialTree}
-            renderRow={renderItem}
+          renderRow={renderItem}
-            rowHeight={31}
+          rowHeight={35.5}
-          />
+        />
-        </ul>
       </div>
     )
   }

packages/@uppy/utils/src/VirtualList.jsx

@@ -1,152 +0,0 @@
-/**
- * Adapted from preact-virtual-list: https://github.com/developit/preact-virtual-list
- *
- * © 2016 Jason Miller
- *
- * Permission is hereby granted, free of charge, to any person obtaining a copy
- * of this software and associated documentation files (the "Software"), to deal
- * in the Software without restriction, including without limitation the rights
- * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
- * copies of the Software, and to permit persons to whom the Software is
- * furnished to do so, subject to the following conditions:
- *
- * The above copyright notice and this permission notice shall be included in all
- * copies or substantial portions of the Software.
- *
- * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
- * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
- * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
- * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
- * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
- * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
- * SOFTWARE.
- *
- * Adaptations:
- * - Added role=presentation to helper elements
- * - Tweaked styles for Uppy's Dashboard use case
- */
-
-import { Component, h } from 'preact'
-
-const STYLE_INNER = {
-  position: 'relative',
-  // Disabled for our use case: the wrapper elements around FileList already deal with overflow,
-  // and this additional property would hide things that we want to show.
-  //
-  // overflow: 'hidden',
-  width: '100%',
-  minHeight: '100%',
-}
-
-const STYLE_CONTENT = {
-  position: 'absolute',
-  top: 0,
-  left: 0,
-  // Because the `top` value gets set to some offset, this `height` being 100% would make the scrollbar
-  // stretch far beyond the content. For our use case, the content div actually can get its height from
-  // the elements inside it, so we don't need to specify a `height` property at all.
-  //
-  // height: '100%',
-  width: '100%',
-  overflow: 'visible',
-}
-
-class VirtualList extends Component {
-  constructor(props) {
-    super(props)
-
-    // The currently focused node, used to retain focus when the visible rows change.
-    // To avoid update loops, this should not cause state updates, so it's kept as a plain property.
-    this.focusElement = null
-
-    this.state = {
-      offset: 0,
-      height: 0,
-    }
-  }
-
-  componentDidMount() {
-    this.resize()
-    window.addEventListener('resize', this.handleResize)
-  }
-
-  // TODO: refactor to stable lifecycle method
-  componentWillUpdate() {
-    if (this.base.contains(document.activeElement)) {
-      this.focusElement = document.activeElement
-    }
-  }
-
-  componentDidUpdate() {
-    // Maintain focus when rows are added and removed.
-    if (
-      this.focusElement?.parentNode &&
-      document.activeElement !== this.focusElement
-    ) {
-      this.focusElement.focus()
-    }
-    this.focusElement = null
-    this.resize()
-  }
-
-  componentWillUnmount() {
-    window.removeEventListener('resize', this.handleResize)
-  }
-
-  handleScroll = () => {
-    this.setState({ offset: this.base.scrollTop })
-  }
-
-  handleResize = () => {
-    this.resize()
-  }
-
-  resize() {
-    const { height } = this.state
-
-    if (height !== this.base.offsetHeight) {
-      this.setState({
-        height: this.base.offsetHeight,
-      })
-    }
-  }
-
-  render({ data, rowHeight, renderRow, overscanCount = 10, ...props }) {
-    const { offset, height } = this.state
-    // first visible row index
-    let start = Math.floor(offset / rowHeight)
-
-    // actual number of visible rows (without overscan)
-    let visibleRowCount = Math.floor(height / rowHeight)
-
-    // Overscan: render blocks of rows modulo an overscan row count
-    // This dramatically reduces DOM writes during scrolling
-    if (overscanCount) {
-      start = Math.max(0, start - (start % overscanCount))
-      visibleRowCount += overscanCount
-    }
-
-    // last visible + overscan row index + padding to allow keyboard focus to travel past the visible area
-    const end = start + visibleRowCount + 4
-
-    // data slice currently in viewport plus overscan items
-    const selection = data.slice(start, end)
-
-    const styleInner = { ...STYLE_INNER, height: data.length * rowHeight }
-    const styleContent = { ...STYLE_CONTENT, top: start * rowHeight }
-
-    // The `role="presentation"` attributes ensure that these wrapper elements are not treated as list
-    // items by accessibility and outline tools.
-    return (
-      <div onScroll={this.handleScroll} {...props}>
-        <div role="presentation" style={styleInner}>
-          <div role="presentation" style={styleContent}>
-            {selection.map(renderRow)}
-          </div>
-        </div>
-      </div>
-    )
-  }
-}
-
-export default VirtualList

packages/@uppy/utils/src/VirtualList.tsx

@@ -0,0 +1,128 @@
+/**
+ * Adapted from preact-virtual-list: https://github.com/developit/preact-virtual-list
+ *
+ * © 2016 Jason Miller
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in all
+ * copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
+ * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
+ * SOFTWARE.
+ *
+ * Adaptations:
+ * - Added role=presentation to helper elements
+ * - Tweaked styles for Uppy's Dashboard use case
+ */
+
+import type { HTMLAttributes, ReactNode } from 'preact/compat'
+import { useCallback, useEffect, useRef, useState } from 'preact/hooks'
+
+const STYLE_INNER = {
+  position: 'relative',
+  // Disabled for our use case: the wrapper elements around FileList already deal with overflow,
+  // and this additional property would hide things that we want to show.
+  //
+  // overflow: 'hidden',
+  width: '100%',
+  minHeight: '100%',
+}
+
+const STYLE_CONTENT = {
+  position: 'absolute',
+  top: 0,
+  left: 0,
+  // Because the `top` value gets set to some offset, this `height` being 100% would make the scrollbar
+  // stretch far beyond the content. For our use case, the content div actually can get its height from
+  // the elements inside it, so we don't need to specify a `height` property at all.
+  //
+  // height: '100%',
+  width: '100%',
+  overflow: 'visible',
+}
+
+export default function VirtualList<T>({
+  data,
+  rowHeight,
+  renderRow,
+  overscanCount = 10,
+  padding = 4,
+  ...props
+}: {
+  data: T[]
+  rowHeight: number
+  padding?: number // last visible + overscan row index + padding to allow keyboard focus to travel past the visible area
+  renderRow: (item: T) => ReactNode
+  overscanCount?: number
+} & HTMLAttributes<HTMLDivElement>) {
+  const scrollerRef = useRef<HTMLDivElement>(null)
+
+  const [offset, setOffset] = useState(0)
+  const [height, setHeight] = useState(0)
+
+  useEffect(() => {
+    function resize() {
+      if (
+        scrollerRef.current != null &&
+        height !== scrollerRef.current.offsetHeight
+      ) {
+        setHeight(scrollerRef.current.offsetHeight)
+      }
+    }
+
+    resize()
+
+    window.addEventListener('resize', resize)
+    return () => {
+      window.removeEventListener('resize', resize)
+    }
+  }, [height])
+
+  const handleScroll = useCallback(() => {
+    if (scrollerRef.current) setOffset(scrollerRef.current.scrollTop)
+  }, [])
+
+  // first visible row index
+  let start = Math.floor(offset / rowHeight)
+
+  // actual number of visible rows (without overscan)
+  let visibleRowCount = Math.floor(height / rowHeight)
+
+  // Overscan: render blocks of rows modulo an overscan row count
+  // This dramatically reduces DOM writes during scrolling
+  if (overscanCount) {
+    start = Math.max(0, start - (start % overscanCount))
+    visibleRowCount += overscanCount
+  }
+
+  const end = start + visibleRowCount + padding
+
+  // data slice currently in viewport plus overscan items
+  const selection = data.slice(start, end)
+
+  const styleInner = { ...STYLE_INNER, height: data.length * rowHeight }
+  const styleContent = { ...STYLE_CONTENT, top: start * rowHeight }
+
+  // The `role="presentation"` attributes ensure that these wrapper elements are not treated as list
+  // items by accessibility and outline tools.
+  return (
+    <div onScroll={handleScroll} ref={scrollerRef} {...props}>
+      <div role="presentation" style={styleInner}>
+        <div role="presentation" style={styleContent}>
+          {selection.map(renderRow)}
+        </div>
+      </div>
+    </div>
+  )
+}

packages/@uppy/utils/tsconfig.json

@@ -5,6 +5,6 @@
     "emitDeclarationOnly": false,
     "noEmit": true
   },
-  "include": ["src/*.ts"],
+  "include": ["src/*.ts", "src/*.tsx"],
   "references": []
 }