fix: occasional highlighter disappearance #805
Conversation
WalkthroughAdds dialog-aware selection to clientSelectorGenerator: dialog elements are detected, collected, and prioritized. Candidate elements now prepend dialog content; sorting favors dialog elements at equal depth. getDeepestElementFromPoint returns the deepest element within a dialog when applicable. New private helpers centralize dialog detection and traversal. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant U as User
participant C as ClientSelectorGenerator
participant D as Dialog Helpers
participant S as Selector Logic
U->>C: click at (x,y)
C->>D: findAllDialogElements(doc)
D-->>C: dialogElements
alt click inside a dialog
C->>D: findDeepestInDialog(dialogElements, dialogAtPoint)
D-->>C: deepestInDialog
C-->>U: return deepestInDialog
else not in dialog
C->>S: collect candidates (prepend getElementsFromDialogs)
S-->>C: candidates
C->>S: sort (dialog-priority, then depth)
S-->>C: bestElement
C-->>U: return bestElement
end
note over C,S: Dialog elements are merged at the front of candidates
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/helpers/clientSelectorGenerator.ts (1)
4149-4161: findAllDialogElements returns descendants, not just dialog containers (perf + correctness)Using isDialogElement with closest(...) on all nodes yields every element inside dialogs, not the dialog containers. This massively inflates the list and causes redundant traversal. Return only visible dialog containers and de-nest nested ones.
Apply this diff:
private findAllDialogElements(doc: Document): HTMLElement[] { - const dialogElements: HTMLElement[] = []; - const allElements = Array.from(doc.querySelectorAll("*")) as HTMLElement[]; - - for (const element of allElements) { - if (this.isDialogElement(element)) { - dialogElements.push(element); - } - } - - return dialogElements; + // Collect likely dialog containers (not their descendants) + const candidates = Array.from( + doc.querySelectorAll( + 'dialog, [role="dialog"], [aria-modal="true"], [id*="modal" i], [class*="modal" i], [id*="dialog" i], [class*="dialog" i], [id*="popup" i], [class*="popup" i]' + ) + ) as HTMLElement[]; + + // Keep only visible, top-most containers (exclude those nested within another candidate) + const unique: HTMLElement[] = []; + for (const el of candidates) { + const rect = el.getBoundingClientRect(); + if (rect.width <= 0 || rect.height <= 0) continue; + if (candidates.some((other) => other !== el && other.contains(el))) continue; + unique.push(el); + } + return unique; }
🧹 Nitpick comments (2)
src/helpers/clientSelectorGenerator.ts (2)
415-418: Prepending dialog elements without de-dup can bloat allElements and slow groupingMerging dialogContentElements in front of allElements introduces duplicates and unnecessary work. Deduplicate while preserving order.
Apply this diff:
- allElements = [...dialogContentElements, ...allElements]; + // Prepend dialog content but avoid duplicates + const combined = [...dialogContentElements, ...allElements]; + const seen = new Set<HTMLElement>(); + allElements = combined.filter((el) => { + if (seen.has(el)) return false; + seen.add(el); + return true; + });
4142-4144: Widen dialog detection to include aria-modalSome libraries set aria-modal="true" without role="dialog". Including it improves coverage.
Apply this diff:
- private isDialogElement(el: HTMLElement): boolean { - return !!el.closest('dialog, [role="dialog"]'); - } + private isDialogElement(el: HTMLElement): boolean { + return !!el.closest('dialog, [role="dialog"], [aria-modal="true"]'); + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/helpers/clientSelectorGenerator.ts(5 hunks)
🔇 Additional comments (1)
src/helpers/clientSelectorGenerator.ts (1)
975-985: LGTM: Dialog-priority tie-breaker in grouped element sortingPrioritizing elements within dialogs before depth is a sensible UX improvement.
| const dialogElement = this.findDialogElement(elements); | ||
| if (dialogElement) { | ||
| const dialogRect = dialogElement.getBoundingClientRect(); | ||
| const isClickInsideDialog = x >= dialogRect.left && x <= dialogRect.right && | ||
| y >= dialogRect.top && y <= dialogRect.bottom; | ||
|
|
||
| if (isClickInsideDialog) { | ||
| const dialogElements = elements.filter( | ||
| (el) => el === dialogElement || dialogElement.contains(el) | ||
| ); | ||
|
|
||
| const deepestInDialog = this.findDeepestInDialog(dialogElements, dialogElement); | ||
| if (deepestInDialog) { | ||
| return deepestInDialog; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Dialog container vs. overlay: ensure we scope to the actual dialog root
findDialogElement can return an overlay/backdrop that doesn’t contain the dialog’s content. Scoping by that element may discard actual dialog children and select the backdrop. Prefer the nearest dialog container and filter within it.
Apply this diff:
- const dialogElement = this.findDialogElement(elements);
- if (dialogElement) {
- const dialogRect = dialogElement.getBoundingClientRect();
+ const dialogElement = this.findDialogElement(elements);
+ if (dialogElement) {
+ const dialogContainer =
+ (dialogElement.closest('dialog, [role="dialog"], [aria-modal="true"]') as HTMLElement) || dialogElement;
+ const dialogRect = dialogContainer.getBoundingClientRect();
const isClickInsideDialog = x >= dialogRect.left && x <= dialogRect.right &&
y >= dialogRect.top && y <= dialogRect.bottom;
if (isClickInsideDialog) {
- const dialogElements = elements.filter(
- (el) => el === dialogElement || dialogElement.contains(el)
- );
-
- const deepestInDialog = this.findDeepestInDialog(dialogElements, dialogElement);
+ const dialogElements = elements.filter(
+ (el) => el === dialogContainer || dialogContainer.contains(el)
+ );
+
+ const deepestInDialog = this.findDeepestInDialog(dialogElements, dialogContainer);
if (deepestInDialog) {
return deepestInDialog;
}
}
}Additionally, there are two different getDeepestElementFromPoint implementations (this one and the local one inside getSelectors). Consider unifying to a single, dialog-aware implementation to avoid divergent behavior.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const dialogElement = this.findDialogElement(elements); | |
| if (dialogElement) { | |
| const dialogRect = dialogElement.getBoundingClientRect(); | |
| const isClickInsideDialog = x >= dialogRect.left && x <= dialogRect.right && | |
| y >= dialogRect.top && y <= dialogRect.bottom; | |
| if (isClickInsideDialog) { | |
| const dialogElements = elements.filter( | |
| (el) => el === dialogElement || dialogElement.contains(el) | |
| ); | |
| const deepestInDialog = this.findDeepestInDialog(dialogElements, dialogElement); | |
| if (deepestInDialog) { | |
| return deepestInDialog; | |
| } | |
| } | |
| } | |
| const dialogElement = this.findDialogElement(elements); | |
| if (dialogElement) { | |
| const dialogContainer = | |
| (dialogElement.closest('dialog, [role="dialog"], [aria-modal="true"]') as HTMLElement) || dialogElement; | |
| const dialogRect = dialogContainer.getBoundingClientRect(); | |
| const isClickInsideDialog = x >= dialogRect.left && x <= dialogRect.right && | |
| y >= dialogRect.top && y <= dialogRect.bottom; | |
| if (isClickInsideDialog) { | |
| const dialogElements = elements.filter( | |
| (el) => el === dialogContainer || dialogContainer.contains(el) | |
| ); | |
| const deepestInDialog = this.findDeepestInDialog(dialogElements, dialogContainer); | |
| if (deepestInDialog) { | |
| return deepestInDialog; | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
In src/helpers/clientSelectorGenerator.ts around lines 3918 to 3934,
findDialogElement may return an overlay/backdrop rather than the dialog root,
causing dialog children to be missed; update the logic to identify the actual
dialog container (the closest ancestor that represents the dialog root, e.g., by
checking role, attributes, or by traversing down from the overlay to the child
dialog node) and then filter elements within that dialog container instead of
the overlay, then call the dialog-aware getDeepestElementFromPoint
implementation on that scoped list; also consolidate the two
getDeepestElementFromPoint implementations (the class-level one and the local
one inside getSelectors) into a single shared, dialog-aware function and replace
both usages so behavior is consistent.
amhsirak
changed the title
2 participants
What this PR does?
Fixes the issue wherein the highlighter was malfunctioning for certain sites.
Summary by CodeRabbit
New Features
Bug Fixes