Fix unable to remove empty blocks on merge #65262
Conversation
kevin940726
added
[Type] Bug
|
Size Change: +1.31 kB (+0.07%) Total Size: 1.77 MB
ℹ️ View Unchanged
|
|
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. |
kevin940726
requested review from
noisysocks,
talldan,
ramonjd,
andrewserong and
aaronrobertshaw
|
Unfortunately it seems this fix breaks the feature introduced in #45075. If I had to pick one, I feel like it'd be better to keep the behavior in trunk, the way it works for lists is pretty useful. I'm not sure if there's a way to keep the behavior for lists (and maybe quotes) but delete blocks like headings and paragraphs in groups. The code here is very generic, not really reasoning about block types, so it might be tricky. |
I think unwrapping list items still works from my testing and the e2e tests? Could you point me to the features that are breaking after this PR? |
I was a bit stumped trying to reproduce this at first, as this PR mostly feels quite good to me in testing. As far as I could tell the main difference for lists appears to be what happens when you press backspace from an empty list item. I.e. in this PR, the overall list becomes selected: 2024-09-13.11.28.01.mp4Whereas on 2024-09-13.11.29.07.mp4
I'm not sure how we'd do it either, but I tend to agree that it'd be ideal if we could have it working as in this PR for headings and paragraphs in groups, but retain the behaviour in Were there any other details from #45075 that aren't working in this PR @talldan? |
packages/blocks/src/api/utils.js
Outdated
| if ( definition.hasOwnProperty( 'default' ) ) { | ||
| return value === definition.default; | ||
| } | ||
| export function isUnmodifiedBlock( block, attributeKeys ) { |
There was a problem hiding this comment.
Might it better to leave isUnmodifiedBlock intact and create a new util with our implementation of whether a block can be considered empty?
Additionally, should the empty block function just check for
if ( definition.type === 'rich-text' ) {
return ! value?.length;
}
Do you see any drawbacks on that?
There was a problem hiding this comment.
Third-party blocks could have content attributes that are not rich-text, (but would it able to call onMerge?). I think it's better to keep it a little bit generic for now. 😅
There was a problem hiding this comment.
I think many third party blocks are also unlikely to have __experimentalRole defined because of the way it's experimental.
There was a problem hiding this comment.
Just FYI it's been stablised now but Dan's point still stands:
There was a problem hiding this comment.
Yeah, I'm just trying to be safe and future-compatible here. (On the other hand, an attribute could also have the rich-text type but not be considered content.)
Thanks for the link! I can rebase this to use the stabilized function.
kevin940726
force-pushed
the
fix/remove-empty-on-merge
branch
4 times, most recently
from
550773b
to
91dcb16
Compare
|
I updated the PR to include some more testing cases and rules. This is getting a lot more complicated though. Some feedback on the desired behavior is appreciated! I applied some predefined rules that I think are less frustrating to regular users but also tried to maintain maximum backward compatibility. See the recordings in the PR descriptions for some edge cases. |
|
Thanks for the updates, Kai!
The behaviour for the first item in a List feels good to me know (conversion to the default block), same with an empty Paragraph block within a Group. However, with the Heading block, I'd expect hitting backspace on an empty Heading block within a Group block to work the same as for the paragraph block — that is, for it to delete the Heading block. In the screengrabs and in local testing, it looks like it now moves up a level and converts to a default block, similar to the List item, which felt a bit unexpected to me: 2024-09-17.16.48.55.mp4In the above video:
Unfortunately I can't quite think of a good way to handle this, but it seems to me like the behaviour of headings and paragraphs should probably be much the same as each other, but the behaviour for list items is the outlier? Are there any other primitive blocks we'd need to consider here, or is it mostly about paragraphs and headings that we want to deal with? Just wondering if it'd help to make it about the specific blocks we're dealing with, rather than keeping the logic generic, or would that only create further complexity? |
|
I can confirm that the base case of deleting an empty paragraph block (and that block being removed) is working on this PR
I would expect the empty block to be deleted in all cases. But, in general, think I failed the IQ test - testing this stuff seems very subtle and I'm not sure what's right or wrong anymore 🤣 For example, not related to this PR, but it's confusing for me how subsequent blocks are pushed outside the Group block when I create new empty blocks. 2024-09-18.11.56.02.mp4 |
Yeah, I think we have a few options, I'm not sure which one is the best TBH.
I think both solutions make sense in a way. The first solution might be the most straightforward but a little bit destructive. The second solution conveys the meaning of "resetting formatting" when pressing backspace on an empty block. This kind of design language can be seen in some rich text editors, but very subtle. It also matches the behavior of pressing backspace on a non-empty block more closely too. That said, I feel it might not be worth it at this stage, so I'll try the first approach first 😅. |
noisysocks
added
the
Backport to WP 6.7 Beta/RC
kevin940726
force-pushed
the
fix/remove-empty-on-merge
branch
from
adfc777
to
4639098
Compare
kevin940726
force-pushed
the
fix/remove-empty-on-merge
branch
2 times, most recently
from
f9a4453
to
163a568
Compare
|
I updated the PR yet again to hopefully handle the list item use case, too 😅. I also updated the PR description, but I'll copy-paste it here for visibility. Now it follows the following rules:
An "empty" block is defined as if the One use case for these complicated rules is to handle the empty list item v.s. empty heading. In #65174, an empty heading is expected to be removed on backspace, but an empty list item should be converted to a default block and lifted to the parent. Both blocks are "empty" and not default blocks. The only difference is that an empty list item cannot be transformed to a default block inline, while an empty heading block can. So we try converting the empty heading block to an empty paragraph block on the first backspace (instead of removing it) to provide users instant feedback on backspace. I think it's an acceptable trade-off but there could be other better solutions! Kapture.2024-09-23.at.16.14.11.mp4 |
|
Tested with paragraph and heading blocks:
I can't really see anything strange with list blocks so far, but like I said - there are many subtleties to this behaviour I'm not completely covering. However in my testing this PR satisfies the reported issue: #65174 |
There was a problem hiding this comment.
Tested with paragraph and heading blocks:
Same, this is testing well for me with headings, paragraphs, and lists 👍. Since it is a little nuanced this one, I'll just ping @WordPress/gutenberg-core for visibility in case anyone has any concerns about the proposed change here.
kevin940726
force-pushed
the
fix/remove-empty-on-merge
branch
from
163a568
to
47b223b
Compare
|
Flaky tests detected in 47b223b. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/11047394625
|
|
Would appreciate an approval from someone else as I didn't get to test this deeply but I do appreciate the changes and I think it's looking good. Thanks |
Co-authored-by: kevin940726 <[email protected]> Co-authored-by: ntsekouras <[email protected]> Co-authored-by: talldan <[email protected]> Co-authored-by: andrewserong <[email protected]> Co-authored-by: youknowriad <[email protected]> Co-authored-by: getdave <[email protected]> Co-authored-by: Mamaduka <[email protected]> Co-authored-by: ramonjd <[email protected]> Co-authored-by: kspilarski <[email protected]> Co-authored-by: ndiego <[email protected]> Co-authored-by: richtabor <[email protected]> * Fix unable to remove empty blocks on merge * Update to the stabilized API * Rename the utils
github-actions
bot
removed
the
Backport to WP 6.7 Beta/RC
|
I just cherry-picked this PR to the wp/6.7 branch to get it included in the next release: 76759ef |
github-actions
bot
added
the
Backported to WP Core
What and why?
Fix #65174.
Arguably considered intended, but it could be clearer for users when trying to delete an empty block on merge.
This PR prevents empty unmodified blocks from being moved to the parent on merge. Modified blocks can still be moved to the parent to retain backward compatibility and user expectations.
How?
Refactor and reorder the
moveFirstItemUpimplementation. Now it follows the following rules:An "empty" block is defined as if the
contentattributes are empty.One use case for these complicated rules is to handle the empty list item v.s. empty heading.
In #65174, an empty heading is expected to be removed on backspace, but an empty list item should be converted to a default block and lifted to the parent. Both blocks are "empty" and not default blocks. The only difference is that an empty list item cannot be transformed to a default block inline, while an empty heading block can. So we try converting the empty heading block to an empty paragraph block on the first backspace (instead of removing it) to provide users instant feedback on backspace. I think it's an acceptable trade-off but there could be other better solutions!
Testing Instructions
Added e2e tests
should handle unwrapping and merging blocks.Follow the testing instructions in the original issue: #65174.
Or, you can try copy-paste this markup and try interacting with it.
Code
Otherwise, follow the steps below:
Testing Instructions for Keyboard
Same as above.
Screenshots or screencast
Kapture.2024-09-23.at.16.14.11.mp4