fix: use measurement clone to avoid disrupting popup animations during alignment#612
Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses a critical cross-platform rendering bug affecting popup components on Linux, where they would appear off-screen. The core problem stemmed from the alignment logic interfering with active CSS animations during measurement. The solution introduces a robust mechanism using a temporary, invisible clone of the popup for all measurement operations, thereby isolating the original element from disruptive style manipulations and preserving its animations. This change not only fixes the visual glitch but also streamlines the alignment code by removing redundant style management. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
Walkthrough将对齐测量从直接修改弹出层样式改为在弹出层父容器中创建一个不可交互、隐藏的克隆元素（measureEl）进行测量；所有 getBoundingClientRect() 读取从该克隆元素获取，测量完成后移除克隆元素，不再恢复或修改原弹出层样式。测试新增断言以验证未改动 popup 的样式属性。 Changes
Sequence Diagram(s)sequenceDiagram
participant Trigger
participant PopupParent as Popup 父容器
participant Popup as Popup 元素
participant MeasureEl as measureEl (克隆)
participant Align as 对齐逻辑
Trigger->>Align: 请求对齐（resize/重排）
Align->>PopupParent: 创建 measureEl = Popup.cloneNode(false)
PopupParent->>MeasureEl: append measureEl（隐藏、不可交互）
MeasureEl->>Align: 返回 getBoundingClientRect()（宽/高/位置信息）
Align->>Align: 计算位置/翻转/缩放
Align->>Popup: 应用最终对齐（不修改测量相关样式）
Align->>MeasureEl: remove()（清理克隆）
代码审查工作量评估🎯 3 (中等) | ⏱️ ~20 分钟 可能相关的 PRs
推荐审阅人
诗歌
🚥 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)
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 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the popup alignment logic in useAlign.ts to prevent interference with CSS animations. It achieves this by using a temporary, cloned measurement element (measureEl) for calculations instead of directly modifying the popupElement's styles. This ensures that getBoundingClientRect() returns accurate values and avoids disrupting active CSS transitions or transforms on the popup. A new test case has been added to align.test.tsx to verify that the popup's styles (specifically transform, transition, and overflow) are no longer directly modified during the alignment measurement phase. The review suggests adding a try...finally block to prevent potential DOM leaks and enhancing the new test case to also assert that left and top styles are not modified on the popup element during measurement.
| measureEl.style.transition = 'none'; | ||
| measureEl.style.visibility = 'hidden'; | ||
| measureEl.style.pointerEvents = 'none'; | ||
| popupElement.parentElement?.appendChild(measureEl); |
There was a problem hiding this comment.
To prevent DOM leaks if an error occurs during alignment, it's best practice to wrap the measurement logic that uses the temporary measureEl in a try...finally block, ensuring removeChild is always called.
For example:
popupElement.parentElement?.appendChild(measureEl);
let popupMirrorRect: DOMRect;
try {
// ... measurement logic ...
popupMirrorRect = measureEl.getBoundingClientRect();
} finally {
popupElement.parentElement?.removeChild(measureEl);
}
// ... rest of the function ...Note that popupMirrorRect would need to be declared as let outside the try block to be accessible later.
| expect(styleChanges).not.toContain('transform'); | ||
| expect(styleChanges).not.toContain('transition'); | ||
| expect(styleChanges).not.toContain('overflow'); |
There was a problem hiding this comment.
To make this test more comprehensive and future-proof, consider also asserting that left and top styles are not modified during measurement. The previous implementation modified these properties, and ensuring they are not touched directly is a key part of this fix.
expect(styleChanges).not.toContain('left');
expect(styleChanges).not.toContain('top');
expect(styleChanges).not.toContain('transform');
expect(styleChanges).not.toContain('transition');
expect(styleChanges).not.toContain('overflow');
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/hooks/useAlign.ts`:
- Around line 194-205: The shallow clone (popupElement.cloneNode(false))
produces an empty measureEl so getBoundingClientRect() returns 0×0 and breaks
the scale/alignment logic; fix by creating a measurement element that preserves
layout—either clone with children (cloneNode(true)) or immediately copy the
original element's computed size into measureEl (set style.width/style.height or
min-/max- styles from getComputedStyle or the original bounding rect) before
appending and measuring; ensure this change affects the same measurement block
(measureEl/popupElement -> popupWidth/popupHeight -> scaleX/scaleY) and apply
the same fix to the other clone block around lines 282-288 referenced in the
comment.
In `@tests/align.test.tsx`:
- Around line 363-381: The current test only spies on
popupElement.style.setProperty so direct assignments like style.transform =
'none' or style.left = 'auto' (seen in the implementation) evade detection;
update the test to spy on the CSSStyleDeclaration property setters used by the
implementation (e.g., spy on CSSStyleDeclaration.prototype for 'transform',
'transition', 'left', 'top', 'overflow' setters or wrap those property
descriptors) so assignments to style.transform/style.left/etc. are captured
instead of only setProperty.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c63dc6c9-4a54-40a3-9300-a632b72a0d5b
📒 Files selected for processing (2)
src/hooks/useAlign.tstests/align.test.tsx
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/hooks/useAlign.ts`:
- Around line 205-207: The temporary measurement element (measureEl) can still
run CSS keyframe animations because cloneNode(false) preserves the
motionClassName; after disabling overflow/transform/transition in useAlign
(measureEl), set measureEl.style.animation = 'none' to stop keyframe animations
from affecting getBoundingClientRect(); update the code near where
measureEl.style.transition = 'none' is set and add a regression test using a
motion config with keyframes (e.g., rc-trigger-popup-zoom) to verify stable
positioning.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: bad5fa62-b289-4653-a7af-fc4882964c91
📒 Files selected for processing (1)
src/hooks/useAlign.ts
…g alignment On some platforms (notably Linux with X11/Wayland compositors), the alignment calculation runs mid-CSS-animation. The previous approach modified the popup element's styles (left, top, transform, overflow) to measure its position, which interfered with active CSS transitions (transition: all) and transforms (transform: scale()) applied during entrance animations. This caused getBoundingClientRect() to return incorrect values, producing wildly wrong popup positions (e.g. top: -20000px). The fix replaces the direct popup style manipulation with a shallow clone (cloneNode(false)) used as a measurement proxy. The clone inherits the popup's classes and attributes but has transform/transition/animation explicitly neutralized. Dimensions are copied via offsetWidth/offsetHeight (not getComputedStyle, which can return empty during the initial render cycle). This allows accurate position measurement without touching the original popup element, fully preserving CSS animations on all platforms. Changes: - Replace placeholder + popup style reset with cloneNode(false) measurement - Copy offsetWidth/offsetHeight to clone for accurate dimensions - Set transform/transition/animation to none on clone only - Measure positions via the clone's getBoundingClientRect() - Remove originLeft/Top/Right/Bottom/Overflow save/restore logic
AnthonyH16
force-pushed
the
fix/neutralize-transform-during-align
branch
from
89469ce to
1543cec
Compare
1 participant
Problem
On Linux (X11 and Wayland), popup components (Popover, Tooltip, Dropdown, Select, DatePicker, etc.) are positioned wildly off-screen (e.g.
top: -20000px) when they open.Root Cause
The
useAlignhook modifies the popup element's inline styles (left,top,transform,overflow) during measurement. On Linux, the alignment calculation runs mid-CSS-animation. This causes two issues:transition: all 1e-05s(set by antd CSS-in-JS during motion prepare phase) causes the browser to transition position values from their old state (-1000vw) instead of instantly applying the reset values (0), sogetBoundingClientRect()reads the transitioning (old) position.transform: scale()(from entrance animation) distortsgetBoundingClientRect()dimensions whilegetComputedStyle()returns untransformed values, producing incorrect scale factors (e.g. 0.1 instead of 1) that multiply offsets by 10x.On macOS/Windows, different compositor frame scheduling means the alignment happens to run after animations settle, masking this issue.
Reproduction
top/leftvaluesSolution
Replace the direct popup style manipulation with a shallow clone (
cloneNode(false)) used as a measurement proxy:transform: noneandtransition: noneare set on the clone only, not the original popupgetBoundingClientRect()instead of the popup'sThis is a net reduction in code (-38 lines, +47 lines) since the save/restore logic for
originLeft,originTop,originRight,originBottom,originOverflow, and the placeholder element are all eliminated.Test Plan
Summary by CodeRabbit
发布说明
Bug 修复
测试