Merge pull request #14559 from overleaf/mj-table-add-row-borders

[visual] Table generator improvements

GitOrigin-RevId: 8d3d1b382c68c13480b3aa50b6764903ff59ae81

services/web/frontend/js/features/source-editor/components/table-generator/cell-input.tsx

@@ -6,7 +6,7 @@ interface CellInputProps
 }
 
 export type CellInputRef = {
-  focus: () => void
+  focus: (options?: FocusOptions) => void
 }
 
 export const CellInput = forwardRef<CellInputRef, CellInputProps>(
@@ -14,9 +14,9 @@ export const CellInput = forwardRef<CellInputRef, CellInputProps>(
     const inputRef = useRef<HTMLTextAreaElement>(null)
     useImperativeHandle(ref, () => {
       return {
-        focus() {
-          inputRef.current?.focus()
+        focus(options) {
           inputRef.current?.setSelectionRange(value.length, value.length)
+          inputRef.current?.focus(options)
         },
       }
     })

services/web/frontend/js/features/source-editor/components/table-generator/cell.tsx

@@ -11,6 +11,7 @@ import { typesetNodeIntoElement } from '../../extensions/visual/utils/typeset-co
 import { parser } from '../../lezer-latex/latex.mjs'
 import { useTableContext } from './contexts/table-context'
 import { CellInput, CellInputRef } from './cell-input'
+import { useCodeMirrorViewContext } from '../codemirror-editor'
 
 export const Cell: FC<{
   cellData: CellData
@@ -40,6 +41,7 @@ export const Cell: FC<{
     commitCellData,
   } = useEditingContext()
   const inputRef = useRef<CellInputRef>(null)
+  const view = useCodeMirrorViewContext()
 
   const editing =
     editingCellData?.rowIndex === rowIndex &&
@@ -132,6 +134,9 @@ export const Cell: FC<{
       .replaceAll(/(^&|[^\\]&)/g, match =>
         match.length === 1 ? '\\&' : `${match[0]}\\&`
       )
+      .replaceAll(/(^%|[^\\]%)/g, match =>
+        match.length === 1 ? '\\%' : `${match[0]}\\%`
+      )
       .replaceAll('\\\\', '')
   }, [])
 
@@ -142,7 +147,7 @@ export const Cell: FC<{
 
   useEffect(() => {
     if (isFocused && !editing && cellRef.current) {
-      cellRef.current.focus()
+      cellRef.current.focus({ preventScroll: true })
     }
   }, [isFocused, editing])
 
@@ -161,11 +166,12 @@ export const Cell: FC<{
         .then(async MathJax => {
           if (renderDiv.current) {
             await MathJax.typesetPromise([renderDiv.current])
+            view.requestMeasure()
           }
         })
         .catch(() => {})
     }
-  }, [cellData.content, editing])
+  }, [cellData.content, editing, view])
 
   const onInput = useCallback(
     e => {
@@ -227,6 +233,7 @@ export const Cell: FC<{
           inSelection && selection?.bordersLeft(rowIndex, columnIndex, table),
         'selection-edge-right':
           inSelection && selection?.bordersRight(rowIndex, columnIndex, table),
+        editing,
       })}
     >
       {body}

services/web/frontend/js/features/source-editor/components/table-generator/contexts/editing-context.tsx

@@ -57,6 +57,7 @@ export const EditingContextProvider: FC = ({ children }) => {
       view.dispatch({
         changes: { from, to, insert: content },
       })
+      view.requestMeasure()
       setCellData(null)
     },
     [view, table, initialContent]
@@ -68,6 +69,7 @@ export const EditingContextProvider: FC = ({ children }) => {
     }
     if (!cellData.dirty) {
       setCellData(null)
+      setInitialContent(undefined)
       return
     }
     const { rowIndex, cellIndex, content } = cellData

services/web/frontend/js/features/source-editor/components/table-generator/table.tsx

@@ -3,6 +3,7 @@ import {
   KeyboardEvent,
   KeyboardEventHandler,
   useCallback,
+  useEffect,
   useMemo,
   useRef,
 } from 'react'
@@ -15,6 +16,8 @@ import {
 import { useEditingContext } from './contexts/editing-context'
 import { useTableContext } from './contexts/table-context'
 import { useCodeMirrorViewContext } from '../codemirror-editor'
+import { undo, redo } from '@codemirror/commands'
+import { ChangeSpec } from '@codemirror/state'
 
 type NavigationKey =
   | 'ArrowRight'
@@ -28,6 +31,8 @@ type NavigationMap = {
   [key in NavigationKey]: [() => TableSelection, () => TableSelection]
 }
 
+const isMac = /Mac/.test(window.navigator?.platform)
+
 export const Table: FC = () => {
   const { selection, setSelection } = useSelectionContext()
   const {
@@ -70,6 +75,7 @@ export const Table: FC = () => {
 
   const isCharacterInput = useCallback((event: KeyboardEvent) => {
     return (
+      Boolean(event.code) && // is a keyboard key
       event.key?.length === 1 &&
       !event.ctrlKey &&
       !event.metaKey &&
@@ -79,6 +85,7 @@ export const Table: FC = () => {
 
   const onKeyDown: KeyboardEventHandler = useCallback(
     event => {
+      const commandKey = isMac ? event.metaKey : event.ctrlKey
       if (event.code === 'Enter' && !event.shiftKey) {
         event.preventDefault()
         event.stopPropagation()
@@ -116,6 +123,18 @@ export const Table: FC = () => {
         event.preventDefault()
         event.stopPropagation()
         clearCells(selection)
+        view.requestMeasure()
+        setTimeout(() => {
+          if (tableRef.current) {
+            const { minY } = selection.normalized()
+            const row = tableRef.current.querySelectorAll('tbody tr')[minY]
+            if (row) {
+              if (row.getBoundingClientRect().top < 0) {
+                row.scrollIntoView({ block: 'center' })
+              }
+            }
+          }
+        }, 0)
       } else if (Object.prototype.hasOwnProperty.call(navigation, event.code)) {
         const [defaultNavigation, shiftNavigation] =
           navigation[event.code as NavigationKey]
@@ -146,6 +165,21 @@ export const Table: FC = () => {
         startEditing(selection.to.row, selection.to.cell)
         updateCellData(event.key)
         setSelection(new TableSelection(selection.to, selection.to))
+      } else if (
+        !cellData &&
+        event.key === 'z' &&
+        !event.shiftKey &&
+        commandKey
+      ) {
+        event.preventDefault()
+        undo(view)
+      } else if (
+        !cellData &&
+        (event.key === 'y' ||
+          (event.key === 'Z' && event.shiftKey && commandKey))
+      ) {
+        event.preventDefault()
+        redo(view)
       }
     },
     [
@@ -163,6 +197,67 @@ export const Table: FC = () => {
       tableData,
     ]
   )
+
+  useEffect(() => {
+    const onPaste = (event: ClipboardEvent) => {
+      if (cellData || !selection) {
+        // We're editing a cell, so allow browser to insert there
+        return false
+      }
+      event.preventDefault()
+      const changes: ChangeSpec[] = []
+      const data = event.clipboardData?.getData('text/plain')
+      if (data) {
+        const rows = data.split('\n')
+        const { minX, minY } = selection.normalized()
+        for (let row = 0; row < rows.length; row++) {
+          if (tableData.rows.length <= minY + row) {
+            // TODO: Add rows
+            continue
+          }
+          const cells = rows[row].split('\t')
+          for (let cell = 0; cell < cells.length; cell++) {
+            if (tableData.columns.length <= minX + cell) {
+              // TODO: Add columns
+              continue
+            }
+            const cellData = tableData.getCell(minY + row, minX + cell)
+            changes.push({
+              from: cellData.from,
+              to: cellData.to,
+              insert: cells[cell],
+            })
+          }
+        }
+      }
+      view.dispatch({ changes })
+    }
+
+    const onCopy = (event: ClipboardEvent) => {
+      if (cellData || !selection) {
+        // We're editing a cell, so allow browser to insert there
+        return false
+      }
+      event.preventDefault()
+      const { minX, minY, maxX, maxY } = selection.normalized()
+      const content = []
+      for (let row = minY; row <= maxY; row++) {
+        const rowContent = []
+        for (let cell = minX; cell <= maxX; cell++) {
+          rowContent.push(tableData.getCell(row, cell).content)
+        }
+        content.push(rowContent.join('\t'))
+      }
+      navigator.clipboard.writeText(content.join('\n'))
+    }
+    window.addEventListener('paste', onPaste)
+    window.addEventListener('copy', onCopy)
+    return () => {
+      window.removeEventListener('paste', onPaste)
+      window.removeEventListener('copy', onCopy)
+    }
+  }, [cellData, selection, tableData, view])
+
   return (
     // eslint-disable-next-line jsx-a11y/no-noninteractive-element-interactions
     <table

services/web/frontend/js/features/source-editor/components/table-generator/utils.ts

@@ -133,6 +133,7 @@ function parseTabularBody(
   node: SyntaxNode,
   state: EditorState
 ): ParsedTableBody {
+  const firstChild = node.firstChild
   const body: ParsedTableBody = {
     rows: [
       {
@@ -155,7 +156,7 @@ function parseTabularBody(
     return getLastRow().cells[getLastRow().cells.length - 1]
   }
   for (
-    let currentChild: SyntaxNode | null = node;
+    let currentChild: SyntaxNode | null = firstChild;
     currentChild;
     currentChild = currentChild.nextSibling
   ) {
@@ -256,7 +257,8 @@ function parseTabularBody(
       continue
     } else if (
       currentChild.type.is('NewLine') ||
-      currentChild.type.is('Whitespace')
+      currentChild.type.is('Whitespace') ||
+      currentChild.type.is('Comment')
     ) {
       const lastCell = getLastCell()
       if (!lastCell?.multiColumn) {
@@ -306,11 +308,22 @@ function parseTabularBody(
     getLastRow().position.to = currentChild.to
   }
   const lastRow = getLastRow()
-  if (lastRow.cells.length === 1 && lastRow.cells[0].content.trim() === '') {
+  if (
+    body.rows.length > 1 &&
+    lastRow.cells.length === 1 &&
+    lastRow.cells[0].content.trim() === ''
+  ) {
     // Remove the last row if it's empty, but move hlines up to previous row
     const hlines = lastRow.hlines.map(hline => ({ ...hline, atBottom: true }))
     body.rows.pop()
     getLastRow().hlines.push(...hlines)
+    const lastLineContents = state.sliceDoc(
+      lastRow.position.from,
+      lastRow.position.to
+    )
+    const lastLineOffset =
+      lastLineContents.length - lastLineContents.trimEnd().length
+    getLastRow().position.to = lastRow.position.to - lastLineOffset
   }
   return body
 }
@@ -339,7 +352,7 @@ export function generateTable(
   const columns = parseColumnSpecifications(
     state.sliceDoc(specification.from, specification.to)
   )
-  const body = node.getChild('Content')?.getChild('TabularContent')?.firstChild
+  const body = node.getChild('Content')?.getChild('TabularContent')
   if (!body) {
     throw new Error('Missing table body')
   }

services/web/frontend/js/features/source-editor/extensions/visual/table-generator.ts

@@ -113,6 +113,8 @@ export const tableGeneratorTheme = EditorView.baseTheme({
       'border-bottom-color': 'var(--table-generator-active-border-color)',
       'border-bottom-width': 'var(--table-generator-active-border-width)',
     },
+    'overflow-x': 'auto',
+    'overflow-y': 'hidden',
   },
 
   '.table-generator-table': {
@@ -122,8 +124,11 @@ export const tableGeneratorTheme = EditorView.baseTheme({
     cursor: 'default',
 
     '& td': {
-      padding: '0 0.25em',
+      '&:not(.editing)': {
+        padding: '0 0.25em',
+      },
       'max-width': '200px',
+      'vertical-align': 'top',
 
       '&.alignment-left': {
         'text-align': 'left',
@@ -280,11 +285,16 @@ export const tableGeneratorTheme = EditorView.baseTheme({
     'background-color': 'transparent',
     width: '100%',
     height: '1.5em',
+    'min-height': '100%',
     border: '1px solid var(--table-generator-toolbar-shadow-color)',
-    padding: '0',
+    padding: '0 0.25em',
     resize: 'none',
     'box-sizing': 'border-box',
     overflow: 'hidden',
+    '&:focus, &:focus-visible': {
+      outline: '2px solid var(--table-generator-focus-border-color)',
+      'outline-offset': '-2px',
+    },
   },
 
   '.table-generator-border-options-coming-soon': {

services/web/frontend/js/features/source-editor/extensions/visual/utils/typeset-content.ts

@@ -1,20 +1,59 @@
 import { EditorState } from '@codemirror/state'
 import { SyntaxNode } from '@lezer/common'
+import { COMMAND_SUBSTITUTIONS } from '../visual-widgets/character'
 
 const isUnknownCommandWithName = (
   node: SyntaxNode,
   command: string,
   getText: (from: number, to: number) => string
 ): boolean => {
-  if (!node.type.is('UnknownCommand')) {
+  const commandName = getUnknownCommandName(node, getText)
+  if (commandName === undefined) {
     return false
   }
+  return commandName === command
+}
+
+const getUnknownCommandName = (
+  node: SyntaxNode,
+  getText: (from: number, to: number) => string
+): string | undefined => {
+  if (!node.type.is('UnknownCommand')) {
+    return undefined
+  }
   const commandNameNode = node.getChild('CtrlSeq')
   if (!commandNameNode) {
-    return false
+    return undefined
   }
   const commandName = getText(commandNameNode.from, commandNameNode.to)
-  return commandName === command
+  return commandName
+}
+
+type NodeMapping = {
+  elementType: keyof HTMLElementTagNameMap
+  className?: string
+}
+type MarkupMapping = {
+  [command: string]: NodeMapping
+}
+const MARKUP_COMMANDS: MarkupMapping = {
+  '\\textit': {
+    elementType: 'i',
+  },
+  '\\textbf': {
+    elementType: 'b',
+  },
+  '\\emph': {
+    elementType: 'em',
+  },
+  '\\texttt': {
+    elementType: 'span',
+    className: 'ol-cm-command-texttt',
+  },
+  '\\textsc': {
+    elementType: 'span',
+    className: 'ol-cm-command-textsc',
+  },
 }
 
 /**
@@ -56,28 +95,27 @@ export function typesetNodeIntoElement(
         ancestor().append(
           document.createTextNode(getText(from, childNode.from))
         )
-
         from = childNode.from
       }
-      if (isUnknownCommandWithName(childNode, '\\textit', getText)) {
-        pushAncestor(document.createElement('i'))
-        const textArgument = childNode.getChild('TextArgument')
-        from = textArgument?.getChild('LongArg')?.from ?? childNode.to
-      } else if (isUnknownCommandWithName(childNode, '\\textbf', getText)) {
-        pushAncestor(document.createElement('b'))
-        const textArgument = childNode.getChild('TextArgument')
-        from = textArgument?.getChild('LongArg')?.from ?? childNode.to
-      } else if (isUnknownCommandWithName(childNode, '\\emph', getText)) {
-        pushAncestor(document.createElement('em'))
-        const textArgument = childNode.getChild('TextArgument')
-        from = textArgument?.getChild('LongArg')?.from ?? childNode.to
-      } else if (isUnknownCommandWithName(childNode, '\\texttt', getText)) {
-        const spanElement = document.createElement('span')
-        spanElement.classList.add('ol-cm-command-texttt')
-        pushAncestor(spanElement)
-        const textArgument = childNode.getChild('TextArgument')
-        from = textArgument?.getChild('LongArg')?.from ?? childNode.to
-      } else if (isUnknownCommandWithName(childNode, '\\and', getText)) {
+
+      if (childNode.type.is('UnknownCommand')) {
+        const commandNameNode = childNode.getChild('CtrlSeq')
+        if (commandNameNode) {
+          const commandName = getText(commandNameNode.from, commandNameNode.to)
+          const mapping: NodeMapping | undefined = MARKUP_COMMANDS[commandName]
+          if (mapping) {
+            const element = document.createElement(mapping.elementType)
+            if (mapping.className) {
+              element.classList.add(mapping.className)
+            }
+            pushAncestor(element)
+            const textArgument = childNode.getChild('TextArgument')
+            from = textArgument?.getChild('LongArg')?.from ?? childNode.to
+            return
+          }
+        }
+      }
+      if (isUnknownCommandWithName(childNode, '\\and', getText)) {
         const spanElement = document.createElement('span')
         spanElement.classList.add('ol-cm-command-and')
         pushAncestor(spanElement)
@@ -94,16 +132,22 @@ export function typesetNodeIntoElement(
       } else if (childNode.type.is('LineBreak')) {
         ancestor().appendChild(document.createElement('br'))
         from = childNode.to
+      } else if (childNode.type.is('UnknownCommand')) {
+        const command = getText(childNode.from, childNode.to)
+        const symbol = COMMAND_SUBSTITUTIONS.get(command.trim())
+        if (symbol !== undefined) {
+          ancestor().append(document.createTextNode(symbol))
+          from = childNode.to
+          return false
+        }
       }
     },
     function leave(childNodeRef) {
       const childNode = childNodeRef.node
+      const commandName = getUnknownCommandName(childNode, getText)
       if (
-        isUnknownCommandWithName(childNode, '\\and', getText) ||
-        isUnknownCommandWithName(childNode, '\\textit', getText) ||
-        isUnknownCommandWithName(childNode, '\\textbf', getText) ||
-        isUnknownCommandWithName(childNode, '\\emph', getText) ||
-        isUnknownCommandWithName(childNode, '\\texttt', getText)
+        (commandName && Boolean(MARKUP_COMMANDS[commandName])) ||
+        isUnknownCommandWithName(childNode, '\\and', getText)
       ) {
         const typeSetElement = popAncestor()
         ancestor().appendChild(typeSetElement)

services/web/frontend/js/features/source-editor/extensions/visual/visual-widgets/character.ts

@@ -26,7 +26,7 @@ export class CharacterWidget extends WidgetType {
   }
 }
 
-const SUBSTITUTIONS = new Map([
+export const COMMAND_SUBSTITUTIONS = new Map([
   ['\\', ' '], // a trimmed \\ '
   ['\\%', '\u0025'],
   ['\\_', '\u005F'],
@@ -151,12 +151,12 @@ const SUBSTITUTIONS = new Map([
 export function createCharacterCommand(
   command: string
 ): CharacterWidget | undefined {
-  const substitution = SUBSTITUTIONS.get(command)
+  const substitution = COMMAND_SUBSTITUTIONS.get(command)
   if (substitution !== undefined) {
     return new CharacterWidget(substitution)
   }
 }
 
 export function hasCharacterSubstitution(command: string): boolean {
-  return SUBSTITUTIONS.has(command)
+  return COMMAND_SUBSTITUTIONS.has(command)
 }

services/web/frontend/js/features/source-editor/extensions/visual/visual-widgets/tabular.tsx

@@ -10,7 +10,7 @@ import {
 
 export class TabularWidget extends WidgetType {
   private element: HTMLElement | undefined
-  private readonly parseResult: ParsedTableData
+  private readonly parseResult: ParsedTableData | null = null
 
   constructor(
     private tabularNode: SyntaxNode,
@@ -19,10 +19,17 @@ export class TabularWidget extends WidgetType {
     state: EditorState
   ) {
     super()
-    this.parseResult = generateTable(tabularNode, state)
+    try {
+      this.parseResult = generateTable(tabularNode, state)
+    } catch (e) {
+      this.parseResult = null
+    }
   }
 
   isValid() {
+    if (!this.parseResult) {
+      return false
+    }
     for (const row of this.parseResult.table.rows) {
       const rowLength = row.cells.reduce(
         (acc, cell) => acc + (cell.multiColumn?.columnSpan ?? 1),
@@ -49,15 +56,17 @@ export class TabularWidget extends WidgetType {
     if (this.tableNode) {
       this.element.classList.add('ol-cm-environment-table')
     }
-    ReactDOM.render(
-      <Tabular
-        view={view}
-        tabularNode={this.tabularNode}
-        parsedTableData={this.parseResult}
-        tableNode={this.tableNode}
-      />,
-      this.element
-    )
+    if (this.parseResult) {
+      ReactDOM.render(
+        <Tabular
+          view={view}
+          tabularNode={this.tabularNode}
+          parsedTableData={this.parseResult}
+          tableNode={this.tableNode}
+        />,
+        this.element
+      )
+    }
     return this.element
   }
 
@@ -71,6 +80,9 @@ export class TabularWidget extends WidgetType {
   }
 
   updateDOM(dom: HTMLElement, view: EditorView): boolean {
+    if (!this.parseResult) {
+      return false
+    }
     this.element = dom
     ReactDOM.render(
       <Tabular

services/web/test/frontend/features/source-editor/components/codemirror-editor-table-generator.spec.tsx

@@ -315,15 +315,22 @@ cell 3 & cell 4 \\\\
       cy.get('.table-generator').findByText('cell 1').click()
       cy.get('.table-generator-floating-toolbar').as('toolbar').should('exist')
 
+      // Set border theme to "All borders" so that we can check that theme is
+      // preserved when adding new rows and columns
+      cy.get('@toolbar').findByText('No borders').click()
+      cy.get('.table-generator').findByText('All borders').click()
+
       cy.get('.table-generator').findByText('cell 1').click()
       cy.get('@toolbar').findByLabelText('Insert').click()
       cy.get('.table-generator').findByText('Insert column left').click()
       checkTable([['', 'cell 1']])
+      checkBordersWithNoMultiColumn([true, true], [true, true, true])
 
       cy.get('.table-generator').findByText('cell 1').click()
       cy.get('@toolbar').findByLabelText('Insert').click()
       cy.get('.table-generator').findByText('Insert column right').click()
       checkTable([['', 'cell 1', '']])
+      checkBordersWithNoMultiColumn([true, true], [true, true, true, true])
 
       cy.get('.table-generator').findByText('cell 1').click()
       cy.get('@toolbar').findByLabelText('Insert').click()
@@ -332,6 +339,10 @@ cell 3 & cell 4 \\\\
         ['', '', ''],
         ['', 'cell 1', ''],
       ])
+      checkBordersWithNoMultiColumn(
+        [true, true, true],
+        [true, true, true, true]
+      )
 
       cy.get('.table-generator').findByText('cell 1').click()
       cy.get('@toolbar').findByLabelText('Insert').click()
@@ -341,6 +352,10 @@ cell 3 & cell 4 \\\\
         ['', 'cell 1', ''],
         ['', '', ''],
       ])
+      checkBordersWithNoMultiColumn(
+        [true, true, true, true],
+        [true, true, true, true]
+      )
     })
 
     it('Removes the table on toolbar button click', function () {

services/web/test/frontend/features/source-editor/components/codemirror-editor-visual.spec.tsx

@@ -467,14 +467,11 @@ describe('<CodeMirrorEditor/> in Visual mode', function () {
       'title with <span class="ol-cm-command-texttt"><b>command</b></span>'
     )
 
-    // unsupported commands
     cy.get('@second-line').type(deleteLine)
     cy.get('@second-line').type('\\title{{}Title with \\& ampersands}')
-    cy.get('.ol-cm-title').should(
-      'contain.html',
-      'Title with \\&amp; ampersands'
-    )
+    cy.get('.ol-cm-title').should('contain.html', 'Title with &amp; ampersands')
 
+    // unsupported command
     cy.get('@second-line').type(deleteLine)
     cy.get('@second-line').type('\\title{{}My \\LaTeX{{}} document}')
     cy.get('.ol-cm-title').should('contain.html', 'My \\LaTeX{} document')