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,