Merge pull request #12721 from overleaf/td-history-improve-diff-highlight

Add markers for addition and deletion on empty lines in history diff

GitOrigin-RevId: 62e329da8bc055ee922854b837fe9465d724402e

services/web/frontend/js/features/history/components/diff-view/document-diff-viewer.tsx

@@ -1,4 +1,4 @@
-import { useCallback, useEffect, useRef, useState } from 'react'
+import { useCallback, useEffect, useState } from 'react'
 import withErrorBoundary from '../../../../infrastructure/error-boundary'
 import { ErrorBoundaryFallback } from '../../../../shared/components/error-boundary-fallback'
 import {
@@ -22,6 +22,7 @@ import {
 } from '../../extensions/highlight-locations'
 import Icon from '../../../../shared/components/icon'
 import { useTranslation } from 'react-i18next'
+import { inlineBackground } from '../../../source-editor/extensions/inline-background'
 
 type FontFamily = 'monaco' | 'lucida'
 type LineHeight = 'compact' | 'normal' | 'wide'
@@ -37,6 +38,7 @@ function extensions(themeOptions: Options): Extension[] {
     highlights(),
     highlightLocations(),
     theme(themeOptions),
+    inlineBackground(false),
   ]
 }
 
@@ -66,8 +68,8 @@ function DocumentDiffViewer({
     })
   })
 
-  const view = useRef(
-    new EditorView({
+  const [view] = useState<EditorView>(() => {
+    return new EditorView({
       state,
       dispatch: tr => {
         view.update([tr])
@@ -76,7 +78,7 @@ function DocumentDiffViewer({
         }
       },
     })
-  ).current
+  })
 
   const highlightLocations = state.field(highlightLocationsField)
 

services/web/frontend/js/features/history/extensions/highlight-locations.ts

@@ -1,7 +1,7 @@
 import { EditorSelection, StateEffect, StateField } from '@codemirror/state'
 import { Highlight } from '../services/types/doc'
 import { EditorView, ViewPlugin, ViewUpdate } from '@codemirror/view'
-import { highlightsField } from './highlights'
+import { highlightDecorationsField } from './highlights'
 import { throttle, isEqual } from 'lodash'
 import { updateHasEffect } from '../../source-editor/utils/effects'
 
@@ -31,7 +31,8 @@ function calculateHighlightLocations(view: EditorView): HighlightLocations {
   let next
   let previous
 
-  const highlights = view.state.field(highlightsField) || []
+  const highlights =
+    view.state.field(highlightDecorationsField).highlights || []
 
   if (highlights.length === 0) {
     return { before: 0, after: 0 }

services/web/frontend/js/features/history/extensions/highlights.ts

@@ -1,12 +1,28 @@
-import { StateEffect, StateField } from '@codemirror/state'
-import { Decoration, EditorView, hoverTooltip, Tooltip } from '@codemirror/view'
-import { Highlight } from '../services/types/doc'
+import {
+  EditorState,
+  Line,
+  Range,
+  RangeSet,
+  StateEffect,
+  StateField,
+} from '@codemirror/state'
+import {
+  Decoration,
+  DecorationSet,
+  EditorView,
+  hoverTooltip,
+  Tooltip,
+  WidgetType,
+} from '@codemirror/view'
+import { Highlight, HighlightType } from '../services/types/doc'
 
 export const setHighlightsEffect = StateEffect.define<Highlight[]>()
 
 function highlightToMarker(highlight: Highlight) {
   const className =
-    highlight.type === 'addition' ? 'ol-addition-marker' : 'ol-deletion-marker'
+    highlight.type === 'addition'
+      ? 'ol-cm-addition-marker'
+      : 'ol-cm-deletion-marker'
   const { from, to } = highlight.range
 
   return Decoration.mark({
@@ -17,8 +33,45 @@ function highlightToMarker(highlight: Highlight) {
   }).range(from, to)
 }
 
+type LineStatus = {
+  line: Line
+  highlights: Highlight[]
+  empty: boolean
+  changeType: HighlightType | 'mixed'
+}
+
+type LineStatuses = Map<number, LineStatus>
+
+function highlightedLines(highlights: Highlight[], state: EditorState) {
+  const lineStatuses = new Map<number, LineStatus>()
+  for (const highlight of highlights) {
+    const fromLine = state.doc.lineAt(highlight.range.from).number
+    const toLine = state.doc.lineAt(highlight.range.to).number
+    for (let lineNum = fromLine; lineNum <= toLine; ++lineNum) {
+      const status = lineStatuses.get(lineNum)
+      if (status) {
+        status.highlights.push(highlight)
+        if (status.changeType !== highlight.type) {
+          status.changeType = 'mixed'
+        }
+      } else {
+        const line = state.doc.line(lineNum)
+        lineStatuses.set(lineNum, {
+          line,
+          highlights: [highlight],
+          empty: line.length === 0,
+          changeType: highlight.type,
+        })
+      }
+    }
+  }
+  return lineStatuses
+}
+
 const theme = EditorView.baseTheme({
-  '.ol-addition-marker': {
+  '.ol-cm-addition-marker': {
+    paddingTop: 'var(--half-leading)',
+    paddingBottom: 'var(--half-leading)',
     backgroundColor: 'hsl(var(--hue), 70%, 85%)',
   },
   '.ol-deletion-marker': {
@@ -40,10 +93,13 @@ const theme = EditorView.baseTheme({
     padding: '4px',
     color: '#fff',
   },
+  '.ol-cm-empty-line-addition-marker': {
+    padding: 'var(--half-leading) 2px',
+  },
 })
 
 const tooltip = (view: EditorView, pos: number, side: any): Tooltip | null => {
-  const highlights = view.state.field(highlightsField)
+  const highlights = view.state.field(highlightDecorationsField).highlights
   const highlight = highlights.find(highlight => {
     const { from, to } = highlight.range
     return !(
@@ -73,27 +129,114 @@ const tooltip = (view: EditorView, pos: number, side: any): Tooltip | null => {
   }
 }
 
-export const highlightsField = StateField.define<Highlight[]>({
-  create() {
-    return []
-  },
-  update(highlightMarkers, tr) {
-    for (const effect of tr.effects) {
-      if (effect.is(setHighlightsEffect)) {
-        return effect.value
-      }
+class EmptyLineAdditionMarkerWidget extends WidgetType {
+  constructor(readonly hue: number) {
+    super()
+  }
+
+  toDOM(view: EditorView): HTMLElement {
+    const element = document.createElement('span')
+    element.className = 'ol-cm-empty-line-addition-marker ol-cm-addition-marker'
+    element.style.setProperty('--hue', this.hue.toString())
+
+    return element
+  }
+}
+
+class EmptyLineDeletionMarkerWidget extends WidgetType {
+  constructor(readonly hue: number) {
+    super()
+  }
+
+  toDOM(view: EditorView): HTMLElement {
+    const element = document.createElement('span')
+    element.className = 'ol-cm-empty-line-deletion-marker ol-deletion-marker'
+    element.style.setProperty('--hue', this.hue.toString())
+    element.textContent = ' '
+
+    return element
+  }
+}
+
+function createMarkers(highlights: Highlight[]) {
+  return RangeSet.of(highlights.map(highlight => highlightToMarker(highlight)))
+}
+
+function createEmptyLineHighlightMarkers(lineStatuses: LineStatuses) {
+  const markers: Range<Decoration>[] = []
+  for (const lineStatus of lineStatuses.values()) {
+    if (lineStatus.line.length === 0) {
+      const highlight = lineStatus.highlights[0]
+      const widget =
+        highlight.type === 'addition'
+          ? new EmptyLineAdditionMarkerWidget(highlight.hue)
+          : new EmptyLineDeletionMarkerWidget(highlight.hue)
+
+      // In order to make the hover tooltip appear for every empty line,
+      // position the widget after the position if this is the first empty line
+      // in a group or before it otherwise. Always using a value of 1 would
+      // mean that the last empty line in a group of more than one would not
+      // trigger the hover tooltip.
+      const side =
+        lineStatuses.get(lineStatus.line.number - 1)?.highlights[0]?.type ===
+        highlight.type
+          ? -1
+          : 1
+
+      markers.push(
+        Decoration.widget({
+          widget,
+          side,
+        }).range(lineStatus.line.from)
+      )
     }
-    return highlightMarkers
-  },
-  provide: field => [
-    EditorView.decorations.from(field, highlights =>
-      Decoration.set(highlights.map(highlight => highlightToMarker(highlight)))
-    ),
-    theme,
-    hoverTooltip(tooltip, { hoverTime: 0 }),
-  ],
-})
+  }
+  return RangeSet.of(markers)
+}
+
+type HighlightDecorations = {
+  highlights: Highlight[]
+  highlightMarkers: DecorationSet
+  emptyLineHighlightMarkers: DecorationSet
+}
+
+export const highlightDecorationsField =
+  StateField.define<HighlightDecorations>({
+    create() {
+      return {
+        highlights: [],
+        highlightMarkers: Decoration.none,
+        emptyLineHighlightMarkers: Decoration.none,
+      }
+    },
+    update(highlightMarkers, tr) {
+      for (const effect of tr.effects) {
+        if (effect.is(setHighlightsEffect)) {
+          const highlights = effect.value
+          const lineStatuses = highlightedLines(highlights, tr.state)
+          const highlightMarkers = createMarkers(highlights)
+          const emptyLineHighlightMarkers =
+            createEmptyLineHighlightMarkers(lineStatuses)
+          return {
+            highlights,
+            highlightMarkers,
+            emptyLineHighlightMarkers,
+          }
+        }
+      }
+      return highlightMarkers
+    },
+    provide: field => [
+      EditorView.decorations.from(field, value => value.highlightMarkers),
+      EditorView.decorations.from(
+        field,
+        value => value.emptyLineHighlightMarkers
+      ),
+      theme,
+      hoverTooltip(tooltip, { hoverTime: 0 }),
+    ],
+  })
 
 export function highlights() {
-  return highlightsField
+  return highlightDecorationsField
 }

services/web/frontend/js/features/history/services/types/doc.ts

@@ -18,11 +18,13 @@ interface Range {
   to: number
 }
 
+export type HighlightType = 'addition' | 'deletion'
+
 export interface Highlight {
   label: string
   hue: number
   range: Range
-  type: 'addition' | 'deletion'
+  type: HighlightType
 }
 
 export type Diff = {

services/web/test/frontend/features/history/components/document-diff-viewer.spec.tsx

@@ -86,12 +86,12 @@ describe('document diff viewer', function () {
       </Container>
     )
 
-    cy.get('.ol-addition-marker').should('have.length', 1)
-    cy.get('.ol-addition-marker').first().as('addition')
+    cy.get('.ol-cm-addition-marker').should('have.length', 1)
+    cy.get('.ol-cm-addition-marker').first().as('addition')
     cy.get('@addition').should('have.text', 'article')
 
-    cy.get('.ol-deletion-marker').should('have.length', 1)
-    cy.get('.ol-deletion-marker').first().as('deletion')
+    cy.get('.ol-cm-deletion-marker').should('have.length', 1)
+    cy.get('.ol-cm-deletion-marker').first().as('deletion')
     cy.get('@deletion').should('have.text', 'Language')
 
     // Check hover tooltips
@@ -108,6 +108,69 @@ describe('document diff viewer', function () {
       .should('have.text', 'Deleted by Wombat on Tuesday')
   })
 
+  it('displays highlights with hover tooltips for empty lines', function () {
+    const scope = mockScope()
+
+    const doc = `1
+Addition
+
+
+End
+2
+Deletion
+
+End
+3`
+    const highlights: Highlight[] = [
+      {
+        type: 'addition',
+        range: { from: 2, to: 16 },
+        hue: 200,
+        label: 'Added by Wombat on Monday',
+      },
+      {
+        type: 'deletion',
+        range: { from: 19, to: 32 },
+        hue: 200,
+        label: 'Deleted by Wombat on Tuesday',
+      },
+    ]
+
+    cy.mount(
+      <Container>
+        <EditorProviders scope={scope}>
+          <DocumentDiffViewer doc={doc} highlights={highlights} />
+        </EditorProviders>
+      </Container>
+    )
+
+    cy.get('.ol-cm-empty-line-addition-marker').should('have.length', 2)
+
+    cy.get('.ol-cm-empty-line-deletion-marker').should('have.length', 1)
+    cy.get('.ol-cm-empty-line-deletion-marker').first().as('deletion')
+
+    // Check hover tooltips
+    cy.get('.ol-cm-empty-line-addition-marker').last().trigger('mousemove')
+    cy.get('.ol-cm-highlight-tooltip').should('have.length', 1)
+    cy.get('.ol-cm-highlight-tooltip')
+      .first()
+      .should('have.text', 'Added by Wombat on Monday')
+
+    cy.get('.ol-cm-empty-line-addition-marker').last().trigger('mouseleave')
+
+    cy.get('.ol-cm-empty-line-addition-marker').first().trigger('mousemove')
+    cy.get('.ol-cm-highlight-tooltip').should('have.length', 1)
+    cy.get('.ol-cm-highlight-tooltip')
+      .first()
+      .should('have.text', 'Added by Wombat on Monday')
+
+    cy.get('@deletion').trigger('mousemove')
+    cy.get('.ol-cm-highlight-tooltip').should('have.length', 1)
+    cy.get('.ol-cm-highlight-tooltip')
+      .first()
+      .should('have.text', 'Deleted by Wombat on Tuesday')
+  })
+
   it("renders 'More updates' buttons", function () {
     const scope = mockScope()
 
@@ -123,8 +186,8 @@ describe('document diff viewer', function () {
 
     // Check the initial state, which should be a "More updates below" button
     // but no "More updates above", with the editor scrolled to the top
-    cy.get('.ol-addition-marker').should('have.length', 1)
-    cy.get('.ol-deletion-marker').should('have.length', 1)
+    cy.get('.ol-cm-addition-marker').should('have.length', 1)
+    cy.get('.ol-cm-deletion-marker').should('have.length', 1)
     cy.get('.previous-highlight-button').should('have.length', 0)
     cy.get('.next-highlight-button').should('have.length', 1)
     cy.get('@scroller').invoke('scrollTop').should('equal', 0)
@@ -158,7 +221,7 @@ describe('document diff viewer', function () {
     )
 
     cy.get('.cm-scroller').first().invoke('scrollTop').should('not.equal', 0)
-    cy.get('.ol-addition-marker')
+    cy.get('.ol-cm-addition-marker')
       .first()
       .then($marker => {
         cy.get('.cm-content')