fix(mobile): fixed toolbar position#14329
Merged
darkskygit merged 4 commits intotoeverything:canaryfrom Feb 23, 2026
Merged
Conversation
|
|
Contributor
📝 WalkthroughWalkthroughAdds mobile-specific toolbar behavior: on mobile the toolbar is appended to the shadow DOM, fixed at the bottom, hidden by default, and shown/positioned based on selection and Visual Viewport (keyboard) resize/scroll; desktop behavior remains unchanged. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Document
participant VisualViewport
participant Toolbar
User->>Document: touch / select text
Document->>Document: dispatch selectionchange / touchend
Document->>Toolbar: detect selection -> request show/hide
VisualViewport->>Toolbar: resize/scroll events (keyboard)
Toolbar->>Toolbar: updateMobilePosition() -> compute bottom offset
Toolbar->>Document: set styles (fixed, bottom: Xpx), render & show
User->>Document: dismiss keyboard or clear selection
Document->>Toolbar: request hide
Toolbar->>Toolbar: hide, remove listeners as needed
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@blocksuite/affine/widgets/toolbar/src/toolbar.ts`:
- Around line 312-315: The code calls sel.getRangeAt(0) which can throw if
sel.rangeCount is 0; update the selection check to ensure sel.rangeCount > 0
before calling getRangeAt (e.g., compute hasSelection as sel && sel.rangeCount >
0 && !sel.isCollapsed && sel.toString().length > 0) or wrap getRangeAt in a safe
conditional/try-catch; modify the variables in this block (sel, hasSelection,
range) so range is only assigned when the rangeCount guard passes.
- Around line 328-335: The pending timeouts created for selectionchange and
touchend (the local timeout variable and the anonymous setTimeout in
disposables.addFromEvent(host, 'touchend', ...)) are not cleared on component
disconnect, risking updateToolbar running after teardown; store the touchend
timeout reference alongside the existing timeout variable and ensure both are
cleared inside the component's dispose/cleanup (where disposables are disposed)
by calling clearTimeout on each reference before nulling them so updateToolbar
is never invoked after disconnect.
- Around line 275-287: The toolbar loses theme-specific styles when appended to
document.body because darkToolbarStyles/lightToolbarStyles are scoped inside
AffineToolbarWidget's shadow DOM and no data-app-theme is set on the toolbar;
update the logic that appends the toolbar (the code that sets styles and calls
document.body.append(toolbar)) to either set the toolbar's data-app-theme
attribute to the current theme (e.g., copy from AffineToolbarWidget or document
root) or inject the darkToolbarStyles/lightToolbarStyles into document.head so
the toolbar can pick up theme rules; locate references to AffineToolbarWidget,
darkToolbarStyles, lightToolbarStyles, and the toolbar element to implement the
chosen fix.
🧹 Nitpick comments (1)
blocksuite/affine/widgets/toolbar/src/toolbar.ts (1)
272-338: Consider consolidating mobile checks to avoid unnecessary flag operations.The current implementation toggles flags (e.g.,
Flag.Text,Flag.Block) on mobile even though the effects that consume them early-return. While this works correctly, it results in unnecessary signal updates and subscription callbacks.For a cleaner separation, consider wrapping the flag-dependent subscription blocks (lines 342-591) in a
!IS_MOBILEcheck, or use a more unified approach where mobile has its own dedicated handlers.This is not blocking since the current approach is functionally correct.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## canary #14329 +/- ##
==========================================
+ Coverage 49.37% 56.99% +7.61%
==========================================
Files 2558 2861 +303
Lines 145614 154768 +9154
Branches 20939 23256 +2317
==========================================
+ Hits 71902 88213 +16311
+ Misses 71727 63626 -8101
- Partials 1985 2929 +944
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:
|
passabilities
changed the title
passabilities
changed the title
Contributor
Author
|
I've updated the description to include screenshots of the problem I'm having on mobile |
- Use fixed positioning at bottom of screen on mobile devices - Handle toolbar visibility directly via selectionchange event - Position toolbar above virtual keyboard using Visual Viewport API - Bypass flag-based rendering system on mobile to avoid rendering issues - Add touchend listener to handle long-press text selection
- Limit toolbar max-width to viewport minus padding - Enable horizontal scrolling for overflow content - Allow pan-x touch action for toolbar scrolling
- Keep toolbar in shadowRoot so theme styles naturally apply - Add top: auto to override default positioning from static styles - Add rangeCount check to prevent DOMException when accessing selection range - Clear timeouts on component disconnect to prevent stale updates
darkskygit
force-pushed
the
fix/mobile-web-toolbar
branch
from
5383703 to
c087613
Compare
darkskygit
approved these changes
Feb 23, 2026
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
blocksuite/affine/widgets/toolbar/src/toolbar.ts (1)
428-433:⚠️ Potential issue | 🟡 MinorMobile toolbar flicker: generic
selectionchangehandler may transiently clearFlag.Textbefore the mobile sync restores it.On mobile, when
std.range.valueisnull(native touch selection not tracked by the BlockSuite range manager):
range?.commonAncestorContainer→undefined, sostd.host.contains(null)returnsfalse.!false = true→flags.toggle(Flag.Text, false)fires synchronously, hiding the toolbar.- The mobile-specific 50 ms-debounced
syncMobileTextSelectionthen re-enablesFlag.Text.Because the toolbar CSS transition is 120 ms, this 50 ms hide→show cycle is likely perceptible.
syncMobileTextSelectionalready handles the "selection outside editor" case via itsinEditorcheck, so this synchronous clear is redundant on mobile.🐛 Proposed fix
// Focues outside: `doc-title` if ( + !IS_MOBILE && flags.check(Flag.Text) && !std.host.contains(range?.commonAncestorContainer ?? null) ) { flags.toggle(Flag.Text, false); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@blocksuite/affine/widgets/toolbar/src/toolbar.ts` around lines 428 - 433, The selectionchange handler is synchronously clearing flags.toggle(Flag.Text, false) when range?.commonAncestorContainer is undefined, which causes a visible hide→show flicker on mobile; change the handler to skip toggling Flag.Text when std.range.value is null on touch/mobile devices (detect via navigator.maxTouchPoints>0 or 'ontouchstart' in window) so that mobile-specific syncMobileTextSelection can handle inEditor logic and re-enable Flag.Text without a transient hide; keep the existing std.host.contains(...) check for non-mobile flows and only short-circuit the toggle when on mobile and std.range.value is null.
🧹 Nitpick comments (2)
blocksuite/affine/widgets/toolbar/src/toolbar.ts (2)
363-370: Optional:scheduleTouchSync'sdelayparameter is always100.
scheduleTouchSyncis called exactly once, with a hard-coded100. The parameter adds no flexibility here.♻️ Proposed simplification
- const scheduleTouchSync = (delay: number) => { + const scheduleTouchSync = () => { if (touchTimeout) clearTimeout(touchTimeout); - touchTimeout = setTimeout(syncMobileTextSelection, delay); + touchTimeout = setTimeout(syncMobileTextSelection, 100); }; ... disposables.addFromEvent(host, 'touchend', () => { - scheduleTouchSync(100); + scheduleTouchSync(); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@blocksuite/affine/widgets/toolbar/src/toolbar.ts` around lines 363 - 370, scheduleTouchSync currently accepts a delay parameter but is always invoked with a hard-coded 100, so remove the unused parameter and simplify the function: change scheduleTouchSync to take no args, have it clear touchTimeout and call setTimeout(syncMobileTextSelection, 100), and keep scheduleSyncMobileTextSelection as-is (uses selectionTimeout and variable delay). Update any callers to call scheduleTouchSync() without arguments and reference touchTimeout, syncMobileTextSelection, and selectionTimeout to locate the functions/vars.
283-296: Optional: factor out the duplicatedisEditingcheck.
isEditingis tested twice; factoring it out makes the intent clearer.♻️ Proposed refactor
if ( dbViewSel && - ((dbViewSel.selectionType === 'area' && dbViewSel.isEditing) || - (dbViewSel.selectionType === 'cell' && dbViewSel.isEditing)) + dbViewSel.isEditing && + (dbViewSel.selectionType === 'area' || dbViewSel.selectionType === 'cell') ) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@blocksuite/affine/widgets/toolbar/src/toolbar.ts` around lines 283 - 296, The isNativeTextSelection function duplicates checks for dbViewSel.isEditing; refactor by extracting a local boolean like isEditing = Boolean(dbViewSel?.isEditing) (or const isEditing = dbViewSel?.isEditing) and then use that single variable in the conditional that tests dbViewSel.selectionType === 'area' || dbViewSel.selectionType === 'cell' to simplify the logic; keep the subsequent TableSelection check (std.selection.find(TableSelection)?.data) unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@blocksuite/affine/widgets/toolbar/src/toolbar.ts`:
- Around line 428-433: The selectionchange handler is synchronously clearing
flags.toggle(Flag.Text, false) when range?.commonAncestorContainer is undefined,
which causes a visible hide→show flicker on mobile; change the handler to skip
toggling Flag.Text when std.range.value is null on touch/mobile devices (detect
via navigator.maxTouchPoints>0 or 'ontouchstart' in window) so that
mobile-specific syncMobileTextSelection can handle inEditor logic and re-enable
Flag.Text without a transient hide; keep the existing std.host.contains(...)
check for non-mobile flows and only short-circuit the toggle when on mobile and
std.range.value is null.
---
Nitpick comments:
In `@blocksuite/affine/widgets/toolbar/src/toolbar.ts`:
- Around line 363-370: scheduleTouchSync currently accepts a delay parameter but
is always invoked with a hard-coded 100, so remove the unused parameter and
simplify the function: change scheduleTouchSync to take no args, have it clear
touchTimeout and call setTimeout(syncMobileTextSelection, 100), and keep
scheduleSyncMobileTextSelection as-is (uses selectionTimeout and variable
delay). Update any callers to call scheduleTouchSync() without arguments and
reference touchTimeout, syncMobileTextSelection, and selectionTimeout to locate
the functions/vars.
- Around line 283-296: The isNativeTextSelection function duplicates checks for
dbViewSel.isEditing; refactor by extracting a local boolean like isEditing =
Boolean(dbViewSel?.isEditing) (or const isEditing = dbViewSel?.isEditing) and
then use that single variable in the conditional that tests
dbViewSel.selectionType === 'area' || dbViewSel.selectionType === 'cell' to
simplify the logic; keep the subsequent TableSelection check
(std.selection.find(TableSelection)?.data) unchanged.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
blocksuite/affine/widgets/toolbar/src/toolbar.ts
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
3 participants
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
Fixed the text formatting toolbar not working properly on mobile web browsers.
Problem
The toolbar had multiple issues on mobile devices:
Solution
selectionchangeeventtouchendlistener to handle long-press text selectionTest plan
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.