feat(cdk/table): virtual scroll directive for tables #21708

MichaelJamesParsons · 2021-01-26T22:27:42Z

Adds a virtualRows directive which enables virtual scrolling for CDK tables.

Open questions

Blocks of code that are subject to further discussion or contain potentially breaking changes are annotated with FIXME.

Out of scope for now

Variable size rows (virtual scroll autosize directive)
Material tables support (let's hash out the CDK variant first)
Checkbox state
Row animations

Known issues

Sticky headers/footers flicker if an animation frame exceeds ~35ms. Karl and I will investigate further.

Performance considerations

We've already improved row rendering by 50% or better. Without headers/footers, the scrolling experience is comparable to most other Angular material virtual scroll demos.

However, the entire table shifts position as the user scrolls. The position of sticky headers and footers must be offset by the distance scrolled to keep them in place. On average, the table is able to keep 15-20ms per animation frame. Frames that occasionally exceed ~35ms cause sticky headers/footers to flicker. It looks like longer than usual style calculations are hindering performance the most.

While I believe we can further optimize rendering, we will likely reach a threshold where the table can no longer be optimized and non-trivial tables will still experience flickering (e.g. tables with inline editing, cells with custom components, etc.). A while back, I experimented with rendering tables in multiple pieces so the body content can scroll independently of header/footers (#20414). There are some a11y challenges to this solution, but it would resolve the flickering.

For example:

<table><!-- table header --></table>
<div class="scroll-viewport">
  <table>
    <thead><!-- visually hidden header --></thead>
    <tbody>...</tbody>
    <tfoot><!--visually hidden footer --></tfoot>
</div>
<table><!-- table footer --></table>

Related PRs

feat(cdk/table): add HTML elements to sticky styler state #21704

src/cdk-experimental/table/virtual-rows.ts

src/cdk/collections/recycle-view-repeater-strategy.ts

src/cdk/table/table.scss

src/cdk/table/table.ts

...ts-examples/cdk-experimental/table/cdk-virtual-flex-table/cdk-virtual-flex-table-example.css

src/cdk-experimental/table/virtual-rows.ts

crisbeto · 2021-01-27T19:43:49Z

Since the description mentions an issue with flickering, could it be the same problem as #21576?

MichaelJamesParsons · 2021-01-27T20:23:15Z

Since the description mentions an issue with flickering, could it be the same problem as #21576?

Yes, though the flickering I'm seeing is not nearly as significant as observed in #21576. Likely due to internal performance optimizations that are present in this PR, but not yet accessible through the public API (e.g. recycle view repeater and enhancements made in #21704).

src/cdk-experimental/table/virtual-rows.ts

src/cdk/table/table.ts

jelbourn · 2021-01-27T21:04:17Z

src/cdk/table/table.ts

+    // FIXME DO NOT SUBMIT: Is the following behavior acceptable?
+    //
+    // The table will override the StickyPositioningListener provider to `null` to prevent child
+    // tables from inheriting it. However, if a directive on the table configures a new provider, it
+    // should be used instead. Therefore, when a directive on the table configures a positioning
+    // listener provider, the provider will be inherited by child tables.
+    this._stickyPositioningListener = this._positioningListener ?? this._parentPositioningListener;


To clarify that I understand this correctly: the goal here is to support multiple different directives adding their own sticky position listener? If that's the case, we should probably refactor this in one of two ways:

Have one "notifier" that all those directives subscribe to

Change this to a multi provider and let multiple directives register listeners

I would definitely lean towards the first option here as being more in line with the rest of the codebase

The table currently retrieves the StickyPositioningListener provider from the parent component and overrides the provider to null to prevent child tables from inheriting it.

<parent-with-listener> <table></table> </parent-with-listener>

However, the virtual scroll directive is bound directly on the table, so the table must get the provider from @Self() when present.

<table virtualScroll></table>

src/cdk-experimental/table/table-virtual-scroll.ts

jelbourn · 2021-02-17T20:23:51Z

src/cdk/table/table.ts

+ * Collection viewer for the `CdkTable`. For backwards compatibility, the collection viewer must
+ * be a `BehaviorSubject`.


We can potentially change this; I assume that this isn't going to be widely used inside Google since viewChange has just been a placeholder

Internal targets could be quickly migrated. What about external apps?

For a behavior change like this, I think this is something we could change for a major version (which we're currently in the window for) so long as we document it.

@jelbourn Following up since it's been a while. Is now a still a good time to make this change?

What's the benefit of changing it?

The behavior subject emits a range the spans the entire data set when the table is initialized. This causes the table to briefly render all rows, then truncate them once it attaches to the virtual scroll viewport. By changing this to a regular Subject, we'd have more control over when the range is emitted.

For now, I've hacked around it by overriding the behavior subject via provider. Then emitting an empty range as the initial value. This prevents rendering the rows before the virtual scroll viewport is attached.

We'd be able to change it starting in August

Revisiting this, could you expand the comment to touch on why this interface is necessary? (IIRC it's so that the table can inject the viewChange behavior from another directive instead of completely implementing it itself).

Actually typing that out, it might make sense to rename this to something named like ViewChangeXxx instead of CollectionViewer since CollectionViewer could potentially be expanded to include more stuff, but that's not super important with the features it has today.

...ts-examples/cdk-experimental/table/cdk-virtual-flex-table/cdk-virtual-flex-table-example.css

src/dev-app/index.html

src/cdk/table/table.ts

kseamon

Looks good!

src/cdk-experimental/table/table-virtual-scroll.ts

kseamon · 2021-06-17T14:20:28Z

src/cdk/table/table.ts

+ * Collection viewer for the `CdkTable`. For backwards compatibility, the collection viewer must
+ * be a `BehaviorSubject`.


What's the benefit of changing it?

MichaelJamesParsons · 2021-06-30T06:10:03Z

@jelbourn The view_engine_build check fails with the following error, but passes locally.

ERROR: /home/circleci/ng/src/dev-app/table/BUILD.bazel:5:10: Compiling Angular templates (ViewEngine - devmode) //src/dev-app/table:table failed
RangeError: Maximum call stack size exceeded

Local command:

 bazel build --build_tag_filters=-docs-package,-release-package --config=view-engine -- src/...

Am I missing something?

src/cdk-experimental/table/table-virtual-scroll.ts

jelbourn · 2021-06-30T19:01:47Z

src/cdk-experimental/table/table-virtual-scroll.ts

+    this._footerRowStickyUpdates.next(update);
+  }
+
+  private _stickHeaderRows(offsetFromTop: number, update: StickyUpdate) {


Could you add some explanation here that touches on the need to apply these extra styles on top of/instead of what's happening with StickyStyler? It might be because it's been long enough that I forgot how sticky positioning works today, but I feel like I'm missing context on why virtual scroll needs the extra styling logic.

On a related note, #21576 (comment) mentioned that the flickering observed while scrolling quickly is potentially caused by the browser applying the viewport's transform after top. So the offsets occasionally get out of sync for 1 frame.

I took some time to experiment with this a bit more today and refactored the code to keep top as-is, and use transformY to shift the table cells into their correct positions. The end result was much worse than just updating top directly.

Moving the headers/footers outside the scroll viewport entirely seems to be the only reliable solution to the flickering. In the future, it would be worth experimenting with an accessible flex table that scrolls only the body content.

jelbourn · 2021-06-30T19:06:10Z

src/cdk/table/table.scss

@@ -1,3 +1,17 @@
 .cdk-table-fixed-layout {
  table-layout: fixed;
 }
+
+.cdk-table-virtual-scroll {
+  border-collapse: collapse;


Add a comment for why border-collapse is important here?

Tables with multiple headers/footers that have border-spacing will initially render with spacing between cells. However, the spacing collapses after scrolling. I'm able to reproduce this with non-virtualized CDK tables, so I believe this is an issue with the stickystyler.

I added this style so border-spacing wouldn't collapse on scroll. It can probably be removed since it's an existing behavior with the sticky styler.

jelbourn · 2021-06-30T19:11:12Z

src/cdk/table/table.ts

+ * Collection viewer for the `CdkTable`. For backwards compatibility, the collection viewer must
+ * be a `BehaviorSubject`.


We'd be able to change it starting in August

Revisiting this, could you expand the comment to touch on why this interface is necessary? (IIRC it's so that the table can inject the viewChange behavior from another directive instead of completely implementing it itself).

Actually typing that out, it might make sense to rename this to something named like ViewChangeXxx instead of CollectionViewer since CollectionViewer could potentially be expanded to include more stuff, but that's not super important with the features it has today.

MichaelJamesParsons · 2021-08-10T20:09:41Z

I resolved the view engine failure described in #21708 (comment) by adding a missing build target to the dev-app/BUILD.bazel file. I'm surprised the dev-app was able to build at all without it. Strange.

@jelbourn Ready for another review.

src/cdk-experimental/table/table-virtual-scroll.ts

src/cdk/collections/recycle-view-repeater-strategy.ts

src/cdk/scrolling/virtual-for-of.ts

src/cdk/table/table.scss

andrewseguin

It may be wise to add an experimental-version table.md focused on documenting this feature to talk about usage, limitations, and perhaps performance considerations like making sure you use trackBy. We'll want it for when it's moved out of experimental, and now would be a good time to write it while the details are fresh in your mind

andrewseguin · 2021-09-02T15:08:27Z

src/components-examples/cdk-experimental/table/cdk-virtual-table/cdk-virtual-table-example.ts

+  {position: 10, name: 'Neon', weight: 20.1797, symbol: 'Ne'},
+];
+
+const EXPANDED_ELEMENT_DATA: PeriodicElement[] = [];


Add comment to explain that this logic is simply to create a large dataset to so the table has a lot of rows

andrewseguin · 2021-09-02T15:10:14Z

src/components-examples/cdk-experimental/table/cdk-virtual-table/cdk-virtual-table-example.ts

+export class CdkVirtualTableExample {
+  displayedColumns: string[] = ['position', 'name', 'weight', 'symbol'];
+  dataSource = EXPANDED_ELEMENT_DATA;
+  trackBy = (index: number, el: PeriodicElement) => el.position;


Is trackBy required for virtual scroll? Adding it to the example may imply that its necessary. If its for performance reasons, this would be a good spot to mention that whether it significantly improves things.

andrewseguin · 2021-09-02T15:13:25Z

src/components-examples/cdk-experimental/table/cdk-virtual-table/cdk-virtual-table-example.html

@@ -0,0 +1,37 @@
+<cdk-virtual-scroll-viewport class="example-container" [itemSize]="48" [maxBufferPx]="1200" [minBufferPx]="400">


Can we provide a comment on why we chose these values for this example?

andrewseguin · 2021-09-02T15:16:07Z

src/components-examples/cdk-experimental/table/cdk-virtual-table/cdk-virtual-table-example.html

+      <th cdk-footer-cell *cdkFooterCellDef> Symbol </th>
+    </ng-container>
+
+    <tr cdk-header-row *cdkHeaderRowDef="displayedColumns; sticky: true;"></tr>


I think these extra rows are added to make sure we don't regress with the feature or performance, but it's not as useful to someone using this example to understand how to use the table.

Can we have a separate example that is more filled out or complex, and leave this as a barebones or typical example (e.g. single sticky header and no footer).

andrewseguin · 2021-09-02T15:18:23Z

src/dev-app/main.ts

@@ -11,5 +11,8 @@ import '@angular/localize/init';

 import {platformBrowser} from '@angular/platform-browser';
 import {MainModuleNgFactory} from './main-module.ngfactory';
+import {enableProdMode} from '@angular/core';
+
+enableProdMode();


How come this is being added?

This should be reverted. Done.

andrewseguin · 2021-09-02T15:23:19Z

src/cdk/collections/recycle-view-repeater-strategy.ts

@@ -37,8 +37,8 @@ import {
 export class _RecycleViewRepeaterStrategy<T, R, C extends _ViewRepeaterItemContext<T>>
    implements _ViewRepeater<T, R, C> {
  /**
-   * The size of the cache used to store unused views.
-   * Setting the cache size to `0` will disable caching. Defaults to 20 views.
+   * The size of the cache used to store unused views. Setting the cache size to `0` will disable


Can be reverted?

andrewseguin · 2021-09-02T15:35:01Z

src/cdk/table/table.ts

  /**
   * Stream containing the latest information on what rows are being displayed on screen.
   * Can be used by the data source to as a heuristic of what data should be provided.
   *
   * @docs-private
   */
-  readonly viewChange =
-      new BehaviorSubject<{start: number, end: number}>({start: 0, end: Number.MAX_VALUE});
+  readonly viewChange: BehaviorSubject<ListRange>;


Would it make more sense to make this a ReplaySubject so that users will get the latest value on subscribe but you don't need to set an initial value?

Changing this to a ReplaySubject would be a breaking change. Is it alright to make that change in this PR?

andrewseguin · 2021-09-02T15:48:38Z

src/cdk/table/table.ts

+        takeUntil(this._onDestroy))
+      .subscribe(([data, range]) => {
+        this._data = data;
+        this._renderedRange = range;


Nit: Should this be _renderRange or _dataRange to imply that this is the range we intend render, rather than the one we have rendered?

andrewseguin · 2021-09-02T15:50:23Z

src/cdk-experimental/table/table-virtual-scroll.ts

+    this._listener = listener;
+  }
+
+  stickyColumnsUpdated(update: StickyUpdate): void {


Can you add documentation to each of these functions? The naming throws me off a little - they sound more like boolean values. Should they be something like onStickyColumnsUpdated?

These method names are inherited from the StickyPositioningListener interface. I added inline docs to clarify their purpose.

andrewseguin · 2021-09-02T15:52:02Z

src/cdk-experimental/table/table-virtual-scroll.ts

+   * Measures the combined size (width for horizontal orientation, height for vertical) of all items
+   * in the specified range.
+   */
+  measureRangeSize(range: ListRange, orientation: 'horizontal' | 'vertical'): number {


Nit: Can this be getRangeSize instead of measure? Right now it sounds like its telling the directive to perform an action rather than to retrieve some values

No, this method is must be implemented as part of the CdkVirtualScrollRepeater interface. It's required to use the virtual scroll autosize behavior.

andrewseguin · 2022-03-24T19:13:50Z

@MichaelJamesParsons Would you like to rebase and continue iterating on this?

MichaelJamesParsons · 2022-03-29T17:24:58Z

Yes, I'll set aside time to wrap this up.

CharlieGreenman · 2024-10-17T09:03:32Z

@andrewseguin would it be ok if I work on this one? I have the same issue here: #10122 (comment) and I see this hasn't been touched for sometime. Would love to help and have this one go over the finish line

I think it makes sense for me to open up a new pull request all together given the time that has elapsed between now and then. Especially since signals now exists

andrewseguin · 2025-01-15T17:21:20Z

Closing since the code is fairly out of date and it's unlikely we'll get this refreshed. @CharlieGreenman you are welcome to open a new up-to-date PR for us to iterate on

CharlieGreenman · 2025-01-15T19:01:10Z

i will see what I can do

MichaelJamesParsons requested review from andrewseguin, crisbeto, devversion and jelbourn as code owners January 26, 2021 22:27

google-cla bot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Jan 26, 2021

MichaelJamesParsons requested review from mmalerba and kseamon January 26, 2021 22:28

MichaelJamesParsons added P2 The issue is important to a large percentage of users, with a workaround area: cdk/table G This is is related to a Google internal issue labels Jan 26, 2021

kseamon reviewed Jan 27, 2021

View reviewed changes

crisbeto reviewed Jan 27, 2021

View reviewed changes

jelbourn reviewed Jan 27, 2021

View reviewed changes

MichaelJamesParsons force-pushed the cdk-virtual-table branch 2 times, most recently from 2f7799d to 2bf17c3 Compare February 12, 2021 01:16

MichaelJamesParsons mentioned this pull request Feb 12, 2021

feat(cdk/table): add element offsets to sticky styler state #21886

Merged

jelbourn reviewed Feb 17, 2021

View reviewed changes

MichaelJamesParsons commented Feb 23, 2021

View reviewed changes

src/cdk/table/table.ts Outdated Show resolved Hide resolved

MichaelJamesParsons force-pushed the cdk-virtual-table branch from 55f1f96 to e7b6450 Compare June 15, 2021 00:56

MichaelJamesParsons requested a review from a team as a code owner June 15, 2021 00:56

MichaelJamesParsons force-pushed the cdk-virtual-table branch 2 times, most recently from 04e4bcf to 904f752 Compare June 15, 2021 01:55

MichaelJamesParsons changed the title ~~WIP: feat(cdk/table): virtual scroll directive for tables~~ feat(cdk/table): virtual scroll directive for tables Jun 15, 2021

MichaelJamesParsons force-pushed the cdk-virtual-table branch 3 times, most recently from ad96443 to 99b0b31 Compare June 15, 2021 03:16

kseamon reviewed Jun 17, 2021

View reviewed changes

josephperrott removed the request for review from a team June 17, 2021 15:55

jelbourn reviewed Jun 30, 2021

View reviewed changes

MichaelJamesParsons force-pushed the cdk-virtual-table branch 2 times, most recently from 2382ec7 to 30d988c Compare August 10, 2021 19:46

devversion removed their request for review August 18, 2021 13:12

mmalerba reviewed Aug 18, 2021

View reviewed changes

MichaelJamesParsons force-pushed the cdk-virtual-table branch from 30d988c to 5d9e66d Compare August 23, 2021 19:57

andrewseguin reviewed Sep 2, 2021

View reviewed changes

andrewseguin removed the cla: yes PR author has agreed to Google's Contributor License Agreement label Dec 28, 2021

googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Dec 29, 2021

andrewseguin added the needs rebase label Jan 19, 2022

andrewseguin added the needs: clarification The issue does not contain enough information for the team to determine if it is a real bug label Mar 24, 2022

MichaelJamesParsons force-pushed the cdk-virtual-table branch from 5d9e66d to bb7fd59 Compare April 30, 2022 23:28

feat(cdk/table): Virtual scroll directive for tables

b29aac0

MichaelJamesParsons force-pushed the cdk-virtual-table branch from bb7fd59 to b29aac0 Compare April 30, 2022 23:49

josephperrott added action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews and removed needs rebase labels Nov 16, 2022

CharlieGreenman mentioned this pull request Oct 17, 2024

virtual-scroll: integrate with relevant existing components #10122

Open

9 tasks

josephperrott requested a review from a team as a code owner December 18, 2024 17:40

josephperrott requested review from crisbeto and amysorto and removed request for a team December 18, 2024 17:40

andrewseguin closed this Jan 15, 2025

andrewseguin self-assigned this Jan 15, 2025

		* Collection viewer for the `CdkTable`. For backwards compatibility, the collection viewer must
		* be a `BehaviorSubject`.

		@@ -0,0 +1,37 @@
		<cdk-virtual-scroll-viewport class="example-container" [itemSize]="48" [maxBufferPx]="1200" [minBufferPx]="400">

feat(cdk/table): virtual scroll directive for tables #21708

feat(cdk/table): virtual scroll directive for tables #21708

Conversation

MichaelJamesParsons commented Jan 26, 2021 • edited Loading

Open questions

Out of scope for now

Known issues

Performance considerations

Related PRs

crisbeto commented Jan 27, 2021

MichaelJamesParsons commented Jan 27, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kseamon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MichaelJamesParsons commented Jun 30, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MichaelJamesParsons commented Aug 10, 2021 • edited Loading

andrewseguin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrewseguin commented Mar 24, 2022

MichaelJamesParsons commented Mar 29, 2022

CharlieGreenman commented Oct 17, 2024 • edited Loading

andrewseguin commented Jan 15, 2025

CharlieGreenman commented Jan 15, 2025

MichaelJamesParsons commented Jan 26, 2021 •

edited

Loading

MichaelJamesParsons commented Aug 10, 2021 •

edited

Loading

CharlieGreenman commented Oct 17, 2024 •

edited

Loading