feat(cdk/scrolling): make scroller element configurable for virtual scrolling

[spike-rabbit]

Decouples the scroller from the virtual-scroll-viewport which allows library consumers
to use any parent element as a scroller. This is especially helpful when building SPAs
with a single global scrollbar.

Closes #13862

@mmalerba this is first draft, test and docs updates are missing. But I would like to have your feedback regarding certain points.

1. WDYT about the API changes in general?

2. There are a couple of lines, only to maintain backward compatibility. I am not sure if it's worth it, as I would expect that the required changes on applications could be done using schematics. Changing <cdk-virtual-scroll-viewport ... to <cdk-virtual-scroll-viewport cdk-virtual-scrollable-element ... should be sufficient.

3. I had some problems linking it to the global-scrollbar. As commented in the code, although documentElement is the scrolling element, no scroll event is fired. The current approach is the shortest one, but seems a little ugly to me. Instead of using html-elements in scrollable, using an adaptor could be an option as well.

4. As done multiple times, the conversion from 'top' | 'left' | 'right' | 'bottom' | 'start' | 'end' to 'left' | 'top' | 'right' | 'bottom' can be extracted to a new method. But where should I put this? Is cdk/src/platform/features/scrolling the correct place?

[spike-rabbit]

Any Ideas how CdkVirtualScrollViewport#measureViewportOffset can be make SSR compatible? It seems like getBoundingClientRect is not available in this environment.

[mmalerba]

Hi @spike-rabbit thanks for working on this!

1. I like the direction its going in general, separating out the scrolling element from the viewport feels right.
2. I think we should keep in the backwards compatible code. There's no reason to make the user write out that super verbose selector when we can just infer that the viewport should also be the scrolling container.
3. Yeah this feels weird. I don't like that we're kind of faking the host element of the directive. I think the problem stems from the fact that the user can't just place a directive on the documentElement. Instead I think it might make sense to separate CdkScrollable into 2 parts: an Injectable that has all of the logic (and takes a Document|Element) and a Directive that basically just delegates to the injectable, passing its host element.
4. I would just put it in src/cdk/scrolling/ in a new file. I don't think its something we want to expose to users, just a utility function we can share between our own code

[spike-rabbit]

Thx a lot for your feedback @mmalerba. While adopting your feedback, I run into another issue, or I am just missing something.

My problem in general is, that Document | Window and HTMLElement do not share the same API to retrieve scrolling related information and that document.documentElement is not behaving like every other HTMLElement in regard to scrolling.

So before I take actions on splitting the CdkScrollable, I would like to be sure that this is what you expect the Injectable part to look like. Due to the problem listed above, the injectable would have an interface like this:

scrollTo(scrollHeight: number, clientHeight: number, scrollWidth: number, clientWidth: number, scrollTop: number, scrollLeft: number, options: ExtendedScrollToOptions): void {
  ...
}

With my most recent commit I just declared the _elementScrolled as protected so that I was able to override it in CdkVirtualScrollableWindow. So this works at least.

[mmalerba]

Ah that's unfortunate that the APIs don't match up exactly, but I think the solution you have is good, having a special subclass for the window scrollable 👍

[lincolnthree]

@spike-rabbit This is absolutely fantastic. Thank you for working on this! I think this feature is one of the only things holding us back from using the cdk virtual-scroll API.

What are the next steps with this PR? Is there anything anyone can do to help? (Looking for ways to make sure this happens!) Happy to assist if I can.

I'm trying to test it right now.

[spike-rabbit]

@lincolnthree I need to complete documentation, tests and probably clean up some stuff. I am very confident, that I can do this already in April, but most likely not next week.

Please let me know the outcome of your tests. I only tried it in the cdk demo app.

[lincolnthree]

@spike-rabbit Awesome! Really looking forward to it.

So... I'm still working on it... but it appears the CI build artifacts have expired/timed out. Had a little trouble getting things linked, but seems like I'm there. I was able to build @angular/cdk and get it installed. Took a bit of hacking since apparently the Angular compiler doesn't like symlinks by default.

Regarding documentation, yes. I'm now trying to figure out how exactly to select and link the scrollable element to the virtual-scroll-viewport.

It looks like your example is fairly straightforward in the CDK sandbox. Is there a way to passing in an ElementRef or HTMLElement programmatically, or via inputs? Trying to figure out how I can plug in.

For instance, in my case, my scrolling element resides within the Shadow DOM of a 3rd party component, and that's currently passed into our existing virtual-scroll implementation directly.

Example of what I mean:

            <td-virtual-scroll #scroll [items]="list"
              [parentScroll]="md.scrollElement$ | async" <!-- Scrolling element passed in here -->
              (vsUpdate)="_cdr.detectChanges()">

[lincolnthree]

Trying to provide my own CdkVirtualScrollable -- but not sure it will work since the element needs to be provided via the constructor the way things are wired currently.

[lincolnthree]

Damn. So unless there's a way to lazily instantiate the cdkVirtualScrollableElement directive and attach it to something in the shadow dom, that's not going to work unless I duplicate the entire CdkScrollable class hierarchy.

I notice there is an abstract method to get the window offset, but why not also one to provide the element itself? E.g.:

import { Directionality } from '@angular/cdk/bidi';
import { CdkVirtualScrollable, ScrollDispatcher, VIRTUAL_SCROLLABLE } from '@angular/cdk/scrolling';
import { Directive, ElementRef, NgZone, Optional } from '@angular/core';

@Directive({
  selector: '[cdk-virtual-scrollable-content], [cdkVirtualScrollableContent]',
  providers: [{ provide: VIRTUAL_SCROLLABLE, useExisting: CdkVirtualScrollableContent }],
  host: {
    'class': 'cdk-virtual-scrollable',
  },
})
export class CdkVirtualScrollableContent extends CdkVirtualScrollable {

  private _elementRef: ElementRef;

  constructor(
    elementRef: ElementRef,
    scrollDispatcher: ScrollDispatcher,
    ngZone: NgZone,
    @Optional() dir: Directionality,
  ) {
    super(elementRef, scrollDispatcher, ngZone, dir);
    this._elementRef = elementRef;
  }

  getElementRef(): ElementRef<HTMLElement> {

   // Unless I'm missing something, this could be abstract in order to allow proper extension of the CdkScrollable base class. Otherwise the element passed in through the constructor will always be used internally. And if the element is not yet constructed, that won't work. So we need some way of passing a lazily created element in.

    const result = this._elementRef.nativeElement.shadowRoot?.querySelector('main');
    console.error('MY SCROLLABLE', result);
    return result;
  }

  getBoundingClientRectWithScrollOffset(from: 'left' | 'top' | 'right' | 'bottom'): number {
    return (
      this.getElementRef().nativeElement.getBoundingClientRect()[from] -
      this.measureScrollOffset(from)
    );
  }
}

[lincolnthree]

Hmm, I'll wait till I hear back. I don't think I'm approaching this the right way.

That said, I think the ability to declaratively pass in a scroll-container or scrollable element is fairly crucial.

E.g. For the scenario where a cdk-virtual-scroll-viewport is defined in a child component or nested template, where the parent scroll element / scrolling div / etc cannot be annotated with the cdk-virtual-scrollable-element directive, or if said scrolling element is synthetically/programmatically created through typescript.

[lincolnthree]

I was able to copy enough code to create a CdkScrollable implementation that provides a deferred native element, but I can't seem to get the scrollable viewport to pick it up. It's not pretty, and I think being able to directly provide a scrollable element in the viewport component itself is the best solution I can think of. I will try that next.

[lincolnthree]

I have to go for the night, but this is what I'm thinking:

export class CdkVirtualScrollViewport extends CdkVirtualScrollable implements OnInit, OnDestroy {
 ...

  /**
   * Directly set an HTMLelement to use as the scroll container.
   */
  @Input()
  get scrollableElement(): HTMLElement | null {
    return this._scrollableElement;
  }
  set scrollableElement(value: HTMLElement | null) {
    this._scrollableElement = value;
  }
  private _scrollableElement: HTMLElement | null = null;

...

  override ngOnInit() {

    if(this.scrollableElement) {
      this.scrollable = new CdkVirtualScrollableElement({ nativeElement: this.scrollableElement }, this.scrollDispatcher, this.ngZone, this.dir as any);
    }
    
 // This code moves down from the constructor to get defer until values are set in inputs'

    if (!this.scrollable) {
      // No scrollable is provided, so the virtual-scroll-viewport needs to become a scrollable
      this.elementRef.nativeElement.classList.add('cdk-virtual-scrollable');
      this.scrollable = this;
    }
...


}

Then this is all it takes:

<cdk-virtual-scroll-viewport
              [scrollableElement]="md.scrollElement$ | async"> ...

[lincolnthree]

Okay, so I finally got things wired up (with my slight modification to set the scrolling container directly) and was able to start testing. This feature works exactly as it should! YES.

With my modification I was able to directly assign the scroll container and that works perfectly, too. (Small note, the reason this modification -- or something like it -- to set the scroll container is necessary after construction (at init time) is for compatibility with the Ionic framework and others like it.)

I noticed that this does not seem to play super nicely with AutoSizeVirtualScrollStrategy implemented in cdk/experimental -- of course, that may be outside the scope of this issue. But I'm looking into that as well.

[lincolnthree]

Update. I have had little luck getting this to work with the experimental auto-size feature. I am probably missing something simple.

[spike-rabbit]

@lincolnthree I am not sure, if your solution is a good idea. I would expect that linking to another container in a 3rd party library is a rare edge case. I would guess, that in those scenarios it is better to provide a custom CDKVirtualScrollable

The only challenging part about the ion-content is, that the container is asynchronously resolved. Something like this should work (not tested):

@Directive({
  selector: 'ion-content[virtual-scrollable], ion-content[virtualScrollable]',
  providers: [{provide: VIRTUAL_SCROLLABLE, useExisting: IonContentVirtualScrollable }],
})
export class IonContentVirtualScrollable extends CdkVirtualScrollableElement {
protected _elementScrolled: Observable<Event> = new Observable((observer: Observer<Event>) =>
    this.ngZone.runOutsideAngular(async () => {
      const ionScrollElement = await this.ionContent.getScrollElement();
      fromEvent(ionScrollElement, 'scroll')
        .pipe(takeUntil(this._destroyed))
        .subscribe(observer),
    }),
  );

  constructor(
    private IonContent: IonContent,
    scrollDispatcher: ScrollDispatcher,
    ngZone: NgZone,
    @Optional() dir: Directionality,
  ) {
    super(undefined!, scrollDispatcher, ngZone, dir);
    this.ionContent.getScrollElement().then(ionScrollElement => {
      this.elementRef = new ElementRef(ionScrollElement);
      ionScrollElement.classList.add('cdk-virtual-scrollable');
    });
  }
}

and using it like this:

<ion-content virtual-scrollable>
  <cdk-virtual-scroll-viewport>
    ...
 </cdk-virtual-scroll-viewport>
</ion-content>

[spike-rabbit]

@mmalerba This is ready for a full review now. Everything you addressed before should be fixed. Tests and documentation are updated as well.

Please let me know if there is more to do.

[andrewseguin]

@spike-rabbit Just a heads up that @mmalerba is out right now and should be able to resume the review in May

[lincolnthree]

@spike-rabbit Interesting. That looks like it should work. I did not think about passing undefined and then setting the elementRef manually. Though, to be honest, asking a developer to do this, versus just setting the element... this seems like a much more complex approach for what is really just setting a value. Though if we are respecting conventions and patterns, I get it.

The remaining question I have about this, is how would this directive work if the <ion-content virtualScrollable> element is in a child component/module from the consumer (think <ng-content> to inject content into slots.).

This directive and it's provided value would need to be able to cross the component boundary. Maybe this is an angular provider question and not so much a question for this PR.

<my-page>
   <child-component-with-ion-content-inside>
     <ng-component slot=content>
       <cdk-virtual-scroll-viewport>
        ... // how does this viewport get a reference to the directive's provided VIRTUAL_SCROLLABLE?
       </cdk-virtual-scroll-viewport>
     </ng-component>
   </child-component-with-ion-content-inside>
</my-page>

Not really 100% sure how this goes.

Or I suppose the virtual scroll itself could be contained in the component, but that complicates things a bit.

I'll give this a try and report back.

[mmalerba]

@spike-rabbit Thanks for the changes this looks good to me. Just one last thing is I want to have a discussion with the rest of the team about the naming for the new directives (always the hardest part of programming 🙄 ). I'll get back to you next week on that, in the mean time LGTM

[spike-rabbit]

@mmalerba can you please help me to solve this error:

error TS1149: File name '/home/<user>/.cache/bazel/_bazel_z003dz7j/1b0f1013d88c2f27d063f2199b9dc125/execroot/angular_material/external/npm/node_modules/@angular/common/locales/extra/ca-ES-valencia.d.ts' differs from already included file name '/home/<user>/.cache/bazel/_bazel_<user>/1b0f1013d88c2f27d063f2199b9dc125/execroot/angular_material/external/npm/node_modules/@angular/common/locales/extra/ca-ES-VALENCIA.d.ts' only in casing.
  The file is in the program because:
    Root file specified for compilation
    Root file specified for compilation

Target //tools/public_api_guard:cdk/scrolling.md_api.accept failed to build
Use --verbose_failures to see the command lines of failed build steps.
ERROR: /home/<user>/simpl-stuff/components/tools/public_api_guard/BUILD.bazel:7:22 Middleman _middlemen/tools_Spublic_Uapi_Uguard_Scdk_Sscrolling.md_Uapi.accept.sh-runfiles failed: (Exit 1): ngc-wrapped.sh failed: error executing command bazel-out/k8-opt-exec-2B5CBBC6/bin/external/npm/@angular/bazel/bin/ngc-wrapped.sh '--node_options=--expose-gc' '--node_options=--stack-trace-limit=100' '--node_options=--max-old-space-size=4096' ... (remaining 1 argument skipped)

This happens, when I try to build, test or to accept API changes. I already deleted ~/.cache/bazel and rebased on main, but the error is still there.

[mmalerba]

Oh yeah that happened to me recently too. Some combination of the below finally fixed it:

yarn bazel clean --expunge
yarn cache clean
rm -rf node_modules ~/.cache/bazel

[spike-rabbit]

@mmalerba thx for the tip. It works now. Hope that everything is ready to merge

[mmalerba]

Yes, all looks good on your side. Now I just have to run it against the tests in Google's codebase to make sure it doesn't break anything

[mmalerba]

@spike-rabbit I started looking into why some tests were failing internally with this change, and it looked like some people were setting display: flex on the viewport. Without this position: absolute the spacer is collapsing and throwing off the scrollbar.

I think we could definitely question why they're making it display: flex, as I don't see any real reason to do that, but I think we could help prevent unexpected breakages by just slapping a flex: 0 0 auto on here.

I also notice that below you still have right: 0 and left: auto which can be removed, now that its no longer position: absolute

[spike-rabbit]

@mmalerba changed as you requested

[tomasdev]

Demo for anyone looking for it available at https://stackblitz.com/run?file=src%2Fapp%2Fcdk-virtual-scroll-window-scrolling-example.html

[lincolnthree]

@tomasdev I think that link isn't working (at least it's not for me?) Thanks!

[angular-automatic-lock-bot]

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

_{This action has been performed automatically by a bot.}