Skip to content

Notes: Port canvas margin layout tests from #79864 - #83790

Closed
adamsilverstein wants to merge 1 commit into
add/notes-canvas-marginfrom
add/notes-canvas-margin-tests
Closed

Notes: Port canvas margin layout tests from #79864#83790
adamsilverstein wants to merge 1 commit into
add/notes-canvas-marginfrom
add/notes-canvas-margin-tests

Conversation

@adamsilverstein

@adamsilverstein adamsilverstein commented Sep 29, 2026

Copy link
Copy Markdown
Member

Hey @Mamaduka, I like where #83786 landed, so I brought over the layout tests from #79864 that your branch doesn't cover yet. Feel free to cherry-pick whatever is useful, or merge this into your branch.

Claude ported the tests over and ran them, notes below:

What?

Stacked on #83786. Adds e2e tests to the Canvas margin group in block-notes-floating.spec.js. Tests only, no source changes.

Why?

#79864 had a few layout checks that don't have an equivalent on #83786 yet. The approaches differ, but the layout rules are the same, so the tests carry over.

How?

Tests ported from #79864, adapted to the helpers on this branch:

  • Notices keep the full width of the canvas while notes are visible (Notices don't span the full width of the editor when there are notes being rendered. #73917).
  • The divider takes the outermost pixel of the reserved space and spans the visible canvas.
  • With a tall full-width cover forcing a scrollbar, the thread stays inside the reserved space rather than under the scrollbar.
  • Notes stay visible beside the Settings sidebar, with the full 280px reserved.
  • Small viewports drop the floating notes and the reservation.

Tests from #79864 left out on purpose, since #83786 hides the notes instead: centering the thread on a resized desktop canvas, and notes staying visible in device preview. "Opening All notes hides the floating notes" is already covered by your Display mode test.

The non-iframed canvas case raised while comparing the two PRs can't be tested here: the post editor is always iframed with Gutenberg active.

Testing Instructions

npm run test:e2e -- test/e2e/specs/editor/various/block-notes-floating.spec.js

Locally, the full spec passes (1 skipped, the existing fixme). One early run hit a timeout inside the shared addBlockWithNote helper that didn't come back on rerun.

AI Use

Claude Code ported these tests over, with me steering. I will review and test.

@adamsilverstein
Notes: Cover more of the canvas margin contract in e2e tests
c7b48c2 
Port the layout checks from #79864 that this branch did not yet cover:
notices keep the full canvas width, the divider sits flush with the
reserved space, a thread stays inside the reserved space when the canvas
has a scrollbar, notes stay beside the Settings sidebar, and small
viewports drop the floating notes.

Claude-Session: https://claude.ai/code/session_01Jx9yDR5xxMRczChFCjzBNu
@github-actions

github-actions Bot commented Sep 29, 2026 •
edited
Loading

Copy link
Copy Markdown

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 props-bot label.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: adamsilverstein <adamsilverstein@git.wordpress.org>
Co-authored-by: Mamaduka <mamaduka@git.wordpress.org>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

@github-actions

github-actions Bot commented Sep 29, 2026 •
edited
Loading

Copy link
Copy Markdown

🤖 PR meta 🤖

📦 Bundle size

Size Change: 0 B

Total Size: 8.24 MB

c7b48c2 Run

🏁 Flaky tests

Show the failures

Some 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.

should cut and paste individual blocks with collapsed selection in /test/e2e/specs/editor/various/copy-cut-paste.spec.js, passed after 1 failed attempt. 
Error: expect(string).toMatchSnapshot(expected) failed

  @@ -1,3 +1,3 @@
   <!-- wp:paragraph -->
  -<p>2Cut - collapsed selection</p>
  +<p>Cut - collapsed selection</p>
   <!-- /wp:paragraph -->


    at /home/runner/work/gutenberg/gutenberg/test/e2e/specs/editor/various/copy-cut-paste.spec.js:45:49

c7b48c2 Run

@Mamaduka
Mamaduka self-requested a review September 29, 2026 16:33
@Mamaduka Mamaduka added the [Type] Automated Testing Testing infrastructure changes impacting the execution of end-to-end (E2E) and/or unit tests. label Sep 29, 2026
@Mamaduka

Mamaduka commented Sep 29, 2026

Copy link
Copy Markdown
Member

Thanks, @adamsilverstein!

I think the notice width check is worth keeping, but as an assertion in the existing "follows its block when an editor notice shifts the canvas" test, not as a new one. It already creates a notice, so there's no extra setup.

The other four either duplicate existing coverage or can't fail:

  • Divider: this only reads back the ::after values that canvas-margin.js writes as a CSS string, so it checks the string, not behavior.
  • Scrollbar: Playwright launches headless Chromium with --hide-scrollbars, so the scrollbar width is 0 in CI and the test passes with or without the offset. Turning scrollbars back on needs launchOptions, which is per worker and starts a separate browser for a single check.
  • Settings sidebar: "minimizes the notes when the canvas is narrow" already opens the Settings sidebar at 1440px and asserts that the note is visible and 280px is reserved.
  • Small viewports: "yields to All notes when the canvas is too narrow" covers the hide path. Below medium it's just a useViewportMatch check, and the media query already removes the reserved space.

What do you think?

@adamsilverstein

adamsilverstein commented Sep 29, 2026

Copy link
Copy Markdown
Member Author

Sounds good!

@Mamaduka

Mamaduka commented Sep 29, 2026

Copy link
Copy Markdown
Member

Extracted the notice width test. Going to close this. Thanks, Adam!

@Mamaduka Mamaduka closed this Sep 29, 2026
@Mamaduka
Mamaduka deleted the add/notes-canvas-margin-tests branch September 29, 2026 19:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Mamaduka Mamaduka Awaiting requested review from Mamaduka

Assignees

No one assigned

Labels

[Type] Automated Testing Testing infrastructure changes impacting the execution of end-to-end (E2E) and/or unit tests.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@adamsilverstein @Mamaduka