Repository navigation
Notes: Refactor floating board measurement - #83485
Conversation
|
Please let me know if you can think of additional edge cases for floating notes. I would like to improve our e2e test coverage for it. |
|
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. |
🤖 PR meta 🤖📦 Bundle sizeSize Change: +730 B (+0.01%) Total Size: 8.24 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
|
0d44321 to
8d85fb2
Compare
80e7834 to
b1be829
Compare
|
I've been doing some stress testing with Claude, and I think I managed to fix all the paper cuts we could find. Updated the PR summary to include a list of all fixes and improvements. |
|
I've also been doing some testing and will post the results soon. manual testing seemed fine, but Claude found a few edge cases worth reviewing. |
|
Nice work on the follow-up fixes. I ran a Playwright stress pass against Claude ran the edge cases side by side with trunk, here is what is left:
|
I feel like we noticed this before and its related to a deeper issue being worked on elsewhere. Do we have a notes specific issue for this? Is it worth having one @Mamaduka ? |
I hadn't noticed this before (but verified it manually), do you want to address it in this PR @Mamaduka? Otherwise I will open a separate issue. |
Confirmed as well. This looks like an issue worth handling, since it's part of the position/layout logic.
This is known issue #71544 / #29693 and not necessarily a layouting problem. We need to figure out how to handle rendering a note that belongs to multiple blocks. Related: #73416. Let's handle it in a follow-up.
I also flagged something similar when fixing the moving blocks problem here. Will double-check. |
1ee2b26 to
ec5d498
Compare
|
@adamsilverstein, I fixed 1 and 3. Let's handle 2 in a follow-up. |
ec5d498 to
5d42246
Compare
- Measure only in ResizeObserver callback; drop rAF, state and heavy effect. - Observer lifetime follows subscriptions; resubscribe re-observes refs. - Content-space anchors, value-compared snapshot; no emit on scroll. - Derive positions in render; re-measure on block reorder. - Move anchor resolution to getNoteAnchorRect util. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Move readInlineSelection, wrapInlineNote and clearInlineNoteMarker from hooks.js to utils.js and fix their JSDoc type warnings. Move NOTE_FORMAT_NAME to constants.js so utils.js stops duplicating it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The move animation offsets blocks with a transform, which resizes nothing, so threads kept their old order until the next change. Watch style changes under the root and request a measurement. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Anchors are read in the canvas frame's viewport, but threads are positioned in the sidebar panel. Content above the canvas (editor notices, device preview inset) moved the frame away from the panel and left threads offset. Measure the frame's offset from the threads' container and shift threads by it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Threads register with the board after the first paint, so a remount of the floating sidebar painted one frame with every thread at the same spot. Move the hide-until-positioned rule from AddNote into FloatingContainer so threads and the new note form share it. Uses opacity, since visibility would block focusing the new note form. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A note inside a closed Details still measured the box its block would have when expanded, so the thread sat below the summary, over the next block. When the anchor fails checkVisibility(), climb to the closest visible block. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Threads jumped to their new spots on selection change. Transition `top`, unless the user prefers reduced motion. A thread's first positioning starts from `top: auto`, which doesn't animate. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Re-measuring on every animation frame re-rendered the board each frame. Skip style mutations while a transform is set. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The title sits outside the block-list root, so a wrapping title moves the root without resizing it and threads were left behind. Observe the root's parent, which grows with it. Rebuild the observers when the root changes instead of swapping each target, and share the floating element cleanup. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
5d42246 to
e8c9374
Compare
| // Threads mount before the first measurement. Not | ||
| // `visibility`, which would block focusing the new note form. | ||
| opacity: floating.y === undefined ? 0 : undefined, |
There was a problem hiding this comment.
Currently, opacity: 0 is applied to all notes without block anchors, as well as the new note form. As a result, even when a note is visually hidden because its linked block is unselected, it can still receive focus. Should we apply the inert attribute to these notes?
There was a problem hiding this comment.
Let's try to resolve the hidden block notes as part of #73565. As the inline comment states, using opacity is intentional; the user might trigger note focus before the position settles, and focus might be missed. Using inert will cause the same problem, similar to the visibility property.
|
Thanks for the feedback, everyone! |
Co-authored-by: Mamaduka <mamaduka@git.wordpress.org> Co-authored-by: jasmussen <joen@git.wordpress.org> Co-authored-by: t-hamano <wildworks@git.wordpress.org> Co-authored-by: adamsilverstein <adamsilverstein@git.wordpress.org> Co-authored-by: i-am-chitti <iamchitti@git.wordpress.org>
Take trunk's board store and floating board hook from #83485; the hidden-block anchor is rebuilt on top of them in the following commits. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LFNXAzK5ogseY5tCtU2x53
The floating board now measures in the store's ResizeObserver pass (#83485), so the stand-in anchor for a block with no rendered element moves there from the old rAF batch. Threads register their block's client ID and the store keeps the document's block order, so it can place the thread between the neighbours that are rendered. With every noted block hidden, the store finds the canvas root from the threads' document instead. Neighbours inside collapsed content are skipped, since they still report their expanded box. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LFNXAzK5ogseY5tCtU2x53


What?
Fixes #83508.
Follow-up to #79877 (review thread).
Rewrites
createBoardStoreanduseFloatingBoardso all DOM reads happen in the store'sResizeObservercallback, React reads a value-compared snapshot throughuseSyncExternalStore, and note positions are derived during render. Moves the inline note helpers fromhooks.jstoutils.js.Additional fixes:
Note
I know it's a sizable change, but it's mostly internal cleanup + bug fixes. Floating notes behavior should be the same, if not better.
Why?
The extra
requestAnimationFrameafter theResizeObservercaused two paints per change and hid resize loops. The block move animation offsets moved blocks with a transform, which doesn't resize anything, so the only measurement read their old positions.How?
The observer exists only while the board has subscribers, and
requestMeasure()re-observes the root to get another pass before the next paint, on register and onthreadschanges. Anchors are stored in canvas content-space, so scrolling never re-renders;--canvas-scrolland--canvas-offsetCSS variables move the threads instead. AMutationObserveronstyleattributes under the root re-measures while blocks animate.Testing Instructions
Closed Details block
Automated: collab-sidebar unit tests (146) pass.
block-notes,block-notes-floatingandcollaboration-notese2e on Chromium: 67 passed, 1 skipped (existingfixme).Testing Instructions for Keyboard
Same as above.
Screencast
Before
CleanShot.2026-09-24.at.13.25.18.mp4
After
CleanShot.2026-09-24.at.13.23.19.mp4
Use of AI Tools
Assisted by Claude.