Merge branch 'defect/jump-to-next-overscroll' into 'master'

Scroll exactly to the top of a discussion on the MR Overview tab

See merge request gitlab-org/gitlab!47970

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

@@ -218,23 +218,36 @@ export const isMetaKey = e => e.metaKey || e.ctrlKey || e.altKey || e.shiftKey;
 export const isMetaClick = e => e.metaKey || e.ctrlKey || e.which === 2;
 
 export const contentTop = () => {
-  const perfBar = $('#js-peek').outerHeight() || 0;
-  const mrTabsHeight = $('.merge-request-tabs').outerHeight() || 0;
-  const headerHeight = $('.navbar-gitlab').outerHeight() || 0;
-  const diffFilesChanged = $('.js-diff-files-changed').outerHeight() || 0;
+  const heightCalculators = [
+    () => $('#js-peek').outerHeight(),
+    () => $('.navbar-gitlab').outerHeight(),
+    () => $('.merge-request-tabs').outerHeight(),
+    () => $('.js-diff-files-changed').outerHeight(),
+    () => {
       const isDesktop = breakpointInstance.isDesktop();
-  const diffFileTitleBar =
-    (isDesktop && $('.diff-file .file-title-flex-parent:visible').outerHeight()) || 0;
-  const compareVersionsHeaderHeight = (isDesktop && $('.mr-version-controls').outerHeight()) || 0;
+      const diffsTabIsActive = window.mrTabs?.currentAction === 'diffs';
+      let size;
 
-  return (
-    perfBar +
-    mrTabsHeight +
-    headerHeight +
-    diffFilesChanged +
-    diffFileTitleBar +
-    compareVersionsHeaderHeight
-  );
+      if (isDesktop && diffsTabIsActive) {
+        size = $('.diff-file .file-title-flex-parent:visible').outerHeight();
+      }
+
+      return size;
+    },
+    () => {
+      let size;
+
+      if (breakpointInstance.isDesktop()) {
+        size = $('.mr-version-controls').outerHeight();
+      }
+
+      return size;
+    },
+  ];
+
+  return heightCalculators.reduce((totalHeight, calculator) => {
+    return totalHeight + (calculator() || 0);
+  }, 0);
 };
 
 export const scrollToElement = (element, options = {}) => {

app/assets/javascripts/notes/mixins/discussion_navigation.js

 import { mapGetters, mapActions, mapState } from 'vuex';
-import { scrollToElementWithContext } from '~/lib/utils/common_utils';
+import { scrollToElementWithContext, scrollToElement } from '~/lib/utils/common_utils';
 import eventHub from '../event_hub';
 
 /**
  * @param {string} selector
  * @returns {boolean}
  */
-function scrollTo(selector) {
+function scrollTo(selector, { withoutContext = false } = {}) {
   const el = document.querySelector(selector);
+  const scrollFunction = withoutContext ? scrollToElement : scrollToElementWithContext;
 
   if (el) {
-    scrollToElementWithContext(el);
+    scrollFunction(el);
     return true;
   }
 
@@ -35,7 +36,7 @@ function diffsJump({ expandDiscussion }, id) {
 function discussionJump({ expandDiscussion }, id) {
   const selector = `div.discussion[data-discussion-id="${id}"]`;
   expandDiscussion({ discussionId: id });
-  return scrollTo(selector);
+  return scrollTo(selector, { withoutContext: true });
 }
 
 /**

changelogs/unreleased/defect-jump-to-next-overscroll.yml

+---
+title: Scroll exactly to the top of a discussion on the MR Overview tab
+merge_request: 47970
+author:
+type: fixed

spec/frontend/lib/utils/common_utils_spec.js

@@ -220,6 +220,7 @@ describe('common_utils', () => {
     beforeEach(() => {
       elem = document.createElement('div');
       window.innerHeight = windowHeight;
+      window.mrTabs = { currentAction: 'show' };
       jest.spyOn($.fn, 'animate');
       jest.spyOn($.fn, 'offset').mockReturnValue({ top: elemTop });
     });

spec/frontend/notes/mixins/discussion_navigation_spec.js

@@ -42,6 +42,7 @@ describe('Discussion navigation mixin', () => {
     );
 
     jest.spyOn(utils, 'scrollToElementWithContext');
+    jest.spyOn(utils, 'scrollToElement');
 
     expandDiscussion = jest.fn();
     const { actions, ...notesRest } = notesModule();
@@ -133,7 +134,7 @@ describe('Discussion navigation mixin', () => {
         });
 
         it('scrolls to element', () => {
-          expect(utils.scrollToElementWithContext).toHaveBeenCalledWith(
+          expect(utils.scrollToElement).toHaveBeenCalledWith(
             findDiscussion('div.discussion', expected),
           );
         });
@@ -200,7 +201,7 @@ describe('Discussion navigation mixin', () => {
           });
 
           it('scrolls to discussion', () => {
-            expect(utils.scrollToElementWithContext).toHaveBeenCalledWith(
+            expect(utils.scrollToElement).toHaveBeenCalledWith(
               findDiscussion('div.discussion', expected),
             );
           });