Notes: restore the sidebar the add-note flow displaced - #81547
adamsilverstein wants to merge 10 commits into
Conversation
Adding a note takes the single complementary-area slot, and cancelling the composer leaves the user with no sidebar at all. Pin the expected behaviour before changing it, including the case the previous attempt (#75455) got wrong: cancelling with nothing open must not open the document sidebar.
Capture the active complementary area at the moment the flow opens the notes sidebar over it, and put it back when the composer is dismissed. Keying the restore to the note selection rather than to the sidebar's own lifecycle makes it fire on every dismissal and regardless of whether the post has other notes, and capturing the real identifier restores the tab or plugin sidebar the user actually had. Nothing open before means nothing open after. Submitting keeps the notes sidebar, where the new note now lives. Fixes #75450.
|
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. |
|
Size Change: +200 B (0%) Total Size: 7.72 MB 📦 View Changed
|
|
Flaky tests detected in bd9fdd3. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/31652840459 Focus management when using the navigation link appender in
|
… add/75450-restore-sidebar-after-note # Conflicts: # packages/editor/CHANGELOG.md
The trunk merge brought the Jest-to-Vitest migration, which runs with `globals: false`. The test's bare `describe`/`it`/`expect` were undefined, so the suite failed to load. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01H22PasvCsxDt17L7sQfa5f
🤖 PR meta 🤖📦 Bundle sizeSize Change: +225 B (0%) Total Size: 8.21 MB 📦 View Changed
⚡ PerformanceShow the resultsClient side metrics exclude the server response time. front-end-block-theme
front-end-classic-theme
media-processing
media-upload
post-editor
site-editor
🏁 Flaky testsShow the failuresSome tests passed with failed attempts. The failures may not be related to this commit but are still reported for visibility. See the documentation for more information. Navigates the items list via UP/DOWN arrow keys in
|
… add/75450-restore-sidebar-after-note
… add/75450-restore-sidebar-after-note # Conflicts: # packages/editor/CHANGELOG.md
Trunk's Prettier update collapses short union types onto one line, so the RestoreTarget type failed lint after the trunk merge. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0171yvn6uH3beNJ6dsUrAyRt
|
Claude wrote up the closing note:
|
What?
Restores the sidebar that was open before a note was composed, when the note form is cancelled.
Stacked on #79864 - please merge that first. This PR only adds the follow-up on top of it.
Fixes #75450
Supersedes #75455 (closed)
Why?
From @talldan's report: open the block inspector, add a note, cancel the note form, and no sidebar is open at all. The inspector is gone and nothing brings it back, so the user loses their place.
Only one complementary area can be active at a time, so opening a notes surface necessarily closes the inspector, the document sidebar, or whatever plugin sidebar was there. @Mamaduka is right that this is ordinary complementary-area behavior - closing one sidebar has never reopened the previous one. What makes notes different is that the switch was programmatic and the thing it opened can be cancelled. An ordinary sidebar switch is something the user asked for; a cancelled note composer is a detour that ended with nothing to show for it.
#79864 already fixes the headline repro at large viewports: floating notes become part of the canvas and stop competing for the sidebar slot, so nothing is displaced and there is nothing to restore. That is what this issue was punted to 7.2 for. What is left after it is the small viewport (below the
mediumbreakpoint, 782px), where "All notes" is the only notes surface and still has to take the slot.How?
Two small pieces in
packages/editor/src/components/collab-sidebar/:Capture at the moment of the switch.
focusNotealready read the active area and threw it away. It now records it in a ref, right beforeenableComplementaryAreaoverwrites the slot, and only for the new-note path - that is the only one that can be cancelled. A capture is skipped when a notes sidebar is already active, which also keeps a second "Add note" during composition from clobbering the original target.Restore keyed to the composer, not to the sidebar. A
useEffectwatches the'new' -> undefinednote-selection transition, which is what every dismissal produces: the Cancel button, Escape, clicking away, and switching blocks.resolveRestoreTargetin the newrestore-sidebar.tsthen decides what to do, and it is a pure function so the rules are readable and unit-testable:nullorundefined)Submitting clears the capture without restoring: on a small viewport the new note lives in the sidebar the user is now looking at, and yanking it away to re-show the inspector would hide their own note. That sidebar has a "Close Notes" button, unlike the old floating one the issue was written about.
The three reasons #75455 didn't land are each addressed by construction, and the table above is where each one lives: it restores the real identifier rather than a hardcoded document sidebar, "nothing was open" maps to closing rather than opening, and the restore no longer hangs off
useEnableFloatingSidebar's enable flag, so it fires whether or not the post has other unresolved notes.The state is a component ref rather than store state on purpose. It is transient memory for a single in-flight composer: it must not persist (the active area is backed by
@wordpress/preferences, and a stale restore target firing in a later session would be a worse bug than the one being fixed), it must not sync to other users, and it is produced and consumed in one component. A store slice would only buy survival across aNotesSidebarremount, and every remount that matters also destroys the composer.Testing Instructions
Enable notes, then in the post editor at a narrow window (under 782px wide):
ctrl+alt+m). "All notes" takes over. Cancel the form - the Block tab comes back, and focus is on the paragraph. Escape behaves the same.At a normal window width, open the Settings sidebar and add a note: the inspector never moves, because the floating notes don't take the slot.
Automated
npm run test:unit packages/editor/src/components/collab-sidebar/test/restore-sidebar.ts- 8 passing.npm run test:e2e -- test/e2e/specs/editor/various/block-notes.spec.js -g "Sidebar restoration"- 7 passing. The four that assert a restore were confirmed to fail against this branch without the source change.Notes for reviewers
A few open questions, none of them blocking:
undefinedbecomes an explicitnullwhen cancelling from a never-toggled state, which suppresses the document sidebar'sisActiveByDefaultauto-open later on.focusNote's ownenableComplementaryAreahas already set the visibility preference by that point, so that state is unrecoverable either way.isApproved) also opens "All notes" over the inspector, but restoring on thread collapse is a different question from restoring after a cancelled composer, so it is left alone here.beforeEach/afterEachin the e2e block reset preferences and close sidebars. The active area persists to user meta on a debounce, and without that cleanup the new tests both inherit and leak sidebar state -block-notes.spec.jshas already been implicated in unrelated specs failing for this reason.AI Use
Code and description both written with 🤖 Claude Code. I will review and test.