fix(selection-toolbar): ignore overlay text selections#1224
fix(selection-toolbar): ignore overlay text selections#1224mengxi-ream merged 2 commits intomainfrom
Conversation
🦋 Changeset detectedLatest commit: e0049ea The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
Documentation Updates 1 document(s) were updated by changes in this PR: Translation Toggle Logic and Content DetectionView Changes@@ -131,6 +131,13 @@
As of v1.31.3 (PR #1185), retargeted interactive clicks are properly filtered. The implementation walks through the event's composed path to identify the true interactive element that was clicked, regardless of shadow DOM boundaries or event retargeting mechanisms. This ensures that clicks originating from buttons or other interactive elements inside shadow roots are correctly recognized, even when the event's `target` property is retargeted to the shadow host element.
The implementation checks whether the interactive element is contained within the current selection using `selection.containsNode()`. If the interactive element is outside the selection, the toolbar is hidden; if it's inside the selection, the toolbar remains visible. This ensures intuitive behavior when users interact with translation controls within the toolbar itself while preventing unexpected toolbar reappearance when clicking unrelated page controls.
+
+**Overlay Selection Filtering:**
+The selection toolbar filters out text selections made within its own overlay UI elements to prevent interference with the main page selection. Overlay elements (popovers, toaster, toolbar surfaces) are marked with the `data-rf-selection-overlay-root` attribute, and the toolbar detects and ignores selection events originating from these marked elements.
+
+When users interact with overlay UI (e.g., selecting text in a translation result, clicking buttons in the toolbar), the system preserves the original page selection session rather than replacing it. This ensures that rerunning toolbar actions (such as regenerating a translation) continues to use the original page paragraphs and context, preventing unwanted toolbar state changes and maintaining a consistent workflow.
+
+The filtering logic inspects `mousedown`, `mouseup`, and `selectionchange` events to determine if they originate from overlay elements. Selection boundary nodes (anchor nodes, focus nodes, and range containers) are checked against the overlay container and any elements with the overlay root attribute. If the selection is contained within an overlay, the main selection state is preserved and the toolbar does not update or dismiss inappropriately.
**Translation Flow and Request Handling:**
The selection toolbar translation flow is designed for reliability and responsiveness. The system includes hardened stale-request cancellation handling to manage rapid selection changes. When a user makes a selection and then quickly changes it, pending translation requests for the previous selection are cancelled, ensuring that only the most current selection is translated. This prevents race conditions and ensures that outdated translations do not overwrite current content, providing a smooth and predictable translation experience.
@@ -187,6 +194,14 @@
- The alert component uses the `role="alert"` attribute for accessibility and proper testing
- Custom action errors follow the same inline alert pattern for consistency across all selection toolbar features
+**Overlay Selection Filtering Tests:**
+Integration tests in `src/entrypoints/selection.content/selection-toolbar/__tests__/request-rerun.test.tsx` and `src/entrypoints/selection.content/selection-toolbar/__tests__/selection-toolbar.test.tsx` verify overlay selection filtering behavior:
+- When users select text inside the translation popover and then rerun the translation, the original page selection and context are preserved
+- Selection boundaries (anchor node, focus node, range containers) inside overlay root elements are correctly detected and filtered
+- Text selections within elements marked with `data-rf-selection-overlay-root` do not trigger toolbar state changes
+- The toolbar does not show or hide inappropriately when selections are made inside overlay UI elements
+- Rerunning toolbar actions after overlay interactions uses the original page paragraphs rather than overlay content
+
**Float Layout Tests:**
Integration tests verify the float layout detection and handling:
- In bilingual mode, block translations are marked with `data-read-frog-float-wrap="true"` when the translated content would drop below an active floated sibling |
There was a problem hiding this comment.
1 issue 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="src/entrypoints/selection.content/selection-toolbar/index.tsx">
<violation number="1" location="src/entrypoints/selection.content/selection-toolbar/index.tsx:89">
P2: Overlay-root detection misses text-node selections because the attribute check only runs for `Element` nodes.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 82a1c22369
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if (node instanceof Element && node.closest(SELECTION_OVERLAY_ROOT_SELECTOR)) { | ||
| return true |
There was a problem hiding this comment.
Handle text-node boundaries when matching overlay roots
isNodeInsideSelectionOverlay only runs the overlay-root attribute check when the boundary node is an Element, but browser selections usually report Text nodes for anchorNode/focusNode and range boundaries. In cases where overlay UI is identified via data-rf-selection-overlay-root rather than overlayContainer.contains(...)/shared shadow root (for example, portal content rendered outside the toolbar container), this returns false and the toolbar can still treat overlay selections as page selections. Resolve this by walking from non-Element nodes to their parent element before calling closest.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
1 issue found across 2 files (changes from recent commits).
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="src/entrypoints/selection.content/selection-toolbar/__tests__/selection-toolbar.test.tsx">
<violation number="1" location="src/entrypoints/selection.content/selection-toolbar/__tests__/selection-toolbar.test.tsx:406">
P2: This test leaks DOM state by appending an overlay root to `document.body` without removing it, which can make later tests order-dependent.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Type of Changes
Description
Prevent selection toolbar state from being replaced when users select text inside the extension's own overlay UI.
mousedown,mouseup, andselectionchangeevents that originate from overlay selectionsRelated Issue
Closes #
How Has This Been Tested?
Added unit tests
Verified through manual testing
SKIP_FREE_API=true pnpm vitest run src/entrypoints/selection.content/selection-toolbar/__tests__/request-rerun.test.tsx src/entrypoints/selection.content/selection-toolbar/__tests__/selection-toolbar.test.tsxSKIP_FREE_API=true pnpm type-checkScreenshots
N/A
Checklist
Additional Information
This intentionally fixes the event/session boundary at the selection overlay layer without changing paragraph extraction logic.