10000 feat(docs-infra): saves the scroll position before the change of loca… by wKoza · Pull Request #28037 · angular/angular · GitHub
[go: up one dir, main page]
Skip to content

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

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.

Sign up for GitHub

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

Closed
wants to merge 3 commits into from
Closed

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
a9d0785
feat(docs-infra): saves the scroll position before the change of loca…
wKoza Jan 10, 2019
09b3d1f
fixup! feat(docs-infra): saves the scroll position before the change …
wKoza Jan 21, 2019
bf6bc41
fixup! feat(docs-infra): saves the scroll position before the change …
wKoza Jan 23, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
fixup! feat(docs-infra): saves the scroll position before the change … 
…of location
  • Loading branch information
@wKoza
wKoza committed Jan 21, 2019
commit 09b3d1f4113ea559f95b1f38c43078df066e9084
9 changes: 2 additions & 7 deletions aio/src/app/app.component.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -127,7 +127,7 @@ export class AppComponent implements OnInit {
}
if (path === this.currentPath) {
// scroll only if on same page (most likely a change to the hash)
this.autoScroll();
this.scrollService.scroll();
} else {
// don't scroll; leave that to `onDocRendered`
this.currentPath = path;
Expand Down Expand Up @@ -187,11 +187,6 @@ export class AppComponent implements OnInit {
.subscribe(() => this.updateShell());
}

// Scroll to the anchor in the hash fragment or top of doc.
autoScroll() {
this.scrollService.scroll();
}

onDocReady() {
// About to transition to new view.
this.isTransitioning = true;
Expand Down Expand Up @@ -253,7 +248,7 @@ export class AppComponent implements OnInit {

@HostListener('click', ['$event.target', '$event.button', '$event.ctrlKey', '$event.metaKey', '$event.altKey'])
onClick(eventTarget: HTMLElement, button: number, ctrlKey: boolean, metaKey: boolean, altKey: boolean): boolean {

// We update the scroll position in the case there is no scroll event on the page before
this.scrollService.updateScrollPositionInHistory();

// Hide the search results if we clicked outside both the "search box" and the "search results"
Expand Down
10 changes: 4 additions & 6 deletions aio/src/app/shared/scroll.service.spec.ts
View file 8000
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ describe('ScrollService', () => {
});

it('should set `scrollRestoration` to `manual` if supported', () => {
if (scrollService.supportManualScrollRestoration()) {
if (scrollService.supportManualScrollRestoration) {
expect(window.history.scrollRestoration).toBe('manual');
} else {
expect(window.history.scrollRestoration).toBeUndefined();
Expand Down Expand Up @@ -246,7 +246,7 @@ describe('ScrollService', () => {
it('should return true when popState event was fired after a back navigation if the browser supports ' +
'scrollRestoration`. Otherwise, needToFixScrollPosition() returns false', () => {

if (scrollService.supportManualScrollRestoration()) {
if (scrollService.supportManualScrollRestoration) {
location.go('/initial-url1');
// We simulate a scroll down
location.replaceState('/initial-url1', 'hack', {scrollPosition: [2000, 0]});
Expand All @@ -271,7 +271,7 @@ describe('ScrollService', () => {
it('should return true when popState event was fired after a forward navigation if the browser supports ' +
'scrollRestoration`. Otherwise, needToFixScrollPosition() returns false', () => {

if (scrollService.supportManualScrollRestoration()) {
if (scrollService.supportManualScrollRestoration) {
location.go('/initial-url1');
location.go('/initial-url2');
// We simulate a scroll down
Expand Down Expand Up @@ -348,10 +348,8 @@ describe('ScrollService', () => {

scrollService.scrollAfterRender(scrollDelay);

expect(viewportScrollerStub.scrollToPosition).not.toHaveBeenCalled();
expect(getStoredScrollPositionSpy).toHaveBeenCalled();
tick(scrollDelay);
expect(viewportScrollerStub.scrollToPosition).toHaveBeenCalled();
expect(getStoredScrollPositionSpy).toHaveBeenCalled();
}));

it('should call `scrollToPosition` after a popState', fakeAsync(() => {
Expand Down
52 changes: 29 additions & 23 deletions aio/src/app/shared/scroll.service.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,8 @@ export class ScrollService {
popStateFired = false;
// scroll position which has to be restored after the popState event
scrollPosition: [number, number] = [0, 0];
// true when the browser supports `scrollTo`, `scrollX`, `scrollY` and `scrollRestoration`
supportManualScrollRestoration: boolean;

// Offset from the top of the document to bottom of any static elements
// at the top (e.g. toolbar) + some margin
Expand All @@ -43,18 +45,29 @@ export class ScrollService {
// On resize, the toolbar might change height, so "invalidate" the top offset.
fromEvent(window, 'resize').subscribe(() => this._topOffset = null);

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

} catch {
this.supportManualScrollRestoration = false;
}

// Change scroll restoration strategy to `manual` if it's supported
if (this.supportManualScrollRestoration()) {
if (this.supportManualScrollRestoration) {
history.scrollRestoration = 'manual';
// we have to detect forward and back navigation
this.location.subscribe(state => {
if (state.type === 'hashchange') {
// we have to detect forward and back navigation thanks to popState event
this.location.subscribe(event => {
// the type is `hashchange` when the fragment identifier of the URL has changed. It allows us to go to position
// just before a click on an anchor
if (event.type === 'hashchange') {
this.popStateFired = false;
this.scrollToPosition();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Before, we called autoscroll() if I recall

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author
@wKoza wKoza Jan 23, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 😉

} else {
// The popstate event is always triggered by doing a browser action such as a click on the back or forward button.
// It can be follow by a event of type `hashchange`.
this.popStateFired = true;
// we always should have a scrollPosition in our state history
this.scrollPosition = state.state ? state.state['scrollPosition'] : null;
this.scrollPosition = event.state ? event.state['scrollPosition'] : null;
}
});
}
Expand Down Expand Up @@ -82,23 +95,27 @@ export class ScrollService {

/**
* When we load a document, we have to scroll to the correct position depending on whether this is a new location
* or a back/forward in the history
* , a back/forward in the history, or a refresh
* @param delay before we scroll to the good position
*/
scrollAfterRender(delay: number) {
if (this.getStoredScrollPosition()) {
setTimeout(() => this.viewportScroller.scrollToPosition(this.getStoredScrollPosition() !), delay);
// If we do rendering following a refresh, we use the scroll Position from the storage.
if (this.getStoredScrollPosition()) {
this.viewportScroller.scrollToPosition(this.getStoredScrollPosition() !);
} else {
if (!this.needToFixScrollPosition()) {
// The document was reloaded following a link. If the location contains a hash, we have to wait for async
// layout.
if (this.isLocationWithHash()) {
// Scroll 500ms after the new document has been inserted into the doc-viewer.
// The delay is to allow time for async layout to complete.
setTimeout(() => this.scroll(), delay);
} else {
// If the location doesn't contain a hash, we scroll to the top of the page.
this.scrollToTop();
}
} else {
// The document was reloaded following a popState` event`, so we manage the scroll scrollPosition
// The document was reloaded following a popState `event`, so we manage the scroll scrollPosition
this.scrollToPosition();
}
}
Expand Down Expand Up @@ -142,7 +159,7 @@ export class ScrollService {
* Update the state with scroll position into history.
*/
updateScrollPositionInHistory() {
if (this.supportManualScrollRestoration()) {
if (this.supportManualScrollRestoration) {
const currentScrollPosition = this.viewportScroller.getScrollPosition();
th 8000 is.location.replaceState(this.location.path(true), undefined, {scrollPosition: currentScrollPosition});
window.sessionStorage.setItem('scrollPosition', currentScrollPosition.toString());
Expand All @@ -159,21 +176,10 @@ export class ScrollService {
}

/**
* Check if the browser support `scrollTo` and `scrollRestoration`
*/
supportManualScrollRestoration(): boolean {
try {
return !!window && !!window.scrollTo && 'scrollX' in window && 'scrollY' in window && !!history && !!history.scrollRestoration;
} catch {
return false;
}
}

/**
* Check if the scroll positon need to manually fix after popState event
* Check if the scroll position need to be manually fixed after popState event
*/
needToFixScrollPosition(): boolean {
return this.popStateFired && this.scrollPosition && this.supportManualScrollRestoration()
return this.popStateFired && this.scrollPosition && this.supportManualScrollRestoration;
}

/**
Expand Down
0