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

scrollPositionRestoration has several problems #24547

Open
jnizet opened this Issue Jun 16, 2018 · 48 comments

Comments

Projects
None yet
@jnizet
Copy link
Contributor

jnizet commented Jun 16, 2018

I'm submitting a...


[ ] Regression (a behavior that used to work and stopped working in a new release)
[x] Bug report  
[ ] Performance issue
[x] Feature request
[x] Documentation issue or request
[ ] Support request => Please do not submit support request here, instead see https://github.com/angular/angular/blob/master/CONTRIBUTING.md#question
[ ] Other... Please describe:

Current behavior

I started experimenting with the new scrollPositionRestoration feature in the RouterModule extra options. I expected scroll restoration to work by default when setting the property to 'enabled', but it doesn't. And the documentation has issues, too.

Expected behavior

The documentation says:

'enabled'--set the scroll position to the stored position. This option will be the default in the future.

So I naïvely thought that setting the flag to 'enabled' would be sufficient to restore the scroll position. But it isn't.

Indeed, the scroll event is fired, and the scroll position is restored, before the ngAfterViewInit hook of the activated component has been called. So the view of the component is not ready yet when the router tries to restore the scroll position (i.e. there is no way to scroll to the end of a long list, because the list isn't there yet).

And even if it was restored after the view is ready, that would only work if the activated component used a resolved guard to load the data.

So, the documentation should, IMHO, at least indicate that restoring the scroll position always requires to

  • explicitly intercept the Scroll event, and scroll imperatively after a delay. This can be done in a single place, but I don't see how to do that in a reliable way, since there is no way to know if the delay is sufficient for the data to have been loaded (but it would at least work if resolve guards are used consistently), or
  • explicitly intercept the Scroll event in each routed component, and imperatively scroll when the data has been loaded and the view has been rendered. This is not a trivial task.

I read the remaining of the documentation, which has examples about doing this kind of stuff (although it doesn't really say that they're required). But those examples are all incorrect.

Here's the first example:

    class AppModule {
     constructor(router: Router, viewportScroller: ViewportScroller, store: Store<AppState>) {
       router.events.pipe(filter(e => e instanceof Scroll), switchMap(e => {
         return store.pipe(first(), timeout(200), map(() => e));
       }).subscribe(e => {
         if (e.position) {
           viewportScroller.scrollToPosition(e.position);
         } else if (e.anchor) {
           viewportScroller.scrollToAnchor(e.anchor);
         } else {
           viewportScroller.scrollToPosition([0, 0]);
         }
       });
     }
    }

This example uses a Store service, which is not part of Angular (I guess it's part of ngrx). So that makes it hard to understand and adapt for those who don't use ngrx.

Besides, it doesn't compile, because a closing parenthesis is missing, and because e is of type Event, and not of type Scroll, and thus has no position property.

The second example is the following:

    class ListComponent {
      list: any[];
      constructor(router: Router, viewportScroller: ViewportScroller, fetcher: ListFetcher) {
        const scrollEvents = router.events.filter(e => e instanceof Scroll);
        listFetcher.fetch().pipe(withLatestFrom(scrollEvents)).subscribe(([list, e]) => {
          this.list = list;
          if (e.position) {
            viewportScroller.scrollToPosition(e.position);
          } else {
            viewportScroller.scrollToPosition([0, 0]);
          }
        });
      }
    }

It doesn't compile because it still uses an old, non-pipeable operator, and because, once again, e is of type Event, not Scroll.

But even after fixing the compilation errors, it doesn't work because the view hasn't been updated with the new list yet when viewportScroller.scrollToPosition(e.position); is called.

So the code would have to be changed to the following in order to compile and work as expected

    class ListComponent {
      list: any[];
      constructor(router: Router, viewportScroller: ViewportScroller, fetcher: ListFetcher) {
        const scrollEvents = router.events.filter(e => e instanceof Scroll);
        listFetcher.fetch().pipe(withLatestFrom(scrollEvents)).subscribe(([list, e]) => {
          this.races = list;
          const scrollEvent = e as Scroll;
          of(scrollEvent).pipe(delay(1)).subscribe(s => {
            if (s.position) {
              viewportScroller.scrollToPosition(s.position);
            } else {
              viewportScroller.scrollToPosition([0, 0]);
            }
          });
        });
      }
    }

I think that none of these solutions is really simple enough, though. Here are two ideas that could maybe make things easier:

  • only fire the Scroll event and try to restore the position after the ngAfterViewInit hook has been called. This should at least make things work when a resolve guard is used to load the list. Or when the list is available immediately.
  • for the other cases, allow to inject a service that the component could call when the list has been loaded. It would be up to this service to get the last scroll position or anchor, to wait until the view has been rendered, and then to restore the scroll position. It would ignore all but the first call after the component has been activated.

Minimal reproduction of the problem with instructions

Here's a repo illustrating the various issues and solutions presented above: https://github.com/jnizet/scrollbug. It's a standard angular-cli project. I can't run it in stackblitz unfortunately (probably because Stackblitz doesn't support the beta release of Angular).

What is the motivation / use case for changing the behavior?

First, the documentation should be fixed and made clearer

  1. it should not use ngrx
  2. it should contain examples that compile, and run as expected
  3. it should make it clear than simply setting the flag to 'enabled' is not sufficient to enable scroll restoration

Second, it should be way easier to make that feature work. See ideas above.

Environment


Angular version: 6.1.0-beta.1


Browser:
- [x] Chrome (desktop) version 67.0.3396.87
- [ ] Chrome (Android) version XX
- [ ] Chrome (iOS) version XX
- [x] Firefox version 60.0.2 
- [ ] Safari (desktop) version XX
- [ ] Safari (iOS) version XX
- [ ] IE version XX
- [ ] Edge version XX
 

@ngbot ngbot bot added this to the needsTriage milestone Jun 21, 2018

@jasonaden

This comment has been minimized.

Copy link
Contributor

jasonaden commented Jun 21, 2018

Thanks for the detailed issue report! Great to get feedback on this new feature and hopefully get these things addressed quickly.

@mhevery

This comment has been minimized.

Copy link
Member

mhevery commented Jun 26, 2018

Closed by #20030

@mhevery mhevery closed this Jun 26, 2018

@jnizet

This comment has been minimized.

Copy link
Contributor Author

jnizet commented Jun 26, 2018

@mhevery @jasonaden please reopen.

This issue is not about a missing scroll position restoration. It's precisely about the new scroll position restoration feature that was introduced in 6.1.0-beta.1.

And it thus can't be closed by #20030: this issue is precisely about several problems in the code and documentation introduced by #20030.

@mhevery

This comment has been minimized.

Copy link
Member

mhevery commented Jun 26, 2018

Sorry

@mhevery mhevery reopened this Jun 26, 2018

@coonmoo

This comment has been minimized.

Copy link

coonmoo commented Jul 9, 2018

I'd like to add in combination with Ionic 4 the ViewportScroller's Scroll event position property is always [0, 0]
See minimal repro: https://github.com/coonmoo/IonicScrollPosBug

@damienwebdev

This comment has been minimized.

Copy link

damienwebdev commented Jul 12, 2018

Just chiming in here, my initial expectation of this feature was the following:

  1. The "scrollPosition" and navigation to it on each page was going to be handled internally by angular with "sane" defaults that are simple to configure ('enabled'|'disabled'|'top' make perfect sense as outlined by @vsavkin).
  2. As a simple use-case, if I navigate halfway down a page, click a link to another page, I should be at the top of the other page, not at the height of the point I was at on the previous page. This simple use-case doesn't seem to be resolved by the aforementioned #20030. Is that PR supposed to solve this use case?

But, just like @jnizet after upgrading a simple project to 6.1.0-rc1 it appears that these options don't actually do anything (maybe I'm missing something)?

I'm curious about the following:

  1. Are we expected to implement this scroll functionality on our own with the help of the ViewportScroller?
  2. What do the flags actually do out-of-the-box?
@damienwebdev

This comment has been minimized.

Copy link

damienwebdev commented Jul 12, 2018

Just leaving this here for all future visitors... as I did some debugging to determine why this wasn't working... For anyone who sees this in the future. If you have the following code in your stylesheet...

html,body {height: 100%}

This will be a bug for you.

Removing that fixes that and makes this PR do what was intended. Granted, removing that style may make a bunch of other things break, but that's a different problem.

Ninja edit:
It also appears that this seems to intermittently work with https://github.com/angular/material2

Working Version (My own personal anecdote):
I have a working app with material (which uses mat-sidenav) and all window.scroll variants work as expected.

Non-working version (as reported by @crisbeto ):
angular/material2#11552

@jasonaden

This comment has been minimized.

Copy link
Contributor

jasonaden commented Jul 17, 2018

@damienwebdev Thanks for that note on the body height. That very well could help quite a number of people

On the other things discussed in this issue, I've talked with @vsavkin about it and we're going to solve this in a couple ways. First is some documentation to make sure your use cases are discussed and solutions given for some of the common patterns. That documentation will be forthcoming.

Another is to fix an issue that was introduced some time ago where it's possible to have a NavigationEnd event fire before change detection. We need to change this so NavigationEnd happens after CD to make sure the page is actually rendered before trying to scroll.

@StefanRein

This comment has been minimized.

Copy link

StefanRein commented Jul 26, 2018

@jasonaden Could you tell, if following use case will be covered:

We are using perceived performance and intentionally are loading the data every time on component initialization, instead of caching, thus the app could be used with multiple devices at the same time.
Will there be a solution to tell the scroll restoration when the data of my component did load, so the view could be updated, elements could be created and the service finally would be able to scroll to the correct scroll position?

Thank you in forward.

@sorakthunly

This comment has been minimized.

Copy link

sorakthunly commented Jul 28, 2018

What's the progress on this issue?

@ericmartinezr

This comment has been minimized.

Copy link
Contributor

ericmartinezr commented Jul 29, 2018

explicitly intercept the Scroll event in each routed component, and imperatively scroll when the data has been loaded and the view has been rendered. This is not a trivial task.

This is a must after watching Jason's video about ng6.1 features where he says : [...] be aware on when and where you're rendering out your position.

Video: https://youtu.be/jNsbB8V9u0A?t=6m10s

@StefanRein

This comment has been minimized.

Copy link

StefanRein commented Jul 30, 2018

@mhevery (Edit: Sorry nevermind!)

I know one could preload all the data with Resolve from '@angular/router'. This would be fine and we could provide for each component an equal named class which could resolve all the needed data.
Then the scroll position restoration could work just as expected.
Usually this would also mean to show an overall spinner on the page, so the user would see there is something going on.

Another also very common practice is to use perceived performance. This is the way we decided to interact with preloading data. In our application we have a lot of times a list, which would show some grey boxes unless the data arrives and the list is shown.

As the component already is constructed and all the events are fired this would mean we would need to have the possibility to provide some asynchronous way to tell the data has been loaded for the scroll restoration service.

Do you see the possibility to provide a:

1. CustomScrollPositionRestorationStrategy or
2. another Lifecycle Hook

interface AfterContentReady {
    ngAfterContentReady(): Promise<boolean>;
}

I am also open for other ways. Maybe one would have some suggestions?

EDIT:
Okay, one way to go would be to store the value and call the scrollToPosition method of the ViewportScroller yourself.
E.g. Create an abstract class, from which you inherit and call a resolve method, every time you know your data is loaded and call in this abstracts class method, which implements this call this.viewportScroller.scrollToPosition(this.scrollPosition); - you get the idea.

Excerpt from:
https://github.com/Ninja-Squad/ninja-squad.github.com/pull/268/files
https://blog.ninja-squad.com/2018/07/26/what-is-new-angular-6.1/

export class PendingRacesComponent {
  scrollPosition: [number, number];
  races: Array<RaceModel>;

  constructor(route: ActivatedRoute, private router: Router, private viewportScroller: ViewportScroller) {
    this.races = route.snapshot.data['races'];
    this.router.events.pipe(
      filter(e => e instanceof Scroll)
    ).subscribe(e => {
      if ((e as Scroll).position) {
        this.scrollPosition = (e as Scroll).position;
      } else {
        this.scrollPosition = [0, 0];
      }
    });
  }

  ngAfterViewInit() {
    this.viewportScroller.scrollToPosition(this.scrollPosition);
  }

}
@akash-pal

This comment has been minimized.

Copy link

akash-pal commented Jul 31, 2018

Whats the status on the issue? Hash anyone found a perfect solution/ when will this issue be fixed?

@StefanRein

This comment has been minimized.

Copy link

StefanRein commented Aug 20, 2018

One additional note I would like to mention here:
In some frameworks like IONIC 3 or other applications, the scrolling element is by default not the document body.

(I did workaround this by setting all parent elements until the body to position: static / relative (they were position: absolute before), which introduced other problems, e.g. with modal dialogs, backdrops or scroll bar issues with fixed elements on different OS with different settings..)

Feature request:

  • A possibility to register another scrolling element.
  • Handle multiple elements for scrolling. (router outlets + custom elements)

Potential problems

  • Multiple / nested router outlets
    • Possibility to set and get the scroll position for each one
      (Note: The default scrolling element could be the parent element of the router outlet element, but we would maybe need to register another one.)
  • Async between different router outlets and their scrolling position until the components are finished with loading and building the desired content (perceived performance strikes here again).
    • Note: Yes one could also resolve first the data. But how to handle perceived performance here? Maybe setting a component into a specific position while resolving the data?
@rankitbishnoi

This comment has been minimized.

Copy link

rankitbishnoi commented Aug 29, 2018

This feature breaks server side rendering with asp .net .
it is using Window while rendering on server.
https://github.com/angular/universal/issues/830#issuecomment-414239486

@twoheaded

This comment has been minimized.

Copy link

twoheaded commented Aug 30, 2018

It's not quite clear, is there any way to set the scroll policy for certain routes?

@ouijan

This comment has been minimized.

Copy link

ouijan commented Sep 7, 2018

On Chrome, Using min-height doesn't break auto-scrolling and will force body t be at least screen height.
https://stackoverflow.com/questions/3740722/min-height-does-not-work-with-body

html {
    min-height: 100%;
    display: flex;
}
body {
    min-height: 100%;
    flex: 1;
}
@jasonaden

This comment has been minimized.

Copy link
Contributor

jasonaden commented Sep 11, 2018

Thanks for the feedback so far on this issue. We're going to be making some minor improvements to the current implementation, but also will be adding some new documentation in the next few weeks to help with some of the scenarios you guys have been having trouble with. Thanks for the patience on this one.

@lnaie

This comment has been minimized.

Copy link

lnaie commented Sep 18, 2018

new insights...
looks like the feature is working when using the browser's Back button.

but when i use something like this, it doesn't:
this.router.navigate(['../'], { relativeTo: this.route });

could somebody explain why the above code is not producing the same behaviour as Location.back(), and what are the options?

@sarunint

This comment has been minimized.

Copy link
Contributor

sarunint commented Sep 18, 2018

@lnaie That is intended. This feature will work when use browser's navigating back. Redirecting to ../ is not navigating back, it's going up one level.

@KroanNL

This comment has been minimized.

Copy link

KroanNL commented Sep 25, 2018

As an addition, I want to report that when using the following code

        this.router.navigate([], {
            relativeTo: this.route,
            queryParams: queryParams,
            replaceUrl: true
        });

The routerevents, including scrolling, will happen as well. This is not expected behaviour, if you ask me. When replacing only the url (for example by changing the queryParams, as in the example above), you don't want the page to suddenly scroll to the top of the page as well.

I tried to use skipLocationChange as well, without success.

jgarciao added a commit to flathub/linux-store-frontend that referenced this issue Sep 26, 2018

Enable scrollPositionRestoration in router. Code cleanup
With the new scrollPositionRestoration option of Angular 6.1 we can
simplify the code controlling the scroll.

Going back to the AppListComponent doesn't work very well, as
scrollPositionRestoration has issues when data is loaded dynamically

More info about this at angular/angular#24547
@lnaie

This comment has been minimized.

Copy link

lnaie commented Oct 1, 2018

@lnaie Your comments from 3 days ago are inappropriate and violate the Code of Conduct. I suggest you review that, and try to communicate with professionalism and respect.

done

@NikolaJankovic

This comment has been minimized.

Copy link

NikolaJankovic commented Oct 17, 2018

scrollPositionRestoration has another issue. If you load data using a 'load more'-type component that updates the query parameters, the default behaviour of scrolling to the top of the page breaks the default ui (additional data is rendered below the current and the scroll position should not change). That's a very common ui that should be accounted for.

There could be an additional argument that specifies the behaviour when a query parameter is updated, rather than just treating it the same as a full route change.

I don't want to sound too critical of the feature, it seems to work great on static pages, but the current solution seems like an oversimplification the second you start loading content dynamically.

@goat67

This comment has been minimized.

Copy link

goat67 commented Oct 22, 2018

Will this functionality support lazy loaded modules? I could not get this to work I am running 6.1.10 and Material 2.0. Also all pages are dynamic in nature no static pages.

I had a simple requirement return to the same scroll position from a view with a list of items after viewing the details of one of the items.

I finally had to resort to reading and storing the scroll position in the method I used to navigate to the detail view
const top = document.querySelector('mat-sidenav-content').scrollTop;
this.store.dispatch(new routePositionActions.SetPosition({route: 'Items', top: top, left: 0}));

I then set this stored position in the ngAfterViewInit method
document.querySelector('mat-sidenav-content').scrollTop = data.routePosition.top;

@Splaktar

This comment has been minimized.

Copy link
Member

Splaktar commented Oct 29, 2018

@goat67 it's a known issue with how this feature works with md-sidenav-container and/or other containers like Ionic uses. This is mentioned above and in angular/material2#4280.

@mberezinski

This comment has been minimized.

Copy link

mberezinski commented Nov 30, 2018

Any news on the topic?

@Lukasz196

This comment has been minimized.

Copy link

Lukasz196 commented Dec 5, 2018

For my project in Angular 6.1.9 work solution described in link bellow (for jQuery but this is only css).
https://stackoverflow.com/a/18573599/4490997

html {
  height: 100%;
  overflow: auto;
} 
body {
  height: 100%;
}

then

{scrollPositionRestoration: 'top'}

works

@CelticParser

This comment has been minimized.

Copy link

CelticParser commented Dec 18, 2018

Angular 7.1.2 | Material 7.1.1 (using 'mat-sidenav'). Having the same issue. However,

  • Lukasz196 css solution doesn't seem to apply.

'@HostListener('window:scroll')' functionality doesn't seem to work as well with this layout type.

@damienwebdev - Can you supply a stackblitz showing your method to how it works for you?

I have a working app with material (which uses mat-sidenav) and all window.scroll variants work as expected.

Side Note: I just started a huge project three months ago using Angular for the first time and I am very disappointed that a simple scrolling function(s) after 7 version releases is still broke. We just started mocking with large data sets and realized that the UI cannot scroll to the top when the user switches views. Imagine scrolling on a mobile device dozens of items down then needing to go back a view that has just as many and having to manually thumb-it back to the top? We can't even add a simple "scroll-to-top" fab because the 'window:scroll' seems to break with Material2 as well (I think the two are related). This is a very basic, 101 function for UX.

Should we (angular/angular) be communicating/working together with the crew at angular/material2 with this bug/issue? I may be wrong, but I am thinking that this is a "right-hand, left-hand" situation(?).

@maenthoven

This comment has been minimized.

Copy link

maenthoven commented Jan 15, 2019

Is there any ETA for this being fixed? This has been an annoyance for my users :(

@SteamWind

This comment has been minimized.

Copy link

SteamWind commented Jan 17, 2019

This issue is in the Backlog (described as: Milestone for triaged issues that have not yet been scheduled for resolution)
So, no schedule...

@havran

This comment has been minimized.

Copy link

havran commented Jan 29, 2019

It's possible disable anchorScrolling for some routes? (For example for auth callback rout, which use #access_token=... Thanks.

@Bretto

This comment has been minimized.

Copy link

Bretto commented Feb 21, 2019

I have been trying this feature, this is what I found:

  • It doesn't work with dynamic content.
  • It doesn't work with transition.
  • Currently doing a simple master/detail of scrolling cards (like Instagram) with transition back and forth is just impossible without hacking it all.

This is a very basic, 101 function for UX

We need a simple and elegant solution to this to stay competitive IMHO.

@JasonAlanKennedy

This comment has been minimized.

Copy link

JasonAlanKennedy commented Feb 21, 2019

Hate to say it but I left for VueJS over this. This was the straw that broke the camel's back. So many basic functions in Angular that's been open issues for a year+

@harm-less

This comment has been minimized.

Copy link

harm-less commented Feb 21, 2019

I too struggled with this issue but I managed to fix it with a polyfill from Ben Nadel!

Links to all important sources and explanations are in his blog post about the polyfill:
https://www.bennadel.com/blog/3534-restoring-and-resetting-the-scroll-position-using-the-navigationstart-event-in-angular-7-0-4.htm

You can check out the demo over here:
http://bennadel.github.io/JavaScript-Demos/demos/router-retain-scroll-polyfill-angular7/with-polyfill/#/app

For example: Go to "Section B", wait for the content to load and scroll a bit > go to "Section C" > press back button > profit!

It works like a charm!
The only thing that might be handy is making this an NPM package as I've copied the code into my project.

P.s.: I'm using Angular Material and even with the mat-sidenav component I managed to get this to work as expected 👍

@JasonAlanKennedy

This comment has been minimized.

Copy link

JasonAlanKennedy commented Feb 21, 2019

Oh yes, I know you can do that but it boils down to UX - Do I want the user have to download more and create more over head on the browser?

@harm-less

This comment has been minimized.

Copy link

harm-less commented Feb 21, 2019

@JasonAlanKennedy Can't argue with that, but that does make me wonder how Vue fixed this issue. Because they too must have had the same problems and had to write code for it.

@JasonAlanKennedy

This comment has been minimized.

Copy link

JasonAlanKennedy commented Feb 21, 2019

I wonder too - especially when the core is literally half the weight (and caries more out of the box!).

@KroanNL

This comment has been minimized.

Copy link

KroanNL commented Feb 21, 2019

Would appreciate it if you discuss other frameworks in private messages and keep the discussion ontopic. Thank you.

@NagRock

This comment has been minimized.

Copy link

NagRock commented Mar 4, 2019

Is there any information when this will be fixed? It is strange that we got to change onSameUrlNavigation to get scrollPositionRestoration working. 👎

@tuurbo

This comment has been minimized.

Copy link
Contributor

tuurbo commented Mar 4, 2019

Until angular addresses the issue, this solution worked best for me. You might need to tweak for your use case. I borrowed some of the code from this blog post https://blog.angularindepth.com/reactive-scroll-position-restoration-with-rxjs-792577f842c

import { Event, Routes, RouterModule, Route, Router, NavigationStart, NavigationEnd } from '@angular/router';

interface ScrollPositionRestore {
    event: Event;
    positions: { [K: number]: number };
    trigger: 'imperative' | 'popstate' | 'hashchange';
    idToRestore: number;
}

@NgModule(...)
export class AppRoutingModule {

    constructor(
        public router: Router,
    ) {
        let lastScrollTop = 0;

        fromEvent(window, 'scroll')
            .pipe(
                debounceTime(50),
                tap(() => {
                    lastScrollTop = document.querySelector('html').scrollTop;
                }),
            )
            .subscribe();

        this.router.events
            .pipe(
                filter(
                    event =>
                        event instanceof NavigationStart || event instanceof NavigationEnd,
                ),
                scan<Event, ScrollPositionRestore>((acc, event) => {
                    return {
                        event,
                        positions: {
                            ...acc.positions,
                            ...(event instanceof NavigationStart
                                ? {
                                    [event.id]: lastScrollTop,
                                } : {}),
                        },
                        trigger:
                            event instanceof NavigationStart
                                ? event.navigationTrigger
                                : acc.trigger,
                        idToRestore:
                            (event instanceof NavigationStart &&
                                event.restoredState &&
                                event.restoredState.navigationId + 1) ||
                            acc.idToRestore,
                    };
                }),
                filter(({ event, trigger }) => event instanceof NavigationEnd && !!trigger),
                observeOn(asyncScheduler),
            )
            .subscribe(({ trigger, positions, idToRestore }) => {
                if (trigger === 'imperative') {
                    document.querySelector('html').scrollTop = 0;
                }

                if (trigger === 'popstate') {
                    document.querySelector('html').scrollTop = positions[idToRestore];
                }
            });
    }
}
@Boooober

This comment has been minimized.

Copy link

Boooober commented Mar 8, 2019

@StefanRein, thanks for your example, it worked for me.
But I don't like to put any code inside the constructor, except injections. And if we move it to the ngOnInit, it won't work.

So, inspired by your solution, I desided to move subscription to the root App component and listen router event there. And in child component we only need to trigger scrollToPosition from the root. It is more reusable and no code inside constructor :)

export class AppComponent implements OnInit, OnDestroy {

    private scrollPosition: [number, number] = [0, 0];
    private scrollPositionSubscription: Subscription = new Subscription();

    constructor(
        private router: Router,
        private viewportScroller: ViewportScroller
    ) {}

    ngOnInit(): void {
        this.scrollPositionSubscription = this.router.events.pipe(
            filter((e: Event) => e instanceof Scroll),
            tap((e: Event) => {
                this.scrollPosition = (e as Scroll).position ? (e as Scroll).position : [0, 0])
            }
        ).subscribe();
    }

    ngOnDestroy(): void {
        this.scrollPositionSubscription.unsubscribe();
    }

    restoreScrollPosition(): void {
        this.viewportScroller.scrollToPosition(this.scrollPosition);
    }
}

...and child component...

export class ChildComponent implements AfterViewInit {
    constructor(
        @Inject(forwardRef(() => AppComponent)) private appComponent: AppComponent
    ) {}

    ngAfterViewInit(): void {
        this.appComponent.restoreScrollPosition();
    }
}
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.