fix: resolve element selection getting stuck on busy pages#287
Open
fix: resolve element selection getting stuck on busy pages#287
Conversation
Two root causes identified via runtime instrumentation on nisarg.io:
1. Detection scheduling used scheduler.postTask({priority:"background"})
which was starved for 350ms–5000ms on pages with continuous React
renders or CSS animations, making hover detection feel frozen.
Replaced with requestAnimationFrame (~16ms per frame).
2. Decorative hover overlay divs (absolute-positioned, transparent,
empty elements used for hover border/glow effects) were returned by
elementFromPoint, blocking detection of actual content underneath.
Added isDecorativeOverlay filter to skip these elements.
- Fix race in rightClickElement by checking overlay state before click - Increase waitForContextMenu/waitForPromptMode timeouts to 5s - Replace fixed post-copy waits with polling for comments button - Convert dismiss assertions from waitForTimeout to expect.poll - Increase freeze cleanup propagation wait in multi-cycle test
Contributor
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
commit: |
…hasTransparentBackground - Delete on-idle.ts; it was a trivial wrapper after the scheduler change. Use nativeRequestAnimationFrame directly in index.tsx (already imported). - Extract hasTransparentBackground helper shared between isDecorativeOverlay and isFullViewportOverlay.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
2 issues found across 6 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/react-grab/src/utils/is-valid-grabbable-element.ts">
<violation number="1" location="packages/react-grab/src/utils/is-valid-grabbable-element.ts:77">
P1: This overlay filter also rejects real absolutely/fixed-positioned elements like `<img>` because they have no children/text and usually compute to a transparent background.</violation>
</file>
<file name="packages/react-grab/e2e/clear-history-prompt.spec.ts">
<violation number="1" location="packages/react-grab/e2e/clear-history-prompt.spec.ts:10">
P1: The updated polling condition is insufficient for subsequent calls to `copyElement`. Both `getClipboardContent()` and `isCommentsButtonVisible()` will return truthy/true immediately if a previous copy has already occurred, causing a race condition where the function returns before the current element has been processed or added to the history.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| await expect.poll(() => reactGrab.getClipboardContent(), { timeout: 5000 }).toBeTruthy(); | ||
| // HACK: Wait for copy feedback transition and comments item addition | ||
| await reactGrab.page.waitForTimeout(300); | ||
| await expect.poll(() => reactGrab.isCommentsButtonVisible(), { timeout: 5000 }).toBe(true); |
Contributor
There was a problem hiding this comment.
P1: The updated polling condition is insufficient for subsequent calls to copyElement. Both getClipboardContent() and isCommentsButtonVisible() will return truthy/true immediately if a previous copy has already occurred, causing a race condition where the function returns before the current element has been processed or added to the history.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/react-grab/e2e/clear-history-prompt.spec.ts, line 10:
<comment>The updated polling condition is insufficient for subsequent calls to `copyElement`. Both `getClipboardContent()` and `isCommentsButtonVisible()` will return truthy/true immediately if a previous copy has already occurred, causing a race condition where the function returns before the current element has been processed or added to the history.</comment>
<file context>
@@ -7,8 +7,7 @@ const copyElement = async (reactGrab: ReactGrabPageObject, selector: string) =>
await expect.poll(() => reactGrab.getClipboardContent(), { timeout: 5000 }).toBeTruthy();
- // HACK: Wait for copy feedback transition and comments item addition
- await reactGrab.page.waitForTimeout(300);
+ await expect.poll(() => reactGrab.isCommentsButtonVisible(), { timeout: 5000 }).toBe(true);
};
</file context>
rAF callbacks run inside the rendering pipeline (before paint), so the elementFromPoint style recalc was eating into the frame budget. setTimeout(0) runs as a separate macrotask outside the render pipeline, fires within ~1-4ms (no starvation), and doesn't impact frame budget. The 32ms detection throttle already limits call frequency.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
Replaced elements (img, video, canvas, svg, iframe, etc.) have no children, no text, and a transparent background-color — matching all isDecorativeOverlay criteria despite being real content. Skip them with a tag name check before the emptiness/transparency heuristics.
Decorative hover overlays are always <div> or <span>. An allowlist of 2 tags is simpler, safer against future HTML elements, and more precise than a 10-item blocklist of replaced elements.
Remove tag-name allowlists and site-specific heuristics. The principle is general: a positioned element with no visual content (no children, no text, transparent background) exists only in the stacking order and shouldn't be selectable. elementFromPoint falls through to the meaningful element underneath via the elementsFromPoint fallback.
A positioned element with transparent background-color can still render visible content via background-image (hero sections, card images) or box-shadow (card panels). Without these checks, hasVisualContent was too aggressive and would incorrectly skip meaningful elements.
Replace the hasVisualContent heuristic with a simpler, more principled approach: among all valid elements at a point, select the one with the smallest bounding area. This naturally picks the most specific content element and skips decorative overlays, container divs, and other large elements without needing tag-name checks or CSS property heuristics.
spatial-index.ts -> element-at-point-index.ts buildSpatialIndex -> buildElementAtPointIndex querySpatialIndex -> queryElementAtPointIndex isSpatialIndexReady -> isElementAtPointIndexReady destroySpatialIndex -> destroyElementAtPointIndex SPATIAL_INDEX_ROOT_MARGIN_PX -> ELEMENT_AT_POINT_INDEX_ROOT_MARGIN_PX
Replace the smallest-area heuristic with stacking order comparison for prehit queries. A modal button (z-index: 9999) now correctly wins over a paragraph behind it, even if the paragraph is smaller. Adds compare-stacking-order.ts which determines visual ordering by walking ancestor chains, comparing z-index within stacking contexts, and falling back to DOM sibling order. The IndexedElement no longer stores area or treeOrder since selection is now based on visual stacking, not geometric size.
- freezePseudoStates: check cursor is within viewport bounds before
using elementFromPoint. store.pointer starts at -1000 (offscreen);
without the bounds check, elementFromPoint returns null and the
querySelectorAll(":hover") fallback never runs.
- compareStackingOrder: fix operator grouping for flex items. Per CSS
spec, flex items only create a stacking context when z-index is not
auto. Was: (zIndex !== auto && position !== static) || isFlexItem.
Now: zIndex !== auto && (position !== static || isFlexItem).
- getElementAtPosition fallback: take the first valid element from
elementsFromPoint (already in paint/z-order) instead of the smallest
by area. Consistent with prehit's stacking-order-based selection.
- freezePseudoStates: use < instead of <= for viewport bounds check. elementFromPoint returns null at exactly innerWidth/innerHeight since viewport coordinates are 0-indexed. - Revert animation caching optimization in freezeGlobalAnimations. Capturing animations at freeze time missed animations created during the freeze (e.g., React adding DOM with CSS animations). The CSS rule pauses them, but finish() on unfreeze only ran on the cached list, causing visual jumps for missed animations. Restoring document.getAnimations() on unfreeze catches all animations including those created mid-session. Correctness over micro-opt.
The >> 8 iteration read levelC/levelD (post-modification) instead of invertedOr/maskedAnd (saved pre-modification values from the >> 4 iteration). This matches the original flatbush which uses lowercase c/d (saved copies) in the >> 8 step, not uppercase C/D (current). Incorrect Hilbert values degraded R-tree spatial locality but didn't break query correctness since the tree structure is still valid.
- Add position:sticky to unconditional stacking context check (per CSS spec, sticky creates a stacking context like fixed does). - Rename isFlexItem to isFlexOrGridItem and add grid/inline-grid parent display checks. Grid items with non-auto z-index create stacking contexts just like flex items do.
- Re-validate prehit result with isValidGrabbableElement and getElementArea before returning. Elements indexed at activation could theoretically change state during the session. - Restore [...accumulatedElements] snapshot when assigning to currentIndex. Without this, the elements array is shared between the IO accumulation buffer and the current index. Subsequent IO batches would mutate the live index's elements array, causing index/element count mismatches during concurrent queries.
|
|
||
| currentIndex = { tree, elements: [...accumulatedElements] }; | ||
| }, | ||
| { rootMargin: `${ELEMENT_AT_POINT_INDEX_ROOT_MARGIN_PX}px` }, |
There was a problem hiding this comment.
IntersectionObserver rootMargin has no filtering effect
Low Severity
The ELEMENT_AT_POINT_INDEX_ROOT_MARGIN_PX constant and rootMargin option are effectively dead configuration. IntersectionObserver always fires initial entries for all observed elements regardless of whether they intersect, and the callback never checks entry.isIntersecting. Every element observed via the TreeWalker is indexed unconditionally, making the rootMargin parameter meaningless. The constant in constants.ts suggests spatial filtering was intended but isn't implemented.
Reviewed by Cursor Bugbot for commit a34a57a. Configure here.
…rom index The bugbot-driven change from smallest-area to first-valid (topmost) in the fallback path reintroduced the original "stuck on decorative overlay" bug. Decorative overlay divs (positioned, empty, transparent) are topmost in elementsFromPoint results, so taking the first valid element returns the overlay instead of the content underneath. Fallback path: restored smallest-area selection. elementsFromPoint returns in z-order; smallest-area naturally skips large overlays in favor of specific content elements. Handles modals correctly because modal content elements are smaller than background content at the same point. Prehit path: exclude positioned elements with no children and no text from the R-tree. These are decorative hover-effect divs that should never be selection targets. The stacking-order sort then operates only on meaningful content elements.
…coverage - Cache fixed element viewport rects and z-indexes at build time to eliminate getBoundingClientRect and compareStackingOrder calls from the hot query path - Add WeakMap clip state cache for isVisibleAtPoint to avoid repeated getComputedStyle calls on shared ancestors - Handle nullable R-tree for fixed-only pages (no regular elements) - Recognize contain:paint/strict/content, backdrop-filter, perspective, and clip-path as stacking context creators in compareStackingOrder - Invalidate position cache when cached element is disconnected - Extract isDecorativeOverlay to shared utility, exclude replaced elements (img, video, canvas, svg, etc.) - Remove duplicated viewport coverage check in isFullViewportOverlay - Combine duplicate freeze-pseudo-states imports in primitives.ts - Add 57 element detection e2e tests covering overflow clipping, CSS containment, stacking order, inline elements, decorative overlays, fixed position, transforms, and dynamic DOM changes
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
There are 2 total unresolved issues (including 1 from previous review).
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit eb5d3ce. Configure here.
| if (PAINT_CONTAIN_VALUES.has(keyword)) return true; | ||
| } | ||
| return false; | ||
| }; |
There was a problem hiding this comment.
Duplicate hasPaintContainment with divergent implementations
Low Severity
Two separate hasPaintContainment functions exist in element-at-point-index.ts and compare-stacking-order.ts with different implementations. One uses a Set lookup after splitting by space, while the other uses exact string matches plus String.includes. They happen to be functionally equivalent today, but divergent implementations risk inconsistent bug fixes in the future.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit eb5d3ce. Configure here.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
scheduler.postTask({priority:"background"})delayed hit-testing by 350ms–5000ms on sites like nisarg.io with continuous React renders. Replaced withrequestAnimationFramefor consistent ~16ms detection.<div>elements (used for card hover effects) were returned byelementFromPoint, preventing selection of actual content underneath. AddedisDecorativeOverlayfilter to skip these.Test plan
pnpm buildpassespnpm test— 565 passed, 0 failedpnpm lint— 0 errorspnpm typecheck— passesNote
Medium Risk
Touches core hit-testing and stacking/visibility heuristics used on every pointer move, so regressions could affect what elements are selectable/copyable across sites. Changes are mitigated by a large new E2E suite covering overflow, containment, stacking contexts, fixed elements, and dynamic DOM updates.
Overview
Improves element selection responsiveness on busy pages by changing the hit-test scheduling in
core/index.tsxfrom idle/background scheduling to asetTimeoutmacrotask, reducing pointer-move detection starvation.Adds a new spatial element-at-point index (IntersectionObserver + Hilbert R-tree) built on activation and queried from
getElementAtPosition, with additional filtering for overflow/paint containment, fixed elements, and visual stacking order (compareStackingOrder) to pick the topmost candidate while skipping empty decorative overlays.Expands the E2E app with new UI fixtures for overflow clipping, CSS containment, stacking/transform contexts, inline elements, and overlays, and adds a comprehensive
element-detection.spec.ts; existing E2E tests were stabilized by replacing fixed sleeps with polling and increasing timeouts.Reviewed by Cursor Bugbot for commit eb5d3ce. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by cubic
Fixes element selection getting stuck on busy pages by deferring hit‑testing with setTimeout(0) and using a z‑index‑aware element‑at‑point index. Selection now respects visual stacking and clipping, ignores decorative overlays, handles offscreen cursor activation, and is faster via cached fixed‑element rects and clip‑state.
New Features
elementsFromPointwhen the index isn’t ready or misses.will-change,contain: paint/strict/content,backdrop-filter,perspective,clip-path).Bug Fixes
elementsFromPointand skips zero‑area candidates; validate fast‑path results and snapshot the index’s elements array.position: sticky, restore finishing all animations on unfreeze, and pass mouse coords from toolbar hovers.Written for commit eb5d3ce. Summary will update on new commits.