fix: flicker-free mobile formatting toolbar via CSS custom properties#2617
Open
Movm wants to merge 1 commit intoTypeCellOS:mainfrom
Open
fix: flicker-free mobile formatting toolbar via CSS custom properties#2617Movm wants to merge 1 commit intoTypeCellOS:mainfrom
Movm wants to merge 1 commit 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. |
📝 WalkthroughWalkthroughRefactored mobile formatting toolbar positioning from React state-driven transforms to CSS custom property mutations, implementing enhanced keyboard detection via the Virtual Keyboard API with fallback to Visual Viewport computation, and added selection-aware scroll adjustments to maintain visibility. Changes
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
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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