Skip to content

Commit d837bfc

Browse files
gkalpakpetebacondarwin
gkalpak
authored and
petebacondarwin
committed
fix(aio): fix scrolling to an element
Previously, the top-bar's height wasn't taken into account when scrolling an element into view. As a result, the element would be hidden behind the top-bar. Taking the top-bar height into account was not necessary before angular#17155, because the top-bar was not fixed (i.e. it scrolled away). This commit fixes the scrolling behavior by accounting for the top-bar's height when scrolling an element into view. (This partially reverts angular#17102.) Fixes angular#17219 Fixes angular#17226
1 parent 4759975 commit d837bfc

4 files changed

Lines changed: 106 additions & 12 deletions

File tree

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

Lines changed: 86 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@ import { DOCUMENT } from '@angular/platform-browser';
55
import { ScrollService, topMargin } from './scroll.service';
66

77
describe('ScrollService', () => {
8+
const topOfPageElem = {} as Element;
89
let injector: ReflectiveInjector;
910
let document: MockDocument;
1011
let location: MockPlatformLocation;
@@ -16,7 +17,8 @@ describe('ScrollService', () => {
1617

1718
class MockDocument {
1819
body = new MockElement();
19-
getElementById = jasmine.createSpy('Document getElementById');
20+
getElementById = jasmine.createSpy('Document getElementById').and.returnValue(topOfPageElem);
21+
querySelector = jasmine.createSpy('Document querySelector');
2022
}
2123

2224
class MockElement {
@@ -38,6 +40,69 @@ describe('ScrollService', () => {
3840
scrollService = injector.get(ScrollService);
3941
});
4042

43+
describe('#topOffset', () => {
44+
it('should query for the top-bar by CSS selector', () => {
45+
expect(document.querySelector).not.toHaveBeenCalled();
46+
47+
expect(scrollService.topOffset).toBe(topMargin);
48+
expect(document.querySelector).toHaveBeenCalled();
49+
});
50+
51+
it('should be calculated based on the top-bar\'s height + margin', () => {
52+
(document.querySelector as jasmine.Spy).and.returnValue({clientHeight: 50});
53+
expect(scrollService.topOffset).toBe(50 + topMargin);
54+
});
55+
56+
it('should only query for the top-bar once', () => {
57+
expect(scrollService.topOffset).toBe(topMargin);
58+
(document.querySelector as jasmine.Spy).calls.reset();
59+
60+
expect(scrollService.topOffset).toBe(topMargin);
61+
expect(document.querySelector).not.toHaveBeenCalled();
62+
});
63+
64+
it('should retrieve the top-bar\'s height again after resize', () => {
65+
let clientHeight = 50;
66+
(document.querySelector as jasmine.Spy).and.callFake(() => ({clientHeight}));
67+
68+
expect(scrollService.topOffset).toBe(50 + topMargin);
69+
expect(document.querySelector).toHaveBeenCalled();
70+
71+
(document.querySelector as jasmine.Spy).calls.reset();
72+
clientHeight = 100;
73+
74+
expect(scrollService.topOffset).toBe(50 + topMargin);
75+
expect(document.querySelector).not.toHaveBeenCalled();
76+
77+
window.dispatchEvent(new Event('resize'));
78+
79+
expect(scrollService.topOffset).toBe(100 + topMargin);
80+
expect(document.querySelector).toHaveBeenCalled();
81+
});
82+
});
83+
84+
describe('#topOfPageElement', () => {
85+
it('should query for the top-of-page element by ID', () => {
86+
expect(document.getElementById).not.toHaveBeenCalled();
87+
88+
expect(scrollService.topOfPageElement).toBe(topOfPageElem);
89+
expect(document.getElementById).toHaveBeenCalled();
90+
});
91+
92+
it('should only query for the top-of-page element once', () => {
93+
expect(scrollService.topOfPageElement).toBe(topOfPageElem);
94+
(document.getElementById as jasmine.Spy).calls.reset();
95+
96+
expect(scrollService.topOfPageElement).toBe(topOfPageElem);
97+
expect(document.getElementById).not.toHaveBeenCalled();
98+
});
99+
100+
it('should return `<body>` if unable to find the top-of-page element', () => {
101+
(document.getElementById as jasmine.Spy).and.returnValue(null);
102+
expect(scrollService.topOfPageElement).toBe(document.body as any);
103+
});
104+
});
105+
41106
describe('#scroll', () => {
42107
it('should scroll to the top if there is no hash', () => {
43108
location.hash = '';
@@ -73,10 +138,28 @@ describe('ScrollService', () => {
73138

74139
describe('#scrollToElement', () => {
75140
it('should scroll to element', () => {
76-
const element = <Element><any> new MockElement();
141+
const element: Element = new MockElement() as any;
77142
scrollService.scrollToElement(element);
78143
expect(element.scrollIntoView).toHaveBeenCalled();
79-
expect(window.scrollBy).toHaveBeenCalledWith(0, -topMargin);
144+
expect(window.scrollBy).toHaveBeenCalledWith(0, -scrollService.topOffset);
145+
});
146+
147+
it('should scroll all the way to the top if close enough', () => {
148+
const element: Element = new MockElement() as any;
149+
150+
(window as any).pageYOffset = 25;
151+
scrollService.scrollToElement(element);
152+
153+
expect(element.scrollIntoView).toHaveBeenCalled();
154+
expect(window.scrollBy).toHaveBeenCalledWith(0, -scrollService.topOffset);
155+
(window.scrollBy as jasmine.Spy).calls.reset();
156+
157+
(window as any).pageYOffset = 15;
158+
scrollService.scrollToElement(element);
159+
160+
expect(element.scrollIntoView).toHaveBeenCalled();
161+
expect(window.scrollBy).toHaveBeenCalledWith(0, -scrollService.topOffset);
162+
expect(window.scrollBy).toHaveBeenCalledWith(0, -15);
80163
});
81164

82165
it('should do nothing if no element', () => {

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

Lines changed: 17 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
import { Injectable, Inject } from '@angular/core';
22
import { PlatformLocation } from '@angular/common';
33
import { DOCUMENT } from '@angular/platform-browser';
4+
import {fromEvent} from 'rxjs/observable/fromEvent';
45

56
export const topMargin = 16;
67
/**
@@ -9,20 +10,20 @@ export const topMargin = 16;
910
@Injectable()
1011
export class ScrollService {
1112

12-
private _topOffset: number;
13+
private _topOffset: number | null;
1314
private _topOfPageElement: Element;
1415

1516
// Offset from the top of the document to bottom of any static elements
1617
// at the top (e.g. toolbar) + some margin
1718
get topOffset() {
1819
if (!this._topOffset) {
19-
// Since the toolbar is not static, we don't need to account for its height.
20-
this._topOffset = topMargin;
20+
const toolbar = this.document.querySelector('md-toolbar.app-toolbar');
21+
this._topOffset = (toolbar && toolbar.clientHeight || 0) + topMargin;
2122
}
2223
return this._topOffset;
2324
}
2425

25-
private get topOfPageElement() {
26+
get topOfPageElement() {
2627
if (!this._topOfPageElement) {
2728
this._topOfPageElement = this.document.getElementById('top-of-page') || this.document.body;
2829
}
@@ -31,7 +32,10 @@ export class ScrollService {
3132

3233
constructor(
3334
@Inject(DOCUMENT) private document: any,
34-
private location: PlatformLocation) { }
35+
private location: PlatformLocation) {
36+
// On resize, the toolbar might change height, so "invalidate" the top offset.
37+
fromEvent(window, 'resize').subscribe(() => this._topOffset = null);
38+
}
3539

3640
/**
3741
* Scroll to the element with id extracted from the current location hash fragment.
@@ -53,7 +57,14 @@ export class ScrollService {
5357
scrollToElement(element: Element) {
5458
if (element) {
5559
element.scrollIntoView();
56-
if (window && window.scrollBy) { window.scrollBy(0, -this.topOffset); }
60+
if (window && window.scrollBy) {
61+
window.scrollBy(0, -this.topOffset);
62+
if (window.pageYOffset < 20) {
63+
// If we are very close to the top (<20px), then scroll all the way up.
64+
// (This can happen if `element` is at the top of the page, but has a small top-margin.)
65+
window.scrollBy(0, -window.pageYOffset);
66+
}
67+
}
5768
}
5869
}
5970

‎aio/src/styles/0-base/_typography.scss‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,7 @@ body {
88
}
99

1010
h1 {
11-
display:inline-block;
11+
display: inline-block;
1212
font-size: 24px;
1313
font-weight: 500;
1414
margin: 8px 0px;
@@ -158,4 +158,4 @@ code {
158158
&:hover {
159159
color: $mediumgray;
160160
}
161-
}
161+
}

‎aio/src/styles/1-layouts/_content-layout.scss‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,7 @@ aio-shell.page-docs {
77

88
.sidenav-content {
99
min-height: 100vh;
10-
padding: 6rem 3rem 1rem;
10+
padding: 80px 3rem 1rem;
1111
}
1212

1313
@media (max-width: 600px) {

0 commit comments

Comments
 (0)