[ew] refactor toolbar - #1175
Open
hannessolo wants to merge 9 commits into
Open
Conversation
…t active surface
The toolbar's visibility was derived from the doc view's `view.hasFocus()` while
that same value was faked to keep collab awareness working in WYSIWYG mode — a
self-contradiction that made the toolbar flicker, drop out, or fail to appear
(especially on the WYSIWYG side of split view).
Introduce a single ToolbarController that owns visibility, derived once per frame
from an explicit active surface ('doc' | 'wysiwyg') plus selection/mode — never
from focus. Editors emit intent; nothing else shows/hides the toolbar.
Key fixes:
- Null `cursor-move` (a da-nx per-block blur) is awareness-only, no longer hides
the toolbar — the dominant drop-out.
- Detect focus entering the cross-origin iframe via `window` blur + shadow-piercing
active element, since the iframe's own focus events don't fire and da-nx sends no
message when a click doesn't change the block selection (e.g. end of a line).
- The focus lie is scoped to mirror dispatches (collab awareness only) and never
read by the visibility layer.
- Coalesce visibility updates to one requestAnimationFrame render.
Design and investigation notes in docs/canvas-toolbar-architecture.md.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
y-prosemirror's cursor plugin broadcasts this user's caret to collaborators only while the doc view "has focus", and clears it on the next update once focus is lost. Its updateCursorInfo is the plugin's view.update callback, so it runs after every transaction. A focus lie scoped to a single mirror dispatch therefore only survived one tick: the next unrelated update (a remote edit, the iframe streaming into Yjs, or the redraw our own setLocalStateField triggers) ran with the real hasFocus() === false and wiped the just-broadcast cursor. The caret flashed to peers and vanished. Gate the lie on the active surface instead: while activeSurface === 'wysiwyg' the doc view reports focus, so the cursor keeps broadcasting for the whole time the iframe owns editing (layout and split). When the user leaves, the real check returns and the cursor is correctly cleared. The lie makes selectionToDOM treat the doc view as owning the selection, but that only writes a DOM selection range (no focus move). The one thing that would steal focus is view.focus(), so neuter it while the iframe is active — no caller (drop handler, command, restoreFocus) can pull focus off the iframe and revive the toolbar-visibility bugs. dispatchWithFakeFocus is retained as a complement for the instant before the surface flips. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The WYSIWYG iframe owns block-level structure, so the shared selection toolbar now renders a surface-appropriate button set. The controller pushes the active surface onto the element, and the element gates its sections: - doc surface: full toolbar (block-type picker, marks, structure, tables, links, images incl. add-image) - wysiwyg surface: inline marks, link controls, and image alt-text editing only — the block-type picker, list/structure, table controls, and the add-image action are dropped (block insertion and block structure belong to the iframe) In split view the button set follows the pane you're editing. Ports the per-surface intent from the earlier PR #1018 into the current architecture. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
handleCursorMove preserved state.storedMarks whenever the new position had no adjacent marks, so moving the caret off bold text left bold queued on unmarked text. That preservation was only meant to survive the same-position cursor-move the iframe re-reports right after a toolbar toggle — not a real move. Track the last cursor offset and branch on it: a genuine move resets stored marks to what's at the new location (nothing on unmarked text), while a same-position re-report keeps a toolbar-toggled mark until the user types or actually moves. Also forget the position on iframe blur so re-entry counts as a move. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
Author
|
Replaces #1018 |
getSelectionToolbar() was a one-line pass-through to toolbarController.ensureToolbar(); production code already calls the controller directly. Remove it and point the remaining test at ensureToolbar(). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
hannessolo
marked this pull request as ready for review
July 24, 2026 10:22
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
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
2 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.
https://tbctrl--da-live--adobe.aem.live