From 60d3ad4ce59a2bd6d0bcd9a3bb6959a018052ddf Mon Sep 17 00:00:00 2001 From: Aleksander Bodurri Date: Sun, 21 Jun 2026 23:50:46 -0400 Subject: [PATCH] fix(devtools): highlight SVG and directive hosts correctly Before SVG hosts could be skipped because the highlighter checked for HTMLElement. A case like would not get a proper overlay. The highlighter now works with Element so SVG hosts can be found and measured. Before Directive only hosts could be skipped because the inspected-page lookup and directive explorer matched component hosts and component ids only. A case like Todos could highlight the parent component instead of the anchor, and the RouterLink only tree node could fail to select, highlight, or request an overlay. Id 0 could also miss the highlighted state because the tree treated it as absent. The lookup now stops at the nearest component or directive host, and tree matching accepts component and directive ids using null as the absent state. Selected overlays could fail to reappear for the same element because unhighlighting removed the overlay but left the selected element cached. selected-element cache is now reused only while its overlay is still connected, and unhighlighting clears the cached element. Overlay geometry used integer coercion. An SVG or transformed element with a 0.5px bounding box could get a 0px overlay. Overlay positioning now preserves fractional DOMRect values. --- .../component-inspector.ts | 11 +- .../src/lib/component-tree/component-tree.ts | 5 +- .../src/lib/highlighter.spec.ts | 125 ++++++++++++++++++ .../src/lib/highlighter.ts | 41 +++--- .../directive-explorer.component.ts | 2 +- .../directive-explorer.spec.ts | 30 +++++ .../directive-forest-utils.spec.ts | 77 ++++++++++- .../directive-forest-utils.ts | 5 + .../directive-forest.component.ts | 9 +- .../tree-node/tree-node.component.spec.ts | 15 +++ .../tree-node/tree-node.component.ts | 8 +- .../demo-app/todo/home/todos.component.html | 6 + .../app/demo-app/todo/home/todos.component.ts | 7 + 13 files changed, 306 insertions(+), 35 deletions(-) diff --git a/devtools/projects/ng-devtools-backend/src/lib/component-inspector/component-inspector.ts b/devtools/projects/ng-devtools-backend/src/lib/component-inspector/component-inspector.ts index 4412d5cc67f6..475ba624eda8 100644 --- a/devtools/projects/ng-devtools-backend/src/lib/component-inspector/component-inspector.ts +++ b/devtools/projects/ng-devtools-backend/src/lib/component-inspector/component-inspector.ts @@ -19,9 +19,6 @@ import { import {initializeOrGetDirectiveForestHooks} from '../hooks'; import {ComponentTreeNode} from '../interfaces'; -interface Type extends Function { - new (...args: any[]): T; -} export interface ComponentInspectorOptions { onComponentEnter: (id: number) => void; onComponentSelect: (id: number) => void; @@ -29,7 +26,7 @@ export interface ComponentInspectorOptions { } export class ComponentInspector { - private _selectedComponent!: {component: Type; host: HTMLElement | null}; + private _selectedComponent!: {component: unknown; host: Element | null}; private readonly _onComponentEnter; private readonly _onComponentSelect; private readonly _onComponentLeave; @@ -73,8 +70,8 @@ export class ComponentInspector { elementMouseOver(e: MouseEvent): void { this.cancelEvent(e); - const el = e.target as HTMLElement; - if (el) { + const el = e.target; + if (el instanceof Node) { this._selectedComponent = findComponentAndHost(el); } @@ -103,7 +100,7 @@ export class ComponentInspector { highlightByPosition(position: ElementPosition): void { const forest: ComponentTreeNode[] = initializeOrGetDirectiveForestHooks().getDirectiveForest(); - const elementToHighlight: HTMLElement | null = findNodeInForest(position, forest); + const elementToHighlight: Element | null = findNodeInForest(position, forest); if (elementToHighlight) { highlightSelectedElement(elementToHighlight); } diff --git a/devtools/projects/ng-devtools-backend/src/lib/component-tree/component-tree.ts b/devtools/projects/ng-devtools-backend/src/lib/component-tree/component-tree.ts index 29324ab2efbf..fdd232a468d2 100644 --- a/devtools/projects/ng-devtools-backend/src/lib/component-tree/component-tree.ts +++ b/devtools/projects/ng-devtools-backend/src/lib/component-tree/component-tree.ts @@ -723,9 +723,10 @@ export const queryDirectiveForest = ( export const findNodeInForest = ( position: ElementPosition, forest: ComponentTreeNode[], -): HTMLElement | null => { +): Element | null => { const foundComponent: ComponentTreeNode | null = queryDirectiveForest(position, forest); - return foundComponent ? (foundComponent.nativeElement as HTMLElement) : null; + const nativeElement = foundComponent?.nativeElement; + return nativeElement instanceof Element ? nativeElement : null; }; export const findNodeFromSerializedPosition = ( diff --git a/devtools/projects/ng-devtools-backend/src/lib/highlighter.spec.ts b/devtools/projects/ng-devtools-backend/src/lib/highlighter.spec.ts index 2f35d38bb02a..c6d6c56ec3ba 100644 --- a/devtools/projects/ng-devtools-backend/src/lib/highlighter.spec.ts +++ b/devtools/projects/ng-devtools-backend/src/lib/highlighter.spec.ts @@ -25,6 +25,34 @@ describe('highlighter', () => { expect(data.component).toEqual(data.host); }); + it('should return same component and host if component exists on an SVG element', () => { + (window as any).ng = { + getComponent: (el: any) => el, + }; + const element = document.createElementNS('http://www.w3.org/2000/svg', 'g'); + const data = highlighter.findComponentAndHost(element); + expect(data.component).toBeTruthy(); + expect(data.host).toBeTruthy(); + expect(data.component).toEqual(data.host); + }); + + it('should return a directive-only host before a parent component host', () => { + const parentComponent = new (class ParentComponent {})(); + const routerLink = new (class RouterLink {})(); + const parent = document.createElement('app-parent'); + const anchor = document.createElement('a'); + parent.appendChild(anchor); + + (window as any).ng = { + getComponent: (el: Element) => (el === parent ? parentComponent : undefined), + getDirectives: (el: Element) => (el === anchor ? [routerLink] : []), + }; + + const data = highlighter.findComponentAndHost(anchor); + expect(data.component).toBe(routerLink); + expect(data.host).toBe(anchor); + }); + it('should return null component and host if component do not exists', () => { (window as any).ng = { getComponent: () => undefined, @@ -184,6 +212,12 @@ describe('highlighter', () => { }); describe('highlightSelectedElement', () => { + afterEach(() => { + highlighter.unHighlight(); + document.body.innerHTML = ''; + delete (window as any).ng; + }); + function createElement(name: string) { const element = document.createElement(name); element.style.width = '25px'; @@ -222,5 +256,96 @@ describe('highlighter', () => { const overlay = document.body.querySelectorAll('.ng-devtools-overlay'); expect(overlay.length).toBe(1); }); + + it('should show overlay again when highlighting the same element after unhighlighting', () => { + const appNode = createElement('app'); + (window as any).ng = { + getComponent: (el: any) => new (class FakeComponent {})(), + }; + + highlighter.highlightSelectedElement(appNode); + highlighter.unHighlight(); + highlighter.highlightSelectedElement(appNode); + + const overlay = document.body.querySelectorAll('.ng-devtools-overlay'); + expect(overlay.length).toBe(1); + }); + + it('should show overlay again if the previous overlay was removed externally', () => { + const appNode = createElement('app'); + (window as any).ng = { + getComponent: (el: any) => new (class FakeComponent {})(), + }; + + highlighter.highlightSelectedElement(appNode); + document.body.querySelector('.ng-devtools-overlay')?.remove(); + highlighter.highlightSelectedElement(appNode); + + const overlay = document.body.querySelectorAll('.ng-devtools-overlay'); + expect(overlay.length).toBe(1); + }); + + it('should retry overlay creation for the same element if it was initially hidden', () => { + const appNode = createElement('app'); + spyOn(appNode, 'getBoundingClientRect').and.returnValues( + new DOMRect(0, 0, 0, 0), + new DOMRect(0, 0, 25, 20), + ); + (window as any).ng = { + getComponent: (el: any) => new (class FakeComponent {})(), + }; + + highlighter.highlightSelectedElement(appNode); + expect(document.body.querySelectorAll('.ng-devtools-overlay').length).toBe(0); + + highlighter.highlightSelectedElement(appNode); + + const overlay = document.body.querySelectorAll('.ng-devtools-overlay'); + expect(overlay.length).toBe(1); + }); + + it('should preserve subpixel overlay dimensions', () => { + const appNode = createElement('app'); + spyOn(appNode, 'getBoundingClientRect').and.returnValue(new DOMRect(0, 0, 0.5, 0.5)); + (window as any).ng = { + getComponent: (el: any) => new (class FakeComponent {})(), + }; + + highlighter.highlightSelectedElement(appNode); + + const overlay = document.body.querySelector('.ng-devtools-overlay'); + expect(overlay?.style.width).toBe('0.5px'); + expect(overlay?.style.height).toBe('0.5px'); + }); + + it('should show overlay for an SVG element', () => { + const svgNode = document.createElementNS('http://www.w3.org/2000/svg', 'g'); + spyOn(svgNode, 'getBoundingClientRect').and.returnValue(new DOMRect(0, 0, 25, 20)); + document.body.appendChild(svgNode); + (window as any).ng = { + getComponent: (el: any) => el, + }; + + highlighter.highlightSelectedElement(svgNode); + + const overlay = document.body.querySelectorAll('.ng-devtools-overlay'); + expect(overlay.length).toBe(1); + expect(overlay[0].innerHTML).toContain('SVGGElement'); + }); + + it('should show overlay for a directive-only element', () => { + const anchor = createElement('a'); + const routerLink = new (class RouterLink {})(); + (window as any).ng = { + getComponent: () => undefined, + getDirectives: (el: Element) => (el === anchor ? [routerLink] : []), + }; + + highlighter.highlightSelectedElement(anchor); + + const overlay = document.body.querySelectorAll('.ng-devtools-overlay'); + expect(overlay.length).toBe(1); + expect(overlay[0].innerHTML).toContain('RouterLink'); + }); }); }); diff --git a/devtools/projects/ng-devtools-backend/src/lib/highlighter.ts b/devtools/projects/ng-devtools-backend/src/lib/highlighter.ts index 3aea68e940a8..a793d03de69a 100644 --- a/devtools/projects/ng-devtools-backend/src/lib/highlighter.ts +++ b/devtools/projects/ng-devtools-backend/src/lib/highlighter.ts @@ -59,16 +59,22 @@ function createOverlay(color: RgbColor): {overlay: HTMLElement; overlayContent: export function findComponentAndHost(el: Node | undefined): { component: any; - host: HTMLElement | null; + host: Element | null; } { const ng = ngDebugClient(); if (!el) { return {component: null, host: null}; } while (el) { - const component = el instanceof HTMLElement && ng.getComponent!(el); - if (component) { - return {component, host: el as HTMLElement}; + if (el instanceof Element) { + const component = ng.getComponent?.(el); + if (component) { + return {component, host: el}; + } + const directive = ng.getDirectives?.(el)?.[0]; + if (directive) { + return {component: directive, host: el}; + } } if (!el.parentElement) { break; @@ -84,12 +90,12 @@ export function getDirectiveName(dir: Type | undefined | null): string } export function highlightSelectedElement(el: Node): void { - if (el === selectedElement) { + if (el === selectedElement && selectedElementOverlay?.isConnected) { return; } unHighlight(); selectedElementOverlay = addHighlightForElement(el); - selectedElement = el; + selectedElement = selectedElementOverlay ? el : null; } export function highlightHydrationElement(el: Node, status: HydrationStatus) { @@ -109,23 +115,18 @@ export function highlightHydrationElement(el: Node, status: HydrationStatus) { export function unHighlight(): void { if (!selectedElementOverlay) { + selectedElement = null; return; } - for (const node of document.body.childNodes) { - if (node === selectedElementOverlay) { - document.body.removeChild(selectedElementOverlay); - - break; - } - } - + selectedElementOverlay.remove(); selectedElementOverlay = null; + selectedElement = null; } export function removeHydrationHighlights(): void { hydrationOverlayItems.forEach((overlay) => { - document.body.removeChild(overlay); + overlay.remove(); }); hydrationOverlayItems = []; } @@ -182,7 +183,7 @@ function addHighlightForElement( } function getComponentRect(el: Node): DOMRect | undefined { - if (!(el instanceof HTMLElement)) { + if (!(el instanceof Element)) { return; } if (!inDoc(el)) { @@ -199,10 +200,10 @@ function showOverlay( labelPosition: 'inside' | 'outside', ): void { const {width, height, top, left} = dimensions; - overlay.style.width = ~~width + 'px'; - overlay.style.height = ~~height + 'px'; - overlay.style.top = ~~top + window.scrollY + 'px'; - overlay.style.left = ~~left + window.scrollX + 'px'; + overlay.style.width = `${width}px`; + overlay.style.height = `${height}px`; + overlay.style.top = `${top + window.scrollY}px`; + overlay.style.left = `${left + window.scrollX}px`; positionOverlayContent(overlayContent, dimensions, labelPosition); overlayContent.replaceChildren(); diff --git a/devtools/projects/ng-devtools/src/lib/devtools-tabs/directive-explorer/directive-explorer.component.ts b/devtools/projects/ng-devtools/src/lib/devtools-tabs/directive-explorer/directive-explorer.component.ts index 9500d86bf68a..72780b940669 100644 --- a/devtools/projects/ng-devtools/src/lib/devtools-tabs/directive-explorer/directive-explorer.component.ts +++ b/devtools/projects/ng-devtools/src/lib/devtools-tabs/directive-explorer/directive-explorer.component.ts @@ -282,7 +282,7 @@ export class DirectiveExplorerComponent { } highlight(node: FlatNode): void { - if (!node.original.component) { + if (!node.hasNativeElement) { return; } this._messageBus.emit('createHighlightOverlay', [node.position]); diff --git a/devtools/projects/ng-devtools/src/lib/devtools-tabs/directive-explorer/directive-explorer.spec.ts b/devtools/projects/ng-devtools/src/lib/devtools-tabs/directive-explorer/directive-explorer.spec.ts index bdce522fdc68..ca424b2c58dd 100644 --- a/devtools/projects/ng-devtools/src/lib/devtools-tabs/directive-explorer/directive-explorer.spec.ts +++ b/devtools/projects/ng-devtools/src/lib/devtools-tabs/directive-explorer/directive-explorer.spec.ts @@ -262,6 +262,36 @@ describe('DirectiveExplorerComponent', () => { }); }); + describe('highlight', () => { + it('should create a highlight overlay for directive-only nodes', () => { + comp.highlight({ + original: { + component: null, + directives: [{id: 9, name: 'RouterLink'}], + hasNativeElement: true, + }, + position: [0], + hasNativeElement: true, + } as any); + + expect(messageBusMock.emit).toHaveBeenCalledWith('createHighlightOverlay', [[0]]); + }); + + it('should not create a highlight overlay for nodes without native elements', () => { + comp.highlight({ + original: { + component: null, + directives: [{id: 9, name: 'RouterLink'}], + hasNativeElement: false, + }, + position: [0], + hasNativeElement: false, + } as any); + + expect(messageBusMock.emit).not.toHaveBeenCalledWith('createHighlightOverlay', [[0]]); + }); + }); + describe('applicaton operations', () => { describe('view source', () => { it('should not call application operations view source if no frames are detected', () => { diff --git a/devtools/projects/ng-devtools/src/lib/devtools-tabs/directive-explorer/directive-forest/directive-forest-utils.spec.ts b/devtools/projects/ng-devtools/src/lib/devtools-tabs/directive-explorer/directive-forest/directive-forest-utils.spec.ts index 8cdc17195647..3ffcfbd7c9fe 100644 --- a/devtools/projects/ng-devtools/src/lib/devtools-tabs/directive-explorer/directive-forest/directive-forest-utils.spec.ts +++ b/devtools/projects/ng-devtools/src/lib/devtools-tabs/directive-explorer/directive-forest/directive-forest-utils.spec.ts @@ -7,9 +7,40 @@ */ import {FlatNode} from './component-data-source'; -import {getDirectivesArrayString, getFullNodeNameString} from './directive-forest-utils'; +import { + getDirectivesArrayString, + getFullNodeNameString, + matchesDirectiveOrComponentId, +} from './directive-forest-utils'; describe('directive-forest-utils', () => { + function createFlatNode({ + component, + directives = [], + }: { + component: FlatNode['original']['component']; + directives?: NonNullable; + }): FlatNode { + return { + id: 'node', + expandable: false, + name: 'node', + directives: directives.map((directive) => directive.name), + position: [], + level: 0, + original: { + position: [], + children: [], + component, + directives, + controlFlowBlock: null, + hasNativeElement: true, + }, + controlFlowBlock: null, + hasNativeElement: true, + }; + } + describe('getDirectivesArrayString', () => { it('should return an empty string, if no directives', () => { const output = getDirectivesArrayString({} as FlatNode); @@ -67,4 +98,48 @@ describe('directive-forest-utils', () => { expect(output).toEqual('app-test[Foo][Bar]'); }); }); + + describe('matchesDirectiveOrComponentId', () => { + it('should match component id zero', () => { + const matches = matchesDirectiveOrComponentId( + createFlatNode({ + component: { + id: 0, + name: 'AppComponent', + isElement: false, + }, + }), + 0, + ); + + expect(matches).toBeTrue(); + }); + + it('should match a directive id on a directive-only node', () => { + const matches = matchesDirectiveOrComponentId( + createFlatNode({ + component: null, + directives: [{id: 9, name: 'RouterLink'}], + }), + 9, + ); + + expect(matches).toBeTrue(); + }); + + it('should not match null highlighted ids', () => { + const matches = matchesDirectiveOrComponentId( + createFlatNode({ + component: { + id: 0, + name: 'AppComponent', + isElement: false, + }, + }), + null, + ); + + expect(matches).toBeFalse(); + }); + }); }); diff --git a/devtools/projects/ng-devtools/src/lib/devtools-tabs/directive-explorer/directive-forest/directive-forest-utils.ts b/devtools/projects/ng-devtools/src/lib/devtools-tabs/directive-explorer/directive-forest/directive-forest-utils.ts index 567433921167..fa19cc098aec 100644 --- a/devtools/projects/ng-devtools/src/lib/devtools-tabs/directive-explorer/directive-forest/directive-forest-utils.ts +++ b/devtools/projects/ng-devtools/src/lib/devtools-tabs/directive-explorer/directive-forest/directive-forest-utils.ts @@ -40,6 +40,11 @@ export const parentCollapsed = ( export const getDirectivesArrayString = (node: FlatNode): string => node.directives ? node.directives.map((dir) => `[${dir}]`).join('') : ''; +export const matchesDirectiveOrComponentId = (node: FlatNode, id: number | null): boolean => + id !== null && + (node.original.component?.id === id || + !!node.original.directives?.some((directive) => directive.id === id)); + /** Returns the full node name string as rendered by the tree-node component. */ export const getFullNodeNameString = (node: FlatNode): string => { const cmp = node.original.component; diff --git a/devtools/projects/ng-devtools/src/lib/devtools-tabs/directive-explorer/directive-forest/directive-forest.component.ts b/devtools/projects/ng-devtools/src/lib/devtools-tabs/directive-explorer/directive-forest/directive-forest.component.ts index 7be000c61384..48f1e40a4d5b 100644 --- a/devtools/projects/ng-devtools/src/lib/devtools-tabs/directive-explorer/directive-forest/directive-forest.component.ts +++ b/devtools/projects/ng-devtools/src/lib/devtools-tabs/directive-explorer/directive-forest/directive-forest.component.ts @@ -30,7 +30,12 @@ import {DevToolsNode, ElementPosition, Events, MessageBus} from '../../../../../ import {TabUpdate} from '../../tab-update/index'; import {ComponentDataSource, FlatNode} from './component-data-source'; -import {getFullNodeNameString, isChildOf, parentCollapsed} from './directive-forest-utils'; +import { + getFullNodeNameString, + isChildOf, + matchesDirectiveOrComponentId, + parentCollapsed, +} from './directive-forest-utils'; import {IndexedNode} from './index-forest'; import {FilterComponent, FilterFn} from '../../../shared/filter/filter.component'; import {TreeNodeComponent, NodeTextMatch} from './tree-node/tree-node.component'; @@ -368,7 +373,7 @@ export class DirectiveForestComponent { } private selectNodeByComponentId(id: number): void { - const foundNode = this.dataSource.data.find((node) => node.original.component?.id === id); + const foundNode = this.dataSource.data.find((node) => matchesDirectiveOrComponentId(node, id)); if (foundNode) { this.selectAndEnsureVisible(foundNode); } diff --git a/devtools/projects/ng-devtools/src/lib/devtools-tabs/directive-explorer/directive-forest/tree-node/tree-node.component.spec.ts b/devtools/projects/ng-devtools/src/lib/devtools-tabs/directive-explorer/directive-forest/tree-node/tree-node.component.spec.ts index 348809822f59..aab87bff81a1 100644 --- a/devtools/projects/ng-devtools/src/lib/devtools-tabs/directive-explorer/directive-forest/tree-node/tree-node.component.spec.ts +++ b/devtools/projects/ng-devtools/src/lib/devtools-tabs/directive-explorer/directive-forest/tree-node/tree-node.component.spec.ts @@ -136,6 +136,21 @@ describe('TreeNodeComponent', () => { expect(classList.contains('highlighted')).toBeTrue(); }); + it('should handle highlighting a directive-only node', async () => { + fixture.componentRef.setInput('node', { + ...srcNode, + original: { + component: null, + directives: [{id: 9, name: 'RouterLink'}], + }, + }); + fixture.componentRef.setInput('highlightedId', 9); + await fixture.whenStable(); + + const classList = fixture.debugElement.nativeElement.classList; + expect(classList.contains('highlighted')).toBeTrue(); + }); + it('should add respective class, if a new node', async () => { fixture.componentRef.setInput('node', { ...srcNode, diff --git a/devtools/projects/ng-devtools/src/lib/devtools-tabs/directive-explorer/directive-forest/tree-node/tree-node.component.ts b/devtools/projects/ng-devtools/src/lib/devtools-tabs/directive-explorer/directive-forest/tree-node/tree-node.component.ts index 0dc51957f401..4fd3b761d24b 100644 --- a/devtools/projects/ng-devtools/src/lib/devtools-tabs/directive-explorer/directive-forest/tree-node/tree-node.component.ts +++ b/devtools/projects/ng-devtools/src/lib/devtools-tabs/directive-explorer/directive-forest/tree-node/tree-node.component.ts @@ -24,7 +24,11 @@ import {MatTooltip} from '@angular/material/tooltip'; import {FlatTreeControl} from '@angular/cdk/tree'; import {FlatNode} from '../component-data-source'; -import {getDirectivesArrayString, getFullNodeNameString} from '../directive-forest-utils'; +import { + getDirectivesArrayString, + getFullNodeNameString, + matchesDirectiveOrComponentId, +} from '../directive-forest-utils'; import {BlockType} from '../../../../shared/utils/control-flow'; import {APP_DATA} from '../../../../application-providers/app_data'; @@ -113,7 +117,7 @@ export class TreeNodeComponent { } protected get isHighlighted(): boolean { - return !!this.highlightedId() && this.highlightedId() === this.node().original.component?.id; + return matchesDirectiveOrComponentId(this.node(), this.highlightedId()); } private handleMatchedText() { diff --git a/devtools/src/app/demo-app/todo/home/todos.component.html b/devtools/src/app/demo-app/todo/home/todos.component.html index 6efb35d588a5..f889b5103266 100644 --- a/devtools/src/app/demo-app/todo/home/todos.component.html +++ b/devtools/src/app/demo-app/todo/home/todos.component.html @@ -40,4 +40,10 @@

{{ title }}

+ + +
SVG component highlighter demo
+ + + diff --git a/devtools/src/app/demo-app/todo/home/todos.component.ts b/devtools/src/app/demo-app/todo/home/todos.component.ts index 99b40f01c99f..7f3728da0a6f 100644 --- a/devtools/src/app/demo-app/todo/home/todos.component.ts +++ b/devtools/src/app/demo-app/todo/home/todos.component.ts @@ -58,6 +58,12 @@ export class RecursiveComponent { level = input(5); } +@Component({ + selector: '[appSvgDemo]', + template: '', +}) +export class SvgDemoComponent {} + @Component({ templateUrl: 'todos.component.html', selector: 'app-todos', @@ -68,6 +74,7 @@ export class RecursiveComponent { SamplePipe, TodosFilter, RecursiveComponent, + SvgDemoComponent, ], }) export class TodosComponent implements OnInit, OnDestroy {