fix: implement editor-owned select-all#2912
Merged
Merged
Conversation
🦋 Changeset detectedLatest commit: 92f9f37 The changes in this PR will be included in the next version bump. This PR includes changesets to release 14 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
📦 Bundle Stats —
|
| Metric | Value | vs main (59d03e2) |
|---|---|---|
| Internal (raw) | 800.7 KB | +1.6 KB, +0.2% |
| Internal (gzip) | 153.1 KB | +445 B, +0.3% |
| Bundled (raw) | 1.41 MB | +1.7 KB, +0.1% |
| Bundled (gzip) | 317.3 KB | +442 B, +0.1% |
| Import time | 95ms | -2ms, -1.9% |
@portabletext/editor/behaviors
| Metric | Value | vs main (59d03e2) |
|---|---|---|
| Internal (raw) | 467 B | - |
| Internal (gzip) | 207 B | - |
| Bundled (raw) | 424 B | - |
| Bundled (gzip) | 171 B | - |
| Import time | 2ms | -0ms, -2.0% |
@portabletext/editor/plugins
| Metric | Value | vs main (59d03e2) |
|---|---|---|
| Internal (raw) | 2.7 KB | - |
| Internal (gzip) | 894 B | - |
| Bundled (raw) | 2.5 KB | - |
| Bundled (gzip) | 827 B | - |
| Import time | 7ms | -0ms, -0.5% |
@portabletext/editor/selectors
| Metric | Value | vs main (59d03e2) |
|---|---|---|
| Internal (raw) | 80.1 KB | - |
| Internal (gzip) | 14.7 KB | - |
| Bundled (raw) | 75.5 KB | - |
| Bundled (gzip) | 13.5 KB | - |
| Import time | 8ms | +0ms, +0.9% |
@portabletext/editor/traversal
| Metric | Value | vs main (59d03e2) |
|---|---|---|
| Internal (raw) | 26.9 KB | - |
| Internal (gzip) | 5.3 KB | - |
| Bundled (raw) | 26.7 KB | - |
| Bundled (gzip) | 5.2 KB | - |
| Import time | 6ms | -0ms, -1.1% |
@portabletext/editor/utils
| Metric | Value | vs main (59d03e2) |
|---|---|---|
| Internal (raw) | 29.7 KB | - |
| Internal (gzip) | 6.2 KB | - |
| Bundled (raw) | 27.1 KB | - |
| Bundled (gzip) | 5.8 KB | - |
| Import time | 6ms | +0ms, +1.3% |
🗺️ . · ./behaviors · ./plugins · ./selectors · ./traversal · ./utils · Artifacts
Details
- Import time regressions over 10% are flagged with
⚠️ - Sizes shown as raw / gzip 🗜️. Internal bytes = own code only. Total bytes = with all dependencies. Import time = Node.js cold-start median.
📦 Bundle Stats — @portabletext/markdown
Compared against main (59d03e2e)
| Metric | Value | vs main (59d03e2) |
|---|---|---|
| Internal (raw) | 53.8 KB | - |
| Internal (gzip) | 9.8 KB | - |
| Bundled (raw) | 348.9 KB | - |
| Bundled (gzip) | 96.3 KB | - |
| Import time | 39ms | +1ms, +3.2% |
🗺️ View treemap · Artifacts
Details
- Import time regressions over 10% are flagged with
⚠️ - Sizes shown as raw / gzip 🗜️. Internal bytes = own code only. Total bytes = with all dependencies. Import time = Node.js cold-start median.
christianhg
force-pushed
the
fix-editor-owned-select-all
branch
from
9ffbb9c to
93c6376
Compare
christianhg
force-pushed
the
fix-editor-owned-select-all
branch
from
93c6376 to
f266024
Compare
christianhg
force-pushed
the
fix-editor-owned-select-all
branch
from
f266024 to
dc95ba3
Compare
christianhg
force-pushed
the
fix-editor-owned-select-all
branch
from
dc95ba3 to
7888032
Compare
Select-all was left entirely to the browser: the native gesture wrote a
DOM selection and the `selectionchange` sync reverse-engineered it into
the model. Chromium builds an already-collapsed range whenever a
non-editable element sits at either content edge of the editing host
(fine in the middle, and nesting inside editable wrappers does not
help). Block objects render `contentEditable={false}` and table renders
carry non-editable chrome, so any document starting or ending with a
void block, and any table-only document, had a select-all no-op: native
select-all emits the collapsed range, the sync faithfully applies it,
and the writeback erases whatever the browser briefly showed.
"Everything" is a model-level statement, so the editor now owns the
gesture: a `selectAll` shortcut joins `defaultKeyboardShortcuts`
(Ctrl+A, Cmd+A on Apple platforms) and an abstract keyboard behavior
raises `select` over the document range, computed from the model via
`getLeaf` edge descent from the root and
`getBlockStartPoint`/`getBlockEndPoint` on the enclosing blocks. This
is the position structured-content editors converge on (ProseMirror
binds Mod-a to its own `selectAll` command for the same reason). Being
an abstract behavior, consumers can override it, e.g. for
spreadsheet-style cell-first escalation.
Net behavior for plain text documents is unchanged in outcome: the
keydown is now consumed and the range computed from the model instead
of trusted from the DOM. The existing cross-container suite exercising
real select-all chords passes without modification.
christianhg
force-pushed
the
fix-editor-owned-select-all
branch
from
7888032 to
5394a7b
Compare
…ection `ControlOrMeta` resolves from the host OS while the shortcut guard's `IS_APPLE` resolves from the user agent, and playwright's webkit reports a Mac user agent on Linux, so the chord missed the guard there and the test passed only via native select-all. Align with the `IS_MAC` chord idiom the other shortcut tests use.
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.
A field report: with a table as the document's only block, Cmd+A does nothing. The event log shows the
keyboard.keydownarrive and then silence, no syntheticselect, no sync, and the caret ends up parked at the table's start.The diagnosis exonerates the engine. A synchronous capture of
document.execCommand('selectAll')over the playground's table DOM shows chromium producing an already-collapsed range (div.playground-table-inner@0 -> @0); theselectionchangesync then faithfully applies the collapse and the writeback erases whatever the browser briefly rendered. Bisecting the DOM ingredients isolated the trigger, and it is broader than tables: chromium's select-all collapses whenever a non-editable element sits at either content edge of the editing host (fine in the middle, and nesting inside editable wrappers does not help; a<table>on its own,<colgroup>, and plain wrapper divs are all innocent). Block objects rendercontentEditable={false}and table renders carry non-editable selection chrome, so any document starting or ending with a void block, not just table-only documents, has had a native select-all no-op in chromium all along. The leafless-subtree fix (#2899) is unrelated: probes against its build behave identically, because no range ever exists for it to resolve.The fix stops trusting the browser with the gesture. "Select everything" is a model-level statement, and round-tripping it through a DOM selection only inherits browser quirks, so the editor now owns the shortcut, the position structured-content editors converge on (ProseMirror binds
Mod-ato its ownselectAllcommand for the same reason). AselectAllentry joinsdefaultKeyboardShortcuts(Ctrl+A, Cmd+A on Apple platforms), and an abstract keyboard behavior beside the undo/redo shortcuts raisesselectover the full document range, computed withgetLeafedge descent from the root plusgetBlockStartPoint/getBlockEndPointon the enclosing blocks, following thebehavior.core.insert-break.tsidiom. The end point matters: a container's block-level point edge-descends to its start duringselectresolution, so the behavior computes leaf-precise points rather than leaning on coercion; the table-only test pins the focus landing at the end of the last cell. Landing as an abstract behavior keeps it consumer-overridable, which leaves the door open for spreadsheet-style escalation (first Cmd+A selects the cell, then the table, then the document) as a plugin behavior later.The regression test renders containers with real
<table>DOM plus a non-editable chrome sibling, reproducing the browser's failure mode inside the harness (with a plain-div render, native select-all works and the test could never go red). A second test pins the block-objects-only document (voids at both content edges, the other real-world instance of the trigger), and a third pins plain-text documents. Net behavior for text documents is unchanged in outcome, only the mechanism shifts from DOM luck to a computed range; the existing cross-container suite, which exercises real select-all chords, passes without modification.