fix: event coordinates relative to lynx-view container#2341
fix: event coordinates relative to lynx-view container#2341
Conversation
…tainer When <lynx-view> is embedded in a larger HTML page with offset from viewport origin (e.g., margin), event coordinates (detail.x/y, clientX/clientY, pageX/pageY) were viewport-relative instead of lynx-view-relative. This caused apps using tap coordinates for positioning to place elements at wrong locations. Pass container rect callback to createCrossThreadEvent and subtract container offset from all coordinate properties. Also fix boundingClientRect to return container-relative values instead of viewport-relative. Affects: click, mouse, and touch events in both web-mainthread-apis and web-core-wasm implementations, plus InvokeUIMethod boundingClientRect.
|
📝 WalkthroughWalkthroughIntroduces container-relative coordinate adjustments: add optional getContainerRect to cross-thread event creators, pass it through event paths, and compute boundingClientRect and touch/mouse coordinates relative to the container's bounding rect. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 Coding Plan
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 Tip CodeRabbit can use Trivy to scan for security misconfigurations and secrets in Infrastructure as Code files.Add a .trivyignore file to your project to customize which findings Trivy reports. |
There was a problem hiding this comment.
Pull request overview
This PR updates the web-platform event bridging layer so pointer/touch/mouse coordinates (and boundingClientRect) are reported relative to the <lynx-view> container rather than the viewport origin, improving correctness when the view is embedded with offsets.
Changes:
- Add an optional
getContainerRect()callback tocreateCrossThreadEventand pass it from the main-thread bindings. - Offset mouse/click/touch coordinates by the container’s bounding rect before sending cross-thread.
- Return container-relative
boundingClientRectvalues from both mainthread-apis and web-core-wasm invokeUI handlers.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/web-platform/web-mainthread-apis/ts/utils/createCrossThreadEvent.ts | Adds container-rect aware coordinate adjustment for touch/mouse/click cross-thread events. |
| packages/web-platform/web-mainthread-apis/ts/createMainThreadGlobalThis.ts | Wires container rect provider into event creation; makes boundingClientRect container-relative. |
| packages/web-platform/web-core-wasm/ts/client/mainthread/elementAPIs/createCrossThreadEvent.ts | Adds container-rect aware coordinate adjustment for WASM runtime cross-thread events. |
| packages/web-platform/web-core-wasm/ts/client/mainthread/elementAPIs/WASMJSBinding.ts | Passes <lynx-view> host rect provider into event creation for WASM runtime. |
| packages/web-platform/web-core-wasm/ts/client/mainthread/crossThreadHandlers/registerInvokeUIMethodHandler.ts | Makes boundingClientRect container-relative in WASM invokeUI method handling. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| return { | ||
| ...t, | ||
| clientX: (t.clientX as number) - rect.left, | ||
| clientY: (t.clientY as number) - rect.top, | ||
| pageX: (t.pageX as number) - rect.left, | ||
| pageY: (t.pageY as number) - rect.top, |
| return { | ||
| ...t, | ||
| clientX: (t.clientX as number) - rect.left, | ||
| clientY: (t.clientY as number) - rect.top, | ||
| pageX: (t.pageX as number) - rect.left, | ||
| pageY: (t.pageY as number) - rect.top, |
| export function createCrossThreadEvent( | ||
| domEvent: MinimalRawEventObject, | ||
| getContainerRect?: () => DOMRect, | ||
| ): LynxCrossThreadEvent { |
❌ 1 Tests Failed:
View the top 2 failed test(s) by shortest run time
To view more test analytics, go to the Test Analytics Dashboard |
TypeScript TS4111: properties from index signatures must be accessed with bracket notation when noPropertyAccessFromIndexSignature is enabled.
![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
🧹 Nitpick comments (1)
packages/web-platform/web-core-wasm/ts/client/mainthread/elementAPIs/createCrossThreadEvent.ts (1)
52-74: Cache container rect once per event instead of per touch item.
getContainerRect()is currently invoked inside each touch mapping path, which can multiply cross-boundary/layout reads. Compute rect/offset once before mapping and reuse it.As per coding guidelines, “Avoid recursive borrowing patterns where Rust calls JS, and JS immediately calls back into Rust to retrieve data that Rust already possesses, as this will cause
RefCellborrowing panics”.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/web-platform/web-core-wasm/ts/client/mainthread/elementAPIs/createCrossThreadEvent.ts` around lines 52 - 74, The code currently calls getContainerRect inside adjustTouch for every touch which can cause repeated cross-boundary reads; compute the container rect once before mapping and reuse it: call getContainerRect() once (e.g. const rect = getContainerRect?.()) before creating adjustTouch, then have adjustTouch close over that rect (or return identity if rect is undefined) and use that single rect when mapping touches/targetTouches/changedTouches in the assignments to touches/targetTouches/changedTouches; update references to clientX/clientY/pageX/pageY inside adjustTouch to use the precomputed rect to avoid per-touch calls to getContainerRect.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@packages/web-platform/web-core-wasm/ts/client/mainthread/elementAPIs/createCrossThreadEvent.ts`:
- Around line 57-60: The touch pageX/pageY adjustment is wrong because
pageX/pageY are document (scroll) coordinates but the code only subtracts
rect.left/top (viewport), causing errors when scrolled; in
createCrossThreadEvent derive adjusted pageX/pageY from the adjusted client
coordinates instead of mirroring t.pageX/t.pageY: compute adjustedClientX =
(t['clientX'] as number) - rect.left and adjustedClientY = (t['clientY'] as
number) - rect.top (these are already used for clientX/clientY) and then set
pageX = adjustedClientX + (window.scrollX || 0) and pageY = adjustedClientY +
(window.scrollY || 0) so page coordinates account for scroll. Use the same local
names (t, rect, clientX/clientY/pageX/pageY) in the createCrossThreadEvent
function to locate and replace the existing pageX/pageY lines.
---
Nitpick comments:
In
`@packages/web-platform/web-core-wasm/ts/client/mainthread/elementAPIs/createCrossThreadEvent.ts`:
- Around line 52-74: The code currently calls getContainerRect inside
adjustTouch for every touch which can cause repeated cross-boundary reads;
compute the container rect once before mapping and reuse it: call
getContainerRect() once (e.g. const rect = getContainerRect?.()) before creating
adjustTouch, then have adjustTouch close over that rect (or return identity if
rect is undefined) and use that single rect when mapping
touches/targetTouches/changedTouches in the assignments to
touches/targetTouches/changedTouches; update references to
clientX/clientY/pageX/pageY inside adjustTouch to use the precomputed rect to
avoid per-touch calls to getContainerRect.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 7a42cade-73bf-43ae-a83f-17f48018f470
📒 Files selected for processing (2)
packages/web-platform/web-core-wasm/ts/client/mainthread/elementAPIs/createCrossThreadEvent.tspackages/web-platform/web-mainthread-apis/ts/utils/createCrossThreadEvent.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/web-platform/web-mainthread-apis/ts/utils/createCrossThreadEvent.ts
| clientX: (t['clientX'] as number) - rect.left, | ||
| clientY: (t['clientY'] as number) - rect.top, | ||
| pageX: (t['pageX'] as number) - rect.left, | ||
| pageY: (t['pageY'] as number) - rect.top, |
There was a problem hiding this comment.
Touch pageX/pageY adjustment is scroll-sensitive and can be wrong.
Touch.pageX/pageY are document-based. Subtracting only rect.left/top (viewport-based) can miscompute coordinates when the page is scrolled. Derive adjusted pageX/pageY from adjusted clientX/clientY (or subtract scroll as well).
Proposed fix
- const adjustTouch = (t: CloneableObject): CloneableObject => {
- if (getContainerRect) {
- const rect = getContainerRect();
- return {
- ...t,
- clientX: (t['clientX'] as number) - rect.left,
- clientY: (t['clientY'] as number) - rect.top,
- pageX: (t['pageX'] as number) - rect.left,
- pageY: (t['pageY'] as number) - rect.top,
- };
- }
- return t;
- };
+ const rect = getContainerRect?.();
+ const offsetX = rect?.left ?? 0;
+ const offsetY = rect?.top ?? 0;
+ const adjustTouch = (t: CloneableObject): CloneableObject => {
+ const cx = (t['clientX'] as number) - offsetX;
+ const cy = (t['clientY'] as number) - offsetY;
+ return {
+ ...t,
+ clientX: cx,
+ clientY: cy,
+ pageX: cx,
+ pageY: cy,
+ };
+ };📝 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.
| clientX: (t['clientX'] as number) - rect.left, | |
| clientY: (t['clientY'] as number) - rect.top, | |
| pageX: (t['pageX'] as number) - rect.left, | |
| pageY: (t['pageY'] as number) - rect.top, | |
| const rect = getContainerRect?.(); | |
| const offsetX = rect?.left ?? 0; | |
| const offsetY = rect?.top ?? 0; | |
| const adjustTouch = (t: CloneableObject): CloneableObject => { | |
| const cx = (t['clientX'] as number) - offsetX; | |
| const cy = (t['clientY'] as number) - offsetY; | |
| return { | |
| ...t, | |
| clientX: cx, | |
| clientY: cy, | |
| pageX: cx, | |
| pageY: cy, | |
| }; | |
| }; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@packages/web-platform/web-core-wasm/ts/client/mainthread/elementAPIs/createCrossThreadEvent.ts`
around lines 57 - 60, The touch pageX/pageY adjustment is wrong because
pageX/pageY are document (scroll) coordinates but the code only subtracts
rect.left/top (viewport), causing errors when scrolled; in
createCrossThreadEvent derive adjusted pageX/pageY from the adjusted client
coordinates instead of mirroring t.pageX/t.pageY: compute adjustedClientX =
(t['clientX'] as number) - rect.left and adjustedClientY = (t['clientY'] as
number) - rect.top (these are already used for clientX/clientY) and then set
pageX = adjustedClientX + (window.scrollX || 0) and pageY = adjustedClientY +
(window.scrollY || 0) so page coordinates account for scroll. Use the same local
names (t, rect, clientX/clientY/pageX/pageY) in the createCrossThreadEvent
function to locate and replace the existing pageX/pageY lines.
Merging this PR will improve performance by 10.72%
Performance Changes
Comparing 
Footnotes
|
Web Explorer#8177 Bundle Size — 384.85KiB (+0.09%).6c84000(current) vs 0991136 main#8171(baseline) Bundle metrics
|
| Current #8177 |
Baseline #8171 |
|
|---|---|---|
 Initial JS |
155.59KiB |
155.59KiB |
| Initial CSS |
35.1KiB |
35.1KiB |
 Cache Invalidation |
7.53% |
0% |
| Chunks |
8 |
8 |
| Assets |
8 |
8 |
| Modules |
238 |
238 |
| Duplicate Modules |
16 |
16 |
| Duplicate Code |
2.98% |
2.98% |
| Packages |
4 |
4 |
| Duplicate Packages |
0 |
0 |
Bundle size by type 1 change 
1 regression
| Current #8177 |
Baseline #8171 |
|
|---|---|---|
| JS |
253.9KiB (+0.14%) |
253.55KiB |
| Other |
95.85KiB |
95.85KiB |
| CSS |
35.1KiB |
35.1KiB |
Bundle analysis report Branch Huxpro/fix-spread-null-ref Project dashboard
Generated by RelativeCI Documentation Report issue
Summary
Fixed tap/touch/mouse event coordinates to be relative to the
<lynx-view>container instead of viewport origin. When<lynx-view>is embedded with offset (e.g.,margin: 200px 300px),detail.x/y,clientX/clientY, andpageX/pageYnow correctly reflect position within the view, not the viewport. Also fixedboundingClientRectto return container-relative values.Changes
createCrossThreadEventin both web-mainthread-apis and web-core-wasmInvokeUIMethod.boundingClientRectto return container-relative positionsChecklist
Summary by CodeRabbit