fix(mobile): fixed toolbar position #14329
Conversation
- 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
|
|
📝 WalkthroughWalkthroughAdds mobile-specific toolbar behavior: when running on mobile the toolbar is appended to the shadow DOM, fixed to the bottom, hidden by default, and shown/positioned based on text selection and Visual Viewport resize/scroll (keyboard) events; desktop behavior is preserved. 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: check selection -> request show
VisualViewport->>Toolbar: provide viewport height/offset (resize/scroll)
Toolbar->>Toolbar: updatePosition() (compute bottom offset)
Toolbar->>Document: set style (position: fixed, bottom: Xpx), render content, show
User->>Document: dismiss keyboard or clear selection
Document->>Toolbar: request hide
Toolbar->>Toolbar: hide, cleanup listeners as needed
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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
🤖 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.
- 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
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## canary #14329 +/- ##
==========================================
- Coverage 56.05% 55.62% -0.43%
==========================================
Files 2809 2809
Lines 147733 147768 +35
Branches 22216 22119 -97
==========================================
- Hits 82805 82193 -612
- Misses 62295 62924 +629
- Partials 2633 2651 +18
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
|
I've updated the description to include screenshots of the problem I'm having on mobile |
2 participants
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.