Remove extra padding for off-screen review panel entries (#19841)

* Remove extra padding for off-screen review panel entries

* use document.activeElement

* cleanup editor padding cases

* using isSelectionWithinOp

* fix formatting

* focusHandler function

* 1px border for focused entry

* use constants

* using isFirstEntry

GitOrigin-RevId: 4509f803b6cb907b40f1745a6fc7f3b1edfe145c

services/web/frontend/js/features/review-panel-new/components/review-panel-change.tsx

@@ -7,22 +7,20 @@ import {
 } from '../../../../../types/change'
 import { useTranslation } from 'react-i18next'
 import classnames from 'classnames'
-import { useCodeMirrorStateContext } from '@/features/source-editor/components/codemirror-editor'
 import { usePermissionsContext } from '@/features/ide-react/context/permissions-context'
-import { isFocused } from '../utils/is-focused'
 import { Button } from 'react-bootstrap'
 import Tooltip from '@/shared/components/tooltip'
 import MaterialIcon from '@/shared/components/material-icon'
 import { formatTimeBasedOnYear } from '@/features/utils/format-date'
 import { useChangesUsersContext } from '../context/changes-users-context'
 import { ReviewPanelChangeUser } from './review-panel-change-user'
+import { ReviewPanelEntry } from './review-panel-entry'
 
 export const ReviewPanelChange = memo<{
   change: Change<EditOperation>
   aggregate?: Change<DeleteOperation>
   top?: number
 }>(({ change, aggregate, top }) => {
-  const state = useCodeMirrorStateContext()
   const { t } = useTranslation()
   const { acceptChanges, rejectChanges } = useRangesActionsContext()
   const permissions = usePermissionsContext()
@@ -33,23 +31,16 @@ export const ReviewPanelChange = memo<{
     return null
   }
 
-  const focused = isFocused(change.op, state.selection.main)
-
   return (
-    <div
-      className={classnames('review-panel-entry', 'review-panel-entry-change', {
-        'review-panel-entry-focused': focused,
+    <ReviewPanelEntry
+      className={classnames('review-panel-entry-change', {
         'review-panel-entry-insert': 'i' in change.op,
         'review-panel-entry-delete': 'd' in change.op,
         // TODO: aggregate
       })}
-      data-top={top}
-      data-pos={change.op.p}
-      style={{
-        position: top === undefined ? 'relative' : 'absolute',
-        visibility: top === undefined ? 'visible' : 'hidden',
-        transition: 'top .3s, left .1s, right .1s',
-      }}
+      top={top}
+      op={change.op}
+      position={change.op.p}
     >
       <div className="review-panel-entry-indicator">
         <MaterialIcon type="edit" className="review-panel-entry-icon" />
@@ -166,7 +157,7 @@ export const ReviewPanelChange = memo<{
           )}
         </div>
       </div>
-    </div>
+    </ReviewPanelEntry>
   )
 })
 ReviewPanelChange.displayName = 'ReviewPanelChange'

services/web/frontend/js/features/review-panel-new/components/review-panel-comment.tsx

@@ -6,11 +6,10 @@ import {
   useThreadsActionsContext,
   useThreadsContext,
 } from '../context/threads-context'
-import { useCodeMirrorStateContext } from '@/features/source-editor/components/codemirror-editor'
 import classnames from 'classnames'
-import { isFocused } from '../utils/is-focused'
 import AutoExpandingTextArea from '@/shared/components/auto-expanding-text-area'
 import MaterialIcon from '@/shared/components/material-icon'
+import { ReviewPanelEntry } from './review-panel-entry'
 
 export const ReviewPanelComment = memo<{
   comment: Change<CommentOperation>
@@ -22,7 +21,6 @@ export const ReviewPanelComment = memo<{
   const [content, setContent] = useState('')
   const threads = useThreadsContext()
   const { resolveThread, addMessage } = useThreadsActionsContext()
-  const state = useCodeMirrorStateContext()
 
   const handleSubmitReply = useCallback(() => {
     setSubmitting(true)
@@ -52,25 +50,14 @@ export const ReviewPanelComment = memo<{
     return null
   }
 
-  const focused = isFocused(comment.op, state.selection.main)
-
   return (
-    <div
-      className={classnames(
-        'review-panel-entry',
-        'review-panel-entry-comment',
-        {
-          'review-panel-entry-loaded': !!threads?.[comment.op.t],
-          'review-panel-entry-focused': focused,
-        }
-      )}
-      data-top={top}
-      data-pos={comment.op.p}
-      style={{
-        position: top === undefined ? 'relative' : 'absolute',
-        visibility: top === undefined ? 'visible' : 'hidden',
-        transition: 'top .3s, left .1s, right .1s',
-      }}
+    <ReviewPanelEntry
+      className={classnames('review-panel-entry-comment', {
+        'review-panel-entry-loaded': !!threads?.[comment.op.t],
+      })}
+      top={top}
+      op={comment.op}
+      position={comment.op.p}
     >
       <div className="review-panel-entry-indicator">
         <MaterialIcon type="edit" className="review-panel-entry-icon" />
@@ -109,7 +96,7 @@ export const ReviewPanelComment = memo<{
 
         {error && <div>{error.message}</div>}
       </div>
-    </div>
+    </ReviewPanelEntry>
   )
 })
 ReviewPanelComment.displayName = 'ReviewPanelComment'

services/web/frontend/js/features/review-panel-new/components/review-panel-current-file.tsx

@@ -16,10 +16,7 @@ import {
   DeleteOperation,
   EditOperation,
 } from '../../../../../types/change'
-import {
-  editorOverflowPadding,
-  editorVerticalTopPadding,
-} from '@/features/source-editor/extensions/vertical-overflow'
+import { editorVerticalTopPadding } from '@/features/source-editor/extensions/vertical-overflow'
 import {
   useCodeMirrorStateContext,
   useCodeMirrorViewContext,
@@ -77,50 +74,25 @@ const ReviewPanelCurrentFile: FC = () => {
   )
 
   const containerRef = useRef<HTMLDivElement | null>(null)
-  const ignoreNextUpdateRef = useRef(false)
   const previousFocusedItem = useRef(0)
 
   const updatePositions = useCallback(() => {
-    if (ignoreNextUpdateRef.current) {
-      ignoreNextUpdateRef.current = false
-      return
-    }
-
     if (containerRef.current) {
       const extents = positionItems(
         containerRef.current,
-        view.scrollDOM as HTMLDivElement,
         previousFocusedItem.current
       )
 
       if (extents) {
         previousFocusedItem.current = extents.focusedItemIndex
-
-        window.setTimeout(() => {
-          const top = extents.min < 0 ? -extents.min : 0
-          const bottom =
-            extents.max > contentRect.bottom
-              ? extents.max - contentRect.bottom
-              : 0
-
-          const currentPadding = editorOverflowPadding(view)
-
-          if (
-            currentPadding?.top !== top ||
-            currentPadding?.bottom !== bottom
-          ) {
-            // ignoreNextUpdateRef.current = true
-            // view.dispatch(setVerticalOverflow({ top, bottom }))
-          }
-        })
       }
     }
-  }, [contentRect.bottom, view])
+  }, [])
 
   useEffect(() => {
     const timer = window.setTimeout(() => {
       updatePositions()
-    }, 100)
+    }, 50)
 
     return () => {
       window.clearTimeout(timer)

services/web/frontend/js/features/review-panel-new/components/review-panel-entry.tsx

@@ -0,0 +1,61 @@
+import { FC, useCallback, useState } from 'react'
+import { AnyOperation } from '../../../../../types/change'
+import {
+  useCodeMirrorStateContext,
+  useCodeMirrorViewContext,
+} from '@/features/source-editor/components/codemirror-editor'
+import { isSelectionWithinOp } from '../utils/is-selection-within-op'
+import { EditorSelection } from '@codemirror/state'
+import { EditorView } from '@codemirror/view'
+import classNames from 'classnames'
+
+export const ReviewPanelEntry: FC<{
+  position: number
+  op: AnyOperation
+  top?: number
+  className?: string
+}> = ({ children, position, top, op, className }) => {
+  const state = useCodeMirrorStateContext()
+  const view = useCodeMirrorViewContext()
+  const [focused, setFocused] = useState(false)
+
+  const highlighted = isSelectionWithinOp(op, state.selection.main)
+
+  const focusHandler = useCallback(() => {
+    setTimeout(() => {
+      // without setTimeout, error "EditorView.update are not allowed while an update is in progress" can occur
+      // this can be avoided by using onClick rather than onFocus but it will then not pick up <Tab> or <Shift+Tab> events for focusing entries
+      view.dispatch({
+        selection: EditorSelection.cursor(position),
+        effects: EditorView.scrollIntoView(position, { y: 'center' }),
+      })
+    }, 0)
+    setFocused(true)
+  }, [view, position])
+
+  return (
+    <div
+      onFocus={focusHandler}
+      onBlur={() => setFocused(false)}
+      role="button"
+      tabIndex={position + 1}
+      className={classNames(
+        'review-panel-entry',
+        {
+          'review-panel-entry-focused': focused,
+          'review-panel-entry-highlighted': highlighted,
+        },
+        className
+      )}
+      data-top={top}
+      data-pos={position}
+      style={{
+        position: top === undefined ? 'relative' : 'absolute',
+        visibility: top === undefined ? 'visible' : 'hidden',
+        transition: 'top .3s, left .1s, right .1s',
+      }}
+    >
+      {children}
+    </div>
+  )
+}

services/web/frontend/js/features/review-panel-new/utils/is-focused.ts → services/web/frontend/js/features/review-panel-new/utils/is-selection-within-op.ts

@@ -2,6 +2,9 @@ import { AnyOperation } from '../../../../../types/change'
 import { SelectionRange } from '@codemirror/state'
 import { visibleTextLength } from '@/utils/operations'
 
-export const isFocused = (op: AnyOperation, range: SelectionRange): boolean => {
+export const isSelectionWithinOp = (
+  op: AnyOperation,
+  range: SelectionRange
+): boolean => {
   return range.to >= op.p && range.from <= op.p + visibleTextLength(op)
 }

services/web/frontend/js/features/review-panel-new/utils/position-items.ts

@@ -1,13 +1,10 @@
 import { debounce } from 'lodash'
 
-export const positionItems = debounce(
-  (
-    element: HTMLDivElement,
-    containerElement: HTMLDivElement,
-    previousFocusedItemIndex: number
-  ) => {
-    const scrollRect = containerElement.getBoundingClientRect()
+const COLLAPSED_HEADER_HEIGHT = 75
+const OFFSET_FOR_ENTRIES_ABOVE = 70
 
+export const positionItems = debounce(
+  (element: HTMLDivElement, previousFocusedItemIndex: number) => {
     const items = Array.from(
       element.querySelectorAll<HTMLDivElement>('.review-panel-entry')
     )
@@ -21,19 +18,25 @@ export const positionItems = debounce(
     let focusedItemIndex = items.findIndex(item =>
       item.classList.contains('review-panel-entry-focused')
     )
+    if (focusedItemIndex === -1) {
+      // if entry was not focused manually
+      // check if there is an entry in selection and use that as the focused item
+      focusedItemIndex = items.findIndex(item =>
+        item.classList.contains('review-panel-entry-highlighted')
+      )
+    }
     if (focusedItemIndex === -1) {
       focusedItemIndex = previousFocusedItemIndex
     }
 
-    // TODO: editorPadding?
-    const topDiff = scrollRect.top - 80
-
     const focusedItem = items[focusedItemIndex]
     if (!focusedItem) {
       return
     }
-    const focusedItemTop = Number(focusedItem.dataset.top)
-    focusedItem.style.top = `${focusedItemTop + topDiff}px`
+
+    const focusedItemTop = getTopPosition(focusedItem, focusedItemIndex === 0)
+
+    focusedItem.style.top = `${focusedItemTop}px`
     focusedItem.style.visibility = 'visible'
     const focusedItemRect = focusedItem.getBoundingClientRect()
 
@@ -42,12 +45,12 @@ export const positionItems = debounce(
     for (let i = focusedItemIndex - 1; i >= 0; i--) {
       const item = items[i]
       const rect = item.getBoundingClientRect()
-      let top = Number(item.dataset.top)
+      let top = getTopPosition(item, i === 0)
       const bottom = top + rect.height
       if (bottom > topLimit) {
         top = topLimit - rect.height - 10
       }
-      item.style.top = `${top + topDiff}px`
+      item.style.top = `${top}px`
       item.style.visibility = 'visible'
       topLimit = top
     }
@@ -57,11 +60,11 @@ export const positionItems = debounce(
     for (let i = focusedItemIndex + 1; i < items.length; i++) {
       const item = items[i]
       const rect = item.getBoundingClientRect()
-      let top = Number(item.dataset.top)
+      let top = getTopPosition(item, false)
       if (top < bottomLimit) {
         top = bottomLimit + 10
       }
-      item.style.top = `${top + topDiff}px`
+      item.style.top = `${top}px`
       item.style.visibility = 'visible'
       bottomLimit = top + rect.height
     }
@@ -75,3 +78,8 @@ export const positionItems = debounce(
   100,
   { leading: false, trailing: true, maxWait: 1000 }
 )
+
+function getTopPosition(item: HTMLDivElement, isFirstEntry: boolean) {
+  const offset = isFirstEntry ? 0 : OFFSET_FOR_ENTRIES_ABOVE
+  return Math.max(COLLAPSED_HEADER_HEIGHT + offset, Number(item.dataset.top))
+}

services/web/frontend/stylesheets/app/editor/review-panel-new.less

@@ -33,9 +33,10 @@
     gap: @spacing-04;
   }
 
-  .review-panel-entry-focused {
+  .review-panel-entry-focused,
+  .review-panel-entry-highlighted {
     margin-left: @spacing-01;
-    border: 2px solid @blue-50;
+    border: 1px solid @blue-50;
   }
   .review-panel-entry-header {
     display: flex;