fix: flicker-free mobile formatting toolbar via CSS custom properties#2617
fix: flicker-free mobile formatting toolbar via CSS custom properties#2617Movm wants to merge 2 commits intoTypeCellOS:mainfrom
Conversation
Replace React state-driven positioning with CSS custom property (--bn-mobile-keyboard-offset) for zero re-render toolbar positioning. Two-tier keyboard detection: 1. VirtualKeyboard API (Chrome/Edge 94+) — exact geometry, no delay 2. Visual Viewport API fallback (Safari iOS 13+, Firefox 68+) — with focus-based prediction for instant initial positioning Additional improvements: - Auto-scroll selection into view when keyboard opens - touch-action: pan-x for horizontal toolbar scrolling - env(safe-area-inset-bottom) for notch/home indicator handling - Smooth 150ms CSS transition instead of React re-renders Closes TypeCellOS#2616
|
@Movm is attempting to deploy a commit to the TypeCell Team on Vercel. A member of the Team first needs to authorize it. |
📝 WalkthroughWalkthroughReplaced React state-driven positioning with direct DOM mutation of CSS custom property Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
packages/react/src/components/FormattingToolbar/ExperimentalMobileFormattingToolbarController.tsx (2)
120-122:focusoutmay briefly flash toolbar to bottom when focus moves between inputs.When the user taps from one input to another,
focusoutfires beforefocusin, causing a momentary reset to0before the nextfocusinrestores the offset. This could cause a brief visual jump.Consider using a small delay or checking
relatedTargetto avoid resetting when focus moves to another editable element.Proposed fix using relatedTarget check
const onFocusOut = (e: FocusEvent) => { + const related = e.relatedTarget as HTMLElement | null; + if ( + related && + (related.isContentEditable || + related.tagName === "INPUT" || + related.tagName === "TEXTAREA") + ) { + return; // Focus moving to another input, don't reset + } setOffset(0); };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react/src/components/FormattingToolbar/ExperimentalMobileFormattingToolbarController.tsx` around lines 120 - 122, The onFocusOut handler currently unconditionally calls setOffset(0) on the focusout event which can cause a brief visual jump when focus moves between inputs; modify onFocusOut to inspect the event.relatedTarget (or use a tiny debounce) and only call setOffset(0) when relatedTarget is null or not an editable element (or after a short timeout if relatedTarget is unavailable), so transitions between editable fields in ExperimentalMobileFormattingToolbarController do not reset the offset prematurely.
73-90: ResetoverlaysContenton cleanup to prevent unexpected behavior in single-page apps.The
overlaysContentproperty persists at the document level even after component unmount. While not strictly required by the spec, explicitly resetting it tofalseon cleanup is defensive best practice—especially for single-page apps, where a known Chromium bug may fail to clear the state across virtual navigations.Proposed fix
return () => { + vk.overlaysContent = false; vk.removeEventListener("geometrychange", onGeometryChange); document.removeEventListener("selectionchange", onSelectionChange); clearTimeout(scrollTimer); };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react/src/components/FormattingToolbar/ExperimentalMobileFormattingToolbarController.tsx` around lines 73 - 90, The VirtualKeyboard handler sets vk.overlaysContent = true but never restores it; update ExperimentalMobileFormattingToolbarController.tsx to capture the previous value (const previousOverlays = vk.overlaysContent) before setting it, and in the cleanup returned from the vk branch (the same place that calls vk.removeEventListener and document.removeEventListener) restore vk.overlaysContent = previousOverlays (or false if you prefer a defensive reset) to avoid leaking the overlaysContent state across navigations; reference the vk variable and the existing onGeometryChange/removeEventListener cleanup when applying the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@packages/react/src/components/FormattingToolbar/ExperimentalMobileFormattingToolbarController.tsx`:
- Around line 120-122: The onFocusOut handler currently unconditionally calls
setOffset(0) on the focusout event which can cause a brief visual jump when
focus moves between inputs; modify onFocusOut to inspect the event.relatedTarget
(or use a tiny debounce) and only call setOffset(0) when relatedTarget is null
or not an editable element (or after a short timeout if relatedTarget is
unavailable), so transitions between editable fields in
ExperimentalMobileFormattingToolbarController do not reset the offset
prematurely.
- Around line 73-90: The VirtualKeyboard handler sets vk.overlaysContent = true
but never restores it; update ExperimentalMobileFormattingToolbarController.tsx
to capture the previous value (const previousOverlays = vk.overlaysContent)
before setting it, and in the cleanup returned from the vk branch (the same
place that calls vk.removeEventListener and document.removeEventListener)
restore vk.overlaysContent = previousOverlays (or false if you prefer a
defensive reset) to avoid leaking the overlaysContent state across navigations;
reference the vk variable and the existing onGeometryChange/removeEventListener
cleanup when applying the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 567f3c38-9b91-4702-9f05-5d0d0ced3135
📒 Files selected for processing (2)
packages/react/src/components/FormattingToolbar/ExperimentalMobileFormattingToolbarController.tsxpackages/react/src/editor/styles.css
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
| import { FormattingToolbar } from "./FormattingToolbar.js"; | ||
| import { FormattingToolbarProps } from "./FormattingToolbarProps.js"; | ||
|
|
||
| const TOOLBAR_HEIGHT = 44; |
There was a problem hiding this comment.
I'm not sure that it is a great idea to hard code this. I understand reading it once & then using a cached value, but hard coding upfront feels wrong to me
There was a problem hiding this comment.
Good call — replaced with el.getBoundingClientRect().height || 44 so it measures the actual rendered toolbar at call time. Works correctly with custom formattingToolbar components too.
| const scrollSelectionIntoView = () => { | ||
| const sel = window.getSelection(); | ||
| if (!sel || sel.rangeCount === 0) return; | ||
| const rect = sel.getRangeAt(0).getBoundingClientRect(); | ||
| const vp = window.visualViewport; | ||
| if (!vp) return; | ||
| const visibleBottom = vp.offsetTop + vp.height - TOOLBAR_HEIGHT; | ||
| if (rect.bottom > visibleBottom) { | ||
| window.scrollBy({ | ||
| top: rect.bottom - visibleBottom + 16, | ||
| behavior: "smooth", | ||
| }); | ||
| } else if (rect.top < vp.offsetTop) { | ||
| window.scrollBy({ | ||
| top: rect.top - vp.offsetTop - 16, | ||
| behavior: "smooth", | ||
| }); | ||
| } | ||
| }; |
There was a problem hiding this comment.
Unclear to me why we would need to be scrolling for this?
There was a problem hiding this comment.
When the keyboard opens, the visible area shrinks. If the cursor is near the bottom, it ends up hidden behind the keyboard + toolbar. This scrolls it back into view so users can see what they're typing. That said — happy to split this into a separate PR if you'd prefer to keep this one focused on the flicker fix.
|
Thanks for looking into this, @Movm I agree that we likely can move a lot of this computation into CSS & avoid re-renders. This seems like a good start for that |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
packages/react/src/components/FormattingToolbar/ExperimentalMobileFormattingToolbarController.tsx (2)
97-104: Document the 50px keyboard height threshold.The
> 50threshold filters out small viewport variations (like address bar hide/show) vs actual keyboard appearance. Consider extracting to a named constant with a brief comment explaining its purpose.📝 Suggested documentation
+ // Minimum height to consider as a "real" keyboard (filters out address bar changes) + const MIN_KEYBOARD_HEIGHT = 50; + const update = () => { const layoutHeight = document.documentElement.clientHeight; const keyboardHeight = layoutHeight - vp.height - vp.offsetTop; - if (keyboardHeight > 50) lastKnownKeyboardHeight = keyboardHeight; + if (keyboardHeight > MIN_KEYBOARD_HEIGHT) lastKnownKeyboardHeight = keyboardHeight; setOffset(keyboardHeight);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react/src/components/FormattingToolbar/ExperimentalMobileFormattingToolbarController.tsx` around lines 97 - 104, Extract the magic number 50 into a named constant (e.g., KEYBOARD_HEIGHT_THRESHOLD = 50) with a short comment explaining it distinguishes small viewport changes (address bar hide/show) from an actual keyboard, then replace the inline check in the update function (the block referencing update, lastKnownKeyboardHeight, setOffset, scrollTimer, scrollSelectionIntoView) to use that constant (if (keyboardHeight > KEYBOARD_HEIGHT_THRESHOLD) ...) so the intent is documented and easy to adjust.
57-57: Consider deriving toolbar height fallback from CSS or a constant.The
44magic number as fallback whengetBoundingClientRect().heightreturns 0 addresses a legitimate edge case, but a hardcoded pixel value can become stale if the toolbar styling changes. Consider defining this as a named constant with a comment explaining its origin, or measuring it once from a reference element.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react/src/components/FormattingToolbar/ExperimentalMobileFormattingToolbarController.tsx` at line 57, Replace the magic fallback "44" used when computing toolbarHeight in ExperimentalMobileFormattingToolbarController.tsx with a named constant or derived value; introduce a constant like TOOLBAR_FALLBACK_HEIGHT (with a comment describing its origin or relation to the CSS) or measure the height from a hidden/reference element/CSS variable at mount, then use that constant/derived value instead of the literal 44 when computing toolbarHeight from el.getBoundingClientRect().height.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@packages/react/src/components/FormattingToolbar/ExperimentalMobileFormattingToolbarController.tsx`:
- Around line 97-104: Extract the magic number 50 into a named constant (e.g.,
KEYBOARD_HEIGHT_THRESHOLD = 50) with a short comment explaining it distinguishes
small viewport changes (address bar hide/show) from an actual keyboard, then
replace the inline check in the update function (the block referencing update,
lastKnownKeyboardHeight, setOffset, scrollTimer, scrollSelectionIntoView) to use
that constant (if (keyboardHeight > KEYBOARD_HEIGHT_THRESHOLD) ...) so the
intent is documented and easy to adjust.
- Line 57: Replace the magic fallback "44" used when computing toolbarHeight in
ExperimentalMobileFormattingToolbarController.tsx with a named constant or
derived value; introduce a constant like TOOLBAR_FALLBACK_HEIGHT (with a comment
describing its origin or relation to the CSS) or measure the height from a
hidden/reference element/CSS variable at mount, then use that constant/derived
value instead of the literal 44 when computing toolbarHeight from
el.getBoundingClientRect().height.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2b247c32-1a7f-4b2f-b5d0-522992e97f1f
📒 Files selected for processing (1)
packages/react/src/components/FormattingToolbar/ExperimentalMobileFormattingToolbarController.tsx
2 participants
Summary
Replaces the React state-driven positioning in
ExperimentalMobileFormattingToolbarControllerwith a CSS custom property (--bn-mobile-keyboard-offset), eliminating the re-render storm that causes visible flickering during keyboard animation.Tested on iOS Safari (iPhone) — the Visual Viewport API fallback works great, smooth positioning with no flicker.
Relates to #938, #2122, #2616
Rationale
The current implementation calls
setTransform()→ React re-render on everyvisualViewportscroll/resize event. During keyboard animation this fires dozens of times per second, causing visible toolbar flickering. Moving positioning to a CSS custom property lets the browser compositor handle animation viatransition: bottom 0.15s ease-out— zero React re-renders for positioning.Changes
ExperimentalMobileFormattingToolbarController.tsxTwo-tier keyboard detection (progressive enhancement):
overlaysContent = trueand listens togeometrychangefor exact keyboard bounding rect before animation startsclientHeight - vp.height - vp.offsetTop, with focus-based prediction that immediately applies the last-known keyboard height onfocusinto avoid toolbar jumping after the keyboard animates inBoth tiers write a single CSS custom property
--bn-mobile-keyboard-offsetdirectly to the wrapper DOM element — nouseState, no re-renders.Additional improvements:
scrollSelectionIntoView()— auto-scrolls the cursor/selection into view when the keyboard opens, accounting for toolbar heightfocusoutlistener resets offset to 0 when keyboard dismissesstyles.cssNew
.bn-mobile-formatting-toolbarclass:bottom: var(--bn-mobile-keyboard-offset, 0px)— positions above keyboardtransition: bottom 0.15s ease-out— smooth animationtouch-action: pan-x— allows horizontal scrolling on toolbar buttons without vertical scroll interferencepadding-bottom: env(safe-area-inset-bottom)— handles notch/home indicator on modern devicesBrowser support
Note on
interactive-widgetFor best results, consumers can add
interactive-widget=resizes-contentto their viewport meta tag. This makesposition: fixedelements work naturally with the keyboard. However, the implementation works without it — the Visual Viewport fallback handles both cases.Summary by CodeRabbit