Jdrush89/tree content visibility #2640
Conversation
🦋 Changeset detectedLatest commit: ad5431b The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
| <PropsTableRow | ||
| name="containIntrinsicSize" | ||
| type="string" | ||
| description="The size of this item's contents. Passing this will set 'content-visiblity: auto' on the content container, delaying rendering until the item is in the viewport." |
There was a problem hiding this comment.
Is there a downside to always setting content-visibility: auto? Since the consumer doesn't control the height of TreeView.Item, it feels strange to ask the consumer to pass a height value here.
There was a problem hiding this comment.
The consumer does control the height by passing in whatever contents they want. Only the min-height is set. If content-visibility is always set and they've made the height larger through the contents, then the scrolling will be choppy.
jdrush89
force-pushed
the
jdrush89/tree-contentVisibility
branch
from
347a209
to
e08c34d
Compare
There was a problem hiding this comment.
Just left a couple of comments! In general, the changes to typeahead seem great 🔥 (thanks for doing that)
The content visibility stuff I feel less sure about, given some of the stuff you said that are coming up to help reduce render time (and lack of support in FF/Safari) do you think this is something that needs to be introduced into the component or is a nice to have?
|
@joshblack Thanks for taking a look! I think the content-visibility is worth using for the file tree since it's easy to add and increases perf for large trees for most users. For FF/Safari, it is just a no-op until they add support for it. If you didn't want to add it to the TreeView component in primer, we could just add css rules to the 'PRIVATE_TreeView-item-container' class in dotcom, assuming that classname doesn't change. |
I tried implementing tree virtualization in a number of ways. First, I tried using intersection observers on the subtree contents and paging in rows as they approached the viewport. The problem with this approach is when the user scrolls quickly, they may end up paging in a large subdirectory which then takes time to render. The pop-in while scrolling was a degraded experience compared to the non-virtualized tree. The complexity added from trying to maintain typeahead and other keyboard shortcuts, and keeping the focused and current rows materialized was also extremely high and likely to be prone to bugs.
I also tried virtualizing each row's contents with intersection observers. The pop-in was much less severs, though the initial render took much longer because each row had to handle and intersection event to figure out whether to show or hide. I settled on using the
content-visiblity: autoproperty, which delays the rendering of each row without removing itfrom the DOM. Each row can pass in a containIntrinsicSize property to specify it's estimated height , then the content-visibilty property will be added as well. Doing this is very simple and won't affect accessibility in any way. In testing with large trees, the initial render time is 20% faster and any lagginess in updating hover styles seems to go away. The drawback is that this property is not supported in FF or Safari yet, though it should be aded at some point.
The main problems virtualization was trying to solve was around the initial render time, hover styles, and typeahead time. I realized the typeahead implementation was causing multiple full rerenders of the tree by keeping the current search term in state. By moving it to a ref instead, the typeahead time sped up drastically. content-visiblity helps the initial render time some. In the file tree on dotcom, we make sure the tree renders after the hpc, so its initial render time isn't extremely critical, we also only render the first 100 items of each directory in the first pass, then do the rest afterwards. This greatly speeds up the initial render and essentially accomplishes the gains virtualizing each subdirectory's contents would make. Once SSR is in place, the initial render should be sped up drastically as well.
Merge checklist