From fd26b37efd4b8593e7fd21981c5195dc43206fe4 Mon Sep 17 00:00:00 2001 From: Tim Down <158919+timdown@users.noreply.github.com> Date: Tue, 5 Sep 2023 15:46:13 +0100 Subject: [PATCH] Merge pull request #14628 from overleaf/td-review-panel-scroll-jump Fix scroll jump and improvements to React review panel layout and performance GitOrigin-RevId: 3123f90abeb1bf9712ba4025a272e13990aebbd9 --- .../entries/add-comment-entry.tsx | 6 +- .../review-panel/entries/comment-entry.tsx | 3 +- .../review-panel/hooks/use-indicator-hover.ts | 2 +- .../review-panel/positioned-entries.tsx | 307 ++++++++++-------- .../review-panel/types/review-panel-state.ts | 5 +- .../extensions/changes/change-manager.ts | 157 +++++++-- .../extensions/vertical-overflow.ts | 7 + .../controllers/ReviewPanelController.js | 37 ++- .../components/auto-expanding-text-area.tsx | 16 +- .../stylesheets/app/editor/review-panel.less | 2 +- services/web/types/review-panel/entry.ts | 1 + 11 files changed, 354 insertions(+), 189 deletions(-) diff --git a/services/web/frontend/js/features/source-editor/components/review-panel/entries/add-comment-entry.tsx b/services/web/frontend/js/features/source-editor/components/review-panel/entries/add-comment-entry.tsx index d4a4aef76d..acebac06a2 100644 --- a/services/web/frontend/js/features/source-editor/components/review-panel/entries/add-comment-entry.tsx +++ b/services/web/frontend/js/features/source-editor/components/review-panel/entries/add-comment-entry.tsx @@ -27,20 +27,20 @@ function AddCommentEntry({ entryId }: AddCommentEntryProps) { const handleStartNewComment = () => { setIsAddingComment(true) - window.setTimeout(handleLayoutChange, 0) + handleLayoutChange({ async: true }) } const handleSubmitNewComment = () => { submitNewComment(content) setIsAddingComment(false) setContent('') - window.setTimeout(handleLayoutChange, 0) + handleLayoutChange({ async: true }) } const handleCancelNewComment = () => { setIsAddingComment(false) setContent('') - window.setTimeout(handleLayoutChange, 0) + handleLayoutChange({ async: true }) } useEffect(() => { diff --git a/services/web/frontend/js/features/source-editor/components/review-panel/entries/comment-entry.tsx b/services/web/frontend/js/features/source-editor/components/review-panel/entries/comment-entry.tsx index 50e2f178a6..52937e66a7 100644 --- a/services/web/frontend/js/features/source-editor/components/review-panel/entries/comment-entry.tsx +++ b/services/web/frontend/js/features/source-editor/components/review-panel/entries/comment-entry.tsx @@ -111,8 +111,7 @@ function CommentEntry({ useEffect(() => { if (!submitting) { // Ensure everything is rendered in the DOM before updating the layout. - // Having to use a timeout seems less than ideal. - window.setTimeout(handleLayoutChange, 0) + handleLayoutChange({ async: true }) } }, [submitting, handleLayoutChange]) diff --git a/services/web/frontend/js/features/source-editor/components/review-panel/hooks/use-indicator-hover.ts b/services/web/frontend/js/features/source-editor/components/review-panel/hooks/use-indicator-hover.ts index a78a3585e7..1a6e25da62 100644 --- a/services/web/frontend/js/features/source-editor/components/review-panel/hooks/use-indicator-hover.ts +++ b/services/web/frontend/js/features/source-editor/components/review-panel/hooks/use-indicator-hover.ts @@ -30,7 +30,7 @@ function useIndicatorHover() { setHoverCoords(null) setLayoutSuspended(false) }) - handleLayoutChange(true) + handleLayoutChange({ force: true }) } }, [handleLayoutChange, layoutToLeft, reviewPanelOpen, setLayoutSuspended]) diff --git a/services/web/frontend/js/features/source-editor/components/review-panel/positioned-entries.tsx b/services/web/frontend/js/features/source-editor/components/review-panel/positioned-entries.tsx index eb3d42ee55..adcbea387a 100644 --- a/services/web/frontend/js/features/source-editor/components/review-panel/positioned-entries.tsx +++ b/services/web/frontend/js/features/source-editor/components/review-panel/positioned-entries.tsx @@ -14,6 +14,7 @@ import { isEqual } from 'lodash' type Positions = { entryTop: number callout: { top: number; height: number; inverted: boolean } + inViewport: boolean } type EntryView = { @@ -25,13 +26,26 @@ type EntryView = { layout: HTMLElement height: number entry: ReviewPanelEntry - visible: boolean + hasScreenPos: boolean positions?: Positions previousPositions?: Positions } type EntryPositions = Pick +type PositionedEntriesProps = { + entries: Array<[keyof ReviewPanelDocEntries, ReviewPanelEntry]> + contentHeight: number + children: React.ReactNode +} + +const initialLayoutInfo = { + focusedEntryIndex: 0, + overflowTop: 0, + height: 0, + positions: [] as EntryPositions[], +} + function css(el: HTMLElement, props: React.CSSProperties) { Object.assign(el.style, props) } @@ -58,43 +72,131 @@ function positionsEqual( return isEqual(entryPos1, entryPos2) } -const calculateEntryViewPositions = ( +function updateEntryPositions( + entryView: EntryView, + entryTop: number, + lineHeight: number +) { + const callout = calculateCalloutPosition( + entryView.entry.screenPos, + entryTop, + lineHeight + ) + entryView.positions = { + entryTop, + callout, + inViewport: entryView.entry.inViewport, + } +} + +function calculateEntryViewPositions( entryViews: EntryView[], lineHeight: number, calculateTop: (originalTop: number, height: number) => number -) => { +) { for (const entryView of entryViews) { - entryView.wrapper.classList.toggle('rp-entry-hidden', !entryView.visible) - if (entryView.visible) { + if (entryView.hasScreenPos) { const entryTop = calculateTop( entryView.entry.screenPos.y, entryView.height ) - const callout = calculateCalloutPosition( - entryView.entry.screenPos, - entryTop, - lineHeight - ) - entryView.positions = { - entryTop, - callout, - } + updateEntryPositions(entryView, entryTop, lineHeight) } - debugConsole.log('ENTRY', { entry: entryView.entry, top }) } } -type PositionedEntriesProps = { - entries: Array<[keyof ReviewPanelDocEntries, ReviewPanelEntry]> - contentHeight: number - children: React.ReactNode +function hideOrShowEntries(entryViews: EntryView[]) { + for (const entryView of entryViews) { + // Completely hide any entry that has no screen position + entryView.wrapper.classList.toggle( + 'rp-entry-hidden', + !entryView.hasScreenPos + ) + } } -const initialLayoutInfo = { - focusedEntryIndex: 0, - overflowTop: 0, - height: 0, - positions: [] as EntryPositions[], +function applyEntryTop(entryView: EntryView, top: number) { + entryView.box.style.top = top + 'px' + + if (entryView.indicator) { + entryView.indicator.style.top = top + 'px' + } +} + +function applyEntryVisibility(entryView: EntryView) { + // The entry element is invisible by default, to avoid flickering when + // positioning for the first time. Here we make sure it becomes visible after + // acquiring a screen position. + if (entryView.entry.inViewport) { + entryView.box.style.visibility = 'visible' + entryView.callout.style.visibility = 'visible' + } +} + +// Position everything where it was before, taking into account the new top +// overflow +function moveEntriesToInitialPosition( + entryViews: EntryView[], + overflowTop: number +) { + for (const entryView of entryViews) { + const { callout: calloutEl, positions } = entryView + if (positions) { + const { entryTop, callout } = positions + + // Position the main wrapper in its original position, if it had + // one, or its new position otherwise + const entryTopInitial = entryView.previousPositions + ? entryView.previousPositions.entryTop + : entryTop + + applyEntryVisibility(entryView) + applyEntryTop(entryView, entryTopInitial + overflowTop) + + // Position the callout element in its original position, if it had + // one, or its new position otherwise + calloutEl.classList.toggle('rp-entry-callout-inverted', callout.inverted) + const calloutTopInitial = entryView.previousPositions + ? entryView.previousPositions.callout.top + : callout.top + + css(calloutEl, { + top: calloutTopInitial + overflowTop + 'px', + height: callout.height + 'px', + }) + } + } +} + +function moveEntriesToFinalPositions( + entryViews: EntryView[], + overflowTop: number, + shouldApplyVisibility: boolean +) { + for (const entryView of entryViews) { + const { callout: calloutEl, positions } = entryView + if (positions) { + const { entryTop, callout } = positions + + if (shouldApplyVisibility) { + applyEntryVisibility(entryView) + } + + // Position the main wrapper, if it's moved + if (entryView.previousPositions?.entryTop !== entryTop) { + entryView.box.style.top = entryTop + overflowTop + 'px' + } + + if (entryView.indicator) { + entryView.indicator.style.top = entryTop + overflowTop + 'px' + } + + // Position the callout element + if (entryView.previousPositions?.callout.top !== callout.top) { + calloutEl.style.top = callout.top + overflowTop + 'px' + } + } + } } function PositionedEntries({ @@ -106,10 +208,9 @@ function PositionedEntries({ useReviewPanelValueContext() const containerRef = useRef(null) const { reviewPanelOpen } = useLayoutContext() - const animationTimerRef = useRef(null) const previousLayoutInfoRef = useRef(initialLayoutInfo) - const layout = () => { + const layout = (animate = true) => { const container = containerRef.current if (!container) { return @@ -150,7 +251,7 @@ function PositionedEntries({ const previousPositions = previousLayoutInfoRef.current?.positions.find( pos => pos.entryId === entryId )?.positions - const visible = Boolean(entry.screenPos) + const hasScreenPos = Boolean(entry.screenPos) entryViews.push({ entryId, wrapper, @@ -158,7 +259,7 @@ function PositionedEntries({ box, callout, layout: layoutElement, - visible, + hasScreenPos, height: 0, entry, previousPositions, @@ -184,29 +285,26 @@ function PositionedEntries({ // - Measure the height of each entry // - Move each entry without animation to their original position // relative to the editor content - // - In the next animation frame, re-enable animation and position each - // entry + // - Re-enable animation and position each entry // // The idea is to batch DOM reads and writes to avoid layout thrashing. In // this case, the best we can do is a write phase, a read phase then a // final write phase. // See https://web.dev/avoid-large-complex-layouts-and-layout-thrashing/ - // First, update visibility for each entry that needs it - for (const entryView of entryViews) { - entryView.wrapper.classList.toggle('rp-entry-hidden', !entryView.visible) - } + // First, update display for each entry that needs it + hideOrShowEntries(entryViews) // Next, measure the height of each entry for (const entryView of entryViews) { - if (entryView.visible) { + if (entryView.hasScreenPos) { entryView.height = entryView.layout.offsetHeight } } - // Calculate positions for all visible entries, starting by calculating the - // entry to put in its desired position and anchor everything else around. - // If there is an explicitly focused entry, use that. + // Calculate positions for all positioned entries, starting by calculating + // which entry to put in its desired position and anchor everything else + // around. If there is an explicitly focused entry, use that. let focusedEntryIndex = entryViews.findIndex(view => view.entry.focused) if (focusedEntryIndex === -1) { // There is no explicitly focused entry, so use the focused entry from the @@ -216,30 +314,30 @@ function PositionedEntries({ previousLayoutInfoRef.current.focusedEntryIndex, entryViews.length - 1 ) - // If the entry from the previous layout is not visible, fall back to the - // first visible entry in the list - if (!entryViews[focusedEntryIndex].visible) { - focusedEntryIndex = entryViews.findIndex(view => view.visible) + // If the entry from the previous layout has no screen position, fall back + // to the first entry in the list that does. + if (!entryViews[focusedEntryIndex].hasScreenPos) { + focusedEntryIndex = entryViews.findIndex(view => view.hasScreenPos) } } - // If there is no visible entry, bail out + // If there is no entry with a screen position, bail out if (focusedEntryIndex === -1) { return } const focusedEntryView = entryViews[focusedEntryIndex] - if (!focusedEntryView.visible) { + if (!focusedEntryView.hasScreenPos) { return } // If the focused entry has no screenPos, we can't position other // entryViews relative to it, so we position all other entryViews as // though the focused entry is at the top and the rest follow it - const entryViewsAfter = focusedEntryView.visible + const entryViewsAfter = focusedEntryView.hasScreenPos ? entryViews.slice(focusedEntryIndex + 1) : [...entryViews] - const entryViewsBefore = focusedEntryView.visible + const entryViewsBefore = focusedEntryView.hasScreenPos ? entryViews.slice(0, focusedEntryIndex).reverse() // Work through backwards, starting with the one just above : [] @@ -248,19 +346,11 @@ function PositionedEntries({ let lastEntryBottom = 0 let firstEntryTop = 0 - // Put the focused entry as close to where it wants to be as possible - if (focusedEntryView.visible) { + // Put the focused entry as close as possible to where it wants to be + if (focusedEntryView.hasScreenPos) { const focusedEntryScreenPos = focusedEntryView.entry.screenPos const entryTop = Math.max(focusedEntryScreenPos.y, toolbarPaddedHeight) - const callout = calculateCalloutPosition( - focusedEntryScreenPos, - entryTop, - lineHeight - ) - focusedEntryView.positions = { - entryTop, - callout, - } + updateEntryPositions(focusedEntryView, entryTop, lineHeight) lastEntryBottom = entryTop + focusedEntryView.height firstEntryTop = entryTop } @@ -292,80 +382,6 @@ function PositionedEntries({ // Calculate the new top overflow const overflowTop = Math.max(0, toolbarPaddedHeight - firstEntryTop) - // Position everything where it was before, taking into account the new top - // overflow - const moveEntriesToInitialPosition = () => { - // Prevent CSS animation of position for this phase - container.classList.add('no-animate') - for (const entryView of entryViews) { - const { callout: calloutEl, positions } = entryView - if (positions) { - const { entryTop, callout } = positions - - // Position the main wrapper in its original position, if it had - // one, or its new position otherwise - const entryTopInitial = entryView.previousPositions - ? entryView.previousPositions.entryTop - : entryTop - - css(entryView.box, { - top: entryTopInitial + overflowTop + 'px', - // The entry element is invisible by default, to avoid flickering - // when positioning for the first time. Here we make sure it becomes - // visible after having a "top" value. - visibility: 'visible', - }) - - if (entryView.indicator) { - entryView.indicator.style.top = entryTopInitial + overflowTop + 'px' - } - - // Position the callout element in its original position, if it had - // one, or its new position otherwise - calloutEl.classList.toggle( - 'rp-entry-callout-inverted', - callout.inverted - ) - const calloutTopInitial = entryView.previousPositions - ? entryView.previousPositions.callout.top - : callout.top - - css(calloutEl, { - top: calloutTopInitial + overflowTop + 'px', - height: callout.height + 'px', - }) - } - } - } - - const moveToFinalPositions = () => { - // Re-enable CSS animation of position for this phase - container.classList.remove('no-animate') - - for (const entryView of entryViews) { - const { callout: calloutEl, positions } = entryView - if (positions) { - const { entryTop, callout } = positions - - // Position the main wrapper, if it's moved - if (entryView.previousPositions?.entryTop !== entryTop) { - entryView.box.style.top = entryTop + overflowTop + 'px' - } - - if (entryView.indicator) { - entryView.indicator.style.top = entryTop + overflowTop + 'px' - } - - // Position the callout element - if (entryView.previousPositions?.callout.top !== callout.top) { - calloutEl.style.top = callout.top + overflowTop + 'px' - } - } - } - - animationTimerRef.current = null - } - // Check whether the positions of any entry have changed since the last // layout const positions = entryViews.map( @@ -386,10 +402,13 @@ function PositionedEntries({ const height = lastEntryBottom + navPaddedHeight const heightChanged = height !== previousLayoutInfoRef.current.height + const isMoveRequired = positionsChanged || overflowTopChanged - // Move entries into their initial positions - if (positionsChanged || overflowTopChanged) { - moveEntriesToInitialPosition() + // Move entries into their initial positions, if animating, avoiding + // animation until the final animated move + if (animate && isMoveRequired) { + container.classList.add('no-animate') + moveEntriesToInitialPosition(entryViews, overflowTop) } // Inform the editor of the new top overflow and/or height if either has @@ -409,8 +428,20 @@ function PositionedEntries({ } // Do the final move - if (positionsChanged || overflowTopChanged) { - moveToFinalPositions() + if (isMoveRequired) { + if (animate) { + container.classList.remove('no-animate') + moveEntriesToFinalPositions(entryViews, overflowTop, false) + } else { + container.classList.add('no-animate') + moveEntriesToFinalPositions(entryViews, overflowTop, true) + + // Force reflow now to ensure that entries are moved without animation + // eslint-disable-next-line no-void + void container.offsetHeight + + container.classList.remove('no-animate') + } } previousLayoutInfoRef.current = { @@ -427,7 +458,7 @@ function PositionedEntries({ if (event.detail.force) { previousLayoutInfoRef.current = initialLayoutInfo } - layout() + layout(event.detail.animate) } }) @@ -437,7 +468,7 @@ function PositionedEntries({ dispatchReviewPanelLayout() }, []) - // Ensure layout is performed after opening or closing the review panel + // Ensure a full layout is performed after opening or closing the review panel useEffect(() => { previousLayoutInfoRef.current = initialLayoutInfo }, [reviewPanelOpen]) diff --git a/services/web/frontend/js/features/source-editor/context/review-panel/types/review-panel-state.ts b/services/web/frontend/js/features/source-editor/context/review-panel/types/review-panel-state.ts index 9917fd9e79..37917f3572 100644 --- a/services/web/frontend/js/features/source-editor/context/review-panel/types/review-panel-state.ts +++ b/services/web/frontend/js/features/source-editor/context/review-panel/types/review-panel-state.ts @@ -11,6 +11,7 @@ import { DocId, MainDocument, } from '../../../../../../../types/project-settings' +import { dispatchReviewPanelLayout } from '../../../extensions/changes/change-manager' /* eslint-disable no-use-before-define */ export interface ReviewPanelState { @@ -49,7 +50,9 @@ export interface ReviewPanelState { } updaterFns: { handleSetSubview: (subView: SubView) => void - handleLayoutChange: (force?: boolean) => void + handleLayoutChange: ( + ...args: Parameters + ) => void gotoEntry: (docId: DocId, entryOffset: number) => void resolveComment: (docId: DocId, entryId: ThreadId) => void deleteComment: (threadId: ThreadId, commentId: CommentId) => void diff --git a/services/web/frontend/js/features/source-editor/extensions/changes/change-manager.ts b/services/web/frontend/js/features/source-editor/extensions/changes/change-manager.ts index 0e587b4108..526d8c7bd0 100644 --- a/services/web/frontend/js/features/source-editor/extensions/changes/change-manager.ts +++ b/services/web/frontend/js/features/source-editor/extensions/changes/change-manager.ts @@ -2,8 +2,9 @@ import { trackChangesAnnotation } from '../realtime' import { clearChangeMarkers, buildChangeMarkers } from '../track-changes' import { setVerticalOverflow, - updateSetsVerticalOverflow, editorVerticalTopPadding, + updateChangesTopPadding, + updateSetsVerticalOverflow, } from '../vertical-overflow' import { EditorSelection, EditorState } from '@codemirror/state' import { EditorView, ViewUpdate } from '@codemirror/view' @@ -31,23 +32,53 @@ export const dispatchEditorEvent = (type: string, payload?: unknown) => { }, 0) } -export const dispatchReviewPanelLayout = (force = false) => { +const dispatchReviewPanelLayoutImmediately = ({ + force = false, + animate = true, +} = {}) => { window.dispatchEvent( - new CustomEvent('review-panel:layout', { detail: { force } }) + new CustomEvent('review-panel:layout', { detail: { force, animate } }) ) } const scheduleDispatchReviewPanelLayout = debounce( - dispatchReviewPanelLayout, + dispatchReviewPanelLayoutImmediately, 10 ) +/** + * @param force If true, forces the entries to be repositioned + * @param animate + * @param async If true, calls are briefly delayed and debounced + */ +export const dispatchReviewPanelLayout = ({ + force = false, + animate = true, + async = false, +} = {}) => { + if (async) { + scheduleDispatchReviewPanelLayout({ force, animate }) + } else { + dispatchReviewPanelLayoutImmediately({ force, animate }) + } +} + export type ChangeManager = { initialize: () => void handleUpdate: (update: ViewUpdate) => void destroy: () => void } +type UpdateType = + | 'edit' + | 'selectionChange' + | 'geometryChange' + | 'viewportChange' + | 'acceptChanges' + | 'rejectChanges' + | 'trackedChangesChange' + | 'topPaddingChange' + export const createChangeManager = ( view: EditorView, currentDoc: CurrentDoc @@ -58,7 +89,13 @@ export const createChangeManager = ( * * Returns a boolean indicating whether the visibility of any entry has changed */ - const recalculateScreenPositions = (entries?: Record) => { + const recalculateScreenPositions = ({ + entries, + updateType, + }: { + entries?: Record + updateType: UpdateType + }) => { const contentRect = view.contentDOM.getBoundingClientRect() const { doc } = view.state @@ -88,8 +125,9 @@ export const createChangeManager = ( } entry.screenPos = { y: y + offsetTop, height, editorPaddingTop } + entry.inViewport = true } else { - entry.screenPos = null + entry.inViewport = false } if (allVisible) { @@ -114,7 +152,7 @@ export const createChangeManager = ( } } - return visibilityChanged + return { visibilityChanged, updateType } } /** @@ -303,15 +341,29 @@ export const createChangeManager = ( } case 'recalculate-screen-positions': { - const changed = recalculateScreenPositions(payload) - if (changed) { + const { visibilityChanged, updateType } = + recalculateScreenPositions(payload) + if (visibilityChanged) { dispatchEditorEvent('track-changes:visibility_changed') } // Ensure the layout is updated once the review panel entries have // updated in the React review panel. The use of a timeout is bad but // the timings are a bit of a mess and will be improved when the review - // panel state is migrated away from Angular - scheduleDispatchReviewPanelLayout() + // panel state is migrated away from Angular. Entries are not animated + // into position when scrolling, or when the editor geometry changes + // (e.g. because the window has been resized), or when the top padding + // is adjusted + const nonAnimatingUpdateTypes: UpdateType[] = [ + 'viewportChange', + 'geometryChange', + 'topPaddingChange', + ] + const animate = !nonAnimatingUpdateTypes.includes(updateType) + dispatchReviewPanelLayout({ + async: true, + animate, + force: false, // updateType === 'geometryChange', + }) break } @@ -321,7 +373,7 @@ export const createChangeManager = ( broadcastChange() // Dispatch a focus:changed event to force the Angular controller to // reassemble the list of entries without bulk actions - dispatchFocusChangedEvent(view.state) + scheduleDispatchFocusChanged(view.state, 'acceptChanges') break } @@ -330,7 +382,7 @@ export const createChangeManager = ( broadcastChange() // Dispatch a focus:changed event to force the Angular controller to // reassemble the list of entries without bulk actions - dispatchFocusChangedEvent(view.state) + scheduleDispatchFocusChanged(view.state, 'rejectChanges') break } @@ -368,17 +420,23 @@ export const createChangeManager = ( } case 'sizes': { + const editorFullContentHeight = view.contentDOM.clientHeight // the content height and top overflow of the review panel const { height, overflowTop } = payload // the difference between the review panel height and the editor content height - const heightDiff = height + overflowTop - view.contentDOM.clientHeight + const heightDiff = height + overflowTop - editorFullContentHeight // the height of the block added at the top of the editor to match the review panel const topPadding = editorVerticalTopPadding(view) + const bottomPadding = view.documentPadding.bottom + const contentHeight = + editorFullContentHeight - (topPadding + bottomPadding) + const newBottomPadding = height - contentHeight + if (overflowTop !== topPadding || heightDiff !== 0) { view.dispatch( setVerticalOverflow({ top: overflowTop, - bottom: heightDiff + view.documentPadding.bottom, + bottom: newBottomPadding, }) ) } @@ -396,13 +454,27 @@ export const createChangeManager = ( * tell the review panel to update. * * @param state object + * @param updateType UpdateType */ - const dispatchFocusChangedEvent = debounce((state: EditorState) => { + const dispatchFocusChangedImmediately = ( + state: EditorState, + updateType: UpdateType + ) => { // TODO: multiple selections? const { from, to, empty } = state.selection.main - dispatchEditorEvent('focus:changed', { from, to, empty }) - }, 50) + dispatchEditorEvent('focus:changed', { + from, + to, + empty, + updateType, + }) + } + + const scheduleDispatchFocusChanged = debounce( + dispatchFocusChangedImmediately, + 50 + ) /** * When the editor is scrolled, tell the review panel so it can scroll in sync. @@ -478,15 +550,25 @@ export const createChangeManager = ( broadcastChange() }, handleUpdate(update: ViewUpdate) { - if (update.geometryChanged && !update.docChanged) { + const changesTopPadding = updateChangesTopPadding(update) + const { + geometryChanged, + viewportChanged, + docChanged, + focusChanged, + selectionSet, + } = update + const setsVerticalOverflow = updateSetsVerticalOverflow(update) + const ignoringGeometryChanges = Date.now() < ignoreGeometryChangesUntil + + if (geometryChanged && !docChanged && !ignoringGeometryChanges) { broadcastChange() } - if (updateSetsVerticalOverflow(update)) { - ignoreGeometryChangesUntil = Date.now() + 50 // ignore changes for 50ms - } else if ( - (update.geometryChanged || update.viewportChanged) && - Date.now() < ignoreGeometryChangesUntil + if ( + !setsVerticalOverflow && + (geometryChanged || viewportChanged) && + ignoringGeometryChanges ) { // Ignore a change to the editor geometry or viewport that occurs immediately after // an update to the vertical padding because otherwise it triggers @@ -495,14 +577,25 @@ export const createChangeManager = ( return } - if ( - update.docChanged || - update.focusChanged || - update.viewportChanged || - update.selectionSet || - update.geometryChanged - ) { - dispatchFocusChangedEvent(update.state) + if (changesTopPadding) { + scheduleDispatchFocusChanged(update.state, 'topPaddingChange') + } else if (docChanged) { + scheduleDispatchFocusChanged(update.state, 'edit') + } else if (focusChanged || selectionSet) { + scheduleDispatchFocusChanged(update.state, 'selectionChange') + } else if (viewportChanged && !geometryChanged) { + // It's better to respond immediately to a viewport change, which + // happens when scrolling, and have previously unpositioned entries + // appear immediately rather than risk a delay due to debouncing + dispatchFocusChangedImmediately(update.state, 'viewportChange') + } else if (geometryChanged) { + scheduleDispatchFocusChanged(update.state, 'geometryChange') + } + + // Wait until after updating the review panel layout before starting the + // interval during which to ignore geometry update + if (setsVerticalOverflow) { + ignoreGeometryChangesUntil = Date.now() + 50 } }, destroy() { diff --git a/services/web/frontend/js/features/source-editor/extensions/vertical-overflow.ts b/services/web/frontend/js/features/source-editor/extensions/vertical-overflow.ts index 1a46311082..96708d9bb7 100644 --- a/services/web/frontend/js/features/source-editor/extensions/vertical-overflow.ts +++ b/services/web/frontend/js/features/source-editor/extensions/vertical-overflow.ts @@ -235,6 +235,13 @@ export function updateSetsVerticalOverflow(update: ViewUpdate): boolean { }) } +export function updateChangesTopPadding(update: ViewUpdate): boolean { + return ( + update.state.field(overflowPaddingState).top !== + update.startState.field(overflowPaddingState).top + ) +} + export function editorVerticalTopPadding(view: EditorView): number { return view.state.field(overflowPaddingState).top } diff --git a/services/web/frontend/js/ide/review-panel/controllers/ReviewPanelController.js b/services/web/frontend/js/ide/review-panel/controllers/ReviewPanelController.js index 9c68717b86..27173bc734 100644 --- a/services/web/frontend/js/ide/review-panel/controllers/ReviewPanelController.js +++ b/services/web/frontend/js/ide/review-panel/controllers/ReviewPanelController.js @@ -554,7 +554,14 @@ export default App.controller( const entries = updateEntries(doc_id) $scope.$broadcast('review-panel:recalculate-screen-positions') - dispatchReviewPanelEvent('recalculate-screen-positions', entries) + dispatchReviewPanelEvent('recalculate-screen-positions', { + entries, + updateType: 'trackedChangesChange', + }) + + // Ensure that watchers, such as the React-based review panel component, + // are informed of the changes to entries + ide.$scope.$apply() return ide.$scope.$broadcast('review-panel:layout') }) @@ -565,7 +572,13 @@ export default App.controller( $scope.$on( 'editor:focus:changed', - function (e, selection_offset_start, selection_offset_end, selection) { + function ( + e, + selection_offset_start, + selection_offset_end, + selection, + updateType = null + ) { const doc_id = $scope.editor.open_doc_id const entries = getDocEntries(doc_id) // All selected changes will be added to this array. @@ -655,7 +668,10 @@ export default App.controller( $scope.$broadcast('review-panel:recalculate-screen-positions') - dispatchReviewPanelEvent('recalculate-screen-positions', entries) + dispatchReviewPanelEvent('recalculate-screen-positions', { + entries, + updateType, + }) // Ensure that watchers, such as the React-based review panel component, // are informed of the changes to entries @@ -1264,6 +1280,11 @@ export default App.controller( } } ide.$scope.reviewPanel.commentThreads = threads + // Update entries so that the view has up-to-date information about + // the entries when handling the loaded_threads events, which avoids + // thrashing + updateEntries($scope.editor.open_doc_id) + dispatchReviewPanelEvent('loaded_threads') return $timeout(() => ide.$scope.$broadcast('review-panel:layout')) }) @@ -1346,8 +1367,14 @@ export default App.controller( } case 'focus:changed': { - const { from, to, empty } = payload - $scope.$broadcast('editor:focus:changed', from, to, !empty) + const { from, to, empty, updateType } = payload + $scope.$broadcast( + 'editor:focus:changed', + from, + to, + !empty, + updateType + ) break } diff --git a/services/web/frontend/js/shared/components/auto-expanding-text-area.tsx b/services/web/frontend/js/shared/components/auto-expanding-text-area.tsx index 9620e27540..1c7af774d7 100644 --- a/services/web/frontend/js/shared/components/auto-expanding-text-area.tsx +++ b/services/web/frontend/js/shared/components/auto-expanding-text-area.tsx @@ -36,7 +36,7 @@ function AutoExpandingTextArea({ ...rest }: AutoExpandingTextAreaProps) { const ref = useRef(null) - const isFirstResizeRef = useRef(true) + const previousHeightRef = useRef(null) useEffect(() => { if (!ref.current || !onResize || !('ResizeObserver' in window)) { @@ -46,12 +46,16 @@ function AutoExpandingTextArea({ const resizeObserver = new ResizeObserver(() => { // Ignore the resize that is triggered when the element is first // inserted into the DOM - if (isFirstResizeRef.current) { - isFirstResizeRef.current = false - } else { + if (!ref.current) { + return + } + const newHeight = ref.current.offsetHeight + const heightChanged = newHeight !== previousHeightRef.current + previousHeightRef.current = newHeight + if (heightChanged) { // Prevent errors like "ResizeObserver loop completed with undelivered - // notifications" that occur if onResize does something complicated. - // The cost of this is that onResize lags one frame behind, but it's + // notifications" that occur if onResize triggers another repaint. The + // cost of this is that onResize lags one frame behind, but it's // unlikely to matter. // Wrap onResize to prevent extra parameters being passed diff --git a/services/web/frontend/stylesheets/app/editor/review-panel.less b/services/web/frontend/stylesheets/app/editor/review-panel.less index 516214f6ff..deb6a7a6ce 100644 --- a/services/web/frontend/stylesheets/app/editor/review-panel.less +++ b/services/web/frontend/stylesheets/app/editor/review-panel.less @@ -1275,7 +1275,7 @@ button when (@is-overleaf-light = true) { .rp-entry-list-react { position: relative; - overflow-x: hidden; + overflow: hidden; } .rp-state-current-file & { diff --git a/services/web/types/review-panel/entry.ts b/services/web/types/review-panel/entry.ts index 5a21de64aa..e4588ac6bd 100644 --- a/services/web/types/review-panel/entry.ts +++ b/services/web/types/review-panel/entry.ts @@ -11,6 +11,7 @@ interface ReviewPanelBaseEntry { offset: number screenPos: ReviewPanelEntryScreenPos visible: boolean + inViewport: boolean } interface ReviewPanelInsertOrDeleteEntry {