Size Change: -9 B (0%)

Total Size: 2.45 MB

_{compressed-size-action}

@Mamaduka - this empty pinned sidebar state will never show after this change:

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: Mamaduka <[email protected]>
Co-authored-by: adamsilverstein <[email protected]>
Co-authored-by: t-hamano <[email protected]>
Co-authored-by: youknowriad <[email protected]>
Co-authored-by: jasmussen <[email protected]>
Co-authored-by: jeryj <[email protected]>
Co-authored-by: jeffpaul <[email protected]>
Co-authored-by: desrosj <[email protected]>

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

The test failures seem to be coming from focus loss, eg here is the first one:

When testing manually, after submitting the new note, I briefly see the note disappear, then reappear and regain focus. That seems off, ideally the new note form would transform into the new thread without that refresh. Are we already working on that? in any case, introducing a brief delay before the focus check might fix this?

Screencast - everything but selecting "... Add note" is done with the keyboard:

That seems off, ideally the new note form would transform into the new thread without that refresh.

Yes, that's how it should seem for a user. Internally, that's not how it's happening. The notes query will be re-triggered and re-render the whole tree, also rendering our new note.

It's odd that the first focus test started failing after this change. It was working normally for the pinned sidebar.

It's odd that the first focus test started failing after this change. It was working normally for the pinned sidebar.

I was able to validate the issue manually by using --debug and pausing the test in-flight. I see focus is not restored to the thread. When I was testing locally I did not see this issue. I noticed it does not happen with the second block created, adding a second block to the first test fixed it.

@Mamaduka The first test failure is odd, it fails to refocus on the first block. I verified by interrupting the test, but when I try with the same dev environment manually following the steps, focus works fine. I adjusted the test to add a second block, where you can see the focus test passes.

The other two failures had to do with testign resolved notes which disappear in the floating mode. For these, I added await blockCommentUtils.openBlockCommentSidebar(); at the top of the test and they now pass fine.

Flaky tests detected in db347d4.
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.

🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/19059318654
📝 Reported issues:

• [Flaky Test] ensures List View global shortcut works properly #51533 in /test/e2e/specs/editor/various/list-view.spec.js

@Mamaduka I was able to validate the test failure is legitimate. it is reproducable If you throttle the network connection focus is not restored to the first new note:

One thing you can see in the video that happens for the first note only is the sidebar disappears entirely briefly.

Once there is one note in place, adding the second note is slightly different. the sidebar never disappears and the focus is set properly on the note i just created:

Thanks for debugging, @adamsilverstein! The manual repro step gives me some ideas why it might be happening - #72872 (comment). I'll continue debugging further.

P.S. Sorry, I accidentally removed your commits with rebasing :( I've restored those changes, except for the real failure, which we've to fix.

I figured it out actually, the issue is with the mutation observer - it is observing the sidebar that disappears and reappears. I have a fix in place I will force push for your review...

Thank you, @adamsilverstein! Confirmed that fix is working correctly and all e2e tests are now passing locally.

@t-hamano, do you mind doing a review? I think a fresh perspective would help us here. If everything is fine, we can merge this for beta 3.

This pull request seems to be working correctly, but I want to double-check whether we should proceed with it. See #72771 (comment)

I think it was mentioned in associad PRs for that issue, that we should supersede it with this one.

I don't have a strong opinion here, but we also don't have time for lengthy arguments for the WP 6.9 release cycle. We should either merge this or punt them.

There was a conflict while trying to cherry-pick the commit to the wp/6.9 branch. Please resolve the conflict manually and create a PR to the wp/6.9 branch.

PRs to wp/6.9 are similar to PRs to trunk, but you should base your PR on the wp/6.9 branch instead of trunk.

# Checkout the wp/6.9 branch instead of trunk.
git checkout wp/6.9

# Create a new branch for your PR.
git checkout -b my-branch

# Cherry-pick the commit.
git cherry-pick 9ad1717c94b0cd80979c146b69a777d3770e216e

# Check which files have conflicts.
git status

# Resolve the conflict...
# Add the resolved files to the staging area.
git status
git add .
git cherry-pick --continue

# Push the branch to the repository
git push origin my-branch

# Create a PR and set the base to the wp/6.9 branch.
# See https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/proposing-changes-to-your-work-with-pull-requests/changing-the-base-branch-of-a-pull-request.

@Mamaduka, do you have the bandwidth to manually cherry-pick this PR into the wp/6.9 branch? I can also do it instead.

@t-hamano, working on it 👍

Here's the manual backport PR - #72961.

Can we avoid the manual backports by rebasing or merging trunk before merging commits?

@adamsilverstein All modifications related to the notes should have been backported, so conflicts should not generally occur.

In this PR, a conflict occurred due to differences between the trunk branch and the wp/6.9 branch in the Editor component, but I believe this was an unusual conflict.

trunk:

wp/6.9: