Fix block selection issues in safari by avoiding conditional event handlers #30968
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
youknowriad
added
[Type] Bug
|
Size Change: +1.43 kB (0%) Total Size: 1.47 MB
ℹ️ View Unchanged
|
|
Tested and works as expected. P.S. I was hoping that this would fix #30249 as well. But I can still reproduce the issue. Do you think it's also related to Async Mode? |
|
@youknowriad not yet. I will do tests later today. Thanks for reminding me. |
youknowriad
commented
Apr 21, 2021
| <BlockNavigation | ||
| onSelect={ onClose } |
There was a problem hiding this comment.
This is actually a bug in trunk that is highlighted by this PR. (In a failing e2e test, focus was moving back to "inner block" after the parent was selected)
youknowriad
commented
Apr 21, 2021
| @@ -113,6 +113,9 @@ describe( 'List', () => { | |||
| await clickBlockAppender(); | |||
| await page.keyboard.press( 'Enter' ); | |||
| await page.keyboard.type( '* ' ); | |||
| // Not entirely certain why this timeout is needed. | |||
| // eslint-disable-next-line no-restricted-syntax | |||
| await page.waitFor( 1000 ); | |||
There was a problem hiding this comment.
@ellatrix any ideas here? I suspect the test to be unstable in trunk too, just less visible but I don't understand how this PR impacts this since it's pretty consistent here.
ellatrix
reviewed
Apr 22, 2021
| getBlockRootClientId, | ||
| getBlockIndex, | ||
| } = select( blockEditorStore ); | ||
|
|
||
| return { | ||
| isSelected: isBlockSelected( clientId ), | ||
| isBlockSelected: _isBlockSelected, | ||
| rootClientId: getBlockRootClientId( clientId ), | ||
| index: getBlockIndex( clientId ), |
There was a problem hiding this comment.
For all of these selectors, we could really use #31078. All of the results are only used in callbacks so it's pointless to call them here.
ellatrix
reviewed
Apr 22, 2021
| @@ -108,6 +108,10 @@ export function useEventHandlers( clientId ) { | |||
| } | |||
|
|
|||
| function onMouseLeave( { buttons } ) { | |||
| if ( ! isBlockSelected( clientId ) ) { | |||
| return; | |||
There was a problem hiding this comment.
I'm not sure if it's a good idea to add this event handler to every block? It's only necessary for the selected block, there's no need to track mouse leaving for all the other blocks. Maybe there's barely a difference in performance though.
7 tasks
|
closing in favor of #31103 |
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.
With Async Mode, useSelect calls for non selected blocks are not run synchronously which means the event handlers added conditionally by this hook are not attached on time after a block get deselected. This could potentially impact all browsers when the content of the post grows, it's just more visible in Safari.
Testing instructions