diff --git a/app/assets/javascripts/rapid_diffs/app/chrome_fix.js b/app/assets/javascripts/rapid_diffs/app/chrome_fix.js new file mode 100644 index 0000000000000000000000000000000000000000..0ed51947006f12dfe8d42f423054f36601f0b8b9 --- /dev/null +++ b/app/assets/javascripts/rapid_diffs/app/chrome_fix.js @@ -0,0 +1,11 @@ +// content-visibility was fixed in Chrome 138, older versions are way too laggy with is so we just disable the feature +// https://issues.chromium.org/issues/40066846 +export const disableContentVisibilityOnOlderChrome = () => { + if (!/Chrome/.test(navigator.userAgent)) return; + const chromeVersion = parseInt(navigator.userAgent.match(/Chrome\/(\d+)/)[1], 10); + if (chromeVersion < 138) { + document + .querySelector('[data-rapid-diffs]') + .style.setProperty('--rd-content-visibility-auto', 'visible'); + } +}; diff --git a/app/assets/javascripts/rapid_diffs/app/index.js b/app/assets/javascripts/rapid_diffs/app/index.js index 2dd78d473508e58dbd9eeabd694bae1fb1584eb0..d615f806289329c14b4f00aeaff661184e932fba 100644 --- a/app/assets/javascripts/rapid_diffs/app/index.js +++ b/app/assets/javascripts/rapid_diffs/app/index.js @@ -12,6 +12,7 @@ import { __ } from '~/locale'; import { fixWebComponentsStreamingOnSafari } from '~/rapid_diffs/app/safari_fix'; import { DIFF_FILE_MOUNTED } from '~/rapid_diffs/dom_events'; import { parseBoolean } from '~/lib/utils/common_utils'; +import { disableContentVisibilityOnOlderChrome } from '~/rapid_diffs/app/chrome_fix'; // This facade interface joins together all the bits and pieces of Rapid Diffs: DiffFile, Settings, File browser, etc. // It's a unified entrypoint for Rapid Diffs and all external communications should happen through this interface. @@ -22,6 +23,7 @@ class RapidDiffsFacade { init() { this.#registerCustomElements(); + disableContentVisibilityOnOlderChrome(); fixWebComponentsStreamingOnSafari( document.querySelector('[data-diffs-list]'), this.DiffFileImplementation, diff --git a/app/assets/javascripts/rapid_diffs/diff_file.js b/app/assets/javascripts/rapid_diffs/diff_file.js index e25d105ac5e8eb6e7aabb06c2a89889a28fc39fa..2333a1f09d463b2fc336744237703459fab72a39 100644 --- a/app/assets/javascripts/rapid_diffs/diff_file.js +++ b/app/assets/javascripts/rapid_diffs/diff_file.js @@ -76,7 +76,14 @@ export class DiffFile extends HTMLElement { } selectFile() { - this.scrollIntoView(); + this.scrollIntoView({ block: 'start' }); + setTimeout(() => { + // with content-visibility we might get a layout shift which we have to account for + // 1. first scroll: renders target file and neighbours, they receive proper dimensions + // 2. layout updates: target file might jump up or down, depending on the intrinsic size mismatch in neighbours + // 3. second scroll: layout is stable, we can now properly scroll the file into the viewport + this.scrollIntoView({ block: 'start' }); + }); // TODO: add outline for active file } diff --git a/app/assets/stylesheets/components/rapid_diffs/_constants.scss b/app/assets/stylesheets/components/rapid_diffs/_constants.scss index 8394c938b83a486dd833d0d45ea19e22612482bd..4e3b7df2d23df97f04637201b8a6b66fda33cc29 100644 --- a/app/assets/stylesheets/components/rapid_diffs/_constants.scss +++ b/app/assets/stylesheets/components/rapid_diffs/_constants.scss @@ -10,3 +10,6 @@ $code-line-height: calc($code-row-height-target / $code-font-size-target); $app-header-vertical-padding: $gl-spacing-scale-3; $app-vertical-breakpoint: md; + +$diff-file-border-width: 1px; +$diff-file-header-padding-y: $gl-spacing-scale-3; diff --git a/app/assets/stylesheets/components/rapid_diffs/app.scss b/app/assets/stylesheets/components/rapid_diffs/app.scss index c5d6004e576036f9b82f44116481fd9c2fedfc6d..1e066e7907f84bd604036c9e48a72027095ebded 100644 --- a/app/assets/stylesheets/components/rapid_diffs/app.scss +++ b/app/assets/stylesheets/components/rapid_diffs/app.scss @@ -31,6 +31,7 @@ .rd-app-content { flex: 1 0; + min-width: 0; } // override .code styles because we can't apply .code directly on the diff file code class diff --git a/app/assets/stylesheets/components/rapid_diffs/diff_file_component.scss b/app/assets/stylesheets/components/rapid_diffs/diff_file_component.scss index 604996d2fb85dc298fb4252c29f28ad77e3fdd35..eeae9bc7e77baf5241122a81a5797f4f5cf018df 100644 --- a/app/assets/stylesheets/components/rapid_diffs/diff_file_component.scss +++ b/app/assets/stylesheets/components/rapid_diffs/diff_file_component.scss @@ -1,10 +1,25 @@ +@use 'constants'; + .rd-diff-file-component { + display: block; scroll-margin-top: var(--rd-app-sticky-top-with-padding); + margin-bottom: $gl-padding; } .rd-diff-file { --rd-diff-file-border-radius: #{calc($gl-border-radius-base - 1px)}; - padding-bottom: $gl-padding; +} + +.rd-diff-file[data-virtual]:not([data-collapsed]) { + $header-padding: constants.$diff-file-header-padding-y * 2; + // top/bottom padding + toggle button size, excluding borders + $file-header-height: calc($header-padding + $gl-button-small-size); + // header top/bottom borders + body border + $total-borders: constants.$diff-file-border-width * 3; + content-visibility: var(--rd-content-visibility-auto, auto); + contain-intrinsic-size: + auto 0 auto + calc(#{$file-header-height} + #{$total-borders} + (#{constants.$code-row-height-target} * var(--total-rows))); } .rd-diff-file-header { @@ -15,10 +30,10 @@ display: flex; background-color: var(--gl-background-color-subtle); border: 1px solid var(--gl-border-color-default); - padding: $gl-spacing-scale-3 $gl-spacing-scale-4; + padding: constants.$diff-file-header-padding-y $gl-spacing-scale-4; border-radius: var(--rd-diff-file-border-radius) var(--rd-diff-file-border-radius) 0 0; word-break: break-word; - z-index: 1; + z-index: 3; } .rd-diff-file-header:has([data-options-toggle] button[aria-expanded='true']) { @@ -86,7 +101,7 @@ } .rd-diff-file-body { - border: 1px solid var(--gl-border-color-default); + border: constants.$diff-file-border-width solid var(--gl-border-color-default); border-top: 0; border-radius: 0 0 var(--rd-diff-file-border-radius) var(--rd-diff-file-border-radius); diff --git a/app/assets/stylesheets/components/rapid_diffs/text_file_viewers.scss b/app/assets/stylesheets/components/rapid_diffs/text_file_viewers.scss index 9a55e8425fafd04d00210ce002cf27c4c10e12c4..93f5ad2b12c5dcf746b809b8e7adf5bd14447c52 100644 --- a/app/assets/stylesheets/components/rapid_diffs/text_file_viewers.scss +++ b/app/assets/stylesheets/components/rapid_diffs/text_file_viewers.scss @@ -22,21 +22,29 @@ display: grid; } -.rd-hunk-lines:last-child > :first-child { - border-bottom-left-radius: var(--rd-diff-file-border-radius); +.rd-text-view-content tr:last-child { + --rd-row-bottom-left-radius: var(--rd-diff-file-border-radius); + --rd-row-bottom-right-radius: var(--rd-diff-file-border-radius); } -.rd-hunk-lines:last-child > :last-child { - border-bottom-right-radius: var(--rd-diff-file-border-radius); +.rd-text-view-content tr:last-child td:not(:last-child) { + --rd-row-bottom-right-radius: 0; +} + +.rd-text-view-content tr:last-child td:not(:first-child) { + --rd-row-bottom-left-radius: 0; } .rd-hunk-header { + position: relative; + // overlap line numbers + z-index: 2; // this is used when a hunk header doesn't have any text, only expand buttons min-height: calc(1em * $code-line-height); - border-top: 1px solid var(--diff-expansion-background-color, var(--gl-border-color-default)); - border-bottom: 1px solid var(--diff-expansion-background-color, var(--gl-border-color-default)); background-color: var(--code-diff-hunk-header-background-color, $gray-50); color: var(--code-diff-hunk-header-color, $gray-400); + border-radius: 0 0 var(--rd-row-bottom-right-radius) var(--rd-row-bottom-left-radius); + outline: 1px solid var(--diff-expansion-background-color, var(--gl-border-color-default)); &:first-child { border-top: 0; @@ -89,6 +97,7 @@ background-color: var(--code-diff-expand-button-background-color, $gray-100); color: var(--code-diff-expand-button-color, var(--gl-text-color-subtle)); + border-radius: 0 0 var(--rd-row-bottom-right-radius) var(--rd-row-bottom-left-radius); &:hover, &:focus { @@ -171,10 +180,12 @@ .rd-line-number:first-child { box-shadow: -1px 0 var(--rd-line-border-color); + border-bottom-left-radius: var(--rd-row-bottom-left-radius); } .rd-line-content:last-child { box-shadow: 1px 0 var(--rd-line-border-color); + border-bottom-right-radius: var(--rd-row-bottom-right-radius); } .rd-line-link { diff --git a/app/components/rapid_diffs/diff_file_component.html.haml b/app/components/rapid_diffs/diff_file_component.html.haml index d770d97314b525f8dcf749915b6052ef9fd0fcaa..2ed79ced44a4045c7a80661f7db06a0bccdf5a82 100644 --- a/app/components/rapid_diffs/diff_file_component.html.haml +++ b/app/components/rapid_diffs/diff_file_component.html.haml @@ -1,7 +1,7 @@ -# TODO: add fork suggestion (commits only) %diff-file.rd-diff-file-component{ id: id, data: { testid: 'rd-diff-file', **server_data } } - .rd-diff-file + .rd-diff-file{ data: { virtual: virtual? }, style: ("--total-rows: #{total_rows}" if virtual?) } = header || default_header -# extra wrapper needed so content-visibility: hidden doesn't require removing border or other styles %div{ data: { file_body: '' } } diff --git a/app/components/rapid_diffs/diff_file_component.rb b/app/components/rapid_diffs/diff_file_component.rb index f109b5f5c7cdb0e8f8ca0478da8918bcf763366e..e354e77fdd066e8e33465892e38e0dc357d28445 100644 --- a/app/components/rapid_diffs/diff_file_component.rb +++ b/app/components/rapid_diffs/diff_file_component.rb @@ -25,7 +25,7 @@ def server_data end def viewer_component - return Viewers::NoPreviewComponent if @diff_file.collapsed? || !@diff_file.modified_file? + return Viewers::NoPreviewComponent if empty_diff? if @diff_file.diffable_text? return Viewers::Text::ParallelViewComponent if @parallel_view @@ -36,8 +36,28 @@ def viewer_component Viewers::NoPreviewComponent end + def empty_diff? + @diff_file.collapsed? || !@diff_file.modified_file? + end + def default_header render RapidDiffs::DiffFileHeaderComponent.new(diff_file: @diff_file) end + + def total_rows + return 0 unless !empty_diff? && @diff_file.diffable_text? + + count = 0 + @diff_file.viewer_hunks.each do |hunk| + count += hunk.header.expand_directions.count if hunk.header + count += @parallel_view ? hunk.parallel_lines.count : hunk.lines.count + end + count + end + + # enables virtual rendering through content-visibility: auto, significantly boosts client performance + def virtual? + total_rows > 0 + end end end diff --git a/spec/components/rapid_diffs/shared.rb b/spec/components/rapid_diffs/shared.rb index 6eab9a352a261e62542394765c30a50a9e2d1ed3..13e488df8ddd7e95ef49a5c2001ae2db2b346605 100644 --- a/spec/components/rapid_diffs/shared.rb +++ b/spec/components/rapid_diffs/shared.rb @@ -32,6 +32,17 @@ def render_component expect(web_component['data-diff-lines-path']).to eq("#{diff_path}/diff_lines") end + it "renders line count" do + render_component + total_count = web_component.all(:css, 'table tbody tr').count + expect(web_component).to have_css("[style~='--total-rows: #{total_count}']") + end + + it "enables virtual rendering" do + render_component + expect(web_component).to have_css("[data-virtual]") + end + context "when is text diff" do before do allow(diff_file).to receive(:diffable_text?).and_return(true) @@ -68,6 +79,11 @@ def render_component render_component expect(web_component['data-viewer']).to eq('no_preview') end + + it "disables virtual rendering" do + render_component + expect(web_component).not_to have_css("[data-virtual]") + end end context "when file is collapsed" do @@ -79,5 +95,10 @@ def render_component render_component expect(web_component['data-viewer']).to eq('no_preview') end + + it "disables virtual rendering" do + render_component + expect(web_component).not_to have_css("[data-virtual]") + end end end