Show comment thread near drawer row when not anchored When clicking a comment row in the Comments drawer whose target element can no longer be found or has been removed: - Mark the thread as orphaned (`isOrphaned`) during `ThreadRevealer.reveal` and display a `(removed)` label and disabled styling on the drawer row. - Pass the clicked row's position via `RevealableThread` as a fallback position to `CommentsOverlayWidget.openThread`, rendering the thread popup directly above the drawer row (`translateY(-100%)`) when no anchor pin is present. - Close the open comment thread popup when clicking outside the overlay content element. Bug: 567396200 Change-Id: I2f28bdbc759db58882eedc1fd01942a0ffd60db9 Reviewed-on: https://chromium-review.googlesource.com/c/devtools/devtools-frontend/+/8585335 Reviewed-by: Yulun Zeng <yulunz@chromium.org> Auto-Submit: Natallia Harshunova <nharshunova@chromium.org> Commit-Queue: Natallia Harshunova <nharshunova@chromium.org>
diff --git a/front_end/models/comment_manager/CommentThread.ts b/front_end/models/comment_manager/CommentThread.ts index 83c2133..d827052 100644 --- a/front_end/models/comment_manager/CommentThread.ts +++ b/front_end/models/comment_manager/CommentThread.ts
@@ -97,6 +97,7 @@ readonly anchor: CommentAnchorSignature; /** True for comments generated from the change tracker. */ readonly isGeneratedComment: boolean; + #isOrphaned = false; comments: Comment[]; status: CommentThreadStatus = 'DRAFT'; transmitted: boolean = false; @@ -108,6 +109,18 @@ this.isGeneratedComment = Boolean(options.isGeneratedComment); } + get isOrphaned(): boolean { + return this.#isOrphaned; + } + + setOrphaned(isOrphaned: boolean): void { + if (this.#isOrphaned === isOrphaned) { + return; + } + this.#isOrphaned = isOrphaned; + this.dispatchEventToListeners(Events.CHANGED); + } + /** * Returns whether it was changed. */ @@ -189,3 +202,11 @@ this.dispatchEventToListeners(Events.CHANGED); } } + +export class RevealableThread { + constructor( + readonly thread: CommentThread, + readonly position?: {left: number, top: number}, + ) { + } +}
diff --git a/front_end/panels/comments/CommentsPane.test.ts b/front_end/panels/comments/CommentsPane.test.ts index 25e4071..1135998 100644 --- a/front_end/panels/comments/CommentsPane.test.ts +++ b/front_end/panels/comments/CommentsPane.test.ts
@@ -123,9 +123,13 @@ const [t1] = seedThreads(commentManager); const {view} = await createWidget(commentManager); - view.input.onThreadClick(t1); + view.input.onThreadClick(t1, {left: 16, top: 450}); - sinon.assert.calledOnceWithExactly(revealStub, t1, false); + sinon.assert.calledOnceWithExactly( + revealStub, + new CommentManager.CommentThread.RevealableThread(t1, {left: 16, top: 450}), + false, + ); }); it('sends all active threads to agent when send to agent button is clicked', async () => { @@ -287,6 +291,9 @@ const threadItem = target.querySelector('.comment-thread-item') as HTMLElement; threadItem.dispatchEvent(new KeyboardEvent('keydown', {key: 'Enter', bubbles: true, cancelable: true})); - sinon.assert.calledOnceWithExactly(onThreadClick, mockThreadData.thread); + sinon.assert.calledOnceWithMatch(onThreadClick, mockThreadData.thread, sinon.match({ + left: sinon.match.number, + top: sinon.match.number, + })); }); });
diff --git a/front_end/panels/comments/CommentsPane.ts b/front_end/panels/comments/CommentsPane.ts index 33fa25f..abfcd7a 100644 --- a/front_end/panels/comments/CommentsPane.ts +++ b/front_end/panels/comments/CommentsPane.ts
@@ -58,6 +58,10 @@ * @description Tab label for sent and resolved comments in the comments drawer. */ sent: 'Sent', + /** + * @description Label shown next to an element anchor in the comments drawer when the target DOM node has been removed or can no longer be found. + */ + removed: 'removed', } as const; const UIStringsNotTranslate = { @@ -85,7 +89,10 @@ threads: ThreadViewData[]; onTabSelect: (tab: Tab) => void; onClearAll: () => void; - onThreadClick: (thread: CommentManager.CommentManager.CommentThread) => void; + onThreadClick: ( + thread: CommentManager.CommentManager.CommentThread, + fallbackPosition?: {left: number, top: number}, + ) => void; onDeleteThread: (threadId: string) => void; onSendToAgent: () => void; } @@ -148,6 +155,12 @@ export const DEFAULT_VIEW: View = (input: ViewInput, _output: undefined, target: HTMLElement): void => { const isQueued = input.activeTab === 'queued'; const isSent = input.activeTab === 'sent'; + const onThreadClick = (e: Event, thread: CommentManager.CommentManager.CommentThread): void => { + const rowEl = e.currentTarget as HTMLElement; + const rowRect = rowEl.getBoundingClientRect(); + const badgeRect = rowEl.querySelector('.comment-pin-badge')?.getBoundingClientRect(); + input.onThreadClick(thread, {left: badgeRect?.left ?? rowRect.left, top: rowRect.top}); + }; // clang-format off render(html` <style>${commentsPaneStyles}</style> @@ -192,22 +205,23 @@ item => item.thread.id, item => html` <li - class="comment-thread-item" + class="comment-thread-item ${item.thread.isOrphaned ? 'orphaned' : ''}" role="listitem" tabindex="0" - @click=${() => input.onThreadClick(item.thread)} + @click=${(e: Event) => onThreadClick(e, item.thread)} @keydown=${(e: KeyboardEvent) => { if (e.key === 'Enter' || e.key === ' ') { e.preventDefault(); - input.onThreadClick(item.thread); + onThreadClick(e, item.thread); } }} jslog=${VisualLogging.item('comment-thread').track({ click: true })}> <div class="comment-pin-badge" aria-hidden="true"></div> - <span class="anchor-chip"> + <span class="anchor-chip ${item.thread.isOrphaned ? 'orphaned' : ''}"> ${'node' in item.title ? - widget(CommonPanels.DOMLinkifier.DOMNodeLink, { node: item.title.node, options: { preventKeyboardFocus: true } }) : + widget(CommonPanels.DOMLinkifier.DOMNodeLink, { node: item.title.node, options: { preventKeyboardFocus: true, disabled: item.thread.isOrphaned } }) : html`<span class="anchor-chip-text">${item.title.text}</span>`} + ${item.thread.isOrphaned ? html`<span class="orphaned-label">(${i18nString(UIStrings.removed)})</span>` : Lit.nothing} </span> <div class="comment-text">${item.commentText}</div> ${renderThreadStatus(item.thread, input.onDeleteThread)} @@ -311,8 +325,11 @@ this.requestUpdate(); }; - #handleThreadClick = (thread: CommentManager.CommentManager.CommentThread): void => { - void Common.Revealer.reveal(thread); + #handleThreadClick = ( + thread: CommentManager.CommentManager.CommentThread, + position?: {left: number, top: number}, + ): void => { + void Common.Revealer.reveal(new CommentManager.CommentThread.RevealableThread(thread, position)); }; #handleDeleteThread = (threadId: string): void => {
diff --git a/front_end/panels/comments/commentsPane.css b/front_end/panels/comments/commentsPane.css index 8c780a3..b66ac91 100644 --- a/front_end/panels/comments/commentsPane.css +++ b/front_end/panels/comments/commentsPane.css
@@ -131,6 +131,11 @@ flex-shrink: 0; } + .comment-thread-item.orphaned .comment-pin-badge { + background-color: var(--sys-color-surface-variant); + color: var(--sys-color-on-surface-subtle); + } + .anchor-chip { display: flex; height: calc(var(--sys-size-8) + var(--sys-size-2)); @@ -149,6 +154,26 @@ pointer-events: none; } + .anchor-chip.orphaned { + border-style: dashed; + border-color: var(--sys-color-neutral-outline); + background: transparent; + color: var(--sys-color-state-disabled); + gap: var(--sys-size-3); + } + + .anchor-chip.orphaned .anchor-chip-text { + color: var(--sys-color-state-disabled); + } + + .orphaned-label { + flex-shrink: 0; + color: var(--sys-color-on-surface-subtle); + font-family: var(--default-font-family); + font-size: var(--sys-typescale-body5-size); + font-style: italic; + } + .anchor-chip devtools-widget, .anchor-chip-text { display: block;
diff --git a/front_end/panels/common/CommentsOverlayWidget.test.ts b/front_end/panels/common/CommentsOverlayWidget.test.ts index bf9f160..f3e02a8 100644 --- a/front_end/panels/common/CommentsOverlayWidget.test.ts +++ b/front_end/panels/common/CommentsOverlayWidget.test.ts
@@ -767,4 +767,64 @@ widget.detach(); } }); + + it('shows the comment thread popup at the fallback position when not anchored to any element', async () => { + const widget = new PanelCommon.CommentsOverlayWidget.CommentsOverlayWidget(undefined, [commentManager]); + widget.setOverlayManagerForTest(overlayManager); + widget.markAsRoot(); + renderElementIntoDOM(widget, {allowMultipleChildren: true}); + + const thread = commentManager.createCommentThread( + { + vePath: 'Panel: elements > TreeItem: removed-node', + textSignature: 'div.removed', + }, + 'Comment on removed element', + ); + thread.save(); + + widget.openThread(thread.id, {left: 28, top: 450}); + await UI.Widget.Widget.allUpdatesComplete; + + assert.isNull(widget.contentElement.querySelector('.comment-pin')); + const popup = widget.contentElement.querySelector('.comment-popup-widget') as HTMLElement; + assert.isNotNull(popup); + assert.include(popup.style.transform, 'translateY(-100%)'); + + widget.detach(); + }); + + it('passes activePin and fallbackPosition to view input when opening an anchored thread', async () => { + const view = createViewFunctionStub(PanelCommon.CommentsOverlayWidget.CommentsOverlayWidget); + const widget = new PanelCommon.CommentsOverlayWidget.CommentsOverlayWidget( + undefined, + [commentManager], + view, + ); + widget.setOverlayManagerForTest(overlayManager); + widget.markAsRoot(); + renderElementIntoDOM(widget, {allowMultipleChildren: true}); + await view.nextInput; + + const testEl = document.createElement('div'); + testEl.setAttribute('jslog', 'TreeItem; context: rematched-target'); + testEl.textContent = 'rematched element'; + testEl.getBoundingClientRect = () => new DOMRect(100, 120, 150, 30); + renderElementIntoDOM(testEl, {allowMultipleChildren: true}); + + const thread = overlayManager.createComment(testEl, 'Anchored comment'); + assert.isNotNull(thread); + thread.save(); + await view.nextInput; + + widget.openThread(thread.id, {left: 16, top: 450}); + const updatedInput = await view.nextInput; + + assert.lengthOf(updatedInput.pins, 1); + assert.strictEqual(updatedInput.activePin, updatedInput.pins[0]); + assert.deepEqual(updatedInput.fallbackPosition, {left: 16, top: 450}); + + testEl.remove(); + widget.detach(); + }); });
diff --git a/front_end/panels/common/CommentsOverlayWidget.ts b/front_end/panels/common/CommentsOverlayWidget.ts index cc2a962..dbd915e 100644 --- a/front_end/panels/common/CommentsOverlayWidget.ts +++ b/front_end/panels/common/CommentsOverlayWidget.ts
@@ -27,6 +27,7 @@ onPinClick: (threadId: string) => void; activeThread: CommentManager.CommentManager.CommentThread|null; activePin: Comments.CommentOverlayManager.PinPositionData|null; + fallbackPosition?: {left: number, top: number}|null; title: Title; onAddComment: (text: string) => void; onDeleteQueuedComment: () => void; @@ -48,6 +49,7 @@ const AUTO_CLOSE_DELAY_MS = 2000; const DEFAULT_VIEW: View = (input: ViewInput, _output: undefined, target: HTMLElement): void => { + const popupPosition = input.activePin ?? input.fallbackPosition; // clang-format off render(html` <style>${commentsOverlayStyles}</style> @@ -91,23 +93,26 @@ </div> `, )} - ${input.activePin && input.activeThread ? repeat( - [{pin: input.activePin, thread: input.activeThread}], + ${popupPosition && input.activeThread ? repeat( + [{position: popupPosition, thread: input.activeThread}], item => item.thread.id, item => { - const popupTop = Math.min( - Math.max(POPUP_MARGIN, item.pin.top + PIN_HEIGHT), - Math.max(POPUP_MARGIN, target.clientHeight - POPUP_HEIGHT), - ); + // Clamp horizontal position so the popup stays within the viewport with POPUP_MARGIN padding. const popupLeft = Math.min( - Math.max(POPUP_MARGIN, item.pin.left), + Math.max(POPUP_MARGIN, item.position.left), Math.max(POPUP_MARGIN, target.clientWidth - POPUP_WIDTH - POPUP_MARGIN), ); + // When anchored to a pin (`activePin`), place the popup below the pin (`top + PIN_HEIGHT`). + // When unanchored (`fallbackPosition` from a drawer row), place the anchor point just above + // the row (`top - POPUP_MARGIN`) and shift the popup up by its own height via `translateY(-100%)`. + const topOffset = input.activePin ? item.position.top + PIN_HEIGHT : item.position.top - POPUP_MARGIN; + const maxTop = target.clientHeight - (input.activePin ? POPUP_HEIGHT : POPUP_MARGIN); + const popupTop = Math.min(Math.max(POPUP_MARGIN, topOffset), Math.max(POPUP_MARGIN, maxTop)); return html` <div class="comment-popup-widget" style=${styleMap({ - transform: `translate3d(${popupLeft}px, ${popupTop}px, 0)`, + transform: `translate3d(${popupLeft}px, ${popupTop}px, 0)${input.activePin ? '' : ' translateY(-100%)'}`, })}> ${UI.Widget.widget(CommentThreadWidget, { title: input.title, @@ -136,11 +141,13 @@ readonly #commentManager: CommentManager.CommentManager.CommentManager; #commentOverlayManager: Comments.CommentOverlayManager.CommentOverlayManager; #activeThreadId: string|null = null; + #fallbackPosition: {left: number, top: number}|null = null; #closeTimeoutId: number|null = null; #cachedTitle: Title = {text: ''}; #cachedTitleAnchor: CommentManager.CommentManager.CommentAnchorSignature|null = null; #setActiveThreadId(threadId: string|null): void { + this.#fallbackPosition = null; if (this.#activeThreadId !== threadId) { this.#clearCloseTimeout(); this.#activeThreadId = threadId; @@ -403,6 +410,7 @@ onPinClick: this.#handlePinClick, activeThread, activePin, + fallbackPosition: this.#fallbackPosition, title, onAddComment: (text: string) => { if (!activeThread) { @@ -471,10 +479,11 @@ this.#renderWithTitle(activeThread, title); } - openThread(threadId: string): void { + openThread(threadId: string, fallbackPosition?: {left: number, top: number}): void { this.#clearCloseTimeout(); this.#commentOverlayManager.clearDraftThreads(); this.#setActiveThreadId(threadId); + this.#fallbackPosition = fallbackPosition ?? null; this.requestUpdate(); } } @@ -496,20 +505,21 @@ return widgetInstance; } -export class ThreadRevealer implements Common.Revealer.Revealer<CommentManager.CommentManager.CommentThread> { - async reveal(thread: CommentManager.CommentManager.CommentThread): Promise<void> { +export class ThreadRevealer implements Common.Revealer.Revealer<CommentManager.CommentThread.RevealableThread> { + async reveal({thread, position}: CommentManager.CommentThread.RevealableThread): Promise<void> { const panelId = Comments.CommentAnchorResolver.extractPanelId(thread.anchor.vePath); if (panelId && UI.InspectorView.InspectorView.instance().hasPanel(panelId)) { await UI.InspectorView.InspectorView.instance().showPanel(panelId); } const element = Comments.CommentAnchorResolver.rematchCommentAnchor(thread); + thread.setOrphaned(!element); if (element) { element.scrollIntoView({block: 'center', inline: 'nearest', behavior: 'smooth'}); } const overlay = getOrCreateOverlayWidget(); - overlay.openThread(thread.id); + overlay.openThread(thread.id, position); } }
diff --git a/front_end/panels/common/common-meta.ts b/front_end/panels/common/common-meta.ts index 392595a..3471361 100644 --- a/front_end/panels/common/common-meta.ts +++ b/front_end/panels/common/common-meta.ts
@@ -78,7 +78,7 @@ Common.Revealer.registerRevealer({ contextTypes() { - return [CommentManager.CommentManager.CommentThread]; + return [CommentManager.CommentThread.RevealableThread]; }, async loadRevealer() { const CommonModule = await loadCommonModule();