Repository navigation
Render block-move pending state - #77979
adamsilverstein wants to merge 14 commits into
Conversation
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
adamsilverstein
added
[Feature] Notes
|
Size Change: +534 B (+0.01%) Total Size: 7.52 MB 📦 View Changed
|
|
Flaky tests detected in 0cb3aee. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/28139684210
|
Current approach: uniform Google Docs green + decoration-driven previewThe pending-suggestion canvas treatment was just simplified to a single suggestion color (
These rules now live in Why this is enough for most casesThe combination of (a) a saturated, semantically loaded color and (b) a decoration that's specific to the op type (strikethrough / dashed / dotted + label) gives the reviewer two independent signals: "this is a suggestion" and "this is the kind of suggestion it is." Both are visible at a glance without resolving any author identity, which means it works correctly even when the suggester is offline or anonymous. Possible follow-up: per-author tintingA nicer refinement (closer to actual Google Docs behavior) would tint each suggestion in the suggester's own color, the same color their cursor and avatar already use during live collaboration. That gives the reviewer a third signal — who suggested this — without an extra UI element to read. The infrastructure already exists: the collab presence system writes
Why it might be worth it
Holding off on this for now; flagging here so we can pick it up after the structural-suggestion stack lands. |
There was a problem hiding this comment.
Thanks for working on this and all the other PRs, @adamsilverstein!
I tested this in Playground with ?gutenberg-pr=77979.
The structural suggestion flows worked for me:
- Block move creates a dotted outline + “Suggested move” tab.
- Block insert creates a dashed outline.
- Block remove creates colored text with line-through.
- The Notes sidebar exposes Accept/Reject controls, and the states update after disposition.
Structural block suggestions:
One usability confusion I hit: “remove block” and “delete selected text” look/behave very differently.
When I remove an entire block in Suggest mode, I see the expected structural marker: the block remains visible with colored line-through text. But when I select text inside a paragraph and delete it while in Suggest mode, the deleted text disappears from the canvas and the block only gets the generic suggestion bracket.
Inline text deletion suggestion:
As a tester, I initially expected deleted inline text to remain visible with strikethrough, similar to Google Docs/Word suggestions. If inline deleted-text visualization is intentionally out of scope for this PR, I think it would help to call that out in the testing notes, e.g.:
- “Remove block” tests the structural block-remove marker.
- “Delete selected text inside a block” is a text/attribute suggestion and does not show the same block-remove strikethrough marker.
A couple of smaller notes from testing this PR:
- The move suggestion card in the sidebar showed
Move block: paragraph, but I did not see an obvious from/to descriptor in the visible card. I had to inspect the canvas to understand where the block moved. - The floating block toolbar can obscure the block-remove line-through marker, which makes the removed content harder to inspect while the block is selected.
|
Thanks for testing @saroshaga !
Yes, that makes sense! I worked on the previewing of these inline changes separately in #77869 - I realized that was confusing so I left a comment explaining how to test the complete feature and how I have broken it up here: #73411 (comment) - in short
What do you think this could say? "Moved up three positions" or "moved from below" or just "moved up". I'm a little unsure what context we could really provide here that would help users. Maybe a visual treatment on the block? eg. a moved block could show the old position as a strike out/removed version and connect the two with an arrow.
We don't control the toolbar directly in floating mode, but maybe something in the styling could improve the placement. |
Hmm, those changes should be in |
|
I created a new feature branch for testing the complete feature with all the parts... Testing the complete featureTo exercise the complete Suggest-mode feature (this structural stack + inline previews #77869) in one shot, check out the git fetch origin try/suggest-mode-combined && git checkout try/suggest-mode-combined
npm install && npm run build
npm run wp-env startTesting-only - code review still happens on the individual PRs. Testing with PlaygroundThe complete feature is testable with the combined PR in Playground: https://playground.wordpress.net/gutenberg.html?pr=78994 (test pr is #78994) |
awesome, thanks!
A ghost placeholder block! I like it :D Moved up / down was something that I felt could be useful. If we can positions, that's also neat. I should've added that this is minor. |
3bb3ba7 to
2ca8de9
Compare
2ca8de9 to
f2d1c3a
Compare
43d6215 to
7d1657c
Compare
f2d1c3a to
688d72e
Compare
7d1657c to
75a6a73
Compare
688d72e to
8b90242
Compare
75a6a73 to
0828f5c
Compare
8b90242 to
b5e52e8
Compare
0828f5c to
9fec5ed
Compare
b5e52e8 to
ce3c626
Compare
Brings the visual treatment for block-move suggestions: - style.scss adds the pending-move treatment: 70% opacity plus a dotted green outline. The dotted style distinguishes it from the dashed pending-insert (the block existed before — only its position is suggested) and the solid bracket used by pending attribute edits. - suggestion-summary.js adds a "Move block: <name>" line via the same friendlyBlockName helper used by the other structural ops. - suggestion-diff.js adds BlockMoveDiff, which renders a "Moved <name> from <position> to <position>" sentence so reviewers can verify the proposed motion without the canvas open. Block content stays the same — only its position changed — so a content diff would just be noise. Refs #77434.
The dotted outline plus 70% opacity alone was not strong enough to unambiguously communicate "this is a preview" on the reviewer's screen. On a fresh load the moved block can read like a regular drag-and-drop result, especially in intents that don't tint the canvas with Suggest- mode chrome. Add a small green tab pinned to the top-left corner that reads "Suggested move" so the preview state is obvious from the canvas without having to open the sidebar.
The structural-marker class HOC reads metadata.suggestion.type from the block-editor store and is intentionally not gated on isSuggestMode — a reviewer in Edit intent must still see the pending-remove strikethrough, the pending-insert dashed outline, and the pending-move dotted outline, since the marker is the only visual signal that a structural change is proposed but not yet applied. The existing test coverage stopped at the marker→class mapping table, so a regression in the gating could have shipped silently. Lift the HOC to a named export (was previously only registered as a filter) and add cases for each marker type in both Edit and Suggest intents.
… bundle The suggestion-mode visual treatment (`is-suggestion-pending`, `is-suggestion-pending-remove`, `is-suggestion-pending-insert`, `is-suggestion-pending-move`) targets `.block-editor-block-list__block` inside the editor canvas iframe. Previously these rules lived in the editor-package stylesheet (`wp-edit-blocks`), which is not loaded into the iframe — only `wp-block-editor-content` (compiled from `block-editor/src/content.scss`) is. As a result the marker class landed on the DOM but no rule matched, so the preview chrome was invisible. Move the canvas-targeted rules to a new partial under `block-editor/src/components/block-list/content-suggestion.scss` and include it from `block-editor/src/content.scss`. The class names describe a block visual state, so block-editor styling them does not violate layering: it doesn't need to know how the marker arrived. The sidebar/diff/header rules stay in the editor package since they target chrome surrounding the iframe rather than blocks within it.
…o Google Docs green The previous treatment dimmed the block to 50% opacity (which read as "greyed out" rather than "proposed for deletion") and drew a single horizontal `::after` line at vertical center, which only struck through whichever line of text happened to fall on the midline. Apply `text-decoration: line-through` directly so every line of a multi-line block gets struck through, and switch the suggestion color to the same green Google Docs uses (`#188038`) for a clearer, consistent "this is a suggestion" cue across remove/insert/move treatments.
When a suggester adds a new block in Suggest mode, the block-insert-after detector tags the live block with `metadata.suggestion = pending-insert`, but any subsequent `setAttributes` calls (the suggester typing into the block) get diverted into the per-peer suggestion overlay. The reviewer therefore sees the inserted block as empty — the typed content is trapped on the suggester's peer. A pending-insert block has no \"before\" state worth preserving — the block itself is the suggestion, and the suggester is the only author of its attributes. Bypass the overlay for any block whose marker is `pending-insert` so edits flow through to the real attributes and sync via CRDT, then color the inserted block's text in suggestion green so the reviewer sees a clear preview of what's being added (matching Google Docs' suggesting-mode insert treatment). The overlay test helper was registering `blockEditorStore` for every test, which silently activated the provider's orphan-prune effect and deleted overlay entries whose synthetic `clientId` had no matching block. Register `blockEditorStore` only when a test explicitly passes `blocks`, so the existing `clientId=\"a\"` tests keep working.
…atar color Capture the suggester's user id when writing each pending-* marker, and have the canvas HOC pipe that id through `getAvatarBorderColor` into a `--suggestion-author-color` CSS variable on the wrapper. The `is-suggestion-pending-*` rules consume the variable with the previous green as the fallback, so individual suggestions now read as the suggester's own color (the same palette live cursors use) — Google Docs-style — and reviewers can tell two suggesters apart at a glance without changing the green-only default for anonymous edits.
…te log (#77675) When two edit sessions create a “room” for the same document, they can encounter a race where WordPress creates two copies of the sync post meta for that document and the editors can work on two different histories of the document, leading to data loss. This change introduces a process to merge updates into a canonical sync post meta to avoid this race. When duplicates are detected, the newest sync’d meta is chosen as the new canonical copy and it replaces the other copies. During this process, additional database activity occurs to resolve the duplicates, though that should normatively present itself when duplicates already exist. Due to a lack of a broadly-supported way to perform database locking, there remains a secondary race when resolving the duplicates; this condition should be rarer than the one which motivated this in the first place. That means that while risk still exists, the overall risk should be much lower. An attempt was made to cover this secondary risk but the solution was complicated and itself unclear. Co-authored-by: danluu <danluu@git.wordpress.org> Co-authored-by: dmsnell <dmsnell@git.wordpress.org> Co-authored-by: peterwilsoncc <peterwilsoncc@git.wordpress.org>
* RTC: Fix find_canonical_storage_post_id() always returning null get_posts() with 'fields' => 'ids' returns an array of IDs, so the previous is_numeric( $post_id ) check was always false. As a result, find_canonical_storage_post_id() always returned null and the storage layer kept promoting the suffixed post to the canonical slug, leaving two posts sharing the same slug. This caused test_first_access_race_does_not_split_room_storage to fail on WordPress 6.8/6.9 where the Gutenberg shim is active (WP 7.0+ skips the test because core ships its own class). Read the first element from the result array and return it as the canonical post ID instead. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * CI: Temporarily run previous-major WP PHP tests on pull_request The race-condition regression test for WP_Sync_Post_Meta_Storage only runs against the Gutenberg shim, which is bypassed on the latest WP. PRs normally exclude the previous-major WP job, so the fix cannot be verified by CI on a PR. Comment out that exclude with a TODO to revert before merging. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * Revert "CI: Temporarily run previous-major WP PHP tests on pull_request" CI confirmed the fix on the previous WP major. Restore the original pull_request exclude. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * Backport changelog: Add PR 78053 to 7.0/11660 entry The find_canonical_storage_post_id() fix needs to ride along with the existing 11660 backport so Core gets the corrected race-resolution logic in one piece. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: t-hamano <wildworks@git.wordpress.org> Co-authored-by: Mamaduka <mamaduka@git.wordpress.org>
9fec5ed to
b0d4b37
Compare
ce3c626 to
55be6bc
Compare
….php The file was dropped from the 6c-move-ui lineage while collaboration.php still requires it, causing a PHP fatal at boot (all e2e shards failing). Restore it from the parent branch.
adamsilverstein
changed the title
adamsilverstein
changed the title
|
Superseded by the fresh-stack restructure of Suggest mode (#73411). The feature has been re-sliced from this 16-deep stack onto current
The whole feature now sits behind a Suggestion Mode experiment (Settings → Experiments → Collaboration). Closing in favour of the new stack — details: #73411 (comment) |
4 participants
What
Adds the visual treatment for block-move suggestions (#77434, task 6c). Pairs with the mechanism PR (#77978).
metadata.suggestion = { type: 'pending-move' }renders at 70% opacity with a dotted green outline at its new position plus a "Suggested move" label tab. Three distinct outline styles communicate three distinct kinds of structural change: solid bracket (attribute edit), dashed (insert), dotted (move).How
style.scss: pending-move styling (70% opacity + dotted green outline + label tab).suggestion-summary.js: handlesblock-moveops with a "Move block: " line.suggestion-diff.js:BlockMoveDiffrenders a from→to descriptor.Cross-cutting polish (applies to all three marker types)
The last few commits on this branch are integration-test polish that emerged after all three structural marker types (
pending-remove,pending-insert,pending-move) were stacked together. They live on this PR rather than being split across 6a / 6b / 6c because the canvas treatment is shared infrastructure — splitting them by marker type would be artificial:is-suggestion-pending-*rules previously lived in the editor-package stylesheet (wp-edit-blocks), which is not loaded into the canvas iframe. Onlywp-block-editor-content(compiled fromblock-editor/src/content.scss) is. The marker class landed on the DOM but no rule matched. Fix: move the canvas-targeted rules into a newblock-editor/src/components/block-list/content-suggestion.scsspartial.#188038) — the previous::afterline-through only struck the midline of multi-line blocks;text-decoration: line-throughon the wrapper covers every line. Applies the same green Google Docs uses for suggestions.getAvatarBorderColorinto a--suggestion-author-colorCSS variable so individual suggestions read as that suggester's color (the same palette live cursors use). Falls through to the green default when the id isn't available.Testing
This is the final PR in the stack — once merged, structural suggestions for insert / remove / move are fully shipped under issue #77434. Phase 6d (Yjs
AttributionManager-backed) tracks separately.Stack
Refs #77434, #73411.