angular / components

Component infrastructure and Material Design components for Angular
https://material.angular.io
MIT License
24.31k stars 6.72k forks source link

bug(table): Sticky header flickering #21576

Open tamtakoe opened 3 years ago

tamtakoe commented 3 years ago

Sticky header works with flickering with infinity scroll especially if scroll opposite direction. This is well-known issue for a couple of years, that appeared in some topics but still has no special topic.

Reproduction

https://user-images.githubusercontent.com/2244675/104481026-54436f80-55d6-11eb-8454-3eccc67c34a7.mov

E.g. with old version of Angular, but the same with last one. Also there are a lot of examples here https://github.com/angular/components/issues/10122#issuecomment-754283238 or in other issues where the effect appeared https://stackblitz.com/edit/mat-table-virtual-scroll-div

crisbeto commented 3 years ago

I wasn't able to reproduce it, but my guess is that it's due to the top value being updated as the user is scrolling. Can you still see it if you apply transform: translateZ(0) to .mat-table-sticky. Note that you may have to do it in your global styles or disable view encapsulation in order for the style to be picked up.

tamtakoe commented 3 years ago

@crisbeto yes, I tried translateZ(0) and some other hacks. Nothing helps. I think you have powerful PC. It has perceptible effect for huge tables with heavy rows. E.g. this example has flickering every time I scroll by vertical scroll bar because it causes lots of renderings. For heavy table I see flickering during wheel scroll also.

P.S. Other laptop with Windows 10 https://user-images.githubusercontent.com/2244675/104974758-f8c81600-5a09-11eb-8d30-f0bc2c60e842.MP4

crisbeto commented 3 years ago

I see. It seems like we shouldn't really have to set the top value while scrolling, considering that we're using position: sticky already. @andrewseguin, do you know the context behind the current behavior?

garrettld commented 3 years ago

@crisbeto I think I can provide some context. I've spent a lot of time digging into implementing virtual scrolling with CDK tables recently and I've been able to address the flickering in my projects at work. Note: I'm using the latest Chrome on Windows 10 for testing this.

The reason that top must be set in the way shown in the example is that the sticky header row is translated the same distance as the viewport, even though it's sticky relative to the <cdk-virtual-scroll-viewport> and not the inner <div> that is being translated. The resulting user experience is that the header row will kind of "jump" down the equivalent of N rows at a time, where N is the number of rows at the "front" of the data set that are not actually rendered due to virtual scrolling. If you scroll down until the first rendered row is at index 2 (the third item) in the original data source, then the virtual scroll viewport's translateY will be 96px (2 rows * 48px) and the sticky header row will be positioned 96px lower than the top of the viewport. I've edited the stackblitz so that you can see that here (scroll down slowly to see it happen):

https://stackblitz.com/edit/mat-table-virtual-scroll-div-qxryk4

The reason that these elements are translated in spite of their position property is that layout (including sticky positioning) is applied by the browser before translation is considered, according to the spec. Since the sticky row is inside both the viewport and the translated div, the browser positions it correctly and then translates it afterwards. Then when we set top on the sticky row to account for the offset, the browser is forced to calculate layout again. The result is that the row is rendered in the incorrect position for one frame, then its top is changed and it gets rendered again in the correct position on the next frame, which causes the flickering. The interactions between sticky elements, scrollable containers, and transformed containers are discussed in various browser bug reports and related github repos (for example: https://github.com/w3c/csswg-drafts/issues/3186). Similar issues occur when elements with position: fixed are contained by a transformed element and/or a scrollable element. The general sense I get from reading through these bug reports and issues is that it's working as intended.

Using position: absolute and top: #px instead of translateY(#px) on the <div> inside the virtual scroll viewport component prevents the flickering issue by avoiding having a transformed container in the first place. It also removes the need to set top on any sticky elements. That's a pretty significant change, so I don't know if that's something that the Angular team would want to or be able to land in the CDK. Since there's no way to change that behavior of the CDK virtual scroll viewport, I've thrown together a proof of concept to show that position: absolute + top: #px fixes the flickering issue, which you can see here: https://stackblitz.com/edit/virtual-scroll-cdk-table-test?file=src/app/my-table/my-table.component.html

If you un-comment the cdk-virtual-scroll-viewport and comment out the app-table-virtual-scroll-viewport you can see the flickering return.

Another solution is to move the sticky element(s) out of the transformed container so that they aren't affected by the translation. This isn't always possible, especially with components that require specific markup like tables. #20414 hints at doing that, but it's a bit unclear to me how that would work with virtual scrolling, and it only works for flex tables.

Sorry for the essay. I hope it helps!

lujian98 commented 3 years ago

I also spent many hours try to put table with header under the virtual scroll and it seems have too many problems. The simple way to avoid these flickering issues of the sticky header is create a separate table header which can align with virtual table.

tamtakoe commented 3 years ago

@lujian98 Yes, I have the same conclusion. Unfortunately cdk-table doesn't provide this way. Probably it is because of bad semantics. I think this can be sacrificed for the infinite scrolling that has been working on computers since the last century.

lujian98 commented 3 years ago

@tamtakoe Below is what I did for the table header columns, and table data rows with virtual scroll. With a separate table header, the table data component can be cdk-table or tree grid with cdk-tree.

// table header component (table with only header columns without data row)
<cdk-table...>
  <ng-container...>
    <cdk-header-cell...>
      ...
    </cdk-header-cell>
  </ng-container>

  <cdk-header-row...>  // for table header columns
  </cdk-header-row>
</cdk-table>

// table data with virtual scroll component (table with only data without header row)
<cdk-virtual-scroll-viewport...>
  <cdk-table...>
    <ng-container...>
      <cdk-cell...>
        ...
      </cdk-cell>
    </<ng-container>

  <cdk-row...>    // for table data
  </cdk-row>
  </cdk-table>
</cdk-virtual-scroll-viewport>
MichaelJamesParsons commented 3 years ago

Moving table headers and footers outside the virtual viewport will resolve the flickering. As @garrettld noted, I experimented with the possibility of this in #20414. There are some challenges with this solution that we're working to resolve:

  1. Accessibility - Detaching the body from the rest of the table breaks a11y interactions between the table and screen readers.
  2. Horizontal scrolling - The header, footer, or body containers need to scroll in sync when scrolling horizontally, while hiding extra scroll bars that might appear between containers.
tamtakoe commented 3 years ago

@lujian98 I tried the same way, but I used native one-row table for header and custom directive which reconciles column width for header and body. It works perfect but it has really terrible UX for developers. Especially if you have many tables which should be supported by several developers. That is why I offered this way which allows to wrap e.g. cdk-table template to custom one. Unfortunately it was declined by angular team.

sunilpanc commented 3 months ago

Any fix for this issue?

nickbelling commented 1 month ago

Inspired by @garrettld's findings above, I've written a directive that simply monkey-patches the <cdk-virtual-scroll-viewport> component to set the top or left values instead of the transform value of the inner content wrapper <div>. In my setup, where the virtual scroll is wrapping a standard HTML <table> and a <thead> row is position: sticky;, it works with zero other modifications - you can just set position: sticky on a row and you should be able to remove any event hooking you're doing.

import { afterNextRender, Directive, inject } from '@angular/core';
import { CdkVirtualScrollViewport } from '@angular/cdk/scrolling';

/**
 * Monkey-patches the `CdkVirtualScrollViewport` component and modifies it to
 * set "`top: {offset}px`" instead of "`transform: translateY({offset}px)`".
 * This fixes a bug where elements (such as a table header row) with
 * `position: sticky` don't correctly stick to the viewport, and enables you to
 * remove any custom handling you may be doing to hook events and set the top of
 * the stick elements manually.
 *
 * This currently works with Angular Material CDK 18.0.x, but uses
 * private accessors and may break without warning. If so, re-replicate the
 * `_doChangeDetection()` method in the `patchedDoChangeDetection()` function
 * below.
 */
@Directive({
    selector: 'cdk-virtual-scroll-viewport[stickyPatch]'
})
export class CdkVirtualScrollStickyPatchDirective {
    private _viewport = inject(CdkVirtualScrollViewport);

    constructor() {
        this._viewport['_doChangeDetection'] = patchedDoChangeDetection;
    }
}

const TRANSLATE_REGEX = /translate([XY])\((\d+)px\)/;

/**
 * This is current as of Angular Material CDK version 18.0.x, but may change
 * without warning. To update, replicate the `_doChangeDetection()` function
 * from https://github.com/angular/components/blob/main/src/cdk/scrolling/virtual-scroll-viewport.ts
 *
 * @param this The CdkVirtualScrollViewport component.
 */
function patchedDoChangeDetection(this: CdkVirtualScrollViewport): void {
    if (this['_isDestroyed']) {
        return;
    }

    this['ngZone'].run(() => {
        this['_changeDetectorRef'].markForCheck();

        const match = this['_renderedContentTransform']?.match(TRANSLATE_REGEX);
        if (match) {
            const axis = match[1];
            const offset = parseInt(match[2]);

            if (axis === 'X') {
                this._contentWrapper.nativeElement.style.left = `${offset}px`;
            } else if (axis === 'Y') {
                this._contentWrapper.nativeElement.style.top = `${offset}px`;
            }
        }

        afterNextRender(
            () => {
                this['_isChangeDetectionPending'] = false;
                const runAfterChangeDetection =
                    this['_runAfterChangeDetection'];
                this['_runAfterChangeDetection'] = [];
                for (const fn of runAfterChangeDetection) {
                    fn();
                }
            },
            { injector: this['_injector'] }
        );
    });
}

And you use it like so:

<cdk-virtual-scroll-viewport stickyPatch [itemSize]="35">
    <table>
        ...
    </table>
</cdk-virtual-scroll-viewport>
tlavarea commented 1 month ago

@nickbelling Thank you for the fix. I tried it and I'm still seeing flickering if I scroll really fast which isn't necessarily a valid use case. I still like this solution more than event hooking on the sticky header.

EDIT - I was using a mat-grid-list to create my table with the sticky header. After cutting over to mat-table with a sticky header the flickering is solved. Thanks again.