fix(runtime-dom): inconsistent behaviour on nested transition groups #8803
Conversation
|
For anyone trying to review this, here's the original reproduction, converted to a Playground: Here's the same example but running on this PR: |
…in transition group positioning
|
@skirtles-code Done! |
Size ReportBundles
Usages
|
|
/ecosystem-ci run |
@vue/compiler-core
@vue/compiler-dom
@vue/compiler-ssr
@vue/compiler-sfc
@vue/reactivity
@vue/runtime-core
@vue/runtime-dom
@vue/server-renderer
@vue/shared
vue
@vue/compat
commit: |
|
📝 Ran ecosystem CI: Open
|
edison1105
added
🐞 bug
|
Any estimate on when it'll be merged? It's been over a year. |
|
/ecosystem-ci run |
|
📝 Ran ecosystem CI: Open
|
|
/ecosystem-ci run vite-plugin-vue |
|
📝 Ran ecosystem CI: Open
|
edison1105
added
the
🔨 p3-minor-bug
WalkthroughThe change introduces a helper function to compute an element's position relative to its parent, replacing the previous use of absolute bounding rectangles. The position-tracking logic in the Changes
Sequence Diagram(s)sequenceDiagram
participant TransitionGroup
participant DOMElement
participant ParentElement
TransitionGroup->>DOMElement: getBoundingClientRect()
TransitionGroup->>ParentElement: getBoundingClientRect()
TransitionGroup->>TransitionGroup: getRelativePosition(el)
Note right of TransitionGroup: Compute relative left/top
TransitionGroup->>TransitionGroup: Store {left, top} in positionMap
Assessment against linked issues
Poem
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![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/runtime-dom/src/components/TransitionGroup.ts (1)
153-155: Pure function call during render – just double-check perf
getRelativePositionis executed for every child on every render. It still triggersgetBoundingClientRect, which forces a layout read twice per element (old/new). While this is inherited from the previous implementation, consider caching the first read inside the loop or switching tooffsetLeft/offsetTopwhere possible to reduce layout thrashing in very large lists.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/runtime-dom/src/components/TransitionGroup.ts(3 hunks)
🔇 Additional comments (2)
packages/runtime-dom/src/components/TransitionGroup.ts (2)
32-35: Small, well-scoped data structure – looks goodA minimal
Positioninterface (onlyleft/top) keeps the payload lean and avoids carrying useless properties that used to be present (width/height).
37-38: Correct use ofWeakMapavoids leaksUsing
VNodeas the key and aWeakMapensures the stored coordinates are garbage-collected when the VNode is discarded – thumbs-up.
| function getRelativePosition(el: Element): Position { | ||
| const elRect = el.getBoundingClientRect() | ||
| if (!el.parentElement) { | ||
| return { | ||
| left: elRect.left, | ||
| top: elRect.top, | ||
| } | ||
| } | ||
|
|
||
| const parentRect = el.parentElement.getBoundingClientRect() | ||
| return { | ||
| left: elRect.left - parentRect.left, | ||
| top: elRect.top - parentRect.top, | ||
| } | ||
| } |
There was a problem hiding this comment.
parentElement may be null – relative delta becomes wrong
getRelativePosition falls back to absolute viewport coords when el.parentElement is null (e.g. the parent is a DocumentFragment, a comment, or the node is temporarily detached).
If that happens between the “old” and “new” snapshots, dx/dy will be computed against mixed coordinate spaces → items jump instead of animating.
Proposed defensive tweak:
function getRelativePosition(el: Element): Position {
- const elRect = el.getBoundingClientRect()
- if (!el.parentElement) {
- return {
- left: elRect.left,
- top: elRect.top,
- }
- }
-
- const parentRect = el.parentElement.getBoundingClientRect()
+ const elRect = el.getBoundingClientRect()
+ const parent = el.parentElement as Element | null
+ if (!parent) {
+ // No element parent (e.g. DocumentFragment) — treat the container itself
+ // as the reference so both snapshots stay in the same coordinate space.
+ return { left: elRect.left, top: elRect.top }
+ }
+
+ const parentRect = parent.getBoundingClientRect()
return {
left: elRect.left - parentRect.left,
top: elRect.top - parentRect.top,
}
}This keeps both snapshots in the same space and avoids incorrect deltas.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In packages/runtime-dom/src/components/TransitionGroup.ts around lines 193 to
207, the getRelativePosition function returns absolute viewport coordinates when
el.parentElement is null, causing inconsistent coordinate spaces and incorrect
delta calculations. To fix this, modify the function to always compute positions
relative to a consistent ancestor or coordinate space, such as the document or a
stable container, ensuring both old and new snapshots use the same reference
frame and preventing animation jumps.
fixes #1531
close #5385
Summary by CodeRabbit