search

aurelia / ui-virtualization

A plugin that provides a virtualized repeater and other virtualization services.
MIT License
90 stars 45 forks source link

Table rows invisible when scrolling a table anchored part way down a document #178

Open SabineWren opened 4 years ago

SabineWren commented 4 years ago

I'm submitting a bug report

Please tell us about your environment:

Use Case I put virtual-repeat.for on a tr element inside a table. The table is inside a scroll container which is inside a custom element using Shadow DOM.

<custom-element>//attached somewhere down the page
  <shadow-root>
    <controls></controls>//other elements in here
    <scroll-container>//has overflow: auto; and a fixed height
      <table>
        <tbody>
          <tr virtual-repeat.for="row of rows">
            <td><div>nested stuff here</div></td>
          </tr>
        </tbody>
      </table>
    </scroll-container>
  <shadow-root>
</custom-element>

Current behavior: Everything works fine with the element anchored to the top of the document. However, scrolling breaks when anchoring the element part way down the page. The farther down the document a table is anchored, and the more a user scrolls down the table, the more rows are hidden and replaced with blank space. Increasing the height of the scroll container proportionally to the anchor distance from the top of the document mitigates the problem.

I tried using fixed row heights with no change in behaviour. Zooming in/out changes the number of rows that become invisible.

Expected/desired behavior: Rows should scroll into view, even when the table isn't anchored to the top of the page.

Fix I got the plugin working perfectly for my use case by cloning the ui-virtualization plugin and editing the function getViewRange in file array-virtual-repeat-strategy.ts. const topBufferDistance = 0;//getDistanceToParent(topBufferEl, scrollerEl); You can see the helper function that I zeroed out. That function's probably important for something, but replacing it with a zero seems to fix the bug.

SabineWren commented 4 years ago

Same table, but anchored in two different places. Nine rows are invisible in the lower one.

virtual_row_bug

bigopon commented 4 years ago

@SabineWren thanks. At the moment, slotting with shadow DOM is not well supported, in case there's something that will affect the container distance to the first repeated item. For your case, forcefully changing it to 0 will probably cause issue in other usages where it's not at the top. I'll come up with some fix for this. Can you help with a repro?

SabineWren commented 4 years ago

Yes, but I don't have time this week.

SabineWren commented 4 years ago

Gist errored when I tried creating creating a fork, and I couldn't get CodeSandbox to work even with a basic custom element. So, you get a zip. I'm including a page and its custom elements.

It's configured for typescript and webpack. You'll need to configure the route in a skeleton app. { route: ["bug-repro"], name: "bug repro", moduleId: PLATFORM.moduleName("virt-bug/page-bug-repro") }, That assumes a folder named virt-bug and navigating with <local_site>#bug-repro

virt-bug.zip

bigopon commented 4 years ago

There's probably a way to make it work seamlessly with shadowDOM mode open. closed mode is probably not, as it's a complete blackbox, from the lib perspective. So I guess we can add a way for the repeat to find a nearest user-specified calculation, maybe something like this:

<div>
  <div virtual-repeat.for="item of items" calc-top-distance.bind="calcTopDistance">...</div>
</div>

And the method can have following signature:

function calcTopDistance(topBuffer: HTMLElement, parent: HTMLElement, repeat: VirtualRepeat); number;

I'm not 100% sure about this yet, thoughts?

SabineWren commented 4 years ago

Yeah, closed is probably a no-go. However, I don't see why there would be a difference to the library between the normal (light) dom and (open) shadow doms except for the root. Replace the hard-coded document.root with virtualizableNode.getRootNode(), and everything else should be exactly the same.

Soon after opening this issue I cloned this repo into our own codebase and fixed it. I couldn't read much of the code, so I deleted everything that wasn't related to tables, and I don't remember everything I changed or why. If you want I can code dump the whole thing, but it's not mergeable with how much I deleted. The most important part was probably the getViewRange function in array-virtual-repeat-strategy.ts

getViewRange(rowHeight: number, rows: HTMLTableRowElement[], minRowsRequired: number, scrollerInfo: IScrollerInfo): [number, number] {
    if(rows.length === 0) { return [0, 0] }
    if(rowHeight <= 0) { return [0, 0] }
    if(minRowsRequired > rows.length) { return [0, rows.length - 1] }

    const firstVisibleIndex = Math.floor(scrollerInfo.scrollTop / rowHeight)
    //yes, the last index is indeed equal to the row length. I don't know why
    const lastVisibleIndex = Math.min(minRowsRequired + firstVisibleIndex, rows.length)
    return [lastVisibleIndex - minRowsRequired, lastVisibleIndex]
}

Maybe the requirements differ for virtualized elements that aren't table rows, but I don't think it matters where the 'top' element is. The library only needs to know how many rows it can show at once, and how many rows it has already scrolled. It shouldn't care where on the page, so no top distance needed. This is true even if the max-height container has other elements besides the table, such as a title or controls. The edge case is when you have less rows in the table than can fit in the max-height.

I deleted the 500ms polling, in favour of subscribing to events that change table visibility or size. That triggers a render on the next event loop and re-measures row height. In our case I subcribed to events from the plugin, but as a standalone library it would just need to expose a "re-render" function and let the caller handle event notifications. I also appended 'overlay' to the scrollable element detection ('auto', 'scroll').

bigopon commented 4 years ago

Aurelia in general, and the virtural repeat specifically, aim to provide a seamless dev experience. For the virtual repeat, this means (1) natural syntax, (2) no matter how you use it, it display things correctly, with little to no configuration. That's why there's some extra code for calculating so many variables.

The main required computation are:

but I don't think it matters where the 'top' element is.

If you have some thing like the following: image

The black box represents your scroll container, the red box represents the static content before your virtual repeat. Shouldn't the virtual repeat know how much space it has to render items, or not render items at all, as the point of the repeat is to not render anything if not necessary. The contrived example for the above image would be like this:

<div style="height: 300px;">
  <table>
    <thead>
      <tr style="height:400px" >...</tr>
    </thead>
    <tr virtual-repeat="item of items"></tr>
  </table>
</div>

And multiple virtual repeats on the same container is supported, so you could even have:

<div style="height: 300px;">
  <table>
    <thead>
      <tr style="height:400px" >...</tr>
    </thead>
    <tr virtual-repeat="item of list1"></tr>
    <tr virtual-repeat="item of list2"></tr>
    <tr virtual-repeat="item of list3"></tr>
  </table>
</div>

Without calculating properly, it would easily give the wrong impression that there's some items of list1, list2, or list3 in the viewport, while there's actually none.

bigopon commented 4 years ago

To continue our discussion, let's spec out what scenarios should work. The implementation should support the following table:

# scroller slotting virtual repeat position
1 inside shadow DOM single level root level of content
2 inside shadow DOM single level nested inside other element of content
3 inside shadow DOM pass through root level of content
4 inside shadow DOM pass through nested inside other element of content
5 inside deep shadow single level root level of content
6 inside deep shadow single level nested inside other element of content
7 (this issue scenario) inside shadow single level same shadow root + inside scroller
8 to be updated
1
  • Single level slotting behavior
  • virtual repeat at content root level
  • list scroller inside shadow DOM
```html list-container: Usage:
${item.name}
 ```
2
  • Single level slotting behavior
  • virtual repeat nested by other elements
  • a list with scroller inside shadow DOM
```html list-container: Usage: 1.
${item.name}
 2.
${item.name}
 3.
${item.name}
4.
  • ${item.name}
```
3
  • Pass through slotting behavior
  • virtual repeat at content root level
  • list scroller inside shadow DOM
```html list-container: panel: Usage:
${item.name}
 ```
.... more examples to be updated
SabineWren commented 4 years ago

It seems you're trying to squeeze every bit of performance by calculating exactly how many rows need to be rendered. You've discovered lots of edge cases, since it's a hard problem. However, I suspect you're over-complicating things for minimal performance gain.

Let's take your example of a bunch of static content (header rows, etc.) followed by 3 virtualized lists of table rows in a single container. We render the static content even if it's scrolled out of view. I never needed to use more than one virtual repeat per table, but it doesn't change the problem complexity.

Let's consider the code I showed above. For each virtual list, we calculate the most rows potentially visible by taking the size of the container divided by the row height. That's usually 30-50 rows per virtual list for a typical table. Scrolled to the very top with 3 virtualized lists, that might render a hundred or so extra rows in overflow, because it fills the container once for each virtual list.

Yes it's a bit wasteful, but the whole point of virtualizing a table is to scale to thousands of rows with the performance of <100 rendered in the DOM. It's not a big deal if a few extras rows are rendered in overflow, as it still clamps the performance hit. If you have a small number of rows, then there's no reason to virtualize in the first place.

With multiple virtual lists, scroll position becomes harder. You could register each virtual-repeat to an internal list of repeats per container, allowing them to communicate. Maybe expose total virtual height of each repeater. I don't know if it's worth the complexity of implementing support for multiple repeaters.

With my approach, Shadow/Light DOM is irrelevant, the algorithm is extremely simple to implement, there are no edge cases, and the performance is acceptable. I haven't encountered any issues since rolling it out. However, I only implemented it for TR elements.

bigopon commented 4 years ago

@SabineWren If I understood your comment right, I should say it's less about performance, but more about correctness. This scenario:

<div class="scroll-container" style="height: 500px;">
  <h1 style="height: 50px">Static content</h1>
  ... more static content 50px
  ... more static content 50px
  <div virtual-repeat.for="item of items"></div>
  ... some static content for sumarizing
</div>

Whenever there's a scroll event in scroll container, it's incorrect to just show based on current scroll top of the scroll container. Assume that each item height is 50px:

If the scroll top is 200px:

My previous comment about multiple repeater is a bit off the point, as it's just an extreme case of this example.

SabineWren commented 4 years ago

Then if you have a single virtual-repeater, would the exact start be (scrolltop) - (height of previous elements), and the total virtual height be (height of previous elements) + (rowHeight * numRows) + (height of subsequent elements)? I'd have to look at the code I used for scroll position to see if it's similar. The original bug I reported was somehow the anchor position of the container's host element affected things, which makes no sense to me.

bigopon commented 4 years ago

@SabineWren yes it is. Except when your repeat is not on the same level with previous elements:

<div class="scroll-container" style="height: 500px;">
  <h1 style="height: 50px">Static content</h1>
  ... more static content 50px
  ... more static content 50px
  <div>
    ... more static content 50px
    <div virtual-repeat.for="item of items"></div>
    ... some static content for sumarizing
  </div>
</div>

This scenario is just an exaggeration of table:

<table>
  <thead>
    <tr>...content of this header row takes some px, and is not on the same level with the repeated row
  </thead>
  <tbody>
    <tr virtual-repeat.for="item of items">
  </tbody>
</table>