From 187e3f855d4c882c78e16725e0125f5e1b63056a Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Wed, 4 Aug 2021 13:52:33 -0600 Subject: [PATCH 1/4] Recall MR tab scroll position when within a single session Changelog: added --- app/assets/javascripts/merge_request_tabs.js | 33 +++++++- spec/frontend/merge_request_tabs_spec.js | 82 +++++++++++++++++++- 2 files changed, 110 insertions(+), 5 deletions(-) diff --git a/app/assets/javascripts/merge_request_tabs.js b/app/assets/javascripts/merge_request_tabs.js index 21fe4356fe0350..171e916271f817 100644 --- a/app/assets/javascripts/merge_request_tabs.js +++ b/app/assets/javascripts/merge_request_tabs.js @@ -83,6 +83,8 @@ export default class MergeRequestTabs { this.peek = document.getElementById('js-peek'); this.paddingTop = 16; + this.scrollPositions = {}; + this.commitsTab = document.querySelector('.tab-content .commits.tab-pane'); this.currentTab = null; @@ -136,11 +138,30 @@ export default class MergeRequestTabs { } } + storeScroll() { + if (this.currentTab) { + this.scrollPositions[this.currentTab] = document.documentElement.scrollTop; + } + } + recallScroll(action) { + const storedPosition = this.scrollPositions[action]; + + setTimeout(() => { + window.scrollTo({ + top: storedPosition || storedPosition === 0 ? storedPosition : 0, + left: 0, + behavior: 'auto', + }); + }, 75); // <100ms is typically indistinguishable from "instant" for users, but allows for re-rendering + } + clickTab(e) { if (e.currentTarget) { e.stopImmediatePropagation(); e.preventDefault(); + this.storeScroll(); + const { action } = e.currentTarget.dataset || {}; if (isMetaClick(e)) { @@ -210,8 +231,14 @@ export default class MergeRequestTabs { this.resetViewContainer(); this.mountPipelinesView(); } else { - this.mergeRequestTabPanes.querySelector('#notes').style.display = 'block'; - this.mergeRequestTabs.querySelector('.notes-tab').classList.add('active'); + const notesTab = this.mergeRequestTabs.querySelector('.notes-tab'); + const notesPane = this.mergeRequestTabPanes.querySelector('#notes'); + if (notesPane) { + notesPane.style.display = 'block'; + } + if (notesTab) { + notesTab.classList.add('active'); + } if (bp.getBreakpointSize() !== 'xs') { this.expandView(); @@ -221,6 +248,8 @@ export default class MergeRequestTabs { } $('.detail-page-description').renderGFM(); + + this.recallScroll(action); } else if (action === this.currentAction) { // ContentTop is used to handle anything at the top of the page before the main content const mainContentContainer = document.querySelector('.content-wrapper'); diff --git a/spec/frontend/merge_request_tabs_spec.js b/spec/frontend/merge_request_tabs_spec.js index 23e9bf8b44792f..ced9b71125b783 100644 --- a/spec/frontend/merge_request_tabs_spec.js +++ b/spec/frontend/merge_request_tabs_spec.js @@ -34,6 +34,44 @@ describe('MergeRequestTabs', () => { gl.mrWidget = {}; }); + describe('clickTab', () => { + let params; + + beforeEach(() => { + document.documentElement.scrollTop = 100; + + params = { + metaKey: false, + ctrlKey: false, + which: 1, + stopImmediatePropagation() {}, + preventDefault() {}, + currentTarget: { + getAttribute(attr) { + return attr === 'href' ? 'a/tab/url' : null; + }, + }, + }; + }); + + it("stores the current scroll position if there's an active tab", () => { + testContext.class.currentTab = 'someTab'; + + testContext.class.clickTab(params); + + expect(testContext.class.scrollPositions.someTab).toBe(100); + }); + + it("doesn't store a scroll position if there's no active tab", () => { + // this happens on first load, and we just don't want to store empty values in the `null` property + testContext.class.currentTab = null; + + testContext.class.clickTab(params); + + expect(testContext.class.scrollPositions).toEqual({}); + }); + }); + describe('opensInNewTab', () => { const windowTarget = '_blank'; let clickTabParams; @@ -258,6 +296,7 @@ describe('MergeRequestTabs', () => { beforeEach(() => { jest.spyOn(mainContent, 'getBoundingClientRect').mockReturnValue({ top: 10 }); jest.spyOn(tabContent, 'getBoundingClientRect').mockReturnValue({ top: 100 }); + jest.spyOn(window, 'scrollTo').mockImplementation(() => {}); jest.spyOn(document, 'querySelector').mockImplementation((selector) => { return selector === '.content-wrapper' ? mainContent : tabContent; }); @@ -267,8 +306,6 @@ describe('MergeRequestTabs', () => { it('calls window scrollTo with options if document has scrollBehavior', () => { document.documentElement.style.scrollBehavior = ''; - jest.spyOn(window, 'scrollTo').mockImplementation(() => {}); - testContext.class.tabShown('commits', 'foobar'); expect(window.scrollTo.mock.calls[0][0]).toEqual({ top: 39, behavior: 'smooth' }); @@ -276,11 +313,50 @@ describe('MergeRequestTabs', () => { it('calls window scrollTo with two args if document does not have scrollBehavior', () => { jest.spyOn(document.documentElement, 'style', 'get').mockReturnValue({}); - jest.spyOn(window, 'scrollTo').mockImplementation(() => {}); testContext.class.tabShown('commits', 'foobar'); expect(window.scrollTo.mock.calls[0]).toEqual([0, 39]); }); + + describe('when switching tabs', () => { + const SCROLL_TOP = 100; + + beforeAll(() => { + jest.useFakeTimers(); + }); + + beforeEach(() => { + jest.spyOn(window, 'scrollTo').mockImplementation(() => {}); + testContext.class.mergeRequestTabs = document.createElement('div'); + testContext.class.mergeRequestTabPanes = document.createElement('div'); + testContext.class.currentTab = 'tab'; + testContext.class.scrollPositions = { newTab: SCROLL_TOP }; + }); + + afterAll(() => { + jest.useRealTimers(); + }); + + it('scrolls to the stored position, if one is stored', () => { + testContext.class.tabShown('newTab'); + + jest.advanceTimersByTime(250); + + expect(window.scrollTo.mock.calls[0][0]).toEqual({ + top: SCROLL_TOP, + left: 0, + behavior: 'auto', + }); + }); + + it('scrolls to 0, if no position is stored', () => { + testContext.class.tabShown('unknownTab'); + + jest.advanceTimersByTime(250); + + expect(window.scrollTo.mock.calls[0][0]).toEqual({ top: 0, left: 0, behavior: 'auto' }); + }); + }); }); }); -- GitLab From 8f1dac69c7c5c11740fbeebbd937cdcaa563f665 Mon Sep 17 00:00:00 2001 From: Jose Ivan Vargas Date: Thu, 19 Aug 2021 21:02:19 +0000 Subject: [PATCH 2/4] Apply 1 suggestion(s) to 1 file(s) --- app/assets/javascripts/merge_request_tabs.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/assets/javascripts/merge_request_tabs.js b/app/assets/javascripts/merge_request_tabs.js index 171e916271f817..2e2bece08d341f 100644 --- a/app/assets/javascripts/merge_request_tabs.js +++ b/app/assets/javascripts/merge_request_tabs.js @@ -148,7 +148,7 @@ export default class MergeRequestTabs { setTimeout(() => { window.scrollTo({ - top: storedPosition || storedPosition === 0 ? storedPosition : 0, + top: storedPosition && storedPosition > 0 ? storedPosition : 0 left: 0, behavior: 'auto', }); -- GitLab From b336e1f56834f1f21e1a34b0941ac5c15e1ab779 Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Thu, 19 Aug 2021 15:10:53 -0600 Subject: [PATCH 3/4] Use a constant for the small "re-render" delay --- app/assets/javascripts/merge_request_tabs.js | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/app/assets/javascripts/merge_request_tabs.js b/app/assets/javascripts/merge_request_tabs.js index 2e2bece08d341f..e7597c2e09bc74 100644 --- a/app/assets/javascripts/merge_request_tabs.js +++ b/app/assets/javascripts/merge_request_tabs.js @@ -64,6 +64,8 @@ import syntaxHighlight from './syntax_highlight'; // // +// <100ms is typically indistinguishable from "instant" for users, but allows for re-rendering +const FAST_DELAY_FOR_RERENDER = 75; // Store the `location` object, allowing for easier stubbing in tests let { location } = window; @@ -152,7 +154,7 @@ export default class MergeRequestTabs { left: 0, behavior: 'auto', }); - }, 75); // <100ms is typically indistinguishable from "instant" for users, but allows for re-rendering + }, FAST_DELAY_FOR_RERENDER); } clickTab(e) { -- GitLab From 35eea95e22a3fc23d9ce340c2a78f1a1100bc538 Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Fri, 20 Aug 2021 14:54:25 -0600 Subject: [PATCH 4/4] Add comma to applied suggestion --- app/assets/javascripts/merge_request_tabs.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/assets/javascripts/merge_request_tabs.js b/app/assets/javascripts/merge_request_tabs.js index e7597c2e09bc74..540959b3a4643c 100644 --- a/app/assets/javascripts/merge_request_tabs.js +++ b/app/assets/javascripts/merge_request_tabs.js @@ -150,7 +150,7 @@ export default class MergeRequestTabs { setTimeout(() => { window.scrollTo({ - top: storedPosition && storedPosition > 0 ? storedPosition : 0 + top: storedPosition && storedPosition > 0 ? storedPosition : 0, left: 0, behavior: 'auto', }); -- GitLab