Fix remaining issues of callout positioning (#12529)#12530
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Walkthrough
ChangesCallout Positioning Overhaul
Sequence Diagram(s)sequenceDiagram
actor User
participant toggle as Callouts.toggle()
participant position as Callouts.position()
participant Utils as Utils.getViewport()
participant ResizeObserver
participant reposition as Callouts.reposition()
User->>toggle: open callout
toggle->>position: position(callout, component, ...)
position->>Utils: getViewport()
Utils-->>position: visualWidth, visualHeight, offsetTop, offsetLeft
position-->>toggle: positioning result
toggle->>ResizeObserver: observeCalloutResize(callout)
toggle-->>User: return result
Note over ResizeObserver: callout content changes height
ResizeObserver->>reposition: reposition()
reposition->>position: position(callout, component, ...)
position->>Utils: getViewport()
Utils-->>position: updated viewport geometry
position-->>reposition: updated positioning result
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/BlazorUI/Bit.BlazorUI/Scripts/Callouts.ts`:
- Around line 220-228: The side-placement logic in Callouts.ts is overwriting
the previously clamped left position for the callout when neither horizontal
side has enough space. Update the placement branch that sets callout.style.left
so it preserves the clamped value or reapplies clamping after the final left
calculation, using the existing positioning logic around the isRtl and
distanceToLeft/distanceToRight checks. This should prevent negative or
off-viewport left offsets in the layout path that also adjusts
scrollContainer.style.maxHeight and callout.style.top.
- Around line 265-271: Do not suppress the first ResizeObserver notification in
Callouts._calloutResizeObserver, because it can already represent the first
post-open size change; remove the initial-flag gating in the resize callback and
let the observer call Callouts.reposition() on the first notification as well so
the callout updates immediately after opening.
- Around line 255-277: `Callouts.clear()` can leave the active ResizeObserver
attached because it goes through `replaceCurrent()` without disconnecting first.
Update the `replaceCurrent()` path in `Callouts` to call
`unobserveCalloutResize()` before clearing/replacing the current callout, so
`_calloutResizeObserver` is always disconnected when the current callout is
removed. Keep the existing cleanup in `reset()` and reuse the same helper for
consistency.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: cc6c5789-7bf8-4583-beee-124a8692dab0
📒 Files selected for processing (1)
src/BlazorUI/Bit.BlazorUI/Scripts/Callouts.ts
| } else if ((isRtl ? distanceToLeft : distanceToRight) >= calloutWidth) { | ||
| callout.style.bottom = (layoutHeight - visibleBottom + 2) + 'px'; | ||
| callout.style.left = (isRtl ? (componentX - calloutWidth - 1) : (componentX + componentWidth + 1)) + 'px'; | ||
| callout.style.left = ((isRtl ? (componentX - calloutWidth - 1) : (componentX + componentWidth + 1)) + offsetLeft) + 'px'; | ||
| scrollContainer.style.maxHeight = Math.max(0, visualHeight - scrollOffset - headerHeight - footerHeight - 10) + 'px'; | ||
| // Beside the component, anchored to the bottom of the visible area. | ||
| callout.style.top = (Math.max(0, visualHeight - callout.offsetHeight - 2) + offsetTop) + 'px'; | ||
| } else { | ||
| callout.style.bottom = (layoutHeight - visibleBottom + 2) + 'px'; | ||
| callout.style.left = (isRtl ? (componentX + componentWidth + 1) : (componentX - calloutWidth - 1)) + 'px'; | ||
| callout.style.left = ((isRtl ? (componentX + componentWidth + 1) : (componentX - calloutWidth - 1)) + offsetLeft) + 'px'; | ||
| scrollContainer.style.maxHeight = Math.max(0, visualHeight - scrollOffset - headerHeight - footerHeight - 10) + 'px'; | ||
| callout.style.top = (Math.max(0, visualHeight - callout.offsetHeight - 2) + offsetTop) + 'px'; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Clamp side-placement left offsets after overriding the initial value.
Lines 221 and 226 replace the already-clamped left value. If both horizontal sides are too narrow, this can set a negative/off-viewport left, leaving the callout partly inaccessible.
Proposed fix
+ const clampLeft = (value: number) => Math.max(0, Math.min(value, visualWidth - calloutWidth - 3));
+
} else if ((isRtl ? distanceToLeft : distanceToRight) >= calloutWidth) {
- callout.style.left = ((isRtl ? (componentX - calloutWidth - 1) : (componentX + componentWidth + 1)) + offsetLeft) + 'px';
+ callout.style.left = (clampLeft(isRtl ? (componentX - calloutWidth - 1) : (componentX + componentWidth + 1)) + offsetLeft) + 'px';
scrollContainer.style.maxHeight = Math.max(0, visualHeight - scrollOffset - headerHeight - footerHeight - 10) + 'px';
// Beside the component, anchored to the bottom of the visible area.
callout.style.top = (Math.max(0, visualHeight - callout.offsetHeight - 2) + offsetTop) + 'px';
} else {
- callout.style.left = ((isRtl ? (componentX + componentWidth + 1) : (componentX - calloutWidth - 1)) + offsetLeft) + 'px';
+ callout.style.left = (clampLeft(isRtl ? (componentX + componentWidth + 1) : (componentX - calloutWidth - 1)) + offsetLeft) + 'px';
scrollContainer.style.maxHeight = Math.max(0, visualHeight - scrollOffset - headerHeight - footerHeight - 10) + 'px';
callout.style.top = (Math.max(0, visualHeight - callout.offsetHeight - 2) + offsetTop) + 'px';🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/BlazorUI/Bit.BlazorUI/Scripts/Callouts.ts` around lines 220 - 228, The
side-placement logic in Callouts.ts is overwriting the previously clamped left
position for the callout when neither horizontal side has enough space. Update
the placement branch that sets callout.style.left so it preserves the clamped
value or reapplies clamping after the final left calculation, using the existing
positioning logic around the isRtl and distanceToLeft/distanceToRight checks.
This should prevent negative or off-viewport left offsets in the layout path
that also adjusts scrollContainer.style.maxHeight and callout.style.top.
| Callouts.unobserveCalloutResize(); | ||
| } | ||
|
|
||
| // Watches the currently open callout for size changes (content updates) and re-runs | ||
| // positioning so its anchored edge stays glued to the component. A single observer is | ||
| // kept for whichever callout is currently open. | ||
| private static observeCalloutResize(callout: HTMLElement) { | ||
| Callouts.unobserveCalloutResize(); | ||
| if (typeof ResizeObserver === 'undefined') return; | ||
|
|
||
| // Skip the initial synchronous callback (the callout was just positioned); only react | ||
| // to subsequent size changes to avoid an unnecessary reposition right after opening. | ||
| let initial = true; | ||
| Callouts._calloutResizeObserver = new ResizeObserver(() => { | ||
| if (initial) { initial = false; return; } | ||
| Callouts.reposition(); | ||
| }); | ||
| Callouts._calloutResizeObserver.observe(callout); | ||
| } | ||
|
|
||
| private static unobserveCalloutResize() { | ||
| Callouts._calloutResizeObserver?.disconnect(); | ||
| Callouts._calloutResizeObserver = null; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Disconnect the observer when the current callout is cleared through replaceCurrent().
reset() now disconnects the observer, but clear() calls replaceCurrent() directly. That path can leave _calloutResizeObserver holding a detached callout until another reset/open occurs.
Proposed fix
//close the previous one
if (callout.calloutId !== current.calloutId) {
+ Callouts.unobserveCalloutResize();
+ Callouts._currentParams = null;
+
const previousCallout = document.getElementById(current.calloutId);
if (previousCallout) {
previousCallout.style.display = 'none';🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/BlazorUI/Bit.BlazorUI/Scripts/Callouts.ts` around lines 255 - 277,
`Callouts.clear()` can leave the active ResizeObserver attached because it goes
through `replaceCurrent()` without disconnecting first. Update the
`replaceCurrent()` path in `Callouts` to call `unobserveCalloutResize()` before
clearing/replacing the current callout, so `_calloutResizeObserver` is always
disconnected when the current callout is removed. Keep the existing cleanup in
`reset()` and reuse the same helper for consistency.
| // Skip the initial synchronous callback (the callout was just positioned); only react | ||
| // to subsequent size changes to avoid an unnecessary reposition right after opening. | ||
| let initial = true; | ||
| Callouts._calloutResizeObserver = new ResizeObserver(() => { | ||
| if (initial) { initial = false; return; } | ||
| Callouts.reposition(); | ||
| }); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major
🧩 Analysis chain
🌐 Web query:
When is the first ResizeObserver callback delivered after observe(), and can element size changes before that callback be coalesced into the first notification?
💡 Result:
First callback timing after observe: ResizeObserver callbacks are delivered by the “broadcast active resize observations” / “deliver resize loop error notification” steps in the ResizeObserver specification, and the delivery happens before paint (i.e., after layout, before the frame is presented). This means the first notification for an already-rendered, non-zero-size target can be delivered in the same frame as the observe call, but still before paint [1][2][3]. Can size changes before that first callback be coalesced into the first notification? - Yes, in general: the spec’s “broadcast active resize observations” gathers active targets and creates entries from “active observations” just before invoking each observer’s callback; it also loops to “deliver all pending notifications” until no pending notifications remain. If the element’s size changes again before the callback is invoked (while those changes are still pending for that observation delivery cycle), the notification mechanism can include the latest measured sizes in the entries delivered by that first callback [3][1]. - More specifically about coalescing vs deferral: ResizeObserver may deliver multiple iterations within the same overall frame until it can’t deliver more without risking infinite resize loops. During this process, cyclic/recursive notifications are constrained by DOM-depth; notifications that don’t meet the depth condition are deferred to later paint/frames rather than being delivered in the same callback wave [1][2][3]. So “coalescing” can happen within the same delivery loop for eligible targets, but some resize changes may be deferred to a later frame depending on dependency depth (to prevent infinite loops) [1][2].
Citations:
- 1: https://www.w3.org/TR/resize-observer/
- 2: https://developer.mozilla.org/en-US/docs/Web/API/ResizeObserver
- 3: https://drafts.csswg.org/resize-observer-1/
🏁 Script executed:
sed -n '220,310p' src/BlazorUI/Bit.BlazorUI/Scripts/Callouts.tsRepository: bitfoundation/bitplatform
Length of output: 5224
Don’t skip the first ResizeObserver callback. It’s async, so the first notification can already include the first post-open size change; suppressing it can leave the callout positioned for the old size until another resize occurs.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/BlazorUI/Bit.BlazorUI/Scripts/Callouts.ts` around lines 265 - 271, Do not
suppress the first ResizeObserver notification in
Callouts._calloutResizeObserver, because it can already represent the first
post-open size change; remove the initial-flag gating in the resize callback and
let the observer call Callouts.reposition() on the first notification as well so
the callout updates immediately after opening.
1 participant
closes #12529
Summary by CodeRabbit
New Features
Bug Fixes