From 709a3f6de7a5a749f61f199e78a61a5e3b792c3f Mon Sep 17 00:00:00 2001
From: Georgios Kalpakas <kalpakas.g@gmail.com>
Date: Tue, 13 Jun 2017 13:14:58 +0300
Subject: [PATCH] fix(aio): fix scrolling to elements near the bottom of the
 page

Previously, we always assumed that elements would be scrolled to the top of the
page, when calling `element.scrollIntoView()`. This is not true for elements
that cannot be scrolled to the top, e.g. when the viewport height is larger than
the height of the content after the element (common for small sections near the
end of the page).
In such cases, we would unnecessarily scroll up to account for the static
toolbar, which was unnecessary (since the element was not behind the toolbar
anyway) and caused ScrollSpy to fail to identify the scrolled-to section as
active.

This commit fixes it by ensuring that we do not scroll more than necessary in
order to align the top of the element with the bottom of the toolbar.

Fixes #17452
---
 aio/src/app/shared/scroll.service.spec.ts | 18 ++++++++++++++++++
 aio/src/app/shared/scroll.service.ts      | 12 +++++++++---
 2 files changed, 27 insertions(+), 3 deletions(-)

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);
         }
       }