feat(docs-infra): saves the scroll position before the change of loca… #28037

wKoza · 2019-01-10T07:30:27Z

…tion or after a reload

closes #27916

PR Checklist

Please check if your PR fulfills the following requirements:

The commit message follows our guidelines: https://github.com/angular/angular/blob/master/CONTRIBUTING.md#commit
Tests for the changes have been added (for bug fixes / features)
Docs have been added / updated (for bug fixes / features)

PR Type

What kind of change does this PR introduce?

Bugfix
Feature
Code style update (formatting, local variables)
Refactoring (no functional changes, no api changes)
Build related changes
CI related changes
Documentation content changes
angular.io application / infrastructure changes
Other... Please describe:

What is the current behavior?

Issue Number: N/A

What is the new behavior?

Does this PR introduce a breaking change?

Yes
No

Other information

petebacondarwin · 2019-01-10T08:30:06Z

Fixes #17308

googlebot · 2019-01-10T15:40:25Z

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again. If the bot doesn't comment, it means it doesn't think anything has changed.

wKoza · 2019-01-10T15:45:07Z

Hi @petebacondarwin ,
What do you think about this kind of implementation / behavior ? I'm agree with @IgorMinar, use history API is more usual but It's currently more tricky because of the current mechanical of displaying.

mary-poppins · 2019-01-10T15:54:02Z

You can preview 4ecf896 at https://pr28037-4ecf896.ngbuilds.io/.
You can preview 9ac71d8 at https://pr28037-9ac71d8.ngbuilds.io/.

petebacondarwin · 2019-01-10T15:56:08Z

@wKoza - it seems that your second commit came from a different account to the first commit, and does not have CLA approval. Could you squash the commits into one committed by the appropriate account?
I have triggered the AIO preview so we can see this in action, before I give an opinion on the appoach :-)

petebacondarwin · 2019-01-10T16:07:35Z

One problem I note immediately in the preview is that this is not working when you use the forward button to navigate.

googlebot · 2019-01-10T16:35:17Z

CLAs look good, thanks!

wKoza · 2019-01-10T16:51:08Z

It's Ok with the CLAs. Can you give me an example of reproduction steps ?

petebacondarwin · 2019-01-10T17:00:05Z

Navigate to any guide, scroll down
Navigate to another guide, scroll down
Navigate to a third guide, scroll down
Click back button: the second guide is scrolled correctly
Click back button again: the first guide is scrolled correctly
Click the forward button: the second guide is displayed but not scrolled.

googlebot · 2019-01-11T11:03:10Z

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again. If the bot doesn't comment, it means it doesn't think anything has changed.

googlebot · 2019-01-11T11:07:10Z

CLAs look good, thanks!

petebacondarwin · 2019-01-21T14:49:16Z

It was a flake! So if you don't mind squashing these commits (as you see fit) and then we'll double-check with @IgorMinar before merging. Great work!

…tion Issue angular#27916, angular#17308

mary-poppins · 2019-01-21T14:59:41Z

You can preview a9d0785 at https://pr28037-a9d0785.ngbuilds.io/.

aio/src/app/shared/scroll.service.ts

gkalpak

Sorry to crash the party, but I was passing by and thought I'd add some comments 😁
The implementation seems a little complicated (I am not even 100% sure how it works 😁), but the end result looks correct ✔️
Nice one, @wKoza 👍

gkalpak · 2019-01-21T11:09:47Z

aio/src/app/app.component.ts

-    // The previous document has been removed.
-    // Scroll to top to restore a clean visual state for the new document.
-    this.scrollService.scrollToTop();
+    this.scrollService.removeStoredScrollPosition();


Why remove the comment from here? I think this is the right place for it.

The name of the method is onDocRemoved. I think the first line is redundant.
The second line have to be removed. We stop to scrollToTop() when the Doc is removed.

aio/src/app/app.component.ts

aio/src/app/shared/scroll.service.ts

gkalpak · 2019-01-21T15:59:03Z

aio/src/app/shared/scroll.service.ts

+      this.location.subscribe(state => {
+        if (state.type === 'hashchange') {
+          this.popStateFired = false;
+          this.scrollToPosition();


Why do we need to call scrolToPosition() here? How was this handled before?

Before, we called autoscroll() if I recall

Do you mean here? If so, why do we need both calls?
More importantly, why do we need to scroll to this.scrollPosition, whent the hash changes (and not scroll to the target element or to the top)?

After, when a location had a hash, we called autoscroll() to scroll to the target element. Now, we can go back and return to a location which has a hash BUT, we can be to a another position (we have scrolled after have been to target element (anchor)). It's clear ?

But doesn't hashchange mean we are still on the same page (and only the hash has changed)? This path shouldn't be hit when we go back, right?

Yeah, but wouldn't the scrolling be taken care of this line in that case?

Yes ,This line is called at the last step but it scrolls to the top since there is no more hash. But I'm agree, sometimes, this line is called for nothing.

I don't understand in what circumstances this could have an effect.
AFAICT, this line is enough for scrolling to the top. It seems to me that one of the two has to go. Am I missing something?

this line provides the scroll to hash (step 3). This line is not called.
And this line provides the scroll to the link (step 7).
If you remove one of this lines , either the step3 or 7 will fail

I see. I played with this locally and the emitted events are very different from what I expected.
Let's leave it as is for now (since it seems to work fine). We can always tweak it later is needed 😉

aio/src/app/shared/scroll.service.ts

gkalpak · 2019-01-21T16:11:15Z

aio/src/app/shared/scroll.service.ts

+        } else {
+          this.popStateFired = true;
+          // we always should have a scrollPosition in our state history
+          this.scrollPosition = state.state ? state.state['scrollPosition'] : null;


It is an edge-case, but I believe we should store the new scrollPosition to sessionStorage. As it is now, if the user reloads before the 500ms delay, they will end up to the previous page's scroll position.

In this case, we doesn't have a delay. You can see this line

The scrolling is synchronous in scrollAfterDelay() (which is not a great name for a method that scrolls synchronously 😛), but calling scrollAfterDelay() is not synchronous wrt this line here. I was able to reproduce this by clicking the back button and reloading quickly.

Your comment is outofdate. The name is now scrollAfterRender() 😇.
I 'm not able to reproduce this. For me, It would mean rather that the event onDocRemoved was been fired .

aio/src/app/shared/scroll.service.ts

wKoza · 2019-01-21T20:22:19Z

Sorry to keep pushing back on this. It is a very good PR!

I just wondered, why are you using session storage rather than history to store the current page position? Is that because unless you move elsewhere there is no item in the history?

Regarding the jank when loading - can we not work out that we are reloading rather than navigating to a new page via a URL, and then avoid the setTimeout(..., scrollDelay)?

@petebacondarwin , finally, the setTimeOut here seems to be unnecessary. I'll remove it.

petebacondarwin · 2019-01-21T20:27:38Z

OK, great!

…of location

mary-poppins · 2019-01-21T21:10:23Z

You can preview 09b3d1f at https://pr28037-09b3d1f.ngbuilds.io/.

aio/src/app/shared/scroll.service.ts

gkalpak · 2019-01-22T20:04:32Z

aio/src/app/app.component.ts

@@ -348,6 +342,9 @@ export class AppComponent implements OnInit {
  // Dynamically change height of table of contents container
  @HostListener('window:scroll')
  onScroll() {
+
+    this.scrollService.updateScrollPositionInHistory();


Since updateScrollPositionInHistory() isn't trivial, it might be a good idea to do some debouncing here. WDYT?

I had asked me the question when I wanted to use the scroll event. For the moment, I did not feel any lag. But, it is an option.

On Chrome (desktop), after some scrolling, I start to get the following warnings in the console:

Throttling history state changes to prevent the browser from hanging.

So, I believe it is a good idea to do some throttling/debouncing here.

aio/src/app/shared/scroll.service.ts

gkalpak · 2019-01-22T20:08:28Z

aio/src/app/shared/scroll.service.ts

+      this.location.subscribe(state => {
+        if (state.type === 'hashchange') {
+          this.popStateFired = false;
+          this.scrollToPosition();


Do you mean here? If so, why do we need both calls?
More importantly, why do we need to scroll to this.scrollPosition, whent the hash changes (and not scroll to the target element or to the top)?

aio/src/app/app.component.ts

wKoza · 2019-01-23T08:21:01Z

@gkalpak, I've covered all of your questions. Notify me before my (last?) commit ;)

gkalpak · 2019-01-23T21:01:39Z

@wKoza, I think you are good to go for that (last?) commit 💪

…of location

mary-poppins · 2019-01-23T22:41:17Z

You can preview bf6bc41 at https://pr28037-bf6bc41.ngbuilds.io/.

gkalpak · 2019-01-24T09:42:44Z

aio/src/app/shared/scroll.service.ts

+
+    try {
+      this.supportManualScrollRestoration = !!window && !!window.scrollTo && 'scrollX' in window
+        && 'scrollY' in window && !!history && !!history.scrollRestoration;


OOC, under what circumstances do we expect this line to throw?

This code come from here. So, maybe window.scrollTo might throw an error or is "just in case". @vsavkin can maybe explain its choice.

It seems redundant (unless there are environments where window is defined and has a scrollTo getter, but trying to access window.scrollTo throws an error.

_{(Not that I haven't seen such strange limitations in certain environments 😉 angular/angular.js#13945)}

gkalpak

Awesome work, @wKoza 💯
Thx so much ❤️ ❤️ ❤️

gkalpak · 2019-01-24T10:17:13Z

BTW, I have created #28332 to track possible improvements to scroll position restoration (but nothing that should block this PR) 😉

Comments have been addressed.

…tion (#28037) Issue #27916, #17308 PR Close #28037

petebacondarwin · 2019-01-24T18:43:11Z

🎉

…tion (angular#28037) Issue angular#27916, angular#17308 PR Close angular#28037

angular-automatic-lock-bot · 2019-09-14T14:53:02Z

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

wKoza requested a review from a team as a code owner January 10, 2019 07:30

googlebot added the cla: yes label Jan 10, 2019

petebacondarwin added this to the docs-infra-aio-app milestone Jan 10, 2019

ngbot bot removed this from the docs-infra-aio-app milestone Jan 10, 2019

petebacondarwin added this to the docs-infra-aio-app milestone Jan 10, 2019

ngbot bot removed this from the docs-infra-aio-app milestone Jan 10, 2019

googlebot added cla: no and removed cla: yes labels Jan 10, 2019

petebacondarwin added the aio: preview label Jan 10, 2019

petebacondarwin added this to the docs-infra-aio-app milestone Jan 10, 2019

ngbot bot removed this from the docs-infra-aio-app milestone Jan 10, 2019

wKoza force-pushed the save_scroll_position_during_change_location branch from 9ac71d8 to 4ecf896 Compare January 10, 2019 16:35

googlebot added cla: yes and removed cla: no labels Jan 10, 2019

googlebot added cla: no and removed cla: yes labels Jan 11, 2019

wKoza force-pushed the save_scroll_position_during_change_location branch from b9407ca to f10e0d8 Compare January 11, 2019 11:07

googlebot added cla: yes and removed cla: no labels Jan 11, 2019

petebacondarwin approved these changes Jan 21, 2019

View reviewed changes

feat(docs-infra): saves the scroll position before the change of loca…

a9d0785

…tion Issue angular#27916, angular#17308

wKoza force-pushed the save_scroll_position_during_change_location branch from 5bcda90 to a9d0785 Compare January 21, 2019 14:54

petebacondarwin reviewed Jan 21, 2019
8000
View reviewed changes

aio/src/app/shared/scroll.service.ts Outdated Show resolved Hide resolved

petebacondarwin reviewed Jan 21, 2019

View reviewed changes

aio/src/app/shared/scroll.service.ts Outdated Show resolved Hide resolved

gkalpak reviewed Jan 21, 2019

View reviewed changes

fixup! feat(docs-infra): saves the scroll position before the change …

09b3d1f

…of location

gkalpak reviewed Jan 22, 2019

View reviewed changes

fixup! feat(docs-infra): saves the scroll position before the change …

bf6bc41

…of location

gkalpak reviewed Jan 24, 2019

View reviewed changes

gkalpak mentioned this pull request Jan 24, 2019

docs-infra: scroll position restoration improvements #28332

Closed

4 tasks

gkalpak approved these changes Jan 24, 2019

View reviewed changes

gkalpak 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 Jan 24, 2019

jasonaden pushed a commit that referenced this pull request Jan 24, 2019

feat(docs-infra): saves the scroll position before the change of loca…

cb9fd9b

…tion (#28037) Issue #27916, #17308 PR Close #28037

jasonaden closed this in 3414316 Jan 24, 2019

vetom pushed a commit to vetom/angular that referenced this pull request Jan 31, 2019

feat(docs-infra): saves the scroll position before the change of loca…

ce5a111

…tion (angular#28037) Issue angular#27916, angular#17308 PR Close angular#28037

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

feat(docs-infra): saves the scroll position before the change of loca… #28037

feat(docs-infra): saves the scroll position before the change of loca… #28037

Uh oh!

Conversation

Uh oh!

PR Checklist

PR Type

What is the current behavior?

What is the new behavior?

Does this PR introduce a breaking change?

Other information

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!