Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

bug(Sidenav+Tabs+Drag n drop): Drag scroll not working if Sidenav-Content height set to 100vh #18737

Closed
KlausHans opened this issue Mar 6, 2020 · 7 comments · Fixed by #18918, lingounet/testage#29 or michael-vasyliv/ngx-virtual-swiper#24
Assignees
Labels
P3 An issue that is relevant to core functions, but does not impede progress. Important, but not urgent

Comments

@KlausHans
Copy link

KlausHans commented Mar 6, 2020

Reproduction

https://stackblitz.com/edit/angular-ebzjz5?file=src%2Fapp%2Fexpansion-overview-example.html

Steps to reproduce:

  1. Put a mat-accordion with drag n drop inside a mat-tab that is inside mat-sidenav-content.
  2. Set the height of mat-sidenav-content to 100vh, so all items of the sidenav are always visible. The mat-accordion overflows the 100vh with scroll due to a large amount of items in it.
  3. Try to drag an item from the list to a position that is not visible without scrolling. It won't scroll.

Expected Behavior

The view autoscrolls while draging the item to the bottom of the screen.

Actual Behavior

It doesn't autoscoll.

It does if i either delete the tab-group or if i delete the max-height of 100vh of the sidenav-content.
Please take a look at the stackblitz.

Environment

  • Angular: 9.0.5
  • CDK/Material: 9.1
  • Browser(s): all
  • Operating System (e.g. Windows, macOS, Ubuntu): all

A possible workaround could be to delete the height of 100vh and set items in the sidenav to a fixed position so they are always visible. Not testet though.

@KlausHans KlausHans added the needs triage This issue needs to be triaged by the team label Mar 6, 2020
@KlausHans KlausHans changed the title bug(Sidenav+Tabs+Drag n drop): Autoscroll not working if Sidenav-Content height set to 100vh bug(Sidenav+Tabs+Drag n drop): Drag scroll not working if Sidenav-Content height set to 100vh Mar 6, 2020
@manu-058
Copy link

manu-058 commented Mar 7, 2020

i think this can be handled from the application end,
https://stackblitz.com/edit/angular-ebzjz5-pe3mev

@KlausHans
Copy link
Author

KlausHans commented Mar 9, 2020

Thanks for the suggestion.
However, your solution seems very hacky to me and the scrolling isn't smooth.

Edit: Just implemented my suggested workaround. Works fine but involves major changes in the app layout structure. Thats not so nice.

@Achilles1515
Copy link

Achilles1515 commented Mar 9, 2020

To some degree, this doesn't have anything to do with the sidenav or tab components. The main issue with your demo is you're trying to scroll an element that is not the CdkDropList element, nor the window.

One solution is to manually register the sidenav content as a scrollable parent of the droplist - something like this:

@ViewChild(MatSidenavContent , {read: ElementRef}) sideNavContentElement: ElementRef;
@ViewChild(CdkDropList) dropList: CdkDropList;
ngAfterViewInit() {
  // register the sidenav content as a scrollable parent element
  if (this.sideNavContentElement) {
    this.dropList._dropListRef.withScrollableParents([this.sideNavContentElement.nativeElement])
  }
}

https://stackblitz.com/edit/angular-ebzjz5-ftwbai?file=src%2Fapp%2Fexpansion-overview-example.ts

If you have a statically-defined number of droplists/sidenav contents then this is fine, but it's not a very scalable solution.

This pull request added the ability to define scrollable containers other than the droplist and window by adding a cdkScrollable directive to elements. Theoretically, you would be able to do this (adding cdkScrollable):

<mat-sidenav-content style="max-height: 100vh" cdkScrollable>
...
</mat-sidenav-content>

and the drop list would "automatically" pick up this element as a scrollable parent. Unfortunately, this doesn't work with your use case, because the drop list is projected (multiple levels) inside the sidenav content and registering the cdkScrollables happens inside the ngAfterContentInit() lifecycle hook. My knowledge on this is a little fuzzy, but the gist of the problem is that during ngAfterContentInit(), you cannot traverse up DOM elements across projection boundaries. Meaning this method of scroll-dispatcher.ts:

/** Returns all registered Scrollables that contain the provided element. */
  getAncestorScrollContainers(elementRef: ElementRef): CdkScrollable[] {
    const scrollingContainers: CdkScrollable[] = [];

    this.scrollContainers.forEach((_subscription: Subscription, scrollable: CdkScrollable) => {
      if (this._scrollableContainsElement(scrollable, elementRef)) {
        scrollingContainers.push(scrollable);
      }
    });

    return scrollingContainers;
  }

will return an empty list of scrollContainers, despite your HTML actually having a cdkScrollable as an ancestor of the drop list.

You can find ways to get around this...for example, if not using Ivy, you can override ngAfterContentInit() and wrap the withScrollableParents() call inside a setTimeout to defer execution until the view is fully initialized:

CdkDropList.prototype.ngAfterViewInit = function(){
    // @breaking-change 11.0.0 Remove null check for _scrollDispatcher once it's required.
    if (this._scrollDispatcher) {
      setTimeout(() => {const scrollableParents = this._scrollDispatcher
         .getAncestorScrollContainers(this.element)
         .map(scrollable => scrollable.getElementRef().nativeElement);
       this._dropListRef.withScrollableParents(scrollableParents);
       })
    }
}

But really, I think the best solution is to use ngAfterViewInit() instead of ngAfterContentInit().

CdkDropList.prototype.ngAfterViewInit = function(){
    // @breaking-change 11.0.0 Remove null check for _scrollDispatcher once it's required.
    if (this._scrollDispatcher) {
      const scrollableParents = this._scrollDispatcher
        .getAncestorScrollContainers(this.element)
        .map(scrollable => scrollable.getElementRef().nativeElement);
      this._dropListRef.withScrollableParents(scrollableParents);
    }
}

Using ngAfterViewInit() allows the cdkScrollable to be picked up across projection boundaries.

Example here:
https://stackblitz.com/edit/angular-ebzjz5-3csse8?file=src/app/expansion-overview-example.ts
(Notice how commenting out the prototype override does not allow the auto-scrolling, despite cdkScrollable being added to the <mat-sidenav-content>)

@crisbeto thoughts on switching the CdkDropList scrollableParents logic from ngAfterContentInit() to ngAfterViewInit()?

@crisbeto crisbeto self-assigned this Mar 25, 2020
@crisbeto crisbeto added has pr P3 An issue that is relevant to core functions, but does not impede progress. Important, but not urgent and removed needs triage This issue needs to be triaged by the team labels Mar 25, 2020
crisbeto added a commit to crisbeto/material2 that referenced this issue Mar 25, 2020
Currently we resolve a drop list's scrollable parents inside `ngAfterViewInit`, but this might be too early if the element is being projected. These changes move the logic to the first time the user starts dragging which should guarantee that the DOM structure has settled.

Fixes angular#18737.
jelbourn pushed a commit that referenced this issue Mar 31, 2020
…18918)

Currently we resolve a drop list's scrollable parents inside `ngAfterViewInit`, but this might be too early if the element is being projected. These changes move the logic to the first time the user starts dragging which should guarantee that the DOM structure has settled.

Fixes #18737.
@KlausHans
Copy link
Author

Thanks. In which version will this fix be released?

@crisbeto
Copy link
Member

crisbeto commented Apr 1, 2020

It'll be in the next patch release.

jelbourn pushed a commit that referenced this issue Apr 6, 2020
…18918)

Currently we resolve a drop list's scrollable parents inside `ngAfterViewInit`, but this might be too early if the element is being projected. These changes move the logic to the first time the user starts dragging which should guarantee that the DOM structure has settled.

Fixes #18737.

(cherry picked from commit 3e984c7)
@KlausHans
Copy link
Author

Released with Material 9.2.1
Works very well, thank you.

@angular-automatic-lock-bot
Copy link

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.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators May 18, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
P3 An issue that is relevant to core functions, but does not impede progress. Important, but not urgent
Projects
None yet
4 participants