-
Notifications
You must be signed in to change notification settings - Fork 4.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix inserting button block when pressing enter in a block with bound text
attribute
#59361
Fix inserting button block when pressing enter in a block with bound text
attribute
#59361
Conversation
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: -3 B (0%) Total Size: 1.71 MB
ℹ️ View Unchanged
|
Hm. Could I have a look at this first tomorrow? |
@ellatrix I was taking a look but will leave you to it! Thanks 🙏 |
.../block-editor/src/components/block-list/use-block-props/use-selected-block-event-handlers.js
Outdated
Show resolved
Hide resolved
text
attribute
Co-authored-by: Greg Ziółkowski <[email protected]>
This reverts commit 63ffff87ca048a218567772710dcc45affcfd2aa.
471c409
to
290273c
Compare
I'm working on adapting the tests to the latest changes added in |
…`text` attribute (#59361) * Add tabindex 0 to all disabled elements in rich text * Insert block when use enter in disabled rich text * Take default block into account * Add e2e tests when pressing enter in bound blocks * Revert initial implementation * Use `insertAfterBlock` instead of `insertDefaultBlock`. * Remove unnecessary `insertDefaultBlock` * Destructure innerBlocks array in tests Co-authored-by: Greg Ziółkowski <[email protected]> * Prettify tests * Remove unnecessary select in tests * Destructuring blocks in tests * Revert "Remove unnecessary select in tests" This reverts commit 63ffff87ca048a218567772710dcc45affcfd2aa. * Remove unnecessary select in tests * Adapt tests to latest changes --------- Co-authored-by: Greg Ziółkowski <[email protected]>
I just cherry-picked this PR to the update/packages-6.5-rc1 branch to get it included in the next release: 77ba590 |
…`text` attribute (#59361) * Add tabindex 0 to all disabled elements in rich text * Insert block when use enter in disabled rich text * Take default block into account * Add e2e tests when pressing enter in bound blocks * Revert initial implementation * Use `insertAfterBlock` instead of `insertDefaultBlock`. * Remove unnecessary `insertDefaultBlock` * Destructure innerBlocks array in tests Co-authored-by: Greg Ziółkowski <[email protected]> * Prettify tests * Remove unnecessary select in tests * Destructuring blocks in tests * Revert "Remove unnecessary select in tests" This reverts commit 63ffff87ca048a218567772710dcc45affcfd2aa. * Remove unnecessary select in tests * Adapt tests to latest changes --------- Co-authored-by: Greg Ziółkowski <[email protected]>
This PR introduced new behaviors for blocks with default blocks inside them (gallery, navigation). For example, adding a new link to the navigation block when a navigation link is selected will add a new link to the navigation block:
Same for adding images to a gallery:
Is this the intended behavior for these blocks? Screen.Recording.2024-03-19.at.2.32.35.PM.mov |
I would say it is the expected behavior. What do you think should be happening? If I am not mistaken, the function used here is using the default block defined by each block, and the According to the docs, For both the navigation and the gallery,
So, if that's not the expected behavior, I am not sure if it is related to this PR or to those properties in the relevant blocks. |
Previously, nothing happened when pressing Enter when the Navigation Link was active. Other possibilities would be:
Adding a new link could be a fine thing to do when pressing Enter. I'm only raising this to make sure it's an intentional decision. This is a big change in the interaction, and it appears accidental right now. This new behavior could be better though! I'm still undecided on it. It feels jarring to me to press Enter on a navigation link item and have a new link item added with a popover open. It's a lot of things unexpectedly happening.
That's the action from the block appender, not pressing enter on the selected block. I don't see a way in the docs to prevent an Enter keypress on the active item from adding a new default block. At the moment, I think I'd have to catch the Enter keypress within the navigation link block. |
I opened a new issue for this -- I don't think this PR is the cause of this behavior, but the design of the blockProps onKeydown: #60051 I'll keep looking, but I don't see a way to prevent adding a new block on Enter, and there are valid cases for not wanting that behavior. |
What?
Discussed in this other pull request.
If a button block text is bound to a custom field, it disables its editing. However, when you try to add a new button by pressing enter when the bound button is selected, nothing happens. It should add a new button the same way it does with not connected ones. To reproduce it you can:
This pull request tries to solve that.
Why?
The current UX is not the expected one.
How?
There are two issues that need to be solved:
I'm solving this by always setting the tabindex to 0 when the rich text is bound: link.
I'm solving that part by manually inserting a new block when the rich text is not editable and the user press Enter: link. Not 100% sure this is the right approach but it seems to be working for the current use cases (paragraph, heading, and button).
Testing Instructions
Repeat the process with a paragraph and a heading and check they work as expected as well.