Merge branch 'hash-mr-scroll-load' into 'master'

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

@@ -71,6 +71,7 @@ export const handleLocationHash = () => {
   // This is required to handle non-unicode characters in hash
   hash = decodeURIComponent(hash);
 
+  const target = document.getElementById(hash) || document.getElementById(`user-content-${hash}`);
   const fixedTabs = document.querySelector('.js-tabs-affix');
   const fixedDiffStats = document.querySelector('.js-diff-files-changed.is-stuck');
   const fixedNav = document.querySelector('.navbar-gitlab');
@@ -78,25 +79,19 @@ export const handleLocationHash = () => {
   let adjustment = 0;
   if (fixedNav) adjustment -= fixedNav.offsetHeight;
 
-  // scroll to user-generated markdown anchor if we cannot find a match
-  if (document.getElementById(hash) === null) {
-    const target = document.getElementById(`user-content-${hash}`);
-    if (target && target.scrollIntoView) {
-      target.scrollIntoView(true);
-      window.scrollBy(0, adjustment);
-    }
-  } else {
-    // only adjust for fixedTabs when not targeting user-generated content
-    if (fixedTabs) {
-      adjustment -= fixedTabs.offsetHeight;
-    }
+  if (target && target.scrollIntoView) {
+    target.scrollIntoView(true);
+  }
 
-    if (fixedDiffStats) {
-      adjustment -= fixedDiffStats.offsetHeight;
-    }
+  if (fixedTabs) {
+    adjustment -= fixedTabs.offsetHeight;
+  }
 
-    window.scrollBy(0, adjustment);
+  if (fixedDiffStats) {
+    adjustment -= fixedDiffStats.offsetHeight;
   }
+
+  window.scrollBy(0, adjustment);
 };
 
 // Check if element scrolled into viewport from above or below

changelogs/unreleased/hash-mr-scroll-load.yml

+---
+title: Fixed notes not being scrolled to in merge requests
+merge_request:
+author:
+type: fixed

spec/javascripts/lib/utils/common_utils_spec.js

@@ -84,6 +84,62 @@ describe('common_utils', () => {
       expectGetElementIdToHaveBeenCalledWith('definição');
       expectGetElementIdToHaveBeenCalledWith('user-content-definição');
     });
+
+    it('scrolls element into view', () => {
+      document.body.innerHTML += `
+        <div id="parent">
+          <div style="height: 2000px;"></div>
+          <div id="test" style="height: 2000px;"></div>
+        </div>
+      `;
+
+      window.history.pushState({}, null, '#test');
+      commonUtils.handleLocationHash();
+
+      expectGetElementIdToHaveBeenCalledWith('test');
+      expect(window.scrollY).toBe(document.getElementById('test').offsetTop);
+
+      document.getElementById('parent').remove();
+    });
+
+    it('scrolls user content element into view', () => {
+      document.body.innerHTML += `
+        <div id="parent">
+          <div style="height: 2000px;"></div>
+          <div id="user-content-test" style="height: 2000px;"></div>
+        </div>
+      `;
+
+      window.history.pushState({}, null, '#test');
+      commonUtils.handleLocationHash();
+
+      expectGetElementIdToHaveBeenCalledWith('test');
+      expectGetElementIdToHaveBeenCalledWith('user-content-test');
+      expect(window.scrollY).toBe(document.getElementById('user-content-test').offsetTop);
+
+      document.getElementById('parent').remove();
+    });
+
+    it('scrolls to element with offset from navbar', () => {
+      spyOn(window, 'scrollBy').and.callThrough();
+      document.body.innerHTML += `
+        <div id="parent">
+          <div class="navbar-gitlab" style="position: fixed; top: 0; height: 50px;"></div>
+          <div style="height: 2000px; margin-top: 50px;"></div>
+          <div id="user-content-test" style="height: 2000px;"></div>
+        </div>
+      `;
+
+      window.history.pushState({}, null, '#test');
+      commonUtils.handleLocationHash();
+
+      expectGetElementIdToHaveBeenCalledWith('test');
+      expectGetElementIdToHaveBeenCalledWith('user-content-test');
+      expect(window.scrollY).toBe(document.getElementById('user-content-test').offsetTop - 50);
+      expect(window.scrollBy).toHaveBeenCalledWith(0, -50);
+
+      document.getElementById('parent').remove();
+    });
   });
 
   describe('setParamInURL', () => {