Block Bindings: Fix focus behavior when pressing Enter on connected block #58958
Conversation
artemiomorales
added
[Type] Bug
|
Size Change: +201 B (0%) Total Size: 1.71 MB
ℹ️ View Unchanged
|
This comment was marked as outdated.
This comment was marked as outdated.
|
|
||
| // selectBlock( blockToFocus.clientId ); | ||
| } | ||
| : onReplace; |
There was a problem hiding this comment.
block-list/block.js already conditionally omits/changes onReplace. Could we move this additional condition there?
There was a problem hiding this comment.
So it turns out we don't need to redefine the onReplace — we can just circumvent the Rich Text's split logic if the block is bound.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
|
Flaky tests detected in 5df6af4. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/7995085102
|
artemiomorales
force-pushed
the
fix/bindings-focus-on-enter
branch
from
9b2592e to
a2cd0cc
Compare
| @@ -57,6 +57,7 @@ export function useEventHandlers( { clientId, isSelected } ) { | |||
| event.preventDefault(); | |||
|
|
|||
| if ( keyCode === ENTER ) { | |||
| target.blur(); | |||
There was a problem hiding this comment.
Will adding this cause issues with any other blocks?
I haven't thought through all of the scenarios yet, but I'm wondering if there's any case in which we wouldn't want to blur the target. By blurring it, we ensure that focus gets set to the newly created block. Any insights here would be appreciated!
There was a problem hiding this comment.
cc @ellatrix Do you foresee any problems with this approach?
There was a problem hiding this comment.
@michalczaplinski @ellatrix As an alternative, I've tried roughly extracting the RichText's shouldDisableEditing logic into a separate hook. If we want to go this route, we should perhaps rename the hook from useShouldDisableEditing to something more explanatory.
I'm not sure this is necessary, but it would probably be the safest approach. What do you think?
There was a problem hiding this comment.
Will adding this cause issues with any other blocks?
What is the purpose of using target.blur()? In which cases it is not working without it?
From my tests, I can see that without that, everything seems to work fine. However, when target.blur() is added, if I follow these steps it doesn't work as expected:
- Reload the page.
- Select in the block list the paragraph with bindings.
- Click enter.
- It doesn't add a new block.
There was a problem hiding this comment.
It doesn't add a new block.
Hmm, I'm not seeing this. But check out this comment for additional ideas regarding the blur.
Does your local now add the new block after the latest changes?
There was a problem hiding this comment.
The issue I saw was caused by the target.blur() so as you removed it I can't reproduce it anymore 🙂
|
Note: Using the NVDA screen reader will cause the expected behavior when pressing Pressing I'll create separate issues to tackle those items. |
|
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. |
artemiomorales
requested review from
gziolo,
michalczaplinski and
SantosGuillamot
|
I've tested it with the Paragraph, Image and Button blocks and works fine! 👍 I would defer to Ella's judgement on whether adding |
artemiomorales
force-pushed
the
fix/bindings-focus-on-enter
branch
from
a2cd0cc to
c8b75aa
Compare
@michalczaplinski Great! Out of curiosity, what did your testing with the Image and Button blocks look like? Since those aren't strictly In my testing, I do know that pressing |
gziolo
added
the
Backport to WP 6.7 Beta/RC
|
@artemiomorales I followed the usual testing instructions:
I've just retested again and realized that you're right on both counts 🙂 :
Sorry for adding unnecessary noise here! |
| const { getSelectionStart, getSelectionEnd, getBlockRootClientId } = | ||
| useSelect( blockEditorStore ); | ||
| // Disable Rich Text editing if block bindings specify that. | ||
| const shouldDisableEditing = useShouldDisableEditing(); |
There was a problem hiding this comment.
Back then, it was discussed to include the logic moved to the useShouldDisableEditing hook into the initial selector: link. It should improve performance and ensure the hooks are not called more than necessary. Is there any reason to move it outside?
There was a problem hiding this comment.
So I've done some additional testing, and it looks like setting shouldDisableEditing in the selector results in inconsistent behavior. You're also right in this comment saying that the blur isn't necessary to get the focus to work. In a previous iteration, blurring the target allowed focus to be set to the new block, though apparently this wasn't addressing the core issue.
The real problem, I'm realizing, is that because the shouldDisableEditing value from the selector is inconsistent, it conflicts with the expected behavior when setting tabIndex, preventing focus from being set to the new block.
Here's a video going over it in more detail:
enter-focus-comparison.mp4
I've pushed another commit removing the blur logic. We can also move hook logic back to the Rich Text index.js — it'll work as long as the logic remains outside of the selector. Personally, I think having the hook separate is cleaner and easier to reason with, but I'm happy to take either approach.
There was a problem hiding this comment.
Thanks for the explanation. I guess there is an issue when the block is selected, but I am not able to identify the why.
Personally, I think having the hook separate is cleaner and easier to reason with, but I'm happy to take either approach.
I don't have a strong opinion whatever you prefer.
There was a problem hiding this comment.
See my comment with the potential refactoring for the new hook that would address most of the discussed performance implications: #58958 (comment).
Apart from the other comments shared, I've been taking a look at the implementation and I believe the button block failing could be related to this pull request. If I am not mistaken, before this pull request, pressing enter in a button would duplicate the button with the bindings (which is not correct). And at this pull request, it just does nothing. I think it is caused by this early return: link. I'm wondering if that could be considered the final solution or just a workaround that could cause issues like this in the future. Anyway, I don't have enough background to answer that question 😄 Maybe we could explore the possibility of adapting the split-value function, or the onReplace function? Apart from that, regarding the button, I think there is an issue with this logic not being always 0, which makes the button non-selectable when contenteditable is false. |
@SantosGuillamot I've tested the button behavior on block-binding-button-test.mp4With that in mind, since this PR already improves the Paragraph experience, I'm inclined to treat the Button experience as a separate issue so we can continue iterating. What do you think? |
|
I tried to explain how I understood the split function and why these changes could be made in this comment and here. Let me know if that makes sense. As explained in this comment, there is yet another issue with metadata being preserved when you actually split the value. But that's not related to this specific PR. |
|
It would be great to rebase the branch to see the results from CI. |
SantosGuillamot
force-pushed
the
fix/bindings-focus-on-enter
branch
from
fdb5517 to
e6f3bb7
Compare
I resolved the conflicts and rebased. Let's see if tests pass. |
| newAttributes | ||
| ); | ||
| if ( isOriginal ) { | ||
| block.clientId = clientId; |
There was a problem hiding this comment.
I assumed it is needed because when we split a paragraph or a button we are actually creating two new blocks, and I guess that createBlock generates a new clientId. However, we might want to keep the original clientId to simulate that it is actually still the original block.
|
Alternative PR #59320 |
| if ( shouldDisableEditing ) { | ||
| // Simulate the cursor is at the end of the rich text. | ||
| _value.start = _value.text?.length; | ||
| _value.end = _value.text?.length; |
There was a problem hiding this comment.
We shouldn't have to do this stuff if we're just disabling rich text
| return false; | ||
| } | ||
| const blockBindings = | ||
| select( blockEditorStore ).getBlockAttributes( clientId ) |
There was a problem hiding this comment.
This is a performance problem because we're adding another subscription to block attributes to every block (at least paragraph and all other blocks with binding support in this case). I used context in #59320.
|
Closed in favor of #59320 |
getdave
removed
the
Backport to WP 6.7 Beta/RC
|
I removed the outdated |
What?
This PR is meant to set focus correctly after pressing Enter on a connected block.
Why?
Addresses fix/bindings-focus-on-enter.
Currently, pressing Enter on a bound block causes two paragraphs to be created — one erroneously gets created before the bound block and another one after, and focus is not set properly to the latter.
How?
This PR blurs focus from a bound block after the Enter button is pressed so that, when a new default block is inserted, focus can be set to the new block accordingly. It also prevents RichText logic from running if the block is bound.
Testing Instructions
Screenshare
enter-focus-fixed.mp4