fix(editor): non-canvas block size/position in embed-edgeless-doc at non-1 zoom #14074
Conversation
WalkthroughRefactors CSS transform computation in gfx-block-component to use viewport's viewScale. Translations (X/Y) and scale are divided by viewScale; wrapper now delegates getCSSTransform to the base GfxBlockComponent implementation. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
blocksuite/framework/std/src/view/element/gfx-block-component.ts (1)
108-119: viewScale-aware transform looks consistent; consider guarding against viewScale = 0 and minor cleanup
- The new math (
scaledX = (bound.x * zoom) / viewScale, same fory, and dividingtranslateX/translateYandzoombyviewScale) preserves the old behavior whenviewScale === 1and correctly compensates for an outer container scale so the effective zoom remainszoom. This matches the PR goal of fixing embed behavior when the container itself is scaled.- One edge case to double-check:
viewport.viewScaleis computed asboundingClientRect.width / _cachedOffsetWidth. IfboundingClientRect.widthcan be0while_cachedOffsetWidth > 0,viewScalebecomes0, and this function will divide by zero, yieldingInfinity/NaNin the CSS transform. It may be safer to clampviewScaleto a small positive value (or1) in the viewport getter so all consumers are protected.- Optional: for readability, you could factor out the combined scale once and reuse it, e.g.:
- const { translateX, translateY, zoom, viewScale } = viewport; + const { translateX, translateY, zoom, viewScale } = viewport; + const scale = zoom / viewScale; const bound = Bound.deserialize(this.model.xywh); - const scaledX = (bound.x * zoom) / viewScale; - const scaledY = (bound.y * zoom) / viewScale; + const scaledX = bound.x * scale; + const scaledY = bound.y * scale; const deltaX = scaledX - bound.x; const deltaY = scaledY - bound.y; - return `translate(${translateX / viewScale + deltaX}px, ${translateY / viewScale + deltaY}px) scale(${zoom / viewScale})`; + return `translate(${translateX / viewScale + deltaX}px, ${translateY / viewScale + deltaY}px) scale(${scale})`;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
blocksuite/framework/std/src/view/element/gfx-block-component.ts(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
blocksuite/framework/std/src/view/element/gfx-block-component.ts (4)
blocksuite/affine/blocks/root/src/edgeless/edgeless-root-service.ts (2)
viewport(103-105)zoom(107-109)blocksuite/affine/blocks/embed/src/common/to-edgeless-embed-block.ts (1)
bound(33-35)tests/blocksuite/e2e/utils/asserts.ts (1)
Bound(106-106)blocksuite/framework/std/src/gfx/viewport.ts (3)
viewScale(216-224)translateX(230-232)translateY(234-236)
🔇 Additional comments (1)
blocksuite/framework/std/src/view/element/gfx-block-component.ts (1)
222-224: Good reuse of base getCSSTransform in wrapperDelegating
toGfxBlockComponent’sgetCSSTransformtoGfxBlockComponent.prototype.getCSSTransform.call(this)removes duplication and guarantees wrapper-based gfx blocks stay in sync with the core transform logic (including future viewScale tweaks). This is a solid refactor for maintainability.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## canary #14074 +/- ##
==========================================
- Coverage 56.53% 56.53% -0.01%
==========================================
Files 2757 2757
Lines 138059 138052 -7
Branches 21048 21052 +4
==========================================
- Hits 78053 78045 -8
- Misses 57741 58319 +578
+ Partials 2265 1688 -577
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
darkskygit
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
blocksuite/framework/std/src/view/element/gfx-block-component.ts (1)
222-224: Excellent refactoring to eliminate code duplication.Delegating to the base class implementation ensures consistency and reduces maintenance overhead. This change automatically benefits from the viewScale fix in the base
getCSSTransformmethod.Consider applying the same delegation pattern to other methods in the wrapper that duplicate base class logic, such as
getRenderingRect(lines 227-246) andscheduleUpdate(lines 269-281), if they remain identical to the base implementation.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
blocksuite/framework/std/src/view/element/gfx-block-component.ts(2 hunks)
🔇 Additional comments (1)
blocksuite/framework/std/src/view/element/gfx-block-component.ts (1)
108-119: Verify that viewScale is always positive and non-zero.The division operations using
viewScaleon lines 113, 114, and 118 could produceInfinityorNaNifviewScaleis0, or incorrect visual behavior if it's negative. Please ensure the viewport guaranteesviewScale > 0.
2 participants
Problem
●Similar to PR#14015, the container's own scaling factor (
viewScale) was not taken into account. This time the issue affects non-canvas blocks (e.g.edgeless-note,edgeless-image, and any component extendingGfxBlockComponent).●The follwing image and video show the case when zoom is 0.5.
before.mp4
Solution
●Incorporated
viewScaleinto the CSStranslatecalculation for allGfxBlockComponentinstances.Additional Improvement
●Minor refactor: the class returned by
toGfxBlockComponent()now reuses the originalgetCSSTransform()implementation fromGfxBlockComponent.prototypevia.call(this), eliminating duplicated code.After
●The refined is as follows.
after.mp4
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.