Commit 28848572 authored by Simon Knox's avatar Simon Knox Committed by Ezekiel Kigbo

Fix autoscroll to comments on issues

Add ResizeObserver to set scroll position if a comment is
selected. Still has a bunch of unwanted side effects like
jumping when window is resized, or when sidebars are collapsed
on smaller screens.

Remove ResizeObserver on user interaction
This avoids a bunch of bad and terrible bugs breaking scrolling
when user does something like toggle sidebar or click on text
editor. That otherwise triggers a resize

Changelog: fixed
parent 4db3faf9
...@@ -2,6 +2,7 @@ import Vue from 'vue'; ...@@ -2,6 +2,7 @@ import Vue from 'vue';
import { mapGetters } from 'vuex'; import { mapGetters } from 'vuex';
import errorTrackingStore from '~/error_tracking/store'; import errorTrackingStore from '~/error_tracking/store';
import { parseBoolean } from '~/lib/utils/common_utils'; import { parseBoolean } from '~/lib/utils/common_utils';
import { scrollToTargetOnResize } from '~/lib/utils/resize_observer';
import IssueApp from './components/app.vue'; import IssueApp from './components/app.vue';
import HeaderActions from './components/header_actions.vue'; import HeaderActions from './components/header_actions.vue';
import IncidentTabs from './components/incidents/incident_tabs.vue'; import IncidentTabs from './components/incidents/incident_tabs.vue';
...@@ -73,6 +74,10 @@ export function initIssueApp(issueData, store) { ...@@ -73,6 +74,10 @@ export function initIssueApp(issueData, store) {
return undefined; return undefined;
} }
if (gon?.features?.fixCommentScroll) {
scrollToTargetOnResize();
}
bootstrapApollo({ ...issueState, issueType: el.dataset.issueType }); bootstrapApollo({ ...issueState, issueType: el.dataset.issueType });
const { canCreateIncident, ...issueProps } = issueData; const { canCreateIncident, ...issueProps } = issueData;
......
...@@ -181,6 +181,7 @@ export const contentTop = () => { ...@@ -181,6 +181,7 @@ export const contentTop = () => {
}, },
() => getOuterHeight('.merge-request-tabs'), () => getOuterHeight('.merge-request-tabs'),
() => getOuterHeight('.js-diff-files-changed'), () => getOuterHeight('.js-diff-files-changed'),
() => getOuterHeight('.issue-sticky-header.gl-fixed'),
({ desktop }) => { ({ desktop }) => {
const diffsTabIsActive = window.mrTabs?.currentAction === 'diffs'; const diffsTabIsActive = window.mrTabs?.currentAction === 'diffs';
let size; let size;
......
import { contentTop } from './common_utils';
const interactionEvents = ['mousedown', 'touchstart', 'keydown', 'wheel'];
export function createResizeObserver() {
return new ResizeObserver((entries) => {
entries.forEach((entry) => {
entry.target.dispatchEvent(new CustomEvent(`ResizeUpdate`, { detail: { entry } }));
});
});
}
// watches for change in size of a container element (e.g. for lazy-loaded images)
// and scroll the target element to the top of the content area
// stop watching after any user input. So if user opens sidebar or manually
// scrolls the page we don't hijack their scroll position
export function scrollToTargetOnResize({
target = window.location.hash,
container = '#content-body',
} = {}) {
if (!target) return null;
const ro = createResizeObserver();
const containerEl = document.querySelector(container);
let interactionListenersAdded = false;
function keepTargetAtTop() {
const anchorEl = document.querySelector(target);
if (!anchorEl) return;
const anchorTop = anchorEl.getBoundingClientRect().top + window.scrollY;
const top = anchorTop - contentTop();
document.documentElement.scrollTo({
top,
});
if (!interactionListenersAdded) {
interactionEvents.forEach((event) =>
// eslint-disable-next-line no-use-before-define
document.addEventListener(event, removeListeners),
);
interactionListenersAdded = true;
}
}
function removeListeners() {
interactionEvents.forEach((event) => document.removeEventListener(event, removeListeners));
ro.unobserve(containerEl);
containerEl.removeEventListener('ResizeUpdate', keepTargetAtTop);
}
containerEl.addEventListener('ResizeUpdate', keepTargetAtTop);
ro.observe(containerEl);
return ro;
}
...@@ -53,6 +53,7 @@ class Projects::IssuesController < Projects::ApplicationController ...@@ -53,6 +53,7 @@ class Projects::IssuesController < Projects::ApplicationController
push_frontend_feature_flag(:confidential_notes, project&.group, default_enabled: :yaml) push_frontend_feature_flag(:confidential_notes, project&.group, default_enabled: :yaml)
push_frontend_feature_flag(:issue_assignees_widget, @project, default_enabled: :yaml) push_frontend_feature_flag(:issue_assignees_widget, @project, default_enabled: :yaml)
push_frontend_feature_flag(:paginated_issue_discussions, @project, default_enabled: :yaml) push_frontend_feature_flag(:paginated_issue_discussions, @project, default_enabled: :yaml)
push_frontend_feature_flag(:fix_comment_scroll, @project, default_enabled: :yaml)
end end
around_action :allow_gitaly_ref_name_caching, only: [:discussions] around_action :allow_gitaly_ref_name_caching, only: [:discussions]
......
---
name: fix_comment_scroll
introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/76340
rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/349638
milestone: '14.7'
type: development
group: group::project management
default_enabled: false
# frozen_string_literal: true
require 'spec_helper'
RSpec.describe 'User scrolls to deep-linked note' do
let_it_be(:project) { create(:project, :public, :repository) }
let_it_be(:issue) { create(:issue, project: project) }
let_it_be(:comment_1) { create(:note_on_issue, noteable: issue, project: project, note: 'written first') }
let_it_be(:comments) { create_list(:note_on_issue, 20, noteable: issue, project: project, note: 'spacer note') }
context 'on issue page', :js do
it 'on comment' do
visit project_issue_path(project, issue, anchor: "note_#{comment_1.id}")
wait_for_requests
expect(first_comment).to have_content(comment_1.note)
bottom_of_title = find('.issue-sticky-header.gl-fixed').evaluate_script("this.getBoundingClientRect().bottom;")
top = first_comment.evaluate_script("this.getBoundingClientRect().top;")
expect(top).to be_within(1).of(bottom_of_title)
end
end
def all_comments
all('.timeline > .note.timeline-entry')
end
def first_comment
all_comments.first
end
end
import { contentTop } from '~/lib/utils/common_utils';
import { scrollToTargetOnResize } from '~/lib/utils/resize_observer';
jest.mock('~/lib/utils/common_utils');
function mockStickyHeaderSize(val) {
contentTop.mockReturnValue(val);
}
describe('ResizeObserver Utility', () => {
let observer;
const triggerResize = () => {
const entry = document.querySelector('#content-body');
entry.dispatchEvent(new CustomEvent(`ResizeUpdate`, { detail: { entry } }));
};
beforeEach(() => {
mockStickyHeaderSize(90);
jest.spyOn(document.documentElement, 'scrollTo');
setFixtures(`<div id="content-body"><div class="target">element to scroll to</div></div>`);
const target = document.querySelector('.target');
jest.spyOn(target, 'getBoundingClientRect').mockReturnValue({ top: 200 });
observer = scrollToTargetOnResize({
target: '.target',
container: '#content-body',
});
});
afterEach(() => {
contentTop.mockReset();
});
describe('Observer behavior', () => {
it('returns null for empty target', () => {
observer = scrollToTargetOnResize({
target: '',
container: '#content-body',
});
expect(observer).toBe(null);
});
it('returns ResizeObserver instance', () => {
expect(observer).toBeInstanceOf(ResizeObserver);
});
it('scrolls body so anchor is just below sticky header (contentTop)', () => {
triggerResize();
expect(document.documentElement.scrollTo).toHaveBeenCalledWith({ top: 110 });
});
const interactionEvents = ['mousedown', 'touchstart', 'keydown', 'wheel'];
it.each(interactionEvents)('does not hijack scroll after user input from %s', (eventType) => {
const event = new Event(eventType);
document.dispatchEvent(event);
triggerResize();
expect(document.documentElement.scrollTo).not.toHaveBeenCalledWith();
});
});
});
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment