fix: Exclude the XFoldviewSlotNg element from touch event element lis…#2304
Conversation
…ts to resolve a stuck issue.
🦋 Changeset detectedLatest commit: aeb2884 The changes in this PR will be included in the next version bump. This PR includes changesets to release 5 packages
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 |
📝 WalkthroughWalkthroughThis PR introduces a changelog entry for a patch release and fixes a bug in the XFoldViewNg touch event handler. The fix excludes the DOM element itself from element collections during path and touch start handling to resolve a foldview stuck issue. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/web-platform/web-elements/src/elements/XFoldViewNg/XFoldviewSlotNgTouchEventsHandler.ts (1)
97-107:⚠️ Potential issue | 🟡 MinorConsider excluding
this.#domfrompointElementsfor consistency.The fix correctly excludes
this.#domfrompathElementson line 101, butpointElementson lines 104-106 omits this exclusion. SinceElement.contains()returnstruefor the element itself,this.#domcan be included inpointElementswhen the touch point falls within its bounds. This would add the container element tothis.#elementswhere it shouldn't belong, inconsistent with the#touchStartmethod (line 130) which explicitly filters with&& e !== this.#dom.Suggested fix for consistency
const pointElements = document.elementsFromPoint(clientX, clientY).filter( - e => this.#dom.contains(e), + e => this.#dom.contains(e) && e !== this.#dom, );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/web-platform/web-elements/src/elements/XFoldViewNg/XFoldviewSlotNgTouchEventsHandler.ts` around lines 97 - 107, pathElements excludes the container element (this.#dom) but pointElements does not, causing this.#dom to be included in this.#elements inconsistently; update the filtering of pointElements in XFoldviewSlotNgTouchEventsHandler (the pointElements variable) to also exclude this.#dom (same predicate used for pathElements: element instanceof Element && this.#dom.contains(element) && element !== this.#dom) so that when you compute this.#elements = [...new Set([...pathElements, ...pointElements])], the container is never added; mirror the check used in `#touchStart` to ensure consistent behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In
`@packages/web-platform/web-elements/src/elements/XFoldViewNg/XFoldviewSlotNgTouchEventsHandler.ts`:
- Around line 97-107: pathElements excludes the container element (this.#dom)
but pointElements does not, causing this.#dom to be included in this.#elements
inconsistently; update the filtering of pointElements in
XFoldviewSlotNgTouchEventsHandler (the pointElements variable) to also exclude
this.#dom (same predicate used for pathElements: element instanceof Element &&
this.#dom.contains(element) && element !== this.#dom) so that when you compute
this.#elements = [...new Set([...pathElements, ...pointElements])], the
container is never added; mirror the check used in `#touchStart` to ensure
consistent behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c633e8ea-ad1a-40b0-bdb5-55a0d31a4f35
📒 Files selected for processing (2)
.changeset/spotty-dryers-cry.mdpackages/web-platform/web-elements/src/elements/XFoldViewNg/XFoldviewSlotNgTouchEventsHandler.ts
Merging this PR will degrade performance by 8.4%
Performance Changes
Comparing Footnotes
|
Web Explorer#7967 Bundle Size — 383.56KiB (~+0.01%).aeb2884(current) vs e7d94be main#7961(baseline) Bundle metrics
Bundle size by type
Bundle analysis report Branch PupilTong:p/hw/try-to-fix-foldvi... Project dashboard Generated by RelativeCI Documentation Report issue |
This PR was opened by the [Changesets release](https://github.com/changesets/action) GitHub action. When you're ready to do a release, you can merge this and the packages will be published to npm automatically. If you're not ready to do a release yet, that's fine, whenever you add more changesets to main, this PR will be updated. # Releases ## @lynx-js/devtool-mcp-server@0.5.0 ### Minor Changes - Use `@lynx-js/devtool-connector` instead of `@lynx-js/debug-router-connector`. ([#2284](#2284)) The new connector avoids using keep-alive connections, which makes the connection much more reliable. - **BREAKING CHANGE**: Remove the `./debug-router-connector` exports. ([#2284](#2284)) ## @lynx-js/web-elements@0.12.0 ### Minor Changes - feat: add `willchange` event to `x-viewpager-ng` ([#2305](#2305)) ### Patch Changes - fix: firefox `@supports(width:1rex)` ([#2288](#2288)) - fix: check computed overflow style in `getTheMostScrollableKid` to avoid treating `overflow: visible` elements as scroll containers ([#2309](#2309)) - fix: the inline-truncation should only work as a direct child of x-text ([#2287](#2287)) - fix: getVisibleCells cannot work in firefox due to contentvisibilityautostatechange not propagate list-item ([#2308](#2308)) - fix: foldview stuck issue ([#2304](#2304)) ## @lynx-js/gesture-runtime@2.1.3 ### Patch Changes - Optimize gesture callbacks and relationships to prevent unnecessary gesture registration and rerenders. ([#2277](#2277)) ## @lynx-js/react@0.116.5 ### Patch Changes - Improve React runtime hook profiling. ([#2235](#2235)) Enable Profiling recording first, then enter the target page so the trace includes full render/hydrate phases. - Record trace events for `useEffect` / `useLayoutEffect` hook entry, callback, and cleanup phases. - Log trace events for `useState` setter calls. - Wire `profileFlowId` support in debug profile utilities and attach flow IDs to related hook traces. - Instrument hydrate/background snapshot profiling around patch operations with richer args (e.g. snapshot id/type, dynamic part index, value type, and source when available). - Capture vnode source mapping in dev and use it in profiling args to improve trace attribution. - Expand debug test coverage for profile utilities, hook profiling behavior, vnode source mapping, and hydrate profiling branches. - refactor: call loadWorkletRuntime once in each module ([#2315](#2315)) ## @lynx-js/rspeedy@0.13.5 ### Patch Changes - feat: opt-in the web platform's new binary output format ([#2281](#2281)) Introduce a new flag to enable the new binary output format. Currently it's an internal-use-only flag that will be removed in the future; set the corresponding environment variable to 'true' to enable it. - Avoid generating `Rsbuild vundefined` in greeting message. ([#2275](#2275)) - Updated dependencies \[]: - @lynx-js/web-rsbuild-server-middleware@0.19.8 ## @lynx-js/lynx-bundle-rslib-config@0.2.2 ### Patch Changes - Support bundle and load css in external bundle ([#2143](#2143)) ## @lynx-js/external-bundle-rsbuild-plugin@0.0.3 ### Patch Changes - Updated dependencies \[[`c28b051`](c28b051), [`4cbf809`](4cbf809)]: - @lynx-js/externals-loading-webpack-plugin@0.0.4 ## @lynx-js/react-rsbuild-plugin@0.12.10 ### Patch Changes - Support bundle and load css in external bundle ([#2143](#2143)) - Updated dependencies \[[`59f2933`](59f2933), [`453e006`](453e006)]: - @lynx-js/template-webpack-plugin@0.10.5 - @lynx-js/css-extract-webpack-plugin@0.7.0 - @lynx-js/react-webpack-plugin@0.7.4 - @lynx-js/react-alias-rsbuild-plugin@0.12.10 - @lynx-js/use-sync-external-store@1.5.0 - @lynx-js/react-refresh-webpack-plugin@0.3.4 ## @lynx-js/web-core-wasm@0.0.5 ### Patch Changes - Updated dependencies \[[`4963907`](4963907), [`8fd936a`](8fd936a), [`0d41253`](0d41253), [`d32c4c6`](d32c4c6), [`7518b72`](7518b72), [`fca9d4a`](fca9d4a)]: - @lynx-js/web-elements@0.12.0 ## @lynx-js/externals-loading-webpack-plugin@0.0.4 ### Patch Changes - perf: optimize external bundle loading by merging multiple `fetchBundle` calls for the same URL into a single request. ([#2307](#2307)) - Support bundle and load css in external bundle ([#2143](#2143)) ## @lynx-js/template-webpack-plugin@0.10.5 ### Patch Changes - feat: allow `templateDebugUrl` to be customized via `output.publicPath` or the `beforeEncode` hook. ([#2274](#2274)) - feat: opt-in the web platform's new binary output format ([#2281](#2281)) Introduce a new flag to enable the new binary output format. Currently it's an internal-use-only flag that will be removed in the future; set the corresponding environment variable to 'true' to enable it. - Updated dependencies \[]: - @lynx-js/web-core-wasm@0.0.5 ## create-rspeedy@0.13.5 ## @lynx-js/react-alias-rsbuild-plugin@0.12.10 ## upgrade-rspeedy@0.13.5 Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
…ts to resolve a stuck issue.
Summary by CodeRabbit
Checklist