From f740342b2377fdd2c8d2b6525e9ecb3afe228bf2 Mon Sep 17 00:00:00 2001 From: Tim Fischbach Date: Wed, 6 May 2026 14:24:39 +0200 Subject: [PATCH 01/12] Add SELECT_COMMENT_THREAD message --- .../PreviewMessageController-spec.js | 18 ++++++ .../inlineEditing/EditableText-spec.js | 44 ++++++++++++++ .../controllers/PreviewMessageController.js | 7 +++ .../inlineEditing/EditableText/BadgeColumn.js | 60 +++++++++++-------- 4 files changed, 103 insertions(+), 26 deletions(-) diff --git a/entry_types/scrolled/package/spec/editor/controllers/PreviewMessageController-spec.js b/entry_types/scrolled/package/spec/editor/controllers/PreviewMessageController-spec.js index 15d55ab30a..4bc21d355f 100644 --- a/entry_types/scrolled/package/spec/editor/controllers/PreviewMessageController-spec.js +++ b/entry_types/scrolled/package/spec/editor/controllers/PreviewMessageController-spec.js @@ -158,6 +158,24 @@ describe('PreviewMessageController', () => { })).resolves.toMatchObject({type: 'SELECT', payload: {id: 1, type: 'contentElement'}}); }); + it('posts SELECT_COMMENT_THREAD message on selectCommentThread event', async () => { + const entry = factories.entry(ScrolledEntry, {}, {entryTypeSeed: normalizeSeed()}); + const iframeWindow = createIframeWindow(); + controller = new PreviewMessageController({entry, iframeWindow}); + + await postReadyMessageAndWaitForAcknowledgement(iframeWindow); + + return expect(new Promise(resolve => { + iframeWindow.addEventListener('message', event => { + if (event.data.type === 'SELECT_COMMENT_THREAD') resolve(event.data); + }); + entry.trigger('selectCommentThread', 7); + })).resolves.toMatchObject({ + type: 'SELECT_COMMENT_THREAD', + payload: {threadId: 7} + }); + }); + it('passes on range from selectContentElement event', async () => { const entry = factories.entry(ScrolledEntry, {}, { entryTypeSeed: normalizeSeed({ diff --git a/entry_types/scrolled/package/spec/frontend/inlineEditing/EditableText-spec.js b/entry_types/scrolled/package/spec/frontend/inlineEditing/EditableText-spec.js index 20d8951492..3fed9c774f 100644 --- a/entry_types/scrolled/package/spec/frontend/inlineEditing/EditableText-spec.js +++ b/entry_types/scrolled/package/spec/frontend/inlineEditing/EditableText-spec.js @@ -488,5 +488,49 @@ describe('EditableText', () => { } }, expect.anything()); }); + + it('runs badge click logic on SELECT_COMMENT_THREAD message', () => { + fakeParentWindow(); + window.parent.postMessage = jest.fn(); + + const subjectRange = {anchor: {path: [0, 0], offset: 0}, focus: {path: [0, 0], offset: 5}}; + const value = [{type: 'paragraph', children: [{text: 'Some text to comment on'}]}]; + + renderWithCommenting( + , + { + wrapper, + commentThreads: [{ + id: 9, + subjectType: 'ContentElement', + subjectId: 10, + subjectRange, + comments: [{id: 1, body: 'A comment', creatorName: 'Alice', creatorId: 1}] + }] + } + ); + + window.parent.postMessage.mockClear(); + + act(() => { + window.postMessage({ + type: 'SELECT_COMMENT_THREAD', + payload: {threadId: 9} + }, '*'); + }); + + return expect(new Promise(resolve => { + const original = window.parent.postMessage; + window.parent.postMessage = (data, ...rest) => { + original(data, ...rest); + if (data.type === 'SELECTED' && data.payload.type === 'contentElementComments') { + resolve(data); + } + }; + })).resolves.toMatchObject({ + type: 'SELECTED', + payload: {type: 'contentElementComments', id: 1, highlightedThreadId: 9} + }); + }); }); }); diff --git a/entry_types/scrolled/package/src/editor/controllers/PreviewMessageController.js b/entry_types/scrolled/package/src/editor/controllers/PreviewMessageController.js index ea5ee3caac..c363fa4d52 100644 --- a/entry_types/scrolled/package/src/editor/controllers/PreviewMessageController.js +++ b/entry_types/scrolled/package/src/editor/controllers/PreviewMessageController.js @@ -113,6 +113,13 @@ export const PreviewMessageController = Object.extend({ }) }); + this.listenTo(this.entry, 'selectCommentThread', threadId => { + postMessage({ + type: 'SELECT_COMMENT_THREAD', + payload: {threadId} + }) + }); + this.listenTo(this.entry, 'selectWidget', widget => { postMessage({ type: 'SELECT', diff --git a/entry_types/scrolled/package/src/frontend/inlineEditing/EditableText/BadgeColumn.js b/entry_types/scrolled/package/src/frontend/inlineEditing/EditableText/BadgeColumn.js index c240b4aeec..5f484a5220 100644 --- a/entry_types/scrolled/package/src/frontend/inlineEditing/EditableText/BadgeColumn.js +++ b/entry_types/scrolled/package/src/frontend/inlineEditing/EditableText/BadgeColumn.js @@ -1,4 +1,4 @@ -import React from 'react'; +import React, {useCallback} from 'react'; import {Range, Transforms} from 'slate'; import {FloatingPortal} from '@floating-ui/react'; @@ -7,6 +7,7 @@ import {useSlate, ReactEditor} from 'slate-react'; import {Badge, useAnchoredFloating} from 'pageflow-scrolled/review'; import {useFloatingPortalRoot} from '../../FloatingPortalRootProvider'; import {useContentElementAttributes} from '../../useContentElementAttributes'; +import {usePostMessageListener} from '../../usePostMessageListener'; import {useEditorSelection} from '../EditorState'; import {highlightOverlapsSelection} from './highlightOverlapsSelection'; @@ -44,30 +45,7 @@ function PositionedBadge({editor, highlight, highlights, editorSelection, anchor const {refs, floatingStyles, hasAnchor} = useAnchoredFloating(highlight.key, anchors, {placement: 'left-start'}); - if (!hasAnchor) return null; - - const isHighlightedThread = !!highlight.thread && - commentsSelection?.highlightedThreadId === highlight.thread.id; - const isActive = isHighlightedThread || - (highlight.key === 'selection' && newThreadActive); - // When a thread is highlighted, fall back to its start point for the - // overlap check so siblings in the same block stay in regular mode - // even if focus has drifted away from the slate editor. Use just the - // start point (not the full range) to stay consistent with - // highlightOverlapsSelection, which anchors to highlight starts. - const highlightedRange = commentsSelection?.highlightedThreadId ? - highlights.find( - h => h.thread?.id === commentsSelection.highlightedThreadId - )?.range : - null; - const fallbackPoint = highlightedRange && Range.start(highlightedRange); - const overlapSelection = editorSelection || - (fallbackPoint && {anchor: fallbackPoint, focus: fallbackPoint}); - const mode = isActive ? 'active' : - highlightOverlapsSelection(highlight, overlapSelection) ? undefined : - 'dot'; - - function handleClick() { + const handleClick = useCallback(() => { if (highlight.key === 'selection') { selectComments(); return; @@ -96,7 +74,37 @@ function PositionedBadge({editor, highlight, highlights, editorSelection, anchor highlightedThreadId: highlight.thread?.id, threadIds }); - } + }, [editor, highlight, highlights, selectComments, contentElementId]); + + usePostMessageListener(useCallback(data => { + if (data.type === 'SELECT_COMMENT_THREAD' && + data.payload.threadId === highlight.thread?.id) { + handleClick(); + } + }, [highlight, handleClick])); + + if (!hasAnchor) return null; + + const isHighlightedThread = !!highlight.thread && + commentsSelection?.highlightedThreadId === highlight.thread.id; + const isActive = isHighlightedThread || + (highlight.key === 'selection' && newThreadActive); + // When a thread is highlighted, fall back to its start point for the + // overlap check so siblings in the same block stay in regular mode + // even if focus has drifted away from the slate editor. Use just the + // start point (not the full range) to stay consistent with + // highlightOverlapsSelection, which anchors to highlight starts. + const highlightedRange = commentsSelection?.highlightedThreadId ? + highlights.find( + h => h.thread?.id === commentsSelection.highlightedThreadId + )?.range : + null; + const fallbackPoint = highlightedRange && Range.start(highlightedRange); + const overlapSelection = editorSelection || + (fallbackPoint && {anchor: fallbackPoint, focus: fallbackPoint}); + const mode = isActive ? 'active' : + highlightOverlapsSelection(highlight, overlapSelection) ? undefined : + 'dot'; return ( From 0c7d41bef64ee238218f98b7d395bf46f129c1cc Mon Sep 17 00:00:00 2001 From: Tim Fischbach Date: Wed, 6 May 2026 15:14:45 +0200 Subject: [PATCH 02/12] Move usePostMessageListener to shared/ --- .../spec/{frontend => shared}/usePostMessageListener-spec.js | 2 +- entry_types/scrolled/package/src/frontend/Content.js | 2 +- .../src/frontend/inlineEditing/EditableText/BadgeColumn.js | 2 +- .../package/src/frontend/inlineEditing/EntryDecorator.js | 2 +- .../scrolled/package/src/frontend/inlineEditing/scrollPoints.js | 2 +- .../package/src/{frontend => shared}/usePostMessageListener.js | 0 6 files changed, 5 insertions(+), 5 deletions(-) rename entry_types/scrolled/package/spec/{frontend => shared}/usePostMessageListener-spec.js (91%) rename entry_types/scrolled/package/src/{frontend => shared}/usePostMessageListener.js (100%) diff --git a/entry_types/scrolled/package/spec/frontend/usePostMessageListener-spec.js b/entry_types/scrolled/package/spec/shared/usePostMessageListener-spec.js similarity index 91% rename from entry_types/scrolled/package/spec/frontend/usePostMessageListener-spec.js rename to entry_types/scrolled/package/spec/shared/usePostMessageListener-spec.js index b917a304f6..bab9aaba82 100644 --- a/entry_types/scrolled/package/spec/frontend/usePostMessageListener-spec.js +++ b/entry_types/scrolled/package/spec/shared/usePostMessageListener-spec.js @@ -1,4 +1,4 @@ -import {usePostMessageListener} from 'frontend/usePostMessageListener'; +import {usePostMessageListener} from 'shared/usePostMessageListener'; import {renderHook} from '@testing-library/react-hooks'; import {fakeParentWindow, tick} from 'support'; diff --git a/entry_types/scrolled/package/src/frontend/Content.js b/entry_types/scrolled/package/src/frontend/Content.js index d51eb17048..79981a366b 100644 --- a/entry_types/scrolled/package/src/frontend/Content.js +++ b/entry_types/scrolled/package/src/frontend/Content.js @@ -6,7 +6,7 @@ import {useActiveExcursion} from './useActiveExcursion'; import {useCurrentSectionIndexState} from './useCurrentChapter'; import {useEntryStructure} from 'pageflow-scrolled/entryState'; import {extensible} from './extensions'; -import {usePostMessageListener} from './usePostMessageListener'; +import {usePostMessageListener} from '../shared/usePostMessageListener'; import {useSectionChangeEvents} from './useSectionChangeEvents'; import {sectionChangeMessagePoster} from './sectionChangeMessagePoster'; import {useScrollToTarget} from './useScrollTarget'; diff --git a/entry_types/scrolled/package/src/frontend/inlineEditing/EditableText/BadgeColumn.js b/entry_types/scrolled/package/src/frontend/inlineEditing/EditableText/BadgeColumn.js index 5f484a5220..e3b4ba8e94 100644 --- a/entry_types/scrolled/package/src/frontend/inlineEditing/EditableText/BadgeColumn.js +++ b/entry_types/scrolled/package/src/frontend/inlineEditing/EditableText/BadgeColumn.js @@ -7,7 +7,7 @@ import {useSlate, ReactEditor} from 'slate-react'; import {Badge, useAnchoredFloating} from 'pageflow-scrolled/review'; import {useFloatingPortalRoot} from '../../FloatingPortalRootProvider'; import {useContentElementAttributes} from '../../useContentElementAttributes'; -import {usePostMessageListener} from '../../usePostMessageListener'; +import {usePostMessageListener} from '../../../shared/usePostMessageListener'; import {useEditorSelection} from '../EditorState'; import {highlightOverlapsSelection} from './highlightOverlapsSelection'; diff --git a/entry_types/scrolled/package/src/frontend/inlineEditing/EntryDecorator.js b/entry_types/scrolled/package/src/frontend/inlineEditing/EntryDecorator.js index cf6d7d6d3d..f8d8199a45 100644 --- a/entry_types/scrolled/package/src/frontend/inlineEditing/EntryDecorator.js +++ b/entry_types/scrolled/package/src/frontend/inlineEditing/EntryDecorator.js @@ -2,7 +2,7 @@ import React, {useEffect, useCallback} from 'react'; import {ReviewStateProvider} from 'pageflow-scrolled/review'; import {useEntryStateDispatch} from 'pageflow-scrolled/entryState'; -import {usePostMessageListener} from '../usePostMessageListener'; +import {usePostMessageListener} from '../../shared/usePostMessageListener'; import {EditorStateProvider, useEditorSelection} from './EditorState'; import { useContentElementEditorCommandEmitter, diff --git a/entry_types/scrolled/package/src/frontend/inlineEditing/scrollPoints.js b/entry_types/scrolled/package/src/frontend/inlineEditing/scrollPoints.js index e33005f8b5..f762312cc2 100644 --- a/entry_types/scrolled/package/src/frontend/inlineEditing/scrollPoints.js +++ b/entry_types/scrolled/package/src/frontend/inlineEditing/scrollPoints.js @@ -1,5 +1,5 @@ import {useRef, useCallback} from 'react'; -import {usePostMessageListener} from '../usePostMessageListener'; +import {usePostMessageListener} from '../../shared/usePostMessageListener'; // Scroll points are used to preserve scroll position when toggling // the editor phone preview. Each ContentElementDecorator renders a diff --git a/entry_types/scrolled/package/src/frontend/usePostMessageListener.js b/entry_types/scrolled/package/src/shared/usePostMessageListener.js similarity index 100% rename from entry_types/scrolled/package/src/frontend/usePostMessageListener.js rename to entry_types/scrolled/package/src/shared/usePostMessageListener.js From 843be52282381714be22165aa7b0b7afe0c869b8 Mon Sep 17 00:00:00 2001 From: Tim Fischbach Date: Wed, 6 May 2026 15:22:41 +0200 Subject: [PATCH 03/12] Scroll comment badges into view on SELECT_COMMENT_THREAD --- .../inlineEditing/EditableText-spec.js | 27 +++++--- .../package/spec/review/ThreadsBadge-spec.js | 67 +++++++++++++++++++ .../inlineEditing/ContentElementDecorator.js | 7 +- .../inlineEditing/EditableText/BadgeColumn.js | 5 +- .../EditableText/BadgeColumn.module.css | 1 + .../scrolled/package/src/review/Badge.js | 9 +-- .../package/src/review/ThreadsBadge.js | 38 ++++++++++- 7 files changed, 135 insertions(+), 19 deletions(-) diff --git a/entry_types/scrolled/package/spec/frontend/inlineEditing/EditableText-spec.js b/entry_types/scrolled/package/spec/frontend/inlineEditing/EditableText-spec.js index 3fed9c774f..6eb1aeb32f 100644 --- a/entry_types/scrolled/package/spec/frontend/inlineEditing/EditableText-spec.js +++ b/entry_types/scrolled/package/spec/frontend/inlineEditing/EditableText-spec.js @@ -489,9 +489,11 @@ describe('EditableText', () => { }, expect.anything()); }); - it('runs badge click logic on SELECT_COMMENT_THREAD message', () => { + it('runs badge click logic and scrolls into view on SELECT_COMMENT_THREAD message', async () => { fakeParentWindow(); window.parent.postMessage = jest.fn(); + const scrollIntoView = jest.fn(); + Element.prototype.scrollIntoView = scrollIntoView; const subjectRange = {anchor: {path: [0, 0], offset: 0}, focus: {path: [0, 0], offset: 5}}; const value = [{type: 'paragraph', children: [{text: 'Some text to comment on'}]}]; @@ -512,14 +514,7 @@ describe('EditableText', () => { window.parent.postMessage.mockClear(); - act(() => { - window.postMessage({ - type: 'SELECT_COMMENT_THREAD', - payload: {threadId: 9} - }, '*'); - }); - - return expect(new Promise(resolve => { + const echoed = new Promise(resolve => { const original = window.parent.postMessage; window.parent.postMessage = (data, ...rest) => { original(data, ...rest); @@ -527,10 +522,22 @@ describe('EditableText', () => { resolve(data); } }; - })).resolves.toMatchObject({ + }); + + act(() => { + window.postMessage({ + type: 'SELECT_COMMENT_THREAD', + payload: {threadId: 9} + }, '*'); + }); + + await expect(echoed).resolves.toMatchObject({ type: 'SELECTED', payload: {type: 'contentElementComments', id: 1, highlightedThreadId: 9} }); + + expect(scrollIntoView).toHaveBeenCalled(); + delete Element.prototype.scrollIntoView; }); }); }); diff --git a/entry_types/scrolled/package/spec/review/ThreadsBadge-spec.js b/entry_types/scrolled/package/spec/review/ThreadsBadge-spec.js index 17df44b9f5..e19b0c8f04 100644 --- a/entry_types/scrolled/package/spec/review/ThreadsBadge-spec.js +++ b/entry_types/scrolled/package/spec/review/ThreadsBadge-spec.js @@ -3,6 +3,7 @@ import '@testing-library/jest-dom/extend-expect'; import {ThreadsBadge} from 'review/ThreadsBadge'; import {renderWithReviewState} from 'testHelpers/renderWithReviewState'; +import {fakeParentWindow} from 'support'; describe('ThreadsBadge', () => { it('does not display count for single thread', () => { @@ -146,4 +147,70 @@ describe('ThreadsBadge', () => { expect(getByRole('status')).toBeInTheDocument(); }); }); + + describe('on SELECT_COMMENT_THREAD message', () => { + let scrollIntoView; + + beforeEach(() => { + fakeParentWindow(); + scrollIntoView = jest.fn(); + Element.prototype.scrollIntoView = scrollIntoView; + }); + + afterEach(() => { + delete Element.prototype.scrollIntoView; + }); + + it('scrolls into view and calls onSelectThread when threadId matches', async () => { + const onSelectThread = jest.fn(); + + renderWithReviewState( + , + { + commentThreads: [ + {id: 5, subjectType: 'ContentElement', subjectId: 10, comments: []} + ] + } + ); + + await new Promise(resolve => { + window.postMessage({ + type: 'SELECT_COMMENT_THREAD', + payload: {threadId: 5} + }, '*'); + setTimeout(resolve, 0); + }); + + expect(scrollIntoView).toHaveBeenCalled(); + expect(onSelectThread).toHaveBeenCalledWith(5); + }); + + it('ignores messages for non-matching threadIds', async () => { + const onSelectThread = jest.fn(); + + renderWithReviewState( + , + { + commentThreads: [ + {id: 5, subjectType: 'ContentElement', subjectId: 10, comments: []} + ] + } + ); + + await new Promise(resolve => { + window.postMessage({ + type: 'SELECT_COMMENT_THREAD', + payload: {threadId: 999} + }, '*'); + setTimeout(resolve, 0); + }); + + expect(scrollIntoView).not.toHaveBeenCalled(); + expect(onSelectThread).not.toHaveBeenCalled(); + }); + }); }); diff --git a/entry_types/scrolled/package/src/frontend/inlineEditing/ContentElementDecorator.js b/entry_types/scrolled/package/src/frontend/inlineEditing/ContentElementDecorator.js index 67b180dd78..f20e290c4b 100644 --- a/entry_types/scrolled/package/src/frontend/inlineEditing/ContentElementDecorator.js +++ b/entry_types/scrolled/package/src/frontend/inlineEditing/ContentElementDecorator.js @@ -70,7 +70,12 @@ function DefaultSelectionRect(props) { selectComments()} />} + onClick={() => selectComments()} + onSelectThread={threadId => selectComments({ + type: 'contentElementComments', + id: props.id, + highlightedThreadId: threadId + })} />} commentBadgeInset={!isSelected} ariaLabel={t('pageflow_scrolled.inline_editing.select_content_element')} insertButtonTitles={t('pageflow_scrolled.inline_editing.insert_content_element')} diff --git a/entry_types/scrolled/package/src/frontend/inlineEditing/EditableText/BadgeColumn.js b/entry_types/scrolled/package/src/frontend/inlineEditing/EditableText/BadgeColumn.js index e3b4ba8e94..0633fa6068 100644 --- a/entry_types/scrolled/package/src/frontend/inlineEditing/EditableText/BadgeColumn.js +++ b/entry_types/scrolled/package/src/frontend/inlineEditing/EditableText/BadgeColumn.js @@ -79,9 +79,12 @@ function PositionedBadge({editor, highlight, highlights, editorSelection, anchor usePostMessageListener(useCallback(data => { if (data.type === 'SELECT_COMMENT_THREAD' && data.payload.threadId === highlight.thread?.id) { + if (refs.floating.current) { + refs.floating.current.scrollIntoView({block: 'nearest', behavior: 'smooth'}); + } handleClick(); } - }, [highlight, handleClick])); + }, [highlight, handleClick, refs.floating])); if (!hasAnchor) return null; diff --git a/entry_types/scrolled/package/src/frontend/inlineEditing/EditableText/BadgeColumn.module.css b/entry_types/scrolled/package/src/frontend/inlineEditing/EditableText/BadgeColumn.module.css index e40b060df4..ae333f8f7a 100644 --- a/entry_types/scrolled/package/src/frontend/inlineEditing/EditableText/BadgeColumn.module.css +++ b/entry_types/scrolled/package/src/frontend/inlineEditing/EditableText/BadgeColumn.module.css @@ -8,6 +8,7 @@ justify-content: center; width: 30px; height: 30px; + scroll-margin: space(8); } .onDark { diff --git a/entry_types/scrolled/package/src/review/Badge.js b/entry_types/scrolled/package/src/review/Badge.js index 39ca4c20a9..57e0c6bdc2 100644 --- a/entry_types/scrolled/package/src/review/Badge.js +++ b/entry_types/scrolled/package/src/review/Badge.js @@ -1,10 +1,10 @@ -import React from 'react'; +import React, {forwardRef} from 'react'; import classNames from 'classnames'; import CommentIcon from './images/comment.svg'; import styles from './Badge.module.css'; -export function Badge({counter, mode, onClick}) { +export const Badge = forwardRef(function Badge({counter, mode, onClick}, ref) { const variant = resolveVariant(mode, counter > 0); if (!variant) { @@ -12,14 +12,15 @@ export function Badge({counter, mode, onClick}) { } return ( - ); -} +}); function resolveVariant(mode, hasThreads) { switch (mode) { diff --git a/entry_types/scrolled/package/src/review/ThreadsBadge.js b/entry_types/scrolled/package/src/review/ThreadsBadge.js index face915829..8ece23ecad 100644 --- a/entry_types/scrolled/package/src/review/ThreadsBadge.js +++ b/entry_types/scrolled/package/src/review/ThreadsBadge.js @@ -1,10 +1,42 @@ -import React from 'react'; +import React, {useCallback, useRef} from 'react'; +import {usePostMessageListener} from '../shared/usePostMessageListener'; import {useCommentThreads} from './ReviewStateProvider'; import {Badge} from './Badge'; -export function ThreadsBadge({subjectType, subjectId, subjectRange, onClick, mode}) { +export function ThreadsBadge({subjectType, subjectId, subjectRange, onClick, onSelectThread, mode}) { const threads = useCommentThreads({subjectType, subjectId, subjectRange}, {resolved: false}); + const ref = useRef(); - return ; + return ( + <> + {threads.length > 0 && + } + + + ); +} + +// Side-effect-only host for the SELECT_COMMENT_THREAD listener. Only +// rendered while the badge has threads, so the listener never attaches +// for empty badges. Returns null because all the work happens in the +// effect. +function SelectThreadListener({threads, badgeRef, onSelectThread}) { + usePostMessageListener(useCallback(data => { + if (data.type !== 'SELECT_COMMENT_THREAD') return; + const threadId = data.payload.threadId; + if (!threads.some(t => t.id === threadId)) return; + + // Prefer scrolling an enclosing selectable (e.g. SelectionRect with + // aria-selected) into view so the user sees surrounding context, + // not just the badge anchored at the side. + const scrollTarget = badgeRef.current?.closest('[aria-selected]') || badgeRef.current; + if (scrollTarget) scrollTarget.scrollIntoView({block: 'nearest', behavior: 'smooth'}); + + if (onSelectThread) onSelectThread(threadId); + }, [threads, badgeRef, onSelectThread])); + + return null; } From fd10efc573f26dc5addc029e71cf61c1adb30e47 Mon Sep 17 00:00:00 2001 From: Tim Fischbach Date: Wed, 6 May 2026 14:10:54 +0200 Subject: [PATCH 04/12] Render highlighted thread and clickable affordance in ThreadList --- .../package/spec/review/ThreadList-spec.js | 43 +++++++++++++++++++ .../scrolled/package/src/review/Thread.js | 10 ++++- .../package/src/review/Thread.module.css | 13 ++++++ .../scrolled/package/src/review/ThreadList.js | 10 +++-- 4 files changed, 71 insertions(+), 5 deletions(-) diff --git a/entry_types/scrolled/package/spec/review/ThreadList-spec.js b/entry_types/scrolled/package/spec/review/ThreadList-spec.js index 1adfd087b2..8afd2375dc 100644 --- a/entry_types/scrolled/package/spec/review/ThreadList-spec.js +++ b/entry_types/scrolled/package/spec/review/ThreadList-spec.js @@ -109,6 +109,49 @@ describe('ThreadList', () => { expect(queryByText('Filtered out')).not.toBeInTheDocument(); }); + it('marks the thread matching highlightedThreadId with aria-current', () => { + const {container, getByText} = renderWithReviewState( + , + { + commentThreads: [ + {id: 1, subjectType: 'ContentElement', subjectId: 10, comments: [ + {id: 10, body: 'first', creatorName: 'Alice', creatorId: 1} + ]}, + {id: 2, subjectType: 'ContentElement', subjectId: 10, comments: [ + {id: 20, body: 'second', creatorName: 'Bob', creatorId: 2} + ]} + ] + } + ); + + const highlighted = container.querySelector('[aria-current="true"]'); + expect(highlighted).toContainElement(getByText('second')); + expect(highlighted).not.toContainElement(getByText('first')); + }); + + it('fires onThreadClick with the clicked thread', async () => { + const user = userEvent.setup(); + const onThreadClick = jest.fn(); + const {getByText} = renderWithReviewState( + , + { + commentThreads: [ + {id: 7, subjectType: 'ContentElement', subjectId: 10, comments: [ + {id: 70, body: 'click me', creatorName: 'Bob', creatorId: 2} + ]} + ] + } + ); + + await user.click(getByText('click me')); + + expect(onThreadClick).toHaveBeenCalledWith(expect.objectContaining({id: 7})); + }); + it('applies filter prop to resolved threads', async () => { const user = userEvent.setup(); const {getByText, queryByText} = renderWithReviewState( diff --git a/entry_types/scrolled/package/src/review/Thread.js b/entry_types/scrolled/package/src/review/Thread.js index 9b50823bdc..708c601c36 100644 --- a/entry_types/scrolled/package/src/review/Thread.js +++ b/entry_types/scrolled/package/src/review/Thread.js @@ -1,4 +1,5 @@ import React from 'react'; +import classNames from 'classnames'; import {useI18n} from '../frontend/i18n'; import {AvatarStack} from './Avatar'; @@ -10,14 +11,19 @@ import ResolveIcon from './images/resolve.svg'; import UnresolveIcon from './images/unresolve.svg'; import styles from './Thread.module.css'; -export function Thread({thread, collapsed, onToggle, onResolve}) { +export function Thread({thread, collapsed, onToggle, onResolve, onClick, highlighted}) { const {t} = useI18n({locale: 'ui'}); const firstComment = thread.comments[0]; const replies = thread.comments.slice(1); const repliesCollapsed = collapsed && replies.length > 0; return ( -
+
{replies.length > 0 &&
}
From 97d1d44365167d2dc592d650323b13d6caa82f43 Mon Sep 17 00:00:00 2001 From: Tim Fischbach Date: Wed, 6 May 2026 14:21:09 +0200 Subject: [PATCH 05/12] Track highlightedThreadId on entry from SELECTED contentElementComments --- .../PreviewMessageController-spec.js | 32 +++++++++++++++++++ .../controllers/PreviewMessageController.js | 5 +++ 2 files changed, 37 insertions(+) diff --git a/entry_types/scrolled/package/spec/editor/controllers/PreviewMessageController-spec.js b/entry_types/scrolled/package/spec/editor/controllers/PreviewMessageController-spec.js index 4bc21d355f..50e4fba082 100644 --- a/entry_types/scrolled/package/spec/editor/controllers/PreviewMessageController-spec.js +++ b/entry_types/scrolled/package/spec/editor/controllers/PreviewMessageController-spec.js @@ -550,6 +550,38 @@ describe('PreviewMessageController', () => { ); }); + it('sets highlightedThreadId on entry on SELECTED contentElementComments', () => { + const editor = factories.editorApi(); + const entry = factories.entry(ScrolledEntry, {}, {entryTypeSeed: normalizeSeed()}); + const iframeWindow = createIframeWindow(); + controller = new PreviewMessageController({entry, iframeWindow, editor}); + + return expect(new Promise(resolve => { + entry.once('change:highlightedThreadId', (model, value) => resolve(value)); + window.postMessage({ + type: 'SELECTED', + payload: {id: 10, type: 'contentElementComments', highlightedThreadId: 5} + }, '*'); + })).resolves.toBe(5); + }); + + it('clears highlightedThreadId on entry on SELECTED with non-comments type', () => { + const editor = factories.editorApi(); + const entry = factories.entry(ScrolledEntry, {}, {entryTypeSeed: normalizeSeed()}); + entry.set('highlightedThreadId', 5); + + const iframeWindow = createIframeWindow(); + controller = new PreviewMessageController({entry, iframeWindow, editor}); + + return expect(new Promise(resolve => { + entry.once('change:highlightedThreadId', (model, value) => resolve(value)); + window.postMessage({ + type: 'SELECTED', + payload: {id: 10, type: 'sectionSettings'} + }, '*'); + })).resolves.toBeUndefined(); + }); + it('navigates to new thread route with encoded payload on SELECTED for newThread', () => { const editor = factories.editorApi(); const entry = factories.entry(ScrolledEntry, {}, { diff --git a/entry_types/scrolled/package/src/editor/controllers/PreviewMessageController.js b/entry_types/scrolled/package/src/editor/controllers/PreviewMessageController.js index c363fa4d52..8093c67fed 100644 --- a/entry_types/scrolled/package/src/editor/controllers/PreviewMessageController.js +++ b/entry_types/scrolled/package/src/editor/controllers/PreviewMessageController.js @@ -161,6 +161,11 @@ export const PreviewMessageController = Object.extend({ const {type, id, position} = message.data.payload; this.preservedScrollTarget = null; + this.entry.set({ + highlightedThreadId: type === 'contentElementComments' ? + message.data.payload.highlightedThreadId : + undefined + }); if (type === 'contentElementComments') { const {threadIds} = message.data.payload; From 2a51b0bdfbf0c90c9f657f6b4a97b286a71a5f2c Mon Sep 17 00:00:00 2001 From: Tim Fischbach Date: Thu, 7 May 2026 17:03:10 +0200 Subject: [PATCH 06/12] Pass comment thread ids at selection through transient state --- .../commentThreadIdsAtSelection-spec.js | 60 +++++++++++++++++++ .../inlineEditing/EditableText/Selection.js | 4 ++ .../commentThreadIdsAtSelection.js | 9 +++ 3 files changed, 73 insertions(+) create mode 100644 entry_types/scrolled/package/spec/frontend/inlineEditing/EditableText/commentThreadIdsAtSelection-spec.js create mode 100644 entry_types/scrolled/package/src/frontend/inlineEditing/EditableText/commentThreadIdsAtSelection.js diff --git a/entry_types/scrolled/package/spec/frontend/inlineEditing/EditableText/commentThreadIdsAtSelection-spec.js b/entry_types/scrolled/package/spec/frontend/inlineEditing/EditableText/commentThreadIdsAtSelection-spec.js new file mode 100644 index 0000000000..a9ccb0b185 --- /dev/null +++ b/entry_types/scrolled/package/spec/frontend/inlineEditing/EditableText/commentThreadIdsAtSelection-spec.js @@ -0,0 +1,60 @@ +import {commentThreadIdsAtSelection} from 'frontend/inlineEditing/EditableText/commentThreadIdsAtSelection'; + +describe('commentThreadIdsAtSelection', () => { + const blockSelection = (block, offset = 0) => ({ + anchor: {path: [block, 0], offset}, + focus: {path: [block, 0], offset} + }); + + const highlight = (id, block) => ({ + thread: {id}, + range: { + anchor: {path: [block, 0], offset: 0}, + focus: {path: [block, 0], offset: 4} + } + }); + + it('returns empty array when selection is null', () => { + expect(commentThreadIdsAtSelection([highlight(5, 0)], null)).toEqual([]); + }); + + it('returns ids of highlights overlapping the selection block', () => { + const highlights = [ + highlight(5, 0), + highlight(6, 0), + highlight(7, 1) + ]; + + expect(commentThreadIdsAtSelection(highlights, blockSelection(0))) + .toEqual([5, 6]); + }); + + it('preserves the order from the highlights list', () => { + const highlights = [ + highlight(7, 1), + highlight(5, 0), + highlight(6, 0) + ]; + + expect(commentThreadIdsAtSelection(highlights, blockSelection(0))) + .toEqual([5, 6]); + }); + + it('skips highlights without a thread (e.g. pending new-thread highlight)', () => { + const pending = { + key: 'selection', + range: { + anchor: {path: [0, 0], offset: 0}, + focus: {path: [0, 0], offset: 4} + } + }; + + expect(commentThreadIdsAtSelection([pending, highlight(5, 0)], blockSelection(0))) + .toEqual([5]); + }); + + it('returns empty array when no highlights overlap', () => { + expect(commentThreadIdsAtSelection([highlight(5, 0)], blockSelection(2))) + .toEqual([]); + }); +}); diff --git a/entry_types/scrolled/package/src/frontend/inlineEditing/EditableText/Selection.js b/entry_types/scrolled/package/src/frontend/inlineEditing/EditableText/Selection.js index 7dae64864c..744d4e48d8 100644 --- a/entry_types/scrolled/package/src/frontend/inlineEditing/EditableText/Selection.js +++ b/entry_types/scrolled/package/src/frontend/inlineEditing/EditableText/Selection.js @@ -15,6 +15,7 @@ import {cursorLeftHighlightedThreadBlock} from './cursorLeftHighlightedThreadBlo import {cursorMovedFromPendingNewThreadRange} from './cursorMovedFromPendingNewThreadRange'; import {getUniformSelectedNode} from './getUniformSelectedNode'; import {toggleBlock, isBlockActive} from './blocks'; +import {commentThreadIdsAtSelection} from './commentThreadIdsAtSelection'; import {computeBounds} from './computeBounds'; import TextIcon from '../images/text.svg'; @@ -135,6 +136,9 @@ export function Selection(props) { typographySize: getUniformSelectedNode(editor, 'size')?.size, color: getUniformSelectedNode(editor, 'color')?.color, textAlign: getUniformSelectedNode(editor, 'textAlign')?.textAlign, + commentThreadIdsAtSelection: commentThreadIdsAtSelection( + props.highlights || [], editor.selection + ), }); boundsRef.current = {start, end}; diff --git a/entry_types/scrolled/package/src/frontend/inlineEditing/EditableText/commentThreadIdsAtSelection.js b/entry_types/scrolled/package/src/frontend/inlineEditing/EditableText/commentThreadIdsAtSelection.js new file mode 100644 index 0000000000..2764a73801 --- /dev/null +++ b/entry_types/scrolled/package/src/frontend/inlineEditing/EditableText/commentThreadIdsAtSelection.js @@ -0,0 +1,9 @@ +import {highlightOverlapsSelection} from './highlightOverlapsSelection'; + +export function commentThreadIdsAtSelection(highlights, selection) { + if (!selection) return []; + + return highlights + .filter(h => h.thread && highlightOverlapsSelection(h, selection)) + .map(h => h.thread.id); +} From b4193ba619595537e9aa12683e37c4fc68c65f01 Mon Sep 17 00:00:00 2001 From: Tim Fischbach Date: Thu, 7 May 2026 17:04:49 +0200 Subject: [PATCH 07/12] Track selectedContentElementCommentsId on entry from SELECTED message --- .../PreviewMessageController-spec.js | 71 +++++++++++++++++++ .../controllers/PreviewMessageController.js | 8 ++- 2 files changed, 78 insertions(+), 1 deletion(-) diff --git a/entry_types/scrolled/package/spec/editor/controllers/PreviewMessageController-spec.js b/entry_types/scrolled/package/spec/editor/controllers/PreviewMessageController-spec.js index 50e4fba082..68f682ea21 100644 --- a/entry_types/scrolled/package/spec/editor/controllers/PreviewMessageController-spec.js +++ b/entry_types/scrolled/package/spec/editor/controllers/PreviewMessageController-spec.js @@ -582,6 +582,77 @@ describe('PreviewMessageController', () => { })).resolves.toBeUndefined(); }); + it('sets selectedContentElementCommentsId on SELECTED contentElementComments', () => { + const editor = factories.editorApi(); + const entry = factories.entry(ScrolledEntry, {}, {entryTypeSeed: normalizeSeed()}); + const iframeWindow = createIframeWindow(); + controller = new PreviewMessageController({entry, iframeWindow, editor}); + + return expect(new Promise(resolve => { + entry.once('change:selectedContentElementCommentsId', (model, value) => resolve(value)); + window.postMessage({ + type: 'SELECTED', + payload: {id: 7, type: 'contentElementComments'} + }, '*'); + })).resolves.toBe(7); + }); + + it('sets selectedContentElementCommentsId on SELECTED contentElement', () => { + const editor = factories.editorApi(); + const entry = factories.entry(ScrolledEntry, {}, { + entryTypeSeed: normalizeSeed({contentElements: [{id: 4}]}) + }); + const iframeWindow = createIframeWindow(); + controller = new PreviewMessageController({entry, iframeWindow, editor}); + + return expect(new Promise(resolve => { + entry.once('change:selectedContentElementCommentsId', (model, value) => resolve(value)); + window.postMessage({ + type: 'SELECTED', + payload: {id: 4, type: 'contentElement'} + }, '*'); + })).resolves.toBe(4); + }); + + it('sets selectedContentElementCommentsId from permaId on SELECTED newThread', () => { + const editor = factories.editorApi(); + const entry = factories.entry(ScrolledEntry, {}, { + entryTypeSeed: normalizeSeed({contentElements: [{id: 4, permaId: 100}]}) + }); + const iframeWindow = createIframeWindow(); + controller = new PreviewMessageController({entry, iframeWindow, editor}); + + return expect(new Promise(resolve => { + entry.once('change:selectedContentElementCommentsId', (model, value) => resolve(value)); + window.postMessage({ + type: 'SELECTED', + payload: { + id: 100, + type: 'newThread', + subjectType: 'ContentElement', + range: {anchor: {path: [0, 0], offset: 0}, focus: {path: [0, 0], offset: 1}} + } + }, '*'); + })).resolves.toBe(4); + }); + + it('clears selectedContentElementCommentsId on SELECTED with non-content-element type', () => { + const editor = factories.editorApi(); + const entry = factories.entry(ScrolledEntry, {}, {entryTypeSeed: normalizeSeed()}); + entry.set('selectedContentElementCommentsId', 9); + + const iframeWindow = createIframeWindow(); + controller = new PreviewMessageController({entry, iframeWindow, editor}); + + return expect(new Promise(resolve => { + entry.once('change:selectedContentElementCommentsId', (model, value) => resolve(value)); + window.postMessage({ + type: 'SELECTED', + payload: {id: 10, type: 'sectionSettings'} + }, '*'); + })).resolves.toBeUndefined(); + }); + it('navigates to new thread route with encoded payload on SELECTED for newThread', () => { const editor = factories.editorApi(); const entry = factories.entry(ScrolledEntry, {}, { diff --git a/entry_types/scrolled/package/src/editor/controllers/PreviewMessageController.js b/entry_types/scrolled/package/src/editor/controllers/PreviewMessageController.js index 8093c67fed..76339f0361 100644 --- a/entry_types/scrolled/package/src/editor/controllers/PreviewMessageController.js +++ b/entry_types/scrolled/package/src/editor/controllers/PreviewMessageController.js @@ -164,7 +164,13 @@ export const PreviewMessageController = Object.extend({ this.entry.set({ highlightedThreadId: type === 'contentElementComments' ? message.data.payload.highlightedThreadId : - undefined + undefined, + selectedContentElementCommentsId: + type === 'contentElement' || type === 'contentElementComments' ? + id : + type === 'newThread' ? + this.entry.contentElements.findWhere({permaId: id})?.id : + undefined }); if (type === 'contentElementComments') { From 6f6ddecacb8574e202d97e247b7faaaf3859931c Mon Sep 17 00:00:00 2001 From: Tim Fischbach Date: Thu, 7 May 2026 17:12:53 +0200 Subject: [PATCH 08/12] Source ContentElementCommentsView from entry state and transient state --- .../PreviewMessageController-spec.js | 23 +- .../controllers/SideBarController-spec.js | 23 +- .../views/ContentElementCommentsView-spec.js | 225 ++++++++++++++++-- .../inlineEditing/EditableText-spec.js | 59 +---- .../controllers/PreviewMessageController.js | 6 +- .../editor/controllers/SideBarController.js | 10 +- .../src/editor/routers/SideBarRouter.js | 3 +- .../views/ContentElementCommentsView.js | 70 +++++- .../ContentElementCommentsView.module.css | 7 - .../package/src/editor/views/ReviewView.js | 28 ++- .../inlineEditing/EditableText/BadgeColumn.js | 14 +- .../scrolled/package/src/review/ThreadList.js | 3 +- 12 files changed, 301 insertions(+), 170 deletions(-) delete mode 100644 entry_types/scrolled/package/src/editor/views/ContentElementCommentsView.module.css diff --git a/entry_types/scrolled/package/spec/editor/controllers/PreviewMessageController-spec.js b/entry_types/scrolled/package/spec/editor/controllers/PreviewMessageController-spec.js index 68f682ea21..56ab7d9bbc 100644 --- a/entry_types/scrolled/package/spec/editor/controllers/PreviewMessageController-spec.js +++ b/entry_types/scrolled/package/spec/editor/controllers/PreviewMessageController-spec.js @@ -526,28 +526,7 @@ describe('PreviewMessageController', () => { return expect(new Promise(resolve => { editor.on('navigate', resolve); window.postMessage({type: 'SELECTED', payload: {id: 1, type: 'contentElementComments'}}, '*'); - })).resolves.toBe('/scrolled/content_elements/1/comments'); - }); - - it('navigates to comments route with threadIds payload on SELECTED contentElementComments', () => { - const editor = factories.editorApi(); - const entry = factories.entry(ScrolledEntry, {}, { - entryTypeSeed: normalizeSeed({contentElements: [{id: 1}]}) - }); - const iframeWindow = createIframeWindow(); - controller = new PreviewMessageController({entry, iframeWindow, editor}); - - const expectedPayload = encodeURIComponent(JSON.stringify({threadIds: [3, 7]})); - - return expect(new Promise(resolve => { - editor.on('navigate', resolve); - window.postMessage({ - type: 'SELECTED', - payload: {id: 10, type: 'contentElementComments', threadIds: [3, 7]} - }, '*'); - })).resolves.toBe( - `/scrolled/content_elements/10/comments?payload=${expectedPayload}` - ); + })).resolves.toBe('/scrolled/content_elements/comments'); }); it('sets highlightedThreadId on entry on SELECTED contentElementComments', () => { diff --git a/entry_types/scrolled/package/spec/editor/controllers/SideBarController-spec.js b/entry_types/scrolled/package/spec/editor/controllers/SideBarController-spec.js index 1a11f02604..9c56182f5a 100644 --- a/entry_types/scrolled/package/spec/editor/controllers/SideBarController-spec.js +++ b/entry_types/scrolled/package/spec/editor/controllers/SideBarController-spec.js @@ -10,7 +10,7 @@ describe('SideBarController', () => { const {createEntry} = useEditorGlobals(); describe('#contentElementComments', () => { - it('shows a ContentElementCommentsView without threadIds when no payload given', () => { + it('shows a ContentElementCommentsView in the region', () => { const entry = createEntry({ contentElements: [{id: 1, permaId: 10, typeName: 'textBlock'}] }); @@ -19,29 +19,10 @@ describe('SideBarController', () => { const region = {show: jest.fn()}; const controller = new SideBarController({region, entry}); - controller.contentElementComments(1); + controller.contentElementComments(); const shown = region.show.mock.calls[0][0]; expect(shown).toBeInstanceOf(ContentElementCommentsView); - expect(shown.options.threadIds).toBeUndefined(); - }); - - it('decodes threadIds from the payload and passes it to the view', () => { - const entry = createEntry({ - contentElements: [{id: 1, permaId: 10, typeName: 'textBlock'}] - }); - entry.reviewSession = factories.reviewSession(); - - const region = {show: jest.fn()}; - const controller = new SideBarController({region, entry}); - - const payload = encodeURIComponent(JSON.stringify({threadIds: [3, 7]})); - - controller.contentElementComments(1, payload); - - const shown = region.show.mock.calls[0][0]; - expect(shown).toBeInstanceOf(ContentElementCommentsView); - expect(shown.options.threadIds).toEqual([3, 7]); }); }); }); diff --git a/entry_types/scrolled/package/spec/editor/views/ContentElementCommentsView-spec.js b/entry_types/scrolled/package/spec/editor/views/ContentElementCommentsView-spec.js index d9f7008f98..ec9efccdcb 100644 --- a/entry_types/scrolled/package/spec/editor/views/ContentElementCommentsView-spec.js +++ b/entry_types/scrolled/package/spec/editor/views/ContentElementCommentsView-spec.js @@ -1,4 +1,5 @@ import '@testing-library/jest-dom/extend-expect'; +import userEvent from '@testing-library/user-event'; import {ContentElementCommentsView} from 'editor/views/ContentElementCommentsView'; @@ -15,10 +16,11 @@ describe('ContentElementCommentsView', () => { 'pageflow_scrolled.review.send': 'Send' }); - it('displays threads from session state', () => { + it('displays threads of selected content element from session state', () => { const entry = createEntry({ contentElements: [{id: 1, permaId: 10, typeName: 'textBlock'}] }); + entry.set('selectedContentElementCommentsId', 1); entry.reviewSession = factories.reviewSession({ commentThreads: [{ id: 1, @@ -28,21 +30,20 @@ describe('ContentElementCommentsView', () => { }] }); - const view = new ContentElementCommentsView({ - entry, - model: entry.contentElements.get(1), - editor: {} - }); + const view = new ContentElementCommentsView({entry, editor: {}}); const {getByText} = renderBackboneView(view); expect(getByText('Looks good')).toBeInTheDocument(); }); - it('filters threads by threadIds when given', () => { + it('filters threads by transient state commentThreadIdsAtSelection', () => { const entry = createEntry({ contentElements: [{id: 1, permaId: 10, typeName: 'textBlock'}] }); + entry.set('selectedContentElementCommentsId', 1); + entry.contentElements.get(1).transientState + .set('commentThreadIdsAtSelection', [1]); entry.reviewSession = factories.reviewSession({ commentThreads: [ {id: 1, subjectType: 'ContentElement', subjectId: 10, @@ -52,12 +53,7 @@ describe('ContentElementCommentsView', () => { ] }); - const view = new ContentElementCommentsView({ - entry, - model: entry.contentElements.get(1), - editor: {}, - threadIds: [1] - }); + const view = new ContentElementCommentsView({entry, editor: {}}); const {getByText, queryByText} = renderBackboneView(view); @@ -65,10 +61,11 @@ describe('ContentElementCommentsView', () => { expect(queryByText('Out of scope')).not.toBeInTheDocument(); }); - it('shows all threads when threadIds is not given', () => { + it('shows all threads when transient state has no commentThreadIdsAtSelection', () => { const entry = createEntry({ contentElements: [{id: 1, permaId: 10, typeName: 'textBlock'}] }); + entry.set('selectedContentElementCommentsId', 1); entry.reviewSession = factories.reviewSession({ commentThreads: [ {id: 1, subjectType: 'ContentElement', subjectId: 10, @@ -78,11 +75,7 @@ describe('ContentElementCommentsView', () => { ] }); - const view = new ContentElementCommentsView({ - entry, - model: entry.contentElements.get(1), - editor: {} - }); + const view = new ContentElementCommentsView({entry, editor: {}}); const {getByText} = renderBackboneView(view); @@ -90,17 +83,201 @@ describe('ContentElementCommentsView', () => { expect(getByText('Second')).toBeInTheDocument(); }); + it('updates filter when transient state changes', async () => { + const entry = createEntry({ + contentElements: [{id: 1, permaId: 10, typeName: 'textBlock'}] + }); + entry.set('selectedContentElementCommentsId', 1); + entry.reviewSession = factories.reviewSession({ + commentThreads: [ + {id: 1, subjectType: 'ContentElement', subjectId: 10, + comments: [{id: 100, body: 'In scope', creatorName: 'Alice'}]}, + {id: 2, subjectType: 'ContentElement', subjectId: 10, + comments: [{id: 200, body: 'Out of scope', creatorName: 'Bob'}]} + ] + }); + + const view = new ContentElementCommentsView({entry, editor: {}}); + const {getByText, queryByText} = renderBackboneView(view); + + expect(getByText('Out of scope')).toBeInTheDocument(); + + act(() => { + entry.contentElements.get(1).transientState + .set('commentThreadIdsAtSelection', [1]); + }); + + await waitFor(() => { + expect(queryByText('Out of scope')).not.toBeInTheDocument(); + }); + expect(getByText('In scope')).toBeInTheDocument(); + }); + + it('updates when selectedContentElementCommentsId changes', async () => { + const entry = createEntry({ + contentElements: [ + {id: 1, permaId: 10, typeName: 'textBlock'}, + {id: 2, permaId: 11, typeName: 'textBlock'} + ] + }); + entry.set('selectedContentElementCommentsId', 1); + entry.reviewSession = factories.reviewSession({ + commentThreads: [ + {id: 1, subjectType: 'ContentElement', subjectId: 10, + comments: [{id: 100, body: 'On first', creatorName: 'Alice'}]}, + {id: 2, subjectType: 'ContentElement', subjectId: 11, + comments: [{id: 200, body: 'On second', creatorName: 'Bob'}]} + ] + }); + + const view = new ContentElementCommentsView({entry, editor: {}}); + const {getByText, queryByText} = renderBackboneView(view); + + expect(getByText('On first')).toBeInTheDocument(); + + act(() => { + entry.set('selectedContentElementCommentsId', 2); + }); + + await waitFor(() => { + expect(getByText('On second')).toBeInTheDocument(); + }); + expect(queryByText('On first')).not.toBeInTheDocument(); + }); + + it('marks thread matching entry.highlightedThreadId with aria-current when scoped', () => { + const entry = createEntry({ + contentElements: [{id: 1, permaId: 10, typeName: 'textBlock'}] + }); + entry.set('selectedContentElementCommentsId', 1); + entry.contentElements.get(1).transientState + .set('commentThreadIdsAtSelection', [1, 2]); + entry.set('highlightedThreadId', 2); + entry.reviewSession = factories.reviewSession({ + commentThreads: [ + {id: 1, subjectType: 'ContentElement', subjectId: 10, + comments: [{id: 10, body: 'first', creatorName: 'Alice'}]}, + {id: 2, subjectType: 'ContentElement', subjectId: 10, + comments: [{id: 20, body: 'second', creatorName: 'Bob'}]} + ] + }); + + const view = new ContentElementCommentsView({entry, editor: {}}); + const {getByText} = renderBackboneView(view); + + expect(getByText('second').closest('[aria-current="true"]')).not.toBeNull(); + expect(getByText('first').closest('[aria-current="true"]')).toBeNull(); + }); + + it('updates highlight when entry.highlightedThreadId changes', () => { + const entry = createEntry({ + contentElements: [{id: 1, permaId: 10, typeName: 'textBlock'}] + }); + entry.set('selectedContentElementCommentsId', 1); + entry.contentElements.get(1).transientState + .set('commentThreadIdsAtSelection', [1, 2]); + entry.reviewSession = factories.reviewSession({ + commentThreads: [ + {id: 1, subjectType: 'ContentElement', subjectId: 10, + comments: [{id: 10, body: 'first', creatorName: 'Alice'}]}, + {id: 2, subjectType: 'ContentElement', subjectId: 10, + comments: [{id: 20, body: 'second', creatorName: 'Bob'}]} + ] + }); + + const view = new ContentElementCommentsView({entry, editor: {}}); + const {getByText} = renderBackboneView(view); + + expect(getByText('first').closest('[aria-current="true"]')).toBeNull(); + + act(() => { entry.set('highlightedThreadId', 1); }); + + expect(getByText('first').closest('[aria-current="true"]')).not.toBeNull(); + }); + + it('triggers selectCommentThread on entry when a thread is clicked while scoped', async () => { + const user = userEvent.setup(); + + const entry = createEntry({ + contentElements: [{id: 1, permaId: 10, typeName: 'textBlock'}] + }); + entry.set('selectedContentElementCommentsId', 1); + entry.contentElements.get(1).transientState + .set('commentThreadIdsAtSelection', [7]); + entry.reviewSession = factories.reviewSession({ + commentThreads: [{ + id: 7, subjectType: 'ContentElement', subjectId: 10, + comments: [{id: 100, body: 'click me', creatorName: 'Alice'}] + }] + }); + + const listener = jest.fn(); + entry.on('selectCommentThread', listener); + + const view = new ContentElementCommentsView({entry, editor: {}}); + const {getByText} = renderBackboneView(view); + + await user.click(getByText('click me')); + + expect(listener).toHaveBeenCalledWith(7); + }); + + it('does not highlight or trigger selectCommentThread when not scoped', async () => { + const user = userEvent.setup(); + + const entry = createEntry({ + contentElements: [{id: 1, permaId: 10, typeName: 'textBlock'}] + }); + entry.set('selectedContentElementCommentsId', 1); + entry.set('highlightedThreadId', 7); + entry.reviewSession = factories.reviewSession({ + commentThreads: [{ + id: 7, subjectType: 'ContentElement', subjectId: 10, + comments: [{id: 100, body: 'click me', creatorName: 'Alice'}] + }] + }); + + const listener = jest.fn(); + entry.on('selectCommentThread', listener); + + const view = new ContentElementCommentsView({entry, editor: {}}); + const {getByText} = renderBackboneView(view); + + expect(getByText('click me').closest('[aria-current="true"]')).toBeNull(); + + await user.click(getByText('click me')); + + expect(listener).not.toHaveBeenCalled(); + }); + + it('does not render a new-topic button', () => { + const entry = createEntry({ + contentElements: [{id: 1, permaId: 10, typeName: 'textBlock'}] + }); + entry.set('selectedContentElementCommentsId', 1); + entry.reviewSession = factories.reviewSession({ + commentThreads: [{ + id: 1, + subjectType: 'ContentElement', + subjectId: 10, + comments: [{id: 100, body: 'A thread', creatorName: 'Alice'}] + }] + }); + + const view = new ContentElementCommentsView({entry, editor: {}}); + const {queryByRole} = renderBackboneView(view); + + expect(queryByRole('button', {name: 'New topic'})).not.toBeInTheDocument(); + }); + it('updates when session emits change:thread', async () => { const entry = createEntry({ contentElements: [{id: 1, permaId: 10, typeName: 'textBlock'}] }); + entry.set('selectedContentElementCommentsId', 1); entry.reviewSession = factories.reviewSession(); - const view = new ContentElementCommentsView({ - entry, - model: entry.contentElements.get(1), - editor: {} - }); + const view = new ContentElementCommentsView({entry, editor: {}}); const {getByText} = renderBackboneView(view); diff --git a/entry_types/scrolled/package/spec/frontend/inlineEditing/EditableText-spec.js b/entry_types/scrolled/package/spec/frontend/inlineEditing/EditableText-spec.js index 6eb1aeb32f..31012f5a41 100644 --- a/entry_types/scrolled/package/spec/frontend/inlineEditing/EditableText-spec.js +++ b/entry_types/scrolled/package/spec/frontend/inlineEditing/EditableText-spec.js @@ -410,81 +410,34 @@ describe('EditableText', () => { expect(badges[2]).toHaveClass(badgeStyles.dot); }); - it('posts SELECTED contentElementComments with overlapping threadIds on badge click', () => { + it('posts SELECTED contentElementComments with highlightedThreadId on badge click', () => { fakeParentWindow(); window.parent.postMessage = jest.fn(); const value = [ - {type: 'paragraph', children: [{text: 'First paragraph'}]}, - {type: 'paragraph', children: [{text: 'Second paragraph'}]} + {type: 'paragraph', children: [{text: 'First paragraph'}]} ]; - const {getAllByRole} = renderWithCommenting( + const {getByRole} = renderWithCommenting( , { wrapper, commentThreads: [ {id: 5, subjectType: 'ContentElement', subjectId: 10, subjectRange: {anchor: {path: [0, 0], offset: 0}, focus: {path: [0, 0], offset: 5}}, - comments: [{id: 1, body: 'p0a', creatorName: 'Alice', creatorId: 1}]}, - {id: 6, subjectType: 'ContentElement', subjectId: 10, - subjectRange: {anchor: {path: [0, 0], offset: 6}, focus: {path: [0, 0], offset: 9}}, - comments: [{id: 2, body: 'p0b', creatorName: 'Bob', creatorId: 2}]}, - {id: 7, subjectType: 'ContentElement', subjectId: 10, - subjectRange: {anchor: {path: [1, 0], offset: 0}, focus: {path: [1, 0], offset: 6}}, - comments: [{id: 3, body: 'p1', creatorName: 'Eve', creatorId: 3}]} + comments: [{id: 1, body: 'a', creatorName: 'Alice', creatorId: 1}]} ] } ); - const badges = getAllByRole('status'); - fireEvent.click(badges[0]); - - expect(window.parent.postMessage).toHaveBeenCalledWith({ - type: 'SELECTED', - payload: { - type: 'contentElementComments', - id: 1, - highlightedThreadId: 5, - threadIds: [5, 6] - } - }, expect.anything()); - }); - - it('scopes threadIds to clicked highlight\'s start block, ignoring later blocks it spans', () => { - fakeParentWindow(); - window.parent.postMessage = jest.fn(); - - const value = [ - {type: 'paragraph', children: [{text: 'First paragraph'}]}, - {type: 'paragraph', children: [{text: 'Second paragraph'}]} - ]; - - const {getAllByRole} = renderWithCommenting( - , - { - wrapper, - commentThreads: [ - {id: 5, subjectType: 'ContentElement', subjectId: 10, - subjectRange: {anchor: {path: [0, 0], offset: 0}, focus: {path: [1, 0], offset: 6}}, - comments: [{id: 1, body: 'spans', creatorName: 'Alice', creatorId: 1}]}, - {id: 7, subjectType: 'ContentElement', subjectId: 10, - subjectRange: {anchor: {path: [1, 0], offset: 7}, focus: {path: [1, 0], offset: 16}}, - comments: [{id: 2, body: 'p1', creatorName: 'Eve', creatorId: 2}]} - ] - } - ); - - const badges = getAllByRole('status'); - fireEvent.click(badges[0]); + fireEvent.click(getByRole('status')); expect(window.parent.postMessage).toHaveBeenCalledWith({ type: 'SELECTED', payload: { type: 'contentElementComments', id: 1, - highlightedThreadId: 5, - threadIds: [5] + highlightedThreadId: 5 } }, expect.anything()); }); diff --git a/entry_types/scrolled/package/src/editor/controllers/PreviewMessageController.js b/entry_types/scrolled/package/src/editor/controllers/PreviewMessageController.js index 76339f0361..6572ba06f7 100644 --- a/entry_types/scrolled/package/src/editor/controllers/PreviewMessageController.js +++ b/entry_types/scrolled/package/src/editor/controllers/PreviewMessageController.js @@ -174,11 +174,7 @@ export const PreviewMessageController = Object.extend({ }); if (type === 'contentElementComments') { - const {threadIds} = message.data.payload; - const query = threadIds ? - `?payload=${encodeURIComponent(JSON.stringify({threadIds}))}` : - ''; - this.editor.navigate(`/scrolled/content_elements/${id}/comments${query}`, {trigger: true}) + this.editor.navigate('/scrolled/content_elements/comments', {trigger: true}) } else if (type === 'newThread') { const {subjectType, range} = message.data.payload; diff --git a/entry_types/scrolled/package/src/editor/controllers/SideBarController.js b/entry_types/scrolled/package/src/editor/controllers/SideBarController.js index 1c511ca48e..c4467e1c32 100644 --- a/entry_types/scrolled/package/src/editor/controllers/SideBarController.js +++ b/entry_types/scrolled/package/src/editor/controllers/SideBarController.js @@ -50,16 +50,10 @@ export const SideBarController = Marionette.Controller.extend({ })); }, - contentElementComments: function(id, payload) { - const threadIds = payload ? - JSON.parse(decodeURIComponent(payload)).threadIds : - undefined; - + contentElementComments: function() { this.region.show(new ContentElementCommentsView({ entry: this.entry, - model: this.entry.contentElements.get(id), - editor, - threadIds + editor })); }, diff --git a/entry_types/scrolled/package/src/editor/routers/SideBarRouter.js b/entry_types/scrolled/package/src/editor/routers/SideBarRouter.js index 503cb91001..5eabc42a8e 100644 --- a/entry_types/scrolled/package/src/editor/routers/SideBarRouter.js +++ b/entry_types/scrolled/package/src/editor/routers/SideBarRouter.js @@ -7,8 +7,7 @@ export const SideBarRouter = Marionette.AppRouter.extend({ 'scrolled/sections/:id/paddings?position=:position': 'sectionPaddings', 'scrolled/sections/:id/paddings': 'sectionPaddings', 'scrolled/sections/:id': 'section', - 'scrolled/content_elements/:id/comments?payload=:payload': 'contentElementComments', - 'scrolled/content_elements/:id/comments': 'contentElementComments', + 'scrolled/content_elements/comments': 'contentElementComments', 'scrolled/content_elements/:id': 'contentElement', 'scrolled/comment_threads/new?subjectType=:subjectType&subjectId=:subjectId&payload=:payload': 'newThread' } diff --git a/entry_types/scrolled/package/src/editor/views/ContentElementCommentsView.js b/entry_types/scrolled/package/src/editor/views/ContentElementCommentsView.js index dd146cbdec..17b82b4cbd 100644 --- a/entry_types/scrolled/package/src/editor/views/ContentElementCommentsView.js +++ b/entry_types/scrolled/package/src/editor/views/ContentElementCommentsView.js @@ -3,18 +3,74 @@ import React from 'react'; import {ThreadList} from 'pageflow-scrolled/review'; import {ReviewView} from './ReviewView'; -import styles from './ContentElementCommentsView.module.css'; export const ContentElementCommentsView = ReviewView.extend({ - renderContent() { - const {threadIds} = this.options; - const filter = threadIds ? thread => threadIds.includes(thread.id) : undefined; + initialize() { + const {entry} = this.options; + + this.listenTo(entry, + 'change:selectedContentElementCommentsId', + this._onSelectedChange); + this.listenTo(entry, + 'change:highlightedThreadId', + () => this.rerender()); + this._observeContentElement(); + }, + + props() { + const contentElement = this._contentElement; + return { + contentElement, + threadIds: contentElement?.transientState.get('commentThreadIdsAtSelection'), + highlightedThreadId: this.options.entry.get('highlightedThreadId'), + onThreadClick: thread => + this.options.entry.trigger('selectCommentThread', thread.id) + }; + }, + + // Highlight and per-thread click are only wired when the view is + // scoped to a list of thread ids. For unscoped content elements + // there is no anchor for an individual thread in the iframe, so + // highlighting one in the list would have no counterpart in the + // preview. + renderContent({contentElement, threadIds, highlightedThreadId, onThreadClick}) { + if (!contentElement) return null; + + if (threadIds === undefined) { + return ( + + ); + } return ( + subjectId={contentElement.get('permaId')} + filter={thread => threadIds.includes(thread.id)} + highlightedThreadId={highlightedThreadId} + onThreadClick={onThreadClick} + hideNewTopicButton /> ); + }, + + _onSelectedChange() { + this._observeContentElement(); + this.rerender(); + }, + + _observeContentElement() { + if (this._contentElement) { + this.stopListening(this._contentElement.transientState); + } + + const id = this.options.entry.get('selectedContentElementCommentsId'); + this._contentElement = id ? this.options.entry.contentElements.get(id) : null; + + if (this._contentElement) { + this.listenTo(this._contentElement.transientState, + 'change:commentThreadIdsAtSelection', + () => this.rerender()); + } } }); diff --git a/entry_types/scrolled/package/src/editor/views/ContentElementCommentsView.module.css b/entry_types/scrolled/package/src/editor/views/ContentElementCommentsView.module.css deleted file mode 100644 index a4f780243d..0000000000 --- a/entry_types/scrolled/package/src/editor/views/ContentElementCommentsView.module.css +++ /dev/null @@ -1,7 +0,0 @@ -.newTopicButton { - composes: secondaryIconButton from './buttons.module.css'; - display: flex !important; - position: absolute; - right: 0; - bottom: 100%; -} diff --git a/entry_types/scrolled/package/src/editor/views/ReviewView.js b/entry_types/scrolled/package/src/editor/views/ReviewView.js index fc3118ea04..b8f64c4a5f 100644 --- a/entry_types/scrolled/package/src/editor/views/ReviewView.js +++ b/entry_types/scrolled/package/src/editor/views/ReviewView.js @@ -9,11 +9,18 @@ import styles from './ReviewView.module.css'; // Base Marionette view for comment-related sidebar panels. Provides the // shared wiring: a container div, a ReviewMessageHandler bridging the // session to the preview iframe, and a ReviewStateProvider seeded from -// the current session state. Subclasses implement `renderContent()` to -// return the React subtree. +// the current session state. Subclasses implement `renderContent(props)` +// to return the React subtree. Props are produced by `props()` (default: +// empty) and re-evaluated whenever the subclass calls `rerender()` — +// useful for re-rendering on backbone change events without requiring +// React subscription hooks inside the rendered tree. export const ReviewView = Marionette.ItemView.extend({ template: () => `
`, + props() { + return {}; + }, + onShow() { const session = this.options.entry.reviewSession; @@ -22,12 +29,7 @@ export const ReviewView = Marionette.ItemView.extend({ targetWindow: window }); - ReactDOM.render( - - {this.renderContent()} - , - this._containerEl() - ); + this.rerender(); }, onClose() { @@ -35,6 +37,16 @@ export const ReviewView = Marionette.ItemView.extend({ ReactDOM.unmountComponentAtNode(this._containerEl()); }, + rerender() { + const session = this.options.entry.reviewSession; + ReactDOM.render( + + {this.renderContent(this.props())} + , + this._containerEl() + ); + }, + _containerEl() { return this.$el.find(`.${styles.container}`)[0]; } diff --git a/entry_types/scrolled/package/src/frontend/inlineEditing/EditableText/BadgeColumn.js b/entry_types/scrolled/package/src/frontend/inlineEditing/EditableText/BadgeColumn.js index 0633fa6068..33338462c0 100644 --- a/entry_types/scrolled/package/src/frontend/inlineEditing/EditableText/BadgeColumn.js +++ b/entry_types/scrolled/package/src/frontend/inlineEditing/EditableText/BadgeColumn.js @@ -59,22 +59,14 @@ function PositionedBadge({editor, highlight, highlights, editorSelection, anchor // on screen until the user's next interaction with the editor; // slate's internal state (which downstream consumers depend on) // stays correct. - const highlightStart = Range.start(highlight.range); - Transforms.select(editor, highlightStart); - - const threadIds = highlights - .filter(h => h.thread && highlightOverlapsSelection( - h, {anchor: highlightStart, focus: highlightStart} - )) - .map(h => h.thread.id); + Transforms.select(editor, Range.start(highlight.range)); selectComments({ type: 'contentElementComments', id: contentElementId, - highlightedThreadId: highlight.thread?.id, - threadIds + highlightedThreadId: highlight.thread?.id }); - }, [editor, highlight, highlights, selectComments, contentElementId]); + }, [editor, highlight, selectComments, contentElementId]); usePostMessageListener(useCallback(data => { if (data.type === 'SELECT_COMMENT_THREAD' && diff --git a/entry_types/scrolled/package/src/review/ThreadList.js b/entry_types/scrolled/package/src/review/ThreadList.js index 9dd9949f79..f8719cd761 100644 --- a/entry_types/scrolled/package/src/review/ThreadList.js +++ b/entry_types/scrolled/package/src/review/ThreadList.js @@ -11,7 +11,7 @@ import ChevronIcon from './images/chevron.svg'; import NewTopicIcon from './images/newTopic.svg'; import styles from './ThreadList.module.css'; -export function ThreadList({subjectType, subjectId, subjectRange, filter, highlightedThreadId, onThreadClick, showNewForm: showNewFormProp, hideNewTopicButton, reversed, onDismiss, newTopicButtonClassName}) { +export function ThreadList({subjectType, subjectId, subjectRange, filter, highlightedThreadId, onThreadClick, showNewForm: showNewFormProp, hideNewTopicButton, reversed, onDismiss}) { const {t} = useI18n({locale: 'ui'}); const allActiveThreads = useCommentThreads({subjectType, subjectId, subjectRange}, {resolved: false}); const allResolvedThreads = useCommentThreads({subjectType, subjectId, subjectRange}, {resolved: true}); @@ -40,7 +40,6 @@ export function ThreadList({subjectType, subjectId, subjectRange, filter, highli
{!showNewForm && !hideNewTopicButton &&