Skip to content

fix(components/docs-tools): links in if statements are correctly registered in table of contents #4339

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
Blackbaud-ErikaMcVey wants to merge 1 commit into
base: main
Choose a base branch
from
+30 −13
Draft

fix(components/docs-tools): links in if statements are correctly registered in table of contents #4339

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion libs/components/docs-tools/src/lib/modules/heading-anchor/heading-anchor.component.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,7 @@ export class SkyDocsHeadingAnchorComponent implements AfterViewInit, OnDestroy {
public readonly strikethrough = input(false, { transform: booleanAttribute });

public ngAfterViewInit(): void {
this.#anchorSvc?.register(this);
this.#anchorSvc?.register(this, this.#elementRef.nativeElement);
}

public ngOnDestroy(): void {
Expand Down
41 changes: 29 additions & 12 deletions libs/components/docs-tools/src/lib/modules/heading-anchor/heading-anchor.service.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,43 +11,60 @@ import { SkyDocsHeadingAnchorComponent } from './heading-anchor.component';
@Injectable()
export class SkyDocsHeadingAnchorService implements OnDestroy {
#anchors: SkyDocsHeadingAnchorComponent[] = [];
#anchorElements = new Map<SkyDocsHeadingAnchorComponent, Element>();
readonly #anchorsChange = new BehaviorSubject<SkyDocsHeadingAnchorLink[]>([]);

public readonly anchorsChange = this.#anchorsChange.asObservable();

public ngOnDestroy(): void {
this.#anchors = [];
this.#anchorElements.clear();
this.#anchorsChange.complete();
}

public register(anchor: SkyDocsHeadingAnchorComponent): void {
public register(
anchor: SkyDocsHeadingAnchorComponent,
element: Element,
): void {
if (!this.#anchors.includes(anchor)) {
this.#anchors.push(anchor);
this.#anchorElements.set(anchor, element);
this.#anchorsChange.next(this.#getLinks());
}
Comment on lines 29 to 33
Copy link

Copilot AI Mar 26, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

register() emits anchorsChange synchronously. In the reported @if timing scenario the host element may still be disconnected from document, so #getLinks() may compute an implementation-specific order (and never re-emit once the element is attached). Consider deferring the emission (e.g., queueing a microtask / next tick) or otherwise scheduling a second recomputation after the element is connected so the TOC order reliably reflects actual DOM order.

Copilot uses AI. Check for mistakes.
}

public unregister(anchor: SkyDocsHeadingAnchorComponent): void {
if (this.#anchors.includes(anchor)) {
this.#anchors.splice(this.#anchors.indexOf(anchor));
this.#anchors.splice(this.#anchors.indexOf(anchor), 1);
this.#anchorElements.delete(anchor);
this.#anchorsChange.next(this.#getLinks());
}
}

#getLinks(): SkyDocsHeadingAnchorLink[] {
const els = document.querySelectorAll('sky-docs-heading-anchor');
const anchorsSorted: SkyDocsHeadingAnchorComponent[] = [];

// Since heading anchors can be registered at any point in the lifecycle
// of the app, we need to sort the links by their location in the DOM,
// rather than the order at which they were registered.
for (const el of els) {
this.#anchors.forEach((anchor) => {
if (anchor.equals(el)) {
Copy link
Copy Markdown
Member

@Blackbaud-SteveBrush Blackbaud-SteveBrush Mar 26, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This public method is no longer used on the anchor component; can you remove it?

anchorsSorted.push(anchor);
}
});
}
const anchorsSorted = [...this.#anchors].sort((a, b) => {
const elA = this.#anchorElements.get(a);
const elB = this.#anchorElements.get(b);

if (!elA || !elB) {
return 0;
}

const position = elA.compareDocumentPosition(elB);

if (position & Node.DOCUMENT_POSITION_FOLLOWING) {
return -1;
}

if (position & Node.DOCUMENT_POSITION_PRECEDING) {
return 1;
}

return 0;
Comment on lines +56 to +66
Copy link

Copilot AI Mar 26, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The sort comparator uses compareDocumentPosition(), but it doesn't account for Node.DOCUMENT_POSITION_DISCONNECTED. For disconnected nodes browsers may also set PRECEDING/FOLLOWING in an implementation-specific way, which can produce a stable but incorrect DOM order. Consider explicitly checking for DISCONNECTED and returning 0 (or another deterministic fallback) until both nodes share a document/root.

Copilot uses AI. Check for mistakes.
});

return anchorsSorted.map((a) => {
return {
Expand Down
Loading