Merge branch '57669-fix-bug-clicking-file-header-refreshes-page' into 'master'

Fix bug where clicking file header in diff refreshes page

Closes #57669

See merge request gitlab-org/gitlab-ce!26422

app/assets/javascripts/diffs/components/diff_file.vue

@@ -170,21 +170,23 @@ export default {
     </div>
     <gl-loading-icon v-if="showLoadingIcon" class="diff-content loading" />
     <template v-else>
-      <div v-if="errorMessage" class="diff-viewer">
-        <div class="nothing-here-block" v-html="errorMessage"></div>
+      <div :id="`diff-content-${file.file_hash}`">
+        <div v-if="errorMessage" class="diff-viewer">
+          <div class="nothing-here-block" v-html="errorMessage"></div>
+        </div>
+        <div v-else-if="isCollapsed" class="nothing-here-block diff-collapsed">
+          {{ __('This diff is collapsed.') }}
+          <a class="click-to-expand js-click-to-expand" href="#" @click.prevent="handleToggle">{{
+            __('Click to expand it.')
+          }}</a>
+        </div>
+        <diff-content
+          v-else
+          :class="{ hidden: isCollapsed || isFileTooLarge }"
+          :diff-file="file"
+          :help-page-path="helpPagePath"
+        />
       </div>
-      <div v-else-if="isCollapsed" class="nothing-here-block diff-collapsed">
-        {{ __('This diff is collapsed.') }}
-        <a class="click-to-expand js-click-to-expand" href="#" @click.prevent="handleToggle">{{
-          __('Click to expand it.')
-        }}</a>
-      </div>
-      <diff-content
-        v-else
-        :class="{ hidden: isCollapsed || isFileTooLarge }"
-        :diff-file="file"
-        :help-page-path="helpPagePath"
-      />
     </template>
     <div v-if="isFileTooLarge" class="nothing-here-block diff-collapsed js-too-large-diff">
       {{ __('This source diff could not be displayed because it is too large.') }}

app/assets/javascripts/diffs/components/diff_file_header.vue

@@ -11,6 +11,7 @@ import { __, s__, sprintf } from '~/locale';
 import { diffViewerModes } from '~/ide/constants';
 import EditButton from './edit_button.vue';
 import DiffStats from './diff_stats.vue';
+import { scrollToElement } from '~/lib/utils/common_utils';
 
 export default {
   components: {
@@ -66,6 +67,9 @@ export default {
     hasExpandedDiscussions() {
       return this.diffHasExpandedDiscussions(this.diffFile);
     },
+    diffContentIDSelector() {
+      return `#diff-content-${this.diffFile.file_hash}`;
+    },
     icon() {
       if (this.diffFile.submodule) {
         return 'archive';
@@ -77,6 +81,11 @@ export default {
       if (this.diffFile.submodule) {
         return this.diffFile.submodule_tree_url || this.diffFile.submodule_link;
       }
+
+      if (!this.discussionPath) {
+        return this.diffContentIDSelector;
+      }
+
       return this.discussionPath;
     },
     filePath() {
@@ -149,6 +158,18 @@ export default {
     handleToggleDiscussions() {
       this.toggleFileDiscussions(this.diffFile);
     },
+    handleFileNameClick(e) {
+      const isLinkToOtherPage =
+        this.diffFile.submodule_tree_url || this.diffFile.submodule_link || this.discussionPath;
+
+      if (!isLinkToOtherPage) {
+        e.preventDefault();
+        const selector = this.diffContentIDSelector;
+
+        scrollToElement(document.querySelector(selector));
+        window.location.hash = selector;
+      }
+    },
   },
 };
 </script>
@@ -168,7 +189,14 @@ export default {
         class="diff-toggle-caret append-right-5"
         @click.stop="handleToggle"
       />
-      <a v-once ref="titleWrapper" :href="titleLink" class="append-right-4 js-title-wrapper">
+      <a
+        v-once
+        id="diffFile.file_path"
+        ref="titleWrapper"
+        class="append-right-4 js-title-wrapper"
+        :href="titleLink"
+        @click="handleFileNameClick"
+      >
         <file-icon
           :file-name="filePath"
           :size="18"

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

@@ -7,6 +7,7 @@ import axios from './axios_utils';
 import { getLocationHash } from './url_utility';
 import { convertToCamelCase } from './text_utility';
 import { isObject } from './type_utility';
+import BreakpointInstance from '../../breakpoints';
 
 export const getPagePath = (index = 0) => {
   const page = $('body').attr('data-page') || '';
@@ -193,16 +194,24 @@ 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').height() || 0;
-  const mrTabsHeight = $('.merge-request-tabs').height() || 0;
-  const headerHeight = $('.navbar-gitlab').height() || 0;
-  const diffFilesChanged = $('.js-diff-files-changed').height() || 0;
-  const diffFileLargeEnoughScreen =
-    'matchMedia' in window ? window.matchMedia('min-width: 768') : true;
+  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 mdScreenOrBigger = ['lg', 'md'].includes(BreakpointInstance.getBreakpointSize());
   const diffFileTitleBar =
-    (diffFileLargeEnoughScreen && $('.diff-file .file-title-flex-parent:visible').height()) || 0;
+    (mdScreenOrBigger && $('.diff-file .file-title-flex-parent:visible').outerHeight()) || 0;
+  const compareVersionsHeaderHeight =
+    (mdScreenOrBigger && $('.mr-version-controls').outerHeight()) || 0;
 
-  return perfBar + mrTabsHeight + headerHeight + diffFilesChanged + diffFileTitleBar;
+  return (
+    perfBar +
+    mrTabsHeight +
+    headerHeight +
+    diffFilesChanged +
+    diffFileTitleBar +
+    compareVersionsHeaderHeight
+  );
 };
 
 export const scrollToElement = element => {

changelogs/unreleased/57669-fix-bug-clicking-file-header-refreshes-page.yml

+---
+title: Scroll to diff file content when clicking on file header name and it is not
+  a link to other page
+merge_request: !26422
+author:
+type: fixed

spec/javascripts/diffs/components/diff_file_header_spec.js

@@ -3,7 +3,7 @@ import Vuex from 'vuex';
 import diffsModule from '~/diffs/store/modules';
 import notesModule from '~/notes/stores/modules';
 import DiffFileHeader from '~/diffs/components/diff_file_header.vue';
-import { mountComponentWithStore } from 'spec/helpers/vue_mount_component_helper';
+import mountComponent, { mountComponentWithStore } from 'spec/helpers/vue_mount_component_helper';
 import diffDiscussionsMockData from '../mock_data/diff_discussions';
 import { diffViewerModes } from '~/ide/constants';
 
@@ -249,6 +249,75 @@ describe('diff_file_header', () => {
         expect(vm.$emit).not.toHaveBeenCalled();
       });
     });
+
+    describe('handleFileNameClick', () => {
+      let e;
+
+      beforeEach(() => {
+        e = { preventDefault: () => {} };
+        spyOn(e, 'preventDefault');
+      });
+
+      describe('when file name links to other page', () => {
+        it('does not call preventDefault if submodule tree url exists', () => {
+          vm = mountComponent(Component, {
+            ...props,
+            diffFile: { ...props.diffFile, submodule_tree_url: 'foobar.com' },
+          });
+
+          vm.handleFileNameClick(e);
+
+          expect(e.preventDefault).not.toHaveBeenCalled();
+        });
+
+        it('does not call preventDefault if submodule_link exists', () => {
+          vm = mountComponent(Component, {
+            ...props,
+            diffFile: { ...props.diffFile, submodule_link: 'foobar.com' },
+          });
+          vm.handleFileNameClick(e);
+
+          expect(e.preventDefault).not.toHaveBeenCalled();
+        });
+
+        it('does not call preventDefault if discussionPath exists', () => {
+          vm = mountComponent(Component, {
+            ...props,
+            discussionPath: 'Foo bar',
+          });
+
+          vm.handleFileNameClick(e);
+
+          expect(e.preventDefault).not.toHaveBeenCalled();
+        });
+      });
+
+      describe('scrolling to diff', () => {
+        let scrollToElement;
+        let el;
+
+        beforeEach(() => {
+          el = document.createElement('div');
+          spyOn(document, 'querySelector').and.returnValue(el);
+          scrollToElement = spyOnDependency(DiffFileHeader, 'scrollToElement');
+          vm = mountComponent(Component, props);
+
+          vm.handleFileNameClick(e);
+        });
+
+        it('calls scrollToElement with file content', () => {
+          expect(scrollToElement).toHaveBeenCalledWith(el);
+        });
+
+        it('element adds the content id to the window location', () => {
+          expect(window.location.hash).toContain(props.diffFile.file_hash);
+        });
+
+        it('calls preventDefault when button does not link to other page', () => {
+          expect(e.preventDefault).toHaveBeenCalled();
+        });
+      });
+    });
   });
 
   describe('template', () => {

spec/javascripts/lib/utils/common_utils_spec.js

@@ -2,6 +2,7 @@ import axios from '~/lib/utils/axios_utils';
 import * as commonUtils from '~/lib/utils/common_utils';
 import MockAdapter from 'axios-mock-adapter';
 import { faviconDataUrl, overlayDataUrl, faviconWithOverlayDataUrl } from './mock_data';
+import BreakpointInstance from '~/breakpoints';
 
 const PIXEL_TOLERANCE = 0.2;
 
@@ -380,6 +381,38 @@ describe('common_utils', () => {
     });
   });
 
+  describe('contentTop', () => {
+    it('does not add height for fileTitle or compareVersionsHeader if screen is too small', () => {
+      spyOn(BreakpointInstance, 'getBreakpointSize').and.returnValue('sm');
+
+      setFixtures(`
+        <div class="diff-file file-title-flex-parent">
+          blah blah blah
+        </div>
+        <div class="mr-version-controls">
+          more blah blah blah
+        </div>
+      `);
+
+      expect(commonUtils.contentTop()).toBe(0);
+    });
+
+    it('adds height for fileTitle and compareVersionsHeader screen is large enough', () => {
+      spyOn(BreakpointInstance, 'getBreakpointSize').and.returnValue('lg');
+
+      setFixtures(`
+        <div class="diff-file file-title-flex-parent">
+          blah blah blah
+        </div>
+        <div class="mr-version-controls">
+          more blah blah blah
+        </div>
+      `);
+
+      expect(commonUtils.contentTop()).toBe(18);
+    });
+  });
+
   describe('parseBoolean', () => {
     const { parseBoolean } = commonUtils;