Fix e2e test for padding appender #66080
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
stokesman
added
the
[Type] Task
|
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: 0 B Total Size: 1.77 MB ℹ️ View Unchanged
|
|
Thanks for the PR! Do you know if the padding appender used to work properly and automatically focused the last empty paragraph block? I'd like to know if the current behavior is a bug or by design 🤔 |
Yes, it used to. Though it’s almost as if it wasn’t by design, the padding appender hook never had to concern itself with focus. The focus happened as part of the inserting the default block. Even if it wasn’t by design, it seems it should be. See #65849 where Andrea points one why without noting that it used to be that way. To see the focus working, the parent commit of d9551aa can be checked out, as that was the commit I found from |
kevin940726
added
the
Backport to WP 6.7 Beta/RC
|
Added the backport label. However, the e2e tests are failing here 😢. |
stokesman
force-pushed
the
fix/e2e-test-padding-appender
branch
from
7699ebc to
88a3b51
Compare
|
Flaky tests detected in 88a3b51. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/11367702882
|
The test failure was expected until rebased on a branch with the bug fix. I don’t get why we’d need to backport this though. Does CI run on the wp/6.7 branch? If so, I guess this can verify that the fix in #66143 worked there. Anyway, to get this in it still needs a approval. |
t-hamano
approved these changes
Oct 16, 2024
github-actions
bot
added
Backported to WP Core
gutenbergplugin
pushed a commit
that referenced
this pull request
Oct 16, 2024
Co-authored-by: t-hamano <wildworks@git.wordpress.org> Co-authored-by: kevin940726 <kevin940726@git.wordpress.org>
|
I just cherry-picked this PR to the wp/6.7 branch to get it included in the next release: 57b99ff |
cbravobernal
added
[Type] Bug
karthick-murugan
pushed a commit
to karthick-murugan/gutenberg
that referenced
this pull request
Nov 13, 2024
Co-authored-by: t-hamano <wildworks@git.wordpress.org> Co-authored-by: kevin940726 <kevin940726@git.wordpress.org>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What?
Adds an assertion that the default block is focused once inserted.
Why?
In trunk the feature has regressed and this test could have caught it. Closes #65138.
Deep thoughts
It might be that another test would ideally have caught this and this test should be left as is. That’s because the padding appender feature’s implementation isn’t responsible for focusing the block it inserts. So the breakage is actually elsewhere, yet given this test is in a general
inserting-blocksspec it seems fine to assert focus here. Still, maybe it’d work just as well to assert it in a different test.Testing Instructions
Note the test will fail until the bug is fixed.