From 3f5847dc49f76f301b7bc1db7df5b1ae6ad5e23e Mon Sep 17 00:00:00 2001 From: George Mamadashvili Date: Tue, 6 Oct 2026 19:42:13 +0400 Subject: [PATCH] Notes: Mark the text an unsent note is about with a draft marker Wrap the selection a new note starts from with a core/note marker whose id is 'new', so the anchor follows edits and survives a block switch until the note is sent. Submit swaps in the saved id; cancel, Escape and the empty-form dismissal strip it. The floating board anchors the pending note to the marker instead of the live selection. Also restore an unsent draft when its block is selected again while no note surface is mounted: the block sync now runs in NotesSidebar, and only the thread focus stays with the threads' DOM in Notes. --- packages/editor/CHANGELOG.md | 4 + .../src/components/collab-sidebar/README.md | 8 +- .../components/collab-sidebar/add-note.jsx | 10 +- .../src/components/collab-sidebar/format.js | 3 +- .../src/components/collab-sidebar/hooks.js | 147 +++++++++++------- .../src/components/collab-sidebar/index.jsx | 106 +++++++------ .../collab-sidebar/note-highlight-styles.js | 15 +- .../components/collab-sidebar/note-thread.jsx | 2 + .../src/components/collab-sidebar/notes.jsx | 8 +- .../collab-sidebar/test/utils.jsdom.test.js | 115 +------------- .../src/components/collab-sidebar/utils.js | 52 +------ .../various/block-notes-floating.spec.js | 60 ++++--- .../specs/editor/various/block-notes.spec.js | 145 ++++++++++++++++- 13 files changed, 383 insertions(+), 292 deletions(-) diff --git a/packages/editor/CHANGELOG.md b/packages/editor/CHANGELOG.md index 69632daf416c03..c6f9b4bf34d966 100644 --- a/packages/editor/CHANGELOG.md +++ b/packages/editor/CHANGELOG.md @@ -2,6 +2,10 @@ ## Unreleased +### Bug Fixes + +- Notes: Highlight the text an unsent note is about, and keep the note anchored to it when the selection changes or the text is edited before the note is sent ([#84125](https://github.com/WordPress/gutenberg/pull/84125)). + ## 15.2.0 (2026-10-07) ### Enhancements diff --git a/packages/editor/src/components/collab-sidebar/README.md b/packages/editor/src/components/collab-sidebar/README.md index db3a094657067e..d7168883c460c0 100644 --- a/packages/editor/src/components/collab-sidebar/README.md +++ b/packages/editor/src/components/collab-sidebar/README.md @@ -24,7 +24,7 @@ collab-sidebar/ ├── note-indicator-toolbar.jsx NoteAvatarIndicator - toolbar participants avatars ├── floating-container.jsx FloatingContainer - stack wrapper that applies `top` in floating mode │ -├── hooks.js useNoteThreads, useNoteActions, useNoteSelection, useFloatingBoard, useEnableFloatingSidebar +├── hooks.js useNoteThreads, useNoteActions, useNoteSelection, useNoteFocus, useFloatingBoard, useEnableFloatingSidebar ├── utils.js focusNoteThread, getNoteExcerpt, sanitizeNoteContent, calculateNotePositions, getAvatarBorderColor ├── board-store.js createBoardStore - DOM measurement for the floating layout ├── constants.js sidebar identifier strings @@ -37,11 +37,11 @@ collab-sidebar/ ``` NotesSidebarContainer (index.jsx) - gates on post type support, owns the unsent drafts Map - └── NotesSidebar (index.jsx) - owns sidebarRef + useNoteThreads + sidebar registration + └── NotesSidebar (index.jsx) - owns sidebarRef + useNoteThreads + useNoteSelection + sidebar registration ├── AddNoteMenuItem - slot fill in the block toolbar ├── NoteAvatarIndicator - slot fill in the block toolbar (per-thread avatars) ├── PluginSidebar (all-notes) - full sidebar - │ └── Notes (notes.jsx) - owns outer Stack + aria-label + useNoteActions + keyboard nav + │ └── Notes (notes.jsx) - owns outer Stack + aria-label + useNoteActions + useNoteFocus + keyboard nav │ ├── AddNote - new note form for the selected block, rendered when selectedNote === 'new' │ └── NoteThread[] - per thread │ └── @@ -56,7 +56,7 @@ NotesSidebarContainer (index.jsx) - gates on post type support, owns the `Notes` is reused for both sidebar surfaces. The only visual difference is driven by `isFloating` (whether to layer threads over the canvas or stack them in a panel). -Unsent note and reply text lives in a `Map` owned by `NotesSidebarContainer`, keyed by block client ID or note ID, so it survives the forms unmounting. It is a cache, not state: `useNoteDraft` reads it on mount and writes it on change, so typing doesn't re-render the sidebar. Slot fills don't inherit context, so `NotesSidebar` provides it inside each fill. +Unsent note and reply text lives in a `Map` owned by `NotesSidebarContainer`, keyed by block client ID or note ID, so it survives the forms unmounting. It is a cache, not state: `useNoteDraft` reads it on mount and writes it on change, so typing doesn't re-render the sidebar. Slot fills don't inherit context, so `NotesSidebar` provides it inside each fill. The text a new note is started from is wrapped with a `core/note` marker whose id is `new`, the same sentinel `selectNote` uses for the form, until the note is sent, so the anchor follows edits like a saved note's does; `onCreate` swaps in the saved id and `onDiscard` strips it. ## Floating board diff --git a/packages/editor/src/components/collab-sidebar/add-note.jsx b/packages/editor/src/components/collab-sidebar/add-note.jsx index d892cc1c56aff1..be0a349af4b1bd 100644 --- a/packages/editor/src/components/collab-sidebar/add-note.jsx +++ b/packages/editor/src/components/collab-sidebar/add-note.jsx @@ -16,7 +16,13 @@ import { unlock } from '../../lock-unlock'; const { useBlockElement } = unlock( blockEditorPrivateApis ); -export function AddNote( { clientId, onSubmit, sidebarRef, floating } ) { +export function AddNote( { + clientId, + onSubmit, + onDiscard, + sidebarRef, + floating, +} ) { const blockElement = useBlockElement( clientId ); const { toggleBlockSpotlight } = unlock( useDispatch( blockEditorStore ) ); const { selectNote } = unlock( useDispatch( editorStore ) ); @@ -50,6 +56,7 @@ export function AddNote( { clientId, onSubmit, sidebarRef, floating } ) { * newly selected note. */ if ( getSelectedNote() === 'new' && ! hasDraft() ) { + onDiscard( clientId ); toggleBlockSpotlight( clientId, false ); selectNote( undefined ); } @@ -57,6 +64,7 @@ export function AddNote( { clientId, onSubmit, sidebarRef, floating } ) { const unselectNote = () => { setDraft( '' ); + onDiscard( clientId ); selectNote( undefined ); blockElement?.focus(); toggleBlockSpotlight( clientId, false ); diff --git a/packages/editor/src/components/collab-sidebar/format.js b/packages/editor/src/components/collab-sidebar/format.js index 2b5551d3473ac3..70a837300352e5 100644 --- a/packages/editor/src/components/collab-sidebar/format.js +++ b/packages/editor/src/components/collab-sidebar/format.js @@ -58,7 +58,8 @@ function NoteFormat( { isActive, activeAttributes } ) { // Select-only; no cleanup on leave. The block-level sync owns // clearing/reverting, and deselecting here would drop the block's // note while the caret is still inside the block. - selectNote( Number( noteId ) ); + // The draft marker of an unsent note carries the form's own id. + selectNote( noteId === 'new' ? noteId : Number( noteId ) ); }, [ isActive, noteId, diff --git a/packages/editor/src/components/collab-sidebar/hooks.js b/packages/editor/src/components/collab-sidebar/hooks.js index 87317cc3a9affa..cde09d97a25b05 100644 --- a/packages/editor/src/components/collab-sidebar/hooks.js +++ b/packages/editor/src/components/collab-sidebar/hooks.js @@ -215,17 +215,51 @@ export function useNoteActions() { __unstableMarkLastChangeAsPersistent(); }; + const removeNoteMarker = ( clientId, noteId ) => { + const removed = removeInlineNote( + getBlockAttributes( clientId ), + noteId + ); + if ( removed ) { + updateNoteAnchor( clientId, { + [ removed.attributeKey ]: removed.value, + } ); + } + }; + + // The draft marker (id `new`) wraps the text an unsent note is about, so + // the anchor follows edits until the note is sent. + const onDiscard = ( clientId ) => removeNoteMarker( clientId, 'new' ); + + // Starting again from another selection moves the marker; a collapsed + // selection keeps it. + const onStart = ( clientId ) => { + const selection = readInlineSelection( + getSelectionStart, + getSelectionEnd + ); + if ( selection?.clientId !== clientId ) { + return; + } + onDiscard( clientId ); + const wrapped = wrapInlineNote( + getBlockAttributes( clientId )?.[ selection.attributeKey ], + 'new', + selection.start, + selection.end + ); + if ( wrapped ) { + updateNoteAnchor( clientId, { + [ selection.attributeKey ]: wrapped, + } ); + } + }; + const onCreate = async ( { content, parent } ) => { try { - // Capture the target block and inline selection *before* the async - // save: selection may shift during the round-trip, attaching the - // note to the wrong block or collapsing its inline anchor. - const inlineSelection = ! parent - ? readInlineSelection( getSelectionStart, getSelectionEnd ) - : null; - const clientId = ! parent - ? inlineSelection?.clientId || getSelectedBlockClientId() - : null; + // Capture the target block *before* the async save: selection may + // shift during the round-trip, attaching the note to the wrong block. + const clientId = ! parent ? getSelectedBlockClientId() : null; const savedRecord = await saveEntityRecord( 'root', @@ -259,18 +293,15 @@ export function useNoteActions() { metadata: cleanEmptyObject( updatedMetadata ), }; - // Inline path: also wrap the selected text with a core/note - // marker so the anchor survives later edits. - if ( inlineSelection ) { - const wrapped = wrapInlineNote( - attributes?.[ inlineSelection.attributeKey ], + // Inline path: the draft marker becomes the note's marker. + const draft = removeInlineNote( attributes, 'new' ); + if ( draft ) { + newAttributes[ draft.attributeKey ] = wrapInlineNote( + draft.value, savedRecord.id, - inlineSelection.start, - inlineSelection.end + draft.start, + draft.end ); - if ( wrapped ) { - newAttributes[ inlineSelection.attributeKey ] = wrapped; - } } updateNoteAnchor( clientId, newAttributes ); @@ -332,17 +363,8 @@ export function useNoteActions() { // Resolving a note drops its inline highlight: strip the marker // so the note falls back to a block-level note in the content. - const removed = - status === 'approved' && - note.blockClientId && - removeInlineNote( - getBlockAttributes( note.blockClientId ), - id - ); - if ( removed ) { - updateNoteAnchor( note.blockClientId, { - [ removed.attributeKey ]: removed.value, - } ); + if ( status === 'approved' && note.blockClientId ) { + removeNoteMarker( note.blockClientId, id ); } // The note visibly updates in place, so there is no snackbar, @@ -511,7 +533,7 @@ export function useNoteActions() { } }; - return { onCreate, onEdit, onDelete }; + return { onStart, onDiscard, onCreate, onEdit, onDelete }; } /** @@ -535,35 +557,35 @@ export function useNoteDraft( key ) { } /** - * Keeps the selected note in step with the selected block, and focuses the - * selected note's thread when the selection asks for it. + * Keeps the selected note in step with the selected block. * - * @param {Object} props - * @param {Array} props.notes Threads shown in the sidebar. - * @param {Object} props.sidebarRef Ref to the sidebar element. + * @param {Object} props + * @param {Array} props.notes Threads shown in the sidebar. + * @param {Map} props.drafts Unsent note drafts, see `NoteDraftsContext`. + * @param {(clientId: string) => void} props.onDiscard Drops a block's draft marker. */ -export function useNoteSelection( { notes, sidebarRef } ) { +export function useNoteSelection( { notes, drafts, onDiscard } ) { const registry = useRegistry(); - const drafts = useContext( NoteDraftsContext ); const { selectNote } = unlock( useDispatch( editorStore ) ); const selectedBlockClientId = useSelect( ( select ) => select( blockEditorStore ).getSelectedBlockClientId(), [] ); - const { selectedNote, noteFocused } = useSelect( ( select ) => { - const { getSelectedNote, isNoteFocused } = unlock( - select( editorStore ) - ); - return { - selectedNote: getSelectedNote(), - noteFocused: isNoteFocused(), - }; - }, [] ); - const syncWithBlock = useEvent( ( clientId ) => { + const syncWithBlock = useEvent( ( clientId, previousClientId ) => { const { getSelectedNote, isNoteFocused } = unlock( registry.select( editorStore ) ); + // Leaving the new note form without a draft closes it for good, so + // its marker goes too. The form's own focus-out can't: the block + // change unmounts it first. + if ( + getSelectedNote() === 'new' && + previousClientId && + ! drafts.has( previousClientId ) + ) { + onDiscard( previousClientId ); + } // A pending focus request is an explicit pick; leave it alone. if ( isNoteFocused() ) { return; @@ -582,17 +604,38 @@ export function useNoteSelection( { notes, sidebarRef } ) { } ); // Sync only on block transitions, so in-block changes (Escape, Cancel, - // the new note form) are left alone. + // the new note form) are left alone. A layout effect, so it reads the + // focus flag before `useNoteFocus` clears it. const prevBlockIdRef = useRef( selectedBlockClientId ); - useEffect( () => { + useLayoutEffect( () => { if ( prevBlockIdRef.current === selectedBlockClientId ) { return; } + const previousClientId = prevBlockIdRef.current; prevBlockIdRef.current = selectedBlockClientId; - syncWithBlock( selectedBlockClientId ); + syncWithBlock( selectedBlockClientId, previousClientId ); }, [ selectedBlockClientId, syncWithBlock ] ); +} + +/** + * Focuses the selected note's thread when the selection asks for it. Lives + * with the threads' DOM: the sidebar ref is only set once they render. + * + * @param {Object} props + * @param {Object} props.sidebarRef Ref to the sidebar element. + */ +export function useNoteFocus( { sidebarRef } ) { + const { selectNote } = unlock( useDispatch( editorStore ) ); + const { selectedNote, noteFocused } = useSelect( ( select ) => { + const { getSelectedNote, isNoteFocused } = unlock( + select( editorStore ) + ); + return { + selectedNote: getSelectedNote(), + noteFocused: isNoteFocused(), + }; + }, [] ); - // Must run after the sync above, which reads the focus flag this clears. useEffect( () => { if ( ! noteFocused || ! selectedNote ) { return; diff --git a/packages/editor/src/components/collab-sidebar/index.jsx b/packages/editor/src/components/collab-sidebar/index.jsx index 0994d55c1d6842..1219ed2ed3a251 100644 --- a/packages/editor/src/components/collab-sidebar/index.jsx +++ b/packages/editor/src/components/collab-sidebar/index.jsx @@ -17,21 +17,21 @@ import { store as editorStore } from '../../store'; import { AddNoteMenuItem } from './add-note-menu-item'; import { NoteAvatarIndicator } from './note-indicator-toolbar'; import { NoteHighlightStyles } from './note-highlight-styles'; -import { NoteDraftsContext, useNoteThreads } from './hooks'; +import { + NoteDraftsContext, + useNoteActions, + useNoteSelection, + useNoteThreads, +} from './hooks'; import { getNoteIdsFromMetadata, pickPrimaryNote } from './utils'; import PostTypeSupportCheck from '../post-type-support-check'; import { CanvasMargin } from '../visual-editor/canvas-margin'; import { unlock } from '../../lock-unlock'; function NotesSidebar( { postId, drafts } ) { - const { getActiveComplementaryArea } = useSelect( interfaceStore ); - const { enableComplementaryArea } = useDispatch( interfaceStore ); - const { toggleBlockSpotlight, selectBlock } = unlock( - useDispatch( blockEditorStore ) - ); - const { selectNote } = unlock( useDispatch( editorStore ) ); - const isLargeViewport = useViewportMatch( 'medium' ); const sidebarRef = useRef( null ); + const isLargeViewport = useViewportMatch( 'medium' ); + const { ref: canvasMarginRef } = useSlot( CanvasMargin.name ); const { clientId, noteId, isClassicBlock } = useSelect( ( select ) => { const { getBlockAttributes, getSelectedBlockClientId, getBlockName } = @@ -47,31 +47,36 @@ function NotesSidebar( { postId, drafts } ) { : false, }; }, [] ); + const { notesDisplayMode, selectedNoteId, isAllNotesSidebarOpen } = + useSelect( ( select ) => { + const { get } = select( preferencesStore ); + return { + notesDisplayMode: get( 'core', 'notesDisplayMode' ), + selectedNoteId: unlock( + select( editorStore ) + ).getSelectedNote(), + isAllNotesSidebarOpen: + select( interfaceStore ).getActiveComplementaryArea( + 'core' + ) === ALL_NOTES_SIDEBAR, + }; + }, [] ); + const { getActiveComplementaryArea } = useSelect( interfaceStore ); + const { notes, unresolvedNotes } = useNoteThreads( postId ); + const { onStart, onDiscard } = useNoteActions(); + // Here rather than in `Notes`, which unmounts with its surface: a draft + // must be restored on block selection even while no note is shown. + useNoteSelection( { notes, drafts, onDiscard } ); - const blockNoteIds = getNoteIdsFromMetadata( { noteId } ); - const { isDistractionFree, notesDisplayMode } = useSelect( ( select ) => { - const { get } = select( preferencesStore ); - return { - isDistractionFree: get( 'core', 'distractionFree' ), - notesDisplayMode: get( 'core', 'notesDisplayMode' ), - }; - }, [] ); - const areNotesHidden = notesDisplayMode === 'hidden'; + const { enableComplementaryArea } = useDispatch( interfaceStore ); const { set: setPreference } = useDispatch( preferencesStore ); - const selectedNoteId = useSelect( - ( select ) => unlock( select( editorStore ) ).getSelectedNote(), - [] - ); - - const { notes, unresolvedNotes } = useNoteThreads( postId ); - const isAllNotesSidebarOpen = useSelect( - ( select ) => - select( interfaceStore ).getActiveComplementaryArea( 'core' ) === - ALL_NOTES_SIDEBAR, - [] + const { toggleBlockSpotlight, selectBlock } = unlock( + useDispatch( blockEditorStore ) ); - const { ref: canvasMarginRef } = useSlot( CanvasMargin.name ); + const { selectNote } = unlock( useDispatch( editorStore ) ); + const blockNoteIds = getNoteIdsFromMetadata( { noteId } ); + const areNotesHidden = notesDisplayMode === 'hidden'; // Fallback to "All notes" sidebar on smaller viewports or a narrow canvas. const showAllNotesSidebar = notes.length > 0 || ! isLargeViewport || isAllNotesSidebarOpen; @@ -100,6 +105,10 @@ function NotesSidebar( { postId, drafts } ) { setPreference( 'core', 'notesDisplayMode', 'full' ); } + if ( targetNoteId === 'new' ) { + onStart( targetClientId ); + } + // A special case for the List View, where block selection isn't required to trigger an action. // The action won't do anything if the block is already selected. selectBlock( targetClientId, null ); @@ -135,7 +144,7 @@ function NotesSidebar( { postId, drafts } ) { addNewNoteForBlock( clientId ); }, { - isDisabled: isDistractionFree || isClassicBlock || ! clientId, + isDisabled: isClassicBlock || ! clientId, } ); @@ -146,10 +155,6 @@ function NotesSidebar( { postId, drafts } ) { : []; const currentThread = pickPrimaryNote( currentThreads ); - if ( isDistractionFree ) { - return ; - } - return ( <> new Map() ); - const { postId, editorMode, revisionsMode } = useSelect( ( select ) => { - const { getCurrentPostId, getEditorMode, isRevisionsMode } = unlock( - select( editorStore ) - ); - return { - postId: getCurrentPostId(), - editorMode: getEditorMode(), - revisionsMode: isRevisionsMode(), - }; - }, [] ); + const { postId, editorMode, revisionsMode, isDistractionFree } = useSelect( + ( select ) => { + const { getCurrentPostId, getEditorMode, isRevisionsMode } = unlock( + select( editorStore ) + ); + return { + postId: getCurrentPostId(), + editorMode: getEditorMode(), + revisionsMode: isRevisionsMode(), + isDistractionFree: select( preferencesStore ).get( + 'core', + 'distractionFree' + ), + }; + }, + [] + ); if ( ! postId || typeof postId !== 'number' ) { return null; @@ -237,7 +249,11 @@ export default function NotesSidebarContainer() { return ( - + { isDistractionFree ? ( + + ) : ( + + ) } ); } diff --git a/packages/editor/src/components/collab-sidebar/note-highlight-styles.js b/packages/editor/src/components/collab-sidebar/note-highlight-styles.js index 39d9d16a595e53..9d2a1b2c57121b 100644 --- a/packages/editor/src/components/collab-sidebar/note-highlight-styles.js +++ b/packages/editor/src/components/collab-sidebar/note-highlight-styles.js @@ -1,5 +1,7 @@ import { useMemo } from '@wordpress/element'; +import { useSelect } from '@wordpress/data'; import { useStyleOverride } from '@wordpress/block-editor'; +import { store as coreStore } from '@wordpress/core-data'; import { getAvatarBorderColor, getNoteMarkerSelector } from './utils'; // Hex alpha suffixes for the rest / active states. Kept low so the marker @@ -63,9 +65,18 @@ export function buildHighlightCss( threads, selectedId = null ) { * @return {null} Renders nothing; styles are applied via `useStyleOverride`. */ export function NoteHighlightStyles( { threads, selectedId } ) { + const currentUserId = useSelect( + ( select ) => select( coreStore ).getCurrentUser()?.id, + [] + ); + // An unsent note's draft marker is tinted like a thread of its author. const css = useMemo( - () => buildHighlightCss( threads, selectedId ), - [ threads, selectedId ] + () => + buildHighlightCss( + [ ...threads, { id: 'new', author: currentUserId } ], + selectedId + ), + [ threads, selectedId, currentUserId ] ); useStyleOverride( { id: 'core-note-highlights', css } ); return null; diff --git a/packages/editor/src/components/collab-sidebar/note-thread.jsx b/packages/editor/src/components/collab-sidebar/note-thread.jsx index 5c75e2f806e1fe..b95d953444a215 100644 --- a/packages/editor/src/components/collab-sidebar/note-thread.jsx +++ b/packages/editor/src/components/collab-sidebar/note-thread.jsx @@ -76,6 +76,7 @@ export function NoteThread( { note, onEditNote, onAddReply, + onDiscard, onDeleteNote, isSelected, sidebarRef, @@ -228,6 +229,7 @@ export function NoteThread( { key={ note.blockClientId } clientId={ note.blockClientId } onSubmit={ onAddReply } + onDiscard={ onDiscard } sidebarRef={ sidebarRef } floating={ { y: floating.y, ref: floatingRef } } /> diff --git a/packages/editor/src/components/collab-sidebar/notes.jsx b/packages/editor/src/components/collab-sidebar/notes.jsx index 71506e5bc4261a..9e4361bcdac9ab 100644 --- a/packages/editor/src/components/collab-sidebar/notes.jsx +++ b/packages/editor/src/components/collab-sidebar/notes.jsx @@ -9,7 +9,7 @@ import { import { unlock } from '../../lock-unlock'; import { NoteThread } from './note-thread'; import { focusNoteThread } from './utils'; -import { useFloatingBoard, useNoteActions, useNoteSelection } from './hooks'; +import { useFloatingBoard, useNoteActions, useNoteFocus } from './hooks'; import { AddNote } from './add-note'; import { store as editorStore } from '../../store'; @@ -18,6 +18,7 @@ const { useBlockElement } = unlock( blockEditorPrivateApis ); export function Notes( { notes, sidebarRef, isFloating = false } ) { const { onCreate: onAddReply, + onDiscard, onEdit: onEditNote, onDelete, } = useNoteActions(); @@ -25,6 +26,7 @@ export function Notes( { notes, sidebarRef, isFloating = false } ) { const { selectBlock, toggleBlockSpotlight } = unlock( useDispatch( blockEditorStore ) ); + useNoteFocus( { sidebarRef } ); const { selectedBlockClientId, orderedBlockIds } = useSelect( ( select ) => { @@ -42,8 +44,6 @@ export function Notes( { notes, sidebarRef, isFloating = false } ) { [] ); - useNoteSelection( { notes, sidebarRef } ); - const relatedBlockElement = useBlockElement( selectedBlockClientId ); const threads = useMemo( () => { @@ -210,6 +210,7 @@ export function Notes( { notes, sidebarRef, isFloating = false } ) { key={ selectedBlockClientId } clientId={ selectedBlockClientId } onSubmit={ onAddReply } + onDiscard={ onDiscard } sidebarRef={ sidebarRef } /> ) } @@ -231,6 +232,7 @@ export function Notes( { notes, sidebarRef, isFloating = false } ) { { expect( getNoteAnchorRect( 12, blockEl ).top ).toBe( 40 ); } ); - describe( 'pending new note', () => { - let blockEl; - - beforeEach( () => { - blockEl = document.createElement( 'p' ); - blockEl.textContent = 'Some longer paragraph text'; - // Selections only work on nodes attached to the document. - document.body.appendChild( blockEl ); - mockRect( blockEl, 100 ); - } ); - - afterEach( () => { - window.getSelection().removeAllRanges(); - document.body.removeChild( blockEl ); - // jsdom's Range has neither getClientRects nor - // getBoundingClientRect; selectText defines both, so drop - // them to keep tests isolated. - delete window.Range.prototype.getClientRects; - delete window.Range.prototype.getBoundingClientRect; - } ); - - /** - * Select a range and give it the client rects a browser would - * report. One rect per line the selection covers; the bounding - * rect is their union, as in a real engine. - * - * @param {Node} node Text node to select within. - * @param {number} start Start offset. - * @param {number} end End offset. - * @param {Object[]} rects Per-line rects the selection reports. - */ - function selectText( - node, - start, - end, - rects = [ { top: 160, width: 50, height: 20 } ] - ) { - const range = document.createRange(); - range.setStart( node, start ); - range.setEnd( node, end ); - window.Range.prototype.getClientRects = () => rects; - window.Range.prototype.getBoundingClientRect = () => - rects.length - ? { - top: Math.min( ...rects.map( ( r ) => r.top ) ), - width: Math.max( ...rects.map( ( r ) => r.width ) ), - height: rects.reduce( - ( sum, r ) => sum + r.height, - 0 - ), - } - : { top: 0, width: 0, height: 0 }; - const selection = window.getSelection(); - selection.removeAllRanges(); - selection.addRange( range ); - } - - it( 'anchors to the text selection it will attach to', () => { - selectText( blockEl.firstChild, 5, 11 ); - - expect( getNoteAnchorRect( 'new', blockEl ).top ).toBe( 160 ); - } ); - - it( 'falls back to the block rect for a collapsed selection', () => { - selectText( blockEl.firstChild, 5, 5 ); - - expect( getNoteAnchorRect( 'new', blockEl ).top ).toBe( 100 ); - } ); - - it( 'falls back to the block rect when the selection is outside the block', () => { - const other = document.createElement( 'p' ); - other.textContent = 'Elsewhere'; - document.body.appendChild( other ); - selectText( other.firstChild, 0, 4 ); - - expect( getNoteAnchorRect( 'new', blockEl ).top ).toBe( 100 ); - document.body.removeChild( other ); - } ); - - it( 'ignores the zero-width rect a selection picks up at a line edge', () => { - // A selection starting at the very end of one line reports a - // zero-width rect there plus the real rect on the next line. - // The union would align the form to the line above the text. - selectText( blockEl.firstChild, 5, 11, [ - { top: 100, width: 0, height: 20 }, - { top: 160, width: 50, height: 20 }, - ] ); - - expect( getNoteAnchorRect( 'new', blockEl ).top ).toBe( 160 ); - } ); - - it( 'falls back to the block rect when the selection has no rendered rects', () => { - // An unrendered range still yields an all-zero rect rather - // than null, which would pin the form to the top of the canvas. - selectText( blockEl.firstChild, 5, 11, [] ); - - expect( getNoteAnchorRect( 'new', blockEl ).top ).toBe( 100 ); - } ); - - it( 'falls back to the block rect when there is no selection', () => { - expect( getNoteAnchorRect( 'new', blockEl ).top ).toBe( 100 ); - } ); - } ); - it( 'anchors a marker split into several runs to its first run', () => { const blockEl = document.createElement( 'p' ); /* diff --git a/packages/editor/src/components/collab-sidebar/utils.js b/packages/editor/src/components/collab-sidebar/utils.js index 87211849ccbec8..ae96c542ee8b4d 100644 --- a/packages/editor/src/components/collab-sidebar/utils.js +++ b/packages/editor/src/components/collab-sidebar/utils.js @@ -1,6 +1,5 @@ import { _x } from '@wordpress/i18n'; import { create, RichTextData } from '@wordpress/rich-text'; -import { getRectangleFromRange } from '@wordpress/dom'; import { NOTE_FORMAT_NAME } from './constants'; /** @@ -232,53 +231,15 @@ export function getNoteMarkerSelector( noteId ) { return `mark.wp-note[data-id="${ escapedId }"]`; } -/** - * Measure the bounding rect of the current text selection within a block - * element, or return null when there is no usable selection (collapsed, or - * not fully inside the block). A pending new note has no in-content marker - * yet, so the selection it will attach to is the only anchor available for - * positioning its floating form. - * - * @param {HTMLElement} blockEl Block DOM element to resolve the selection in. - * @return {?DOMRect} Selection rect, or null. - */ -export function getSelectionRect( blockEl ) { - const selection = blockEl.ownerDocument.defaultView?.getSelection(); - if ( ! selection || selection.rangeCount === 0 || selection.isCollapsed ) { - return null; - } - const range = selection.getRangeAt( 0 ); - // `isCollapsed` can be false with a collapsed first range, and - // `getRectangleFromRange` measures those by inserting a temporary node. - if ( range.collapsed ) { - return null; - } - if ( ! blockEl.contains( range.commonAncestorContainer ) ) { - return null; - } - // `getRectangleFromRange` over `Range.getBoundingClientRect()`: it drops - // the hairline rects a selection picks up at a line's edge, so a - // selection starting at the end of one line aligns to the line that - // actually holds the text rather than to the line above it. - const rect = getRectangleFromRange( range ); - // A range with no rendered client rects still yields an all-zero rect - // rather than null, which would pin the thread to the top of the canvas. - // Treat it as "no usable selection" so callers fall back to the block. - if ( ! rect || ( rect.width === 0 && rect.height === 0 ) ) { - return null; - } - return rect; -} - /** * Measure where a note's floating thread should line up in the canvas. * * An inline note anchors to its in-content marker, so the thread aligns with - * the noted text rather than the block. A marker split into several runs - * (crossing overlaps) resolves to its first run. The pending new note has no - * marker yet, so it anchors to the text selection it will attach to. Anything - * else falls back to the block itself. An anchor inside collapsed content - * (e.g. a closed Details) falls back to the closest visible block. + * the noted text rather than the block, and so does a pending new note through + * its draft marker. A marker split into several runs (crossing overlaps) + * resolves to its first run. Anything else falls back to the block itself. An + * anchor inside collapsed content (e.g. a closed Details) falls back to the + * closest visible block. * * Resolved at read time, because rich-text re-renders replace the marker. * @@ -287,9 +248,6 @@ export function getSelectionRect( blockEl ) { * @return {DOMRect} Anchor rect, in viewport coordinates. */ export function getNoteAnchorRect( noteId, blockEl ) { - if ( noteId === 'new' ) { - return getSelectionRect( blockEl ) ?? blockEl.getBoundingClientRect(); - } let anchor = blockEl.querySelector( getNoteMarkerSelector( noteId ) ) ?? blockEl; // Collapsed content still reports the box it would have when expanded, diff --git a/test/e2e/specs/editor/various/block-notes-floating.spec.js b/test/e2e/specs/editor/various/block-notes-floating.spec.js index 79f932b0394791..ac6164831df2c2 100644 --- a/test/e2e/specs/editor/various/block-notes-floating.spec.js +++ b/test/e2e/specs/editor/various/block-notes-floating.spec.js @@ -863,21 +863,6 @@ test.describe( 'Block Notes: floating notes', () => { return paragraph; } - // Selection top in page coordinates. - async function getSelectionTop( editor ) { - const frameBox = await editor.canvas.owner().boundingBox(); - const top = await editor.canvas.locator( 'body' ).evaluate( () => { - const selection = window.getSelection(); - return selection.rangeCount - ? selection.getRangeAt( 0 ).getBoundingClientRect().top - : null; - } ); - if ( top === null ) { - throw new Error( 'The canvas has no text selection.' ); - } - return frameBox.y + top; - } - test( 'aligns the floating thread with its inline marker', async ( { editor, page, @@ -898,7 +883,7 @@ test.describe( 'Block Notes: floating notes', () => { await expectAligned( thread, mark ); } ); - test( 'aligns the pending new-note form with the text selection', async ( { + test( 'aligns the pending new-note form with its draft marker', async ( { editor, page, blockNoteUtils, @@ -909,15 +894,50 @@ test.describe( 'Block Notes: floating notes', () => { } ); await editor.clickBlockOptionsMenuItem( 'Add note' ); - // There is no marker yet, so the form anchors to the selection the - // note will attach to. The canvas keeps it while the form has focus. + // The selected text carries a draft marker until the note is sent, + // so the form anchors to it even once the canvas selection is gone. const form = getFloatingNotes( page ).getByRole( 'treeitem', { name: 'New note', exact: true, } ); + const mark = editor.canvas.locator( 'mark.wp-note[data-id="new"]' ); await expect( form ).toHaveClass( /is-floating/ ); - await expectBelow( () => getSelectionTop( editor ), paragraph ); - await expectAligned( form, () => getSelectionTop( editor ) ); + await expectBelow( mark, paragraph ); + await expectAligned( form, mark ); + } ); + } ); + + test.describe( 'Drafts', () => { + test( 'restores an unsent draft once its block is selected again', async ( { + editor, + page, + } ) => { + // The middle block keeps the selected block's toolbar from + // covering the other block's click target. + for ( const content of [ + 'First block', + 'Middle block', + 'Second block', + ] ) { + await editor.insertBlock( { + name: 'core/paragraph', + attributes: { content }, + } ); + } + const newNoteForm = page.getByRole( 'textbox', { + name: 'New note', + exact: true, + } ); + + await editor.canvas.getByText( 'First block' ).click(); + await editor.clickBlockOptionsMenuItem( 'Add note' ); + await newNoteForm.pressSequentially( 'Unsent draft' ); + + // With no notes to show, the floating board unmounts entirely. + await editor.canvas.getByText( 'Second block' ).click(); + await expect( getFloatingNotes( page ) ).toBeHidden(); + await editor.canvas.getByText( 'First block' ).click(); + await expect( newNoteForm ).toHaveText( 'Unsent draft' ); } ); } ); diff --git a/test/e2e/specs/editor/various/block-notes.spec.js b/test/e2e/specs/editor/various/block-notes.spec.js index fb053dc1c44b5d..ef7a7f401abc15 100644 --- a/test/e2e/specs/editor/various/block-notes.spec.js +++ b/test/e2e/specs/editor/various/block-notes.spec.js @@ -1488,10 +1488,10 @@ test.describe( 'Block Notes', () => { .getByRole( 'button', { name: 'Add note', exact: true } ) .click(); - // Wait for the inline-note `` to appear in the canvas; the - // `core/note` format serializes the marker as `mark.wp-note`. + // The draft marker shows while the note is saved; wait for the + // saved note's id so the thread focus that follows has settled. const mark = editor.canvas.locator( 'mark.wp-note' ).first(); - await expect( mark ).toBeVisible(); + await expect( mark ).toHaveAttribute( 'data-id', /^\d+$/ ); // Creating a note auto-selects it, which renders the marker at the // active opacity. Move focus to the title to deselect so the marker @@ -1740,6 +1740,145 @@ test.describe( 'Block Notes', () => { await expect( paragraph ).toHaveText( 'Hello brave new world.' ); } ); + test( 'highlights the drafted text and anchors the note to it across a block switch', async ( { + editor, + page, + blockNoteUtils, + } ) => { + await editor.insertBlock( { + name: 'core/paragraph', + attributes: { content: 'Hello brave new world.' }, + } ); + // The middle block keeps the selected block's toolbar from + // covering the other block's click target. + for ( const content of [ 'Middle block', 'Another block' ] ) { + await editor.insertBlock( { + name: 'core/paragraph', + attributes: { content }, + } ); + } + const paragraph = editor.canvas.getByText( + 'Hello brave new world.' + ); + const newNoteForm = page.getByRole( 'textbox', { + name: 'New note', + exact: true, + } ); + const draftMark = editor.canvas.locator( + 'mark.wp-note[data-id="new"]' + ); + + await paragraph.click(); + await blockNoteUtils.selectBlockText( { start: 6, length: 5 } ); + await editor.clickBlockOptionsMenuItem( 'Add note' ); + await expect( draftMark ).toHaveText( 'brave' ); + await newNoteForm.pressSequentially( 'Still brave' ); + + // Leaving collapses the canvas selection; the marker stays. + await editor.canvas.getByText( 'Another block' ).click(); + await expect( newNoteForm ).toBeHidden(); + await expect( draftMark ).toHaveText( 'brave' ); + await paragraph.click(); + await expect( newNoteForm ).toHaveText( 'Still brave' ); + await page + .getByRole( 'region', { name: 'Editor settings' } ) + .getByRole( 'button', { name: 'Add note', exact: true } ) + .click(); + + const mark = editor.canvas.locator( 'mark.wp-note' ); + await expect( mark ).toHaveCount( 1 ); + await expect( mark ).toHaveText( 'brave' ); + await expect( mark ).toHaveAttribute( 'data-id', /^\d+$/ ); + } ); + + test( 'removes the draft marker when the empty form is dismissed', async ( { + editor, + page, + blockNoteUtils, + } ) => { + await editor.insertBlock( { + name: 'core/paragraph', + attributes: { content: 'Hello brave new world.' }, + } ); + // The middle block keeps the selected block's toolbar from + // covering the other block's click target. + for ( const content of [ 'Middle block', 'Another block' ] ) { + await editor.insertBlock( { + name: 'core/paragraph', + attributes: { content }, + } ); + } + const paragraph = editor.canvas.getByText( + 'Hello brave new world.' + ); + const mark = editor.canvas.locator( 'mark.wp-note' ); + + await paragraph.click(); + await blockNoteUtils.selectBlockText( { start: 6, length: 5 } ); + await editor.clickBlockOptionsMenuItem( 'Add note' ); + await expect( mark ).toHaveText( 'brave' ); + await page + .getByRole( 'region', { name: 'Editor settings' } ) + .getByRole( 'button', { name: 'Cancel' } ) + .click(); + await expect( mark ).toHaveCount( 0 ); + expect( await editor.getEditedPostContent() ).not.toContain( + 'wp-note' + ); + + // Selecting another block closes the form before its own + // focus-out runs. + await paragraph.click(); + await blockNoteUtils.selectBlockText( { start: 6, length: 5 } ); + await editor.clickBlockOptionsMenuItem( 'Add note' ); + await expect( mark ).toHaveText( 'brave' ); + await editor.canvas.getByText( 'Another block' ).click(); + await expect( mark ).toHaveCount( 0 ); + } ); + + test( 'moves the draft marker to a new selection when Add note runs again', async ( { + editor, + page, + blockNoteUtils, + } ) => { + await editor.insertBlock( { + name: 'core/paragraph', + attributes: { content: 'Hello brave new world.' }, + } ); + const paragraph = editor.canvas.getByRole( 'document', { + name: 'Block: Paragraph', + } ); + const newNoteForm = page.getByRole( 'textbox', { + name: 'New note', + exact: true, + } ); + const draftMark = editor.canvas.locator( + 'mark.wp-note[data-id="new"]' + ); + + await paragraph.click(); + await blockNoteUtils.selectBlockText( { start: 6, length: 5 } ); + await editor.clickBlockOptionsMenuItem( 'Add note' ); + await newNoteForm.pressSequentially( 'Second thoughts' ); + + // Select a word before the marker: stepping across its boundary + // would skew keyboard offsets. + await paragraph.click(); + await blockNoteUtils.selectBlockText( { start: 0, length: 5 } ); + await editor.clickBlockOptionsMenuItem( 'Add note' ); + await expect( draftMark ).toHaveCount( 1 ); + await expect( draftMark ).toHaveText( 'Hello' ); + await expect( newNoteForm ).toHaveText( 'Second thoughts' ); + await page + .getByRole( 'region', { name: 'Editor settings' } ) + .getByRole( 'button', { name: 'Add note', exact: true } ) + .click(); + + const mark = editor.canvas.locator( 'mark.wp-note' ); + await expect( mark ).toHaveCount( 1 ); + await expect( mark ).toHaveText( 'Hello' ); + } ); + test( 'boosts the marker opacity when its note is selected', async ( { editor, page,