diff --git a/aio/src/app/shared/scroll.service.spec.ts b/aio/src/app/shared/scroll.service.spec.ts index 643136aa8a..078bad131c 100644 --- a/aio/src/app/shared/scroll.service.spec.ts +++ b/aio/src/app/shared/scroll.service.spec.ts @@ -22,6 +22,8 @@ describe('ScrollService', () => { } class MockElement { + getBoundingClientRect = jasmine.createSpy('Element getBoundingClientRect') + .and.returnValue({top: 0}); scrollIntoView = jasmine.createSpy('Element scrollIntoView'); } @@ -144,6 +146,22 @@ describe('ScrollService', () => { expect(window.scrollBy).toHaveBeenCalledWith(0, -scrollService.topOffset); }); + it('should not scroll more than necessary (e.g. for elements close to the bottom)', () => { + const element: Element = new MockElement() as any; + const getBoundingClientRect = element.getBoundingClientRect as jasmine.Spy; + const topOffset = scrollService.topOffset; + + getBoundingClientRect.and.returnValue({top: topOffset + 100}); + scrollService.scrollToElement(element); + expect(element.scrollIntoView).toHaveBeenCalledTimes(1); + expect(window.scrollBy).toHaveBeenCalledWith(0, 100); + + getBoundingClientRect.and.returnValue({top: topOffset - 10}); + scrollService.scrollToElement(element); + expect(element.scrollIntoView).toHaveBeenCalledTimes(2); + expect(window.scrollBy).toHaveBeenCalledWith(0, -10); + }); + it('should scroll all the way to the top if close enough', () => { const element: Element = new MockElement() as any; diff --git a/aio/src/app/shared/scroll.service.ts b/aio/src/app/shared/scroll.service.ts index 0d51b7c3d4..b6dba4dbed 100644 --- a/aio/src/app/shared/scroll.service.ts +++ b/aio/src/app/shared/scroll.service.ts @@ -57,11 +57,17 @@ export class ScrollService { scrollToElement(element: Element) { if (element) { element.scrollIntoView(); + if (window && window.scrollBy) { - window.scrollBy(0, -this.topOffset); + // Scroll as much as necessary to align the top of `element` at `topOffset`. + // (Usually, `.top` will be 0, except for cases where the element cannot be scrolled all the + // way to the top, because the viewport is larger than the height of the content after the + // element.) + window.scrollBy(0, element.getBoundingClientRect().top - this.topOffset); + + // If we are very close to the top (<20px), then scroll all the way up. + // (This can happen if `element` is at the top of the page, but has a small top-margin.) if (window.pageYOffset < 20) { - // If we are very close to the top (<20px), then scroll all the way up. - // (This can happen if `element` is at the top of the page, but has a small top-margin.) window.scrollBy(0, -window.pageYOffset); } }