feat(router): implement scroll position restoration #20030

vsavkin · 2017-10-30T18:35:32Z

No description provided.

EreckGordon · 2017-10-30T18:51:04Z

@vsavkin see https://github.com/angular/angular/pull/20030/files#diff-ff3b9c3c80dbf5eb828cf6d919746075R265

wahtItDoes <-- typo of what may cause this note to not appear in docs?

gkalpak · 2017-10-30T19:41:08Z

packages/router/src/router_scroll_manager.ts

+ */
+export class RouterScrollManager implements OnDestroy {
+  private subscription: Subscription;
+  private windowObj: any = window;


Is this going to affect SSR?

I'm just creeping in random angular CRs again.. sorry.. one solution to that would be a NoopRouterScrollManager similar to NoopAnimationDriver

gkalpak · 2017-10-30T19:44:32Z

packages/router/src/router_scroll_manager.ts

+  protected scrollIntoView(anchor: string, navigationEnd: NavigationEnd) {
+    if (this.options.anchorScrolling === 'enabled') {
+      const doc = this.dom.getDefaultDocument();
+      const el = this.dom.querySelector(doc.body, `#${anchor}`);


Browsers would also scroll to a[name="${anchor}"], so users might expect that here too.

zimme · 2017-10-31T15:49:11Z

Does this take into account, async data loading and rendering?

jasonaden

Looks good overall. I added a couple comments to those already there.

jasonaden · 2017-11-07T20:00:52Z

packages/router/src/router_scroll_manager.ts

+      private router: Router,
+      private options: {scrollPositionRestoration?: string, anchorScrolling?: string} = {}) {}
+
+  setUpScrollManager(): void {


Why not just have this happen in the constructor? As-is you could instantiate this class and end up running ngOnDestroy which will result in an error if setUpScrollManager() isn't called.

bootstrapListener, where these things are set up, is pretty complex, so I prefer to explicitly invoke side effects, rather than rely on DI. That's why I prefer to keep the constructor side-effect free. Also, RouterPreloader works in the same way. I added an if statement into ngOnDestroy instead.

jasonaden · 2017-11-07T20:07:47Z

packages/router/src/router_scroll_manager.ts

+                  });
+                }
+
+              } else if (e instanceof NavigationEnd) {


Is there a possibility of this not working due to #17473? Where NavigationEnd can run before the UI has settled.

jasonaden · 2017-11-07T20:29:52Z

packages/router/src/router_scroll_manager.ts

+        scan.call(this.router.events, (s: ScrollState, e: RouterEvent) => {
+              if (e instanceof NavigationStart) {
+                const navigationSource = e.navigationSource;
+                if (navigationSource === 'popstate') {


I think there might be a problem with the logic here. Does the array get zeroed-out in this case (states are after NavigationEnd:

positions: [[0,0]] source: imperative restore: undefined --- scroll to 0, 10 --- click link positions: [[0,0], [0,10]] source: popstate restore: undefined --- back button positions: [[0,0]] source: popstate restore: [0,10] --- forward button positions: [] source: popstate restore: [0,0]

jasonaden · 2017-11-07T20:32:22Z

packages/router/src/events.ts

+      /** @docsNotRequired */
+      url: string,
+      /** @docsNotRequired */
+      public navigationSource: NavigationSource = 'imperative') {


Add to NavigationEnd? You're not using it there in this PR, but seems like it should be there for consistency.

I was thinking about it. The reason why I didn't go with this is that all navigations have NavigationStart, but not all of them have NavigationEnd. So if I add the source to NavigationEnd, I'll need to add it to NavigationError and NavigationCancel. It's a bigger API change, so I decided not to do it until the need is there. Especially because the user can always get the source by using groupBy.

But if you think this is required, I'll add it.

jsgoupil · 2017-11-06T02:35:29Z

packages/router/src/router_scroll_manager.ts

+              }
+            }, initState).subscribe((s: ScrollState) => {
+          if (s.restore && s.navigationEnd) {
+            this.restoreScrollPosition(s.restore, s.navigationEnd);


What's the reason to pass in the NavigationEnd to your restoreScrollPosition API and not pass any RouterEvent to your getCurrentScrollPosition?

This ScrollManager covers the basic scenario for top-window scrolling, but since Angular advocates the usage of SPA, most likely I will be scrolling a DIV somewhere on the page on navigation (used with a router-outlet). Something like a left menu and a main page. Could we get this scenario covered a little easier?

It would be interesting to have a way to save multiple scroll positions of different DIV that are about to disappear. Then on re-appearing, restore all of them.

I think this would make the window jumping couple times, because of redirections int the routing configs.

jasonaden · 2017-11-27T21:06:03Z

packages/common/src/location/location.ts

@@ -13,6 +13,7 @@ import {LocationStrategy} from './location_strategy';
 /** @experimental */
 export interface PopStateEvent {
  pop?: boolean;
+  state?: any;


Concrete types here would be great if it works for this use-case.

I added types in the router package cause the router knows what is stores. The field here has to remain any because the location cannot depend on the router.

MRosenlind · 2017-11-28T14:01:24Z

Any news on this?

jlbeard84 · 2017-12-01T17:48:16Z

packages/platform-server/src/domino_adapter.ts

@@ -193,6 +193,7 @@ export class DominoAdapter extends BrowserDomAdapter {

  getHistory(): History { throw _notImplemented('getHistory'); }
  getLocation(): Location { throw _notImplemented('getLocation'); }
+  getWindow(): Window { throw _notImplemented('getLocation'); }


Looks like this might need to pass a different string into the notImplemented error.

jasonaden

Looks good. Please rebase & push. We will need approval on the public API changes in order to get this merged.

IgorMinar · 2018-06-07T00:32:05Z

Reviewed in person with @jasonaden. LGTM now. Thank you for all the changes. Jason will still change the commit message to add a note about how to use this feature.

For documentation, see `RouterModule.scrollPositionRestoration` Fixes angular#13636 angular#10929 angular#7791 angular#6595

alfaproject · 2018-06-08T22:15:45Z

packages/common/src/viewport_scroller.ts

+ * @whatItDoes Provides an empty implementation of the viewport scroller. This will
+ * live in @angular/common as it will be used by both platform-server and platform-webworker.
+ */
+export class NullViewportScroller implements ViewportScroller {


We have NoopAnimations. Should this follow the same naming? NoopViewport or so

raysuelzer · 2018-06-18T19:22:35Z

I see this is closed but not merged? Is this going to be merged?

ericmartinezr · 2018-06-18T19:25:12Z

@raysuelzer Check the last feature https://github.com/angular/angular/blob/master/CHANGELOG.md#610-beta1-2018-06-13

sarunint · 2018-06-18T19:25:57Z

@raysuelzer It's been merged. The way Angular do the merge is to cherry pick the commit into the master branch. So that the commit history is beautifully linear.

jek-bao-choo · 2018-06-25T10:16:00Z

May I know how do we use this feature?

jek-bao-choo · 2018-06-25T10:17:08Z

https://medium.com/@sarunint/scroll-position-restoration-in-angular-5a09df8bf626

jek-bao-choo · 2018-06-25T10:18:58Z

#24547

devalmehta · 2018-08-09T00:34:35Z

so is there a solution to this issue?

MikeKovetsky · 2018-08-09T10:56:55Z

Thank you for this feature, @vsavkin and Angular team.
Although, most of the features work as expected, I have trouble using scrollPositionRestoration: 'enabled'. with query params. The scrollPosition feature scrolls viewport to the top. This causes some UX problems on filter page.

When I switch scrollPositionRestoration to disabled it does not scroll and work as expected.
Is there a valid way to ignore queryParams change?

Splaktar · 2018-10-29T20:54:07Z

@MikeKovetsky this appears to be tracked in #24547 (comment) now.

angular-automatic-lock-bot · 2019-09-14T06:22:30Z

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.}

googlebot added the cla: yes label Oct 30, 2017

vsavkin force-pushed the router_scrolling branch from 140f3c0 to 7f05e91 Compare October 30, 2017 18:37

ericmartinezr mentioned this pull request Oct 30, 2017

Unexpected RouteReuseStrategy behaviour #20018

Closed

ericmartinezr mentioned this pull request Oct 30, 2017

Keep scroll position when navigating back #10929

Closed

vsavkin force-pushed the router_scrolling branch from 7f05e91 to c256ba5 Compare October 30, 2017 19:19

gkalpak reviewed Oct 30, 2017

View reviewed changes

ericmartinezr mentioned this pull request Nov 6, 2017

Changing route doesn't scroll to top in the new page #7791

Closed

jasonaden suggested changes Nov 7, 2017

View reviewed changes

jsgoupil reviewed Nov 8, 2017

View reviewed changes

jasonaden added the area: router label Nov 14, 2017

vsavkin force-pushed the router_scrolling branch 3 times, most recently from 873bb55 to 7748c26 Compare November 17, 2017 17:11

jasonaden approved these changes Nov 27, 2017

View reviewed changes

jasonaden reviewed Nov 27, 2017

View reviewed changes

vsavkin force-pushed the router_scrolling branch from 7748c26 to 3dcd424 Compare November 28, 2017 00:01

jasonaden added action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews feature Issue that requests a new feature labels Nov 30, 2017

lazarljubenovic mentioned this pull request Dec 1, 2017

page not save its position #20733

Closed

jlbeard84 reviewed Dec 1, 2017

View reviewed changes

vsavkin force-pushed the router_scrolling branch from 3dcd424 to ab86195 Compare December 4, 2017 19:28

jasonaden approved these changes Dec 5, 2017

View reviewed changes

jasonaden requested a review from IgorMinar December 5, 2017 22:50

jasonaden added action: merge The PR is ready for merge by the caretaker target: major This PR is targeted for the next major release and removed action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews labels Dec 5, 2017

vsavkin force-pushed the router_scrolling branch from ab86195 to 68c660a Compare December 6, 2017 20:28

jasonaden force-pushed the router_scrolling branch from ac7f99b to 8ec1002 Compare June 6, 2018 23:33

IgorMinar approved these changes Jun 7, 2018

View reviewed changes

feat(router): implement scrolling restoration service

f0f9c04

For documentation, see `RouterModule.scrollPositionRestoration` Fixes angular#13636 angular#10929 angular#7791 angular#6595

jasonaden force-pushed the router_scrolling branch from 8ec1002 to f0f9c04 Compare June 7, 2018 22:09

fixup! feat(router): implement scrolling restoration service

1847ee7

jasonaden added action: merge The PR is ready for merge by the caretaker and removed action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews labels Jun 8, 2018

alfaproject reviewed Jun 8, 2018

View reviewed changes

mhevery closed this in 49c5234 Jun 8, 2018

brandonroberts mentioned this pull request Jun 8, 2018

Router: Provide support for handling the URL hash #6595

Closed

jasonaden mentioned this pull request Jun 19, 2018

RouterLinkActive scroll to visible #24404

Closed

sis0k0 mentioned this pull request Jun 22, 2018

[Tracking] Angular 6.1 NativeScript/nativescript-angular#1398

Closed

2 tasks

mhevery mentioned this pull request Jun 26, 2018

scrollPositionRestoration has several problems #24547

Closed

damienwebdev mentioned this pull request Jul 20, 2018

Feature: Sidenav or Route Scroll Service angular/components#4280

Open

karptonite mentioned this pull request Aug 17, 2018

Angular 6.1: page snaps to top when clicking router-link #25555

Closed

hoangviet1993 mentioned this pull request Sep 20, 2018

Clicking "Back" button should preserve relative position on page (as with other websites) oppia/foundation-website#64

Closed

jasonaden mentioned this pull request Apr 4, 2019

Anchor scrolling only works first time it is clicked and not on subsequent clicks. #29099

Closed

angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 14, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(router): implement scroll position restoration #20030

feat(router): implement scroll position restoration #20030

vsavkin commented Oct 30, 2017

EreckGordon commented Oct 30, 2017

gkalpak Oct 30, 2017

josephliccini Oct 30, 2017 •

edited

Loading

gkalpak Oct 30, 2017

zimme commented Oct 31, 2017

jasonaden left a comment

jasonaden Nov 7, 2017

vsavkin Nov 14, 2017

jasonaden Dec 5, 2017 •

edited

Loading

jasonaden Nov 7, 2017

jasonaden Nov 7, 2017

jasonaden Nov 7, 2017

vsavkin Nov 14, 2017

jsgoupil Nov 6, 2017

ErfanNaz Feb 13, 2018

jasonaden Nov 27, 2017

vsavkin Dec 4, 2017

jasonaden Dec 5, 2017

MRosenlind commented Nov 28, 2017

jlbeard84 Dec 1, 2017

jasonaden left a comment

IgorMinar commented Jun 7, 2018

alfaproject Jun 8, 2018

raysuelzer commented Jun 18, 2018

ericmartinezr commented Jun 18, 2018

sarunint commented Jun 18, 2018

jek-bao-choo commented Jun 25, 2018 •

edited

Loading

jek-bao-choo commented Jun 25, 2018

jek-bao-choo commented Jun 25, 2018

devalmehta commented Aug 9, 2018

MikeKovetsky commented Aug 9, 2018

Splaktar commented Oct 29, 2018

angular-automatic-lock-bot bot commented Sep 14, 2019

feat(router): implement scroll position restoration #20030

feat(router): implement scroll position restoration #20030

Conversation

vsavkin commented Oct 30, 2017

EreckGordon commented Oct 30, 2017

Choose a reason for hiding this comment

josephliccini Oct 30, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zimme commented Oct 31, 2017

jasonaden left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasonaden Dec 5, 2017 • edited Loading

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

MRosenlind commented Nov 28, 2017

Choose a reason for hiding this comment

jasonaden left a comment

Choose a reason for hiding this comment

IgorMinar commented Jun 7, 2018

Choose a reason for hiding this comment

raysuelzer commented Jun 18, 2018

ericmartinezr commented Jun 18, 2018

sarunint commented Jun 18, 2018

jek-bao-choo commented Jun 25, 2018 • edited Loading

jek-bao-choo commented Jun 25, 2018

jek-bao-choo commented Jun 25, 2018

devalmehta commented Aug 9, 2018

MikeKovetsky commented Aug 9, 2018

Splaktar commented Oct 29, 2018

angular-automatic-lock-bot bot commented Sep 14, 2019

josephliccini Oct 30, 2017 •

edited

Loading

jasonaden Dec 5, 2017 •

edited

Loading

jek-bao-choo commented Jun 25, 2018 •

edited

Loading