From d11616c828fa76b0fea100872b59d904560e6570 Mon Sep 17 00:00:00 2001 From: GitLab Bot Date: Fri, 14 Jan 2022 06:14:22 +0000 Subject: Add latest changes from gitlab-org/gitlab@master --- app/assets/javascripts/issues/show/index.js | 5 ++ app/assets/javascripts/lib/utils/common_utils.js | 1 + .../javascripts/lib/utils/resize_observer.js | 58 ++++++++++++++++++ app/controllers/projects/issues_controller.rb | 1 + app/models/clusters/applications/runner.rb | 4 +- .../development/fix_comment_scroll.yml | 8 +++ doc/administration/operations/rails_console.md | 2 +- doc/push_rules/push_rules.md | 4 +- .../issues/user_scrolls_to_deeplinked_note_spec.rb | 33 +++++++++++ spec/frontend/lib/utils/resize_observer_spec.js | 68 ++++++++++++++++++++++ spec/models/clusters/applications/runner_spec.rb | 13 ----- 11 files changed, 178 insertions(+), 19 deletions(-) create mode 100644 app/assets/javascripts/lib/utils/resize_observer.js create mode 100644 config/feature_flags/development/fix_comment_scroll.yml create mode 100644 spec/features/issues/user_scrolls_to_deeplinked_note_spec.rb create mode 100644 spec/frontend/lib/utils/resize_observer_spec.js diff --git a/app/assets/javascripts/issues/show/index.js b/app/assets/javascripts/issues/show/index.js index 3e6736d14bf..7f5a0e32f72 100644 --- a/app/assets/javascripts/issues/show/index.js +++ b/app/assets/javascripts/issues/show/index.js @@ -2,6 +2,7 @@ import Vue from 'vue'; import { mapGetters } from 'vuex'; import errorTrackingStore from '~/error_tracking/store'; import { parseBoolean } from '~/lib/utils/common_utils'; +import { scrollToTargetOnResize } from '~/lib/utils/resize_observer'; import IssueApp from './components/app.vue'; import HeaderActions from './components/header_actions.vue'; import IncidentTabs from './components/incidents/incident_tabs.vue'; @@ -73,6 +74,10 @@ export function initIssueApp(issueData, store) { return undefined; } + if (gon?.features?.fixCommentScroll) { + scrollToTargetOnResize(); + } + bootstrapApollo({ ...issueState, issueType: el.dataset.issueType }); const { canCreateIncident, ...issueProps } = issueData; diff --git a/app/assets/javascripts/lib/utils/common_utils.js b/app/assets/javascripts/lib/utils/common_utils.js index 1938b9da3f2..eff00dff7a7 100644 --- a/app/assets/javascripts/lib/utils/common_utils.js +++ b/app/assets/javascripts/lib/utils/common_utils.js @@ -181,6 +181,7 @@ export const contentTop = () => { }, () => getOuterHeight('.merge-request-tabs'), () => getOuterHeight('.js-diff-files-changed'), + () => getOuterHeight('.issue-sticky-header.gl-fixed'), ({ desktop }) => { const diffsTabIsActive = window.mrTabs?.currentAction === 'diffs'; let size; diff --git a/app/assets/javascripts/lib/utils/resize_observer.js b/app/assets/javascripts/lib/utils/resize_observer.js new file mode 100644 index 00000000000..e72c6fe1679 --- /dev/null +++ b/app/assets/javascripts/lib/utils/resize_observer.js @@ -0,0 +1,58 @@ +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; +} diff --git a/app/controllers/projects/issues_controller.rb b/app/controllers/projects/issues_controller.rb index 36c1897cf10..785fbdaa611 100644 --- a/app/controllers/projects/issues_controller.rb +++ b/app/controllers/projects/issues_controller.rb @@ -53,6 +53,7 @@ class Projects::IssuesController < Projects::ApplicationController 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(:paginated_issue_discussions, @project, default_enabled: :yaml) + push_frontend_feature_flag(:fix_comment_scroll, @project, default_enabled: :yaml) end around_action :allow_gitaly_ref_name_caching, only: [:discussions] diff --git a/app/models/clusters/applications/runner.rb b/app/models/clusters/applications/runner.rb index 0ca3bcbf852..33cd5de3518 100644 --- a/app/models/clusters/applications/runner.rb +++ b/app/models/clusters/applications/runner.rb @@ -41,9 +41,7 @@ module Clusters end def prepare_uninstall - ::Gitlab::Database::QueryAnalyzers::PreventCrossDatabaseModification.allow_cross_database_modification_within_transaction(url: 'https://gitlab.com/gitlab-org/gitlab/-/issues/350180') do - runner&.update!(active: false) - end + # No op, see https://gitlab.com/gitlab-org/gitlab/-/issues/350180. end def post_uninstall diff --git a/config/feature_flags/development/fix_comment_scroll.yml b/config/feature_flags/development/fix_comment_scroll.yml new file mode 100644 index 00000000000..706cd816288 --- /dev/null +++ b/config/feature_flags/development/fix_comment_scroll.yml @@ -0,0 +1,8 @@ +--- +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 diff --git a/doc/administration/operations/rails_console.md b/doc/administration/operations/rails_console.md index 8c366e311b8..a93365c08b2 100644 --- a/doc/administration/operations/rails_console.md +++ b/doc/administration/operations/rails_console.md @@ -35,7 +35,7 @@ sudo -u git -H bundle exec rails console -e production **For Kubernetes deployments** -The console is in the task-runner pod. Refer to our [Kubernetes cheat sheet](../troubleshooting/kubernetes_cheat_sheet.md#gitlab-specific-kubernetes-information) for details. +The console is in the toolbox pod. Refer to our [Kubernetes cheat sheet](../troubleshooting/kubernetes_cheat_sheet.md#gitlab-specific-kubernetes-information) for details. To exit the console, type: `quit`. diff --git a/doc/push_rules/push_rules.md b/doc/push_rules/push_rules.md index 94de8e46240..992415d8b17 100644 --- a/doc/push_rules/push_rules.md +++ b/doc/push_rules/push_rules.md @@ -147,10 +147,10 @@ Secrets such as credential files, SSH private keys, and other files containing s GitLab enables you to turn on a predefined denylist of files which can't be pushed to a repository. The list stops those commits from reaching the remote repository. -By selecting the checkbox *Prevent committing secrets to Git*, GitLab prevents +By selecting the checkbox *Prevent pushing secret files*, GitLab prevents pushes to the repository when a file matches a regular expression as read from [`files_denylist.yml`](https://gitlab.com/gitlab-org/gitlab/-/blob/master/ee/lib/gitlab/checks/files_denylist.yml) (make sure you are at the right branch -as your GitLab version when viewing this file). +as your GitLab version when viewing this file). This checkbox is able to be set globally via **Admin Area > Push Rules**, or per-project via **Settings > Repository > Push Rules**. NOTE: Files already committed aren't restricted by this push rule. diff --git a/spec/features/issues/user_scrolls_to_deeplinked_note_spec.rb b/spec/features/issues/user_scrolls_to_deeplinked_note_spec.rb new file mode 100644 index 00000000000..1fa8f533869 --- /dev/null +++ b/spec/features/issues/user_scrolls_to_deeplinked_note_spec.rb @@ -0,0 +1,33 @@ +# 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 diff --git a/spec/frontend/lib/utils/resize_observer_spec.js b/spec/frontend/lib/utils/resize_observer_spec.js new file mode 100644 index 00000000000..419aff28935 --- /dev/null +++ b/spec/frontend/lib/utils/resize_observer_spec.js @@ -0,0 +1,68 @@ +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(`
element to scroll to
`); + + 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(); + }); + }); +}); diff --git a/spec/models/clusters/applications/runner_spec.rb b/spec/models/clusters/applications/runner_spec.rb index 434d7ad4a90..8f02161843b 100644 --- a/spec/models/clusters/applications/runner_spec.rb +++ b/spec/models/clusters/applications/runner_spec.rb @@ -101,19 +101,6 @@ RSpec.describe Clusters::Applications::Runner do end end - describe '#prepare_uninstall' do - it 'pauses associated runner' do - active_runner = create(:ci_runner, contacted_at: 1.second.ago) - - expect(active_runner.active).to be_truthy - - application_runner = create(:clusters_applications_runner, :scheduled, runner: active_runner) - application_runner.prepare_uninstall - - expect(active_runner.active).to be_falsey - end - end - describe '#make_uninstalling!' do subject { create(:clusters_applications_runner, :scheduled, runner: ci_runner) } -- cgit v1.2.3