Stop scroll misbehavior when linking to comments

Stop trying to scroll to the comment if it goes offscreen.
This usually happens if you want to scroll to the description.
There is supposed to be an offset of the scroll position but
it does not work very well.

Changelog: fixed

app/assets/javascripts/lib/utils/resize_observer.js

@@ -12,6 +12,7 @@ export function createResizeObserver() {
 /**
  * Watches for change in size of a container element (e.g. for lazy-loaded images)
  * and scrolls the target note to the top of the content area.
+ * Stops watching if the target element is scrolled out of viewport
  *
  * @param {Object} options
  * @param {string} options.targetId - id of element to scroll to
@@ -38,16 +39,21 @@ export function scrollToTargetOnResize({
   // can't tell difference between user and el.scrollTo, so use a flag
   let skipProgrammaticScrollEvent = false;
 
+  let intersectionObserver = null;
+  let targetElement = null;
+  let contentTopValue = contentTop();
+
   const containerEl = document.querySelector(container);
   const ro = createResizeObserver();
 
   function handleScroll() {
     if (skipProgrammaticScrollEvent) {
+      contentTopValue = contentTop();
       skipProgrammaticScrollEvent = false;
       return;
     }
     currentScrollPosition = scrollContainerIsDocument ? window.scrollY : scrollContainer.scrollTop;
-    userScrollOffset = currentScrollPosition - targetTop;
+    userScrollOffset = currentScrollPosition - targetTop - contentTopValue;
   }
 
   function addScrollListener() {
@@ -67,16 +73,39 @@ export function scrollToTargetOnResize({
     }
   }
 
+  function setupIntersectionObserver() {
+    intersectionObserver = new IntersectionObserver(
+      (entries) => {
+        const [entry] = entries;
+
+        // if element gets scrolled off screen then remove listeners
+        if (!entry.isIntersecting) {
+          // eslint-disable-next-line no-use-before-define
+          cleanup();
+        }
+      },
+      {
+        root: scrollContainerIsDocument ? null : scrollContainer,
+      },
+    );
+
+    intersectionObserver.observe(targetElement);
+  }
+
   function keepTargetAtTop() {
     if (document.activeElement !== document.body) return;
 
     const anchorEl = document.getElementById(targetId);
-    if (!anchorEl) return;
+    if (!anchorEl) {
+      return;
+    }
 
     scrollContainer = ScrollParent(document.getElementById(targetId)) || document.documentElement;
     scrollContainerIsDocument = scrollContainer === document.documentElement;
 
-    if (!scrollContainer) return;
+    if (!scrollContainer) {
+      return;
+    }
 
     skipProgrammaticScrollEvent = true;
 
@@ -86,7 +115,7 @@ export function scrollToTargetOnResize({
     // Add scrollPosition as getBoundingClientRect is relative to viewport
     // Add the accumulated scroll offset to maintain relative position
     // subtract contentTop so it goes below sticky headers, rather than top of viewport
-    targetTop = anchorTop - contentTop() + currentScrollPosition + userScrollOffset;
+    targetTop = anchorTop - contentTopValue + currentScrollPosition + userScrollOffset;
 
     scrollContainer.scrollTo({
       top: targetTop,
@@ -97,18 +126,26 @@ export function scrollToTargetOnResize({
       addScrollListener();
       scrollListenerEnabled = true;
     }
+
+    if (!intersectionObserver) {
+      targetElement = anchorEl;
+      setupIntersectionObserver();
+    }
+  }
+
+  function cleanup() {
+    ro.unobserve(containerEl);
+    containerEl.removeEventListener('ResizeUpdate', keepTargetAtTop);
+    removeScrollListener();
+
+    if (intersectionObserver) {
+      intersectionObserver.unobserve(targetElement);
+      intersectionObserver.disconnect();
+    }
   }
 
   containerEl.addEventListener('ResizeUpdate', keepTargetAtTop);
   ro.observe(containerEl);
 
-  return function cleanup() {
-    // add a slight delay to this to allow for a final scroll to the
-    // element once notes have finished
-    setTimeout(() => {
-      ro.unobserve(containerEl);
-      containerEl.removeEventListener('ResizeUpdate', keepTargetAtTop);
-      removeScrollListener();
-    }, 100);
-  };
+  return cleanup;
 }

spec/frontend/lib/utils/resize_observer_spec.js

@@ -10,13 +10,14 @@ function mockStickyHeaderSize(val) {
 
 describe('ResizeObserver Utility', () => {
   let cleanup;
+  const mockHeaderSize = 90;
   const triggerResize = () => {
     const entry = document.querySelector('#content-body');
     entry.dispatchEvent(new CustomEvent(`ResizeUpdate`, { detail: { entry } }));
   };
 
   beforeEach(() => {
-    mockStickyHeaderSize(90);
+    mockStickyHeaderSize(mockHeaderSize);
 
     jest.spyOn(document.documentElement, 'scrollTo');
 
@@ -34,83 +35,114 @@ describe('ResizeObserver Utility', () => {
     resetHTMLFixture();
   });
 
-  describe('Observer behavior', () => {
-    it('returns null for empty target', () => {
-      cleanup = scrollToTargetOnResize({
-        targetId: '',
-        container: '#content-body',
-      });
+  it('returns null for empty target', () => {
+    cleanup = scrollToTargetOnResize({
+      targetId: '',
+      container: '#content-body',
+    });
 
-      expect(cleanup).toBe(null);
+    expect(cleanup).toBe(null);
+  });
+
+  it('does not scroll if target does not exist', () => {
+    scrollToTargetOnResize({
+      targetId: 'some_imaginary_id',
+      container: '#content-body',
     });
 
-    it('does not scroll if target does not exist', () => {
-      scrollToTargetOnResize({
-        targetId: 'some_imaginary_id',
+    triggerResize();
+
+    expect(document.documentElement.scrollTo).not.toHaveBeenCalled();
+  });
+
+  describe('with existing target', () => {
+    const topHeight = 110;
+    const scrollAmount = 160;
+
+    beforeEach(() => {
+      cleanup = scrollToTargetOnResize({
+        targetId: 'note_1234',
         container: '#content-body',
       });
+    });
+
+    it('returns cleanup function', () => {
+      cleanup();
 
       triggerResize();
 
       expect(document.documentElement.scrollTo).not.toHaveBeenCalled();
     });
 
-    describe('with existing target', () => {
-      const cleanupTimeoutMs = 100;
-      const topHeight = 110;
-      const scrollAmount = 160;
+    it('scrolls body so anchor is just below sticky header (contentTop)', () => {
+      triggerResize();
 
-      beforeEach(() => {
-        cleanup = scrollToTargetOnResize({
-          targetId: 'note_1234',
-          container: '#content-body',
-        });
+      expect(document.documentElement.scrollTo).toHaveBeenCalledWith({
+        behavior: 'instant',
+        top: topHeight,
       });
+    });
 
-      it('returns cleanup function', () => {
-        cleanup();
+    it('maintains scroll position relative to anchor after user scroll', () => {
+      // Initial scroll to anchor
+      triggerResize();
 
-        jest.advanceTimersByTime(cleanupTimeoutMs);
+      // Simulate user scrolling down
+      window.scrollY = scrollAmount;
+      window.dispatchEvent(new Event('scroll'));
 
-        triggerResize();
+      // Trigger resize again
+      triggerResize();
 
-        expect(document.documentElement.scrollTo).not.toHaveBeenCalled();
+      // Should maintain the 50px offset from original position
+      expect(document.documentElement.scrollTo).toHaveBeenCalledWith({
+        top: topHeight + scrollAmount,
+        behavior: 'instant',
       });
+    });
 
-      it('scrolls body so anchor is just below sticky header (contentTop)', () => {
-        triggerResize();
+    it('does not scroll if another element is focused', () => {
+      const anchorEl = document.getElementById('reply-field');
+      anchorEl.focus();
 
-        expect(document.documentElement.scrollTo).toHaveBeenCalledWith({
-          behavior: 'instant',
-          top: topHeight,
-        });
-      });
+      triggerResize();
 
-      it('maintains scroll position relative to anchor after user scroll', () => {
-        // Initial scroll to anchor
-        triggerResize();
+      expect(document.documentElement.scrollTo).not.toHaveBeenCalled();
+    });
 
-        // Simulate user scrolling down
-        window.scrollY = scrollAmount;
-        window.dispatchEvent(new Event('scroll'));
+    describe('intersection observer', () => {
+      let intersectionCallback;
+      let mockIntersectionObserver;
 
-        // Trigger resize again
+      beforeEach(() => {
+        mockIntersectionObserver = jest.fn((callback) => {
+          intersectionCallback = callback;
+          return {
+            observe: jest.fn(),
+            unobserve: jest.fn(),
+            disconnect: jest.fn(),
+          };
+        });
+
+        global.IntersectionObserver = mockIntersectionObserver;
+      });
+
+      it('sets up intersection observer after first scroll', () => {
         triggerResize();
 
-        // Should maintain the 50px offset from original position
-        expect(document.documentElement.scrollTo).toHaveBeenCalledWith({
-          top: topHeight + scrollAmount,
-          behavior: 'instant',
+        expect(mockIntersectionObserver).toHaveBeenCalled();
+        expect(mockIntersectionObserver.mock.calls[0][1]).toEqual({
+          root: null,
         });
       });
 
-      it('does not scroll if another element is focused', () => {
-        const anchorEl = document.getElementById('reply-field');
-        anchorEl.focus();
-
+      it('cleans up when target is no longer visible', () => {
         triggerResize();
 
-        expect(document.documentElement.scrollTo).not.toHaveBeenCalled();
+        intersectionCallback([{ isIntersecting: false }]);
+
+        triggerResize();
+        expect(document.documentElement.scrollTo).toHaveBeenCalledTimes(1);
       });
     });
   });