Editor: Fix block highlight rendering after block is moved #23425
Conversation
ItsJonQ
added
[Type] Bug
|
Size Change: +728 B (0%) Total Size: 1.15 MB
ℹ️ View Unchanged
|
|
Is it intended that the block stays highlighted after I drop it? Seems a bit weird to me personally, I thought the block highlighting was only needed when we "hover" the switcher. |
@youknowriad It's highlight as long as the BlockSwitcher / Move arrows are hovered or focused. At least, that's how the logic works right now :) |
Makes sense I guess after drag and drop the switcher/movers are focused. maybe there are better heuristics but definitely not the PR for it. |
|
Travis was complaining about snapshots 🤔 . I'm not sure what command I need to run to regenerate these (correctly). I tried |
|
Hmm! Not sure how to fix these E2E snapshot test failures 🤔 |
|
👋 I think I need some help with these E2E tests. For example, the Admin 1 test: GH actions is saying: However, then I run it locally: It seems okay: |
|
Hmm. I'm a bit stuck on this E2E test. It says that the snapshot doesn't match. In doing the interaction in real life (and also observing Chromium), it seems to work okay. useEffect( () => {
// On mount, we make sure to cancel any pending animation frame request
// that hasn't been completed yet. Components like NavigableToolbar may
// mount and unmount quickly.
if ( requestAnimationFrameId ) {
cancelAnimationFrame( requestAnimationFrameId );
}
return () => {
// Sequences state change to enable editor updates (e.g. cursor
// position) to render correctly.
requestAnimationFrameId = requestAnimationFrame( () => {
updateBlockHighlight( false );
} );
};
}, [ updateBlockHighlight ] );I've tried a bunch of things, including moving the functionality to the |
youknowriad
force-pushed
the
fix/toolbar-block-highlight-rendering
branch
3 times, most recently
from
e07f460 to
239e07a
Compare
|
I tried playing with the tests, added some timeouts... but unfortunately, I didn't learn a lot. I think it's a race condition between the "backspace to remove empty lists" and "backspace to work as undo after a transform". cc @ellatrix I noticed other bugs (on master too) that could be related: If you have "two empty lists" and click backspace when the last one is focused, both are removed. |
|
@ItsJonQ This doesn't fix the issue for me. If I select a block, press up/down, then select another block, and you'll see that the highlight is still on the previous block. |
|
@youknowriad Thank you so much for looking into the E2E for me 🙏 @ellatrix Thank you for extra context. Lemme see what I can do. Maybe it'll fix the E2E complaints 🤷♀️ |
|
@ellatrix Hmm! I'm not noticing the bug on my end - I'd love to reproduce it though! I recorded a screencast of me walking through the steps:
Let me know if I'm missing something! 🙏 |
|
Still figuring out what's happening with this test. I tried something that may point to regarding the delete action - something that @youknowriad touched upon. If I remove my custom // packages/block-editor/src/components/block-toolbar/index.js
const { toggleBlockHighlight } = useDispatch( 'core/block-editor' );
useEffect( () => {
return () => {
toggleBlockHighlight( blockClientId, false );
};
}, [ blockClientId ] );
// const toggleBlockHighlight = useToggleBlockHighlight( blockClientId );Test Failure: |
|
Revisited this one. Still unsure what's up 🤔 |
|
@ItsJonQ You're right it does fix the issue. |
| @@ -195,14 +195,15 @@ export function useToggleBlockHighlight( clientId ) { | |||
| if ( requestAnimationFrameId ) { | |||
| cancelAnimationFrame( requestAnimationFrameId ); | |||
| } | |||
|
|
|||
| return () => { | |||
| // Sequences state change to enable editor updates (e.g. cursor | |||
| // position) to render correctly. | |||
| requestAnimationFrameId = requestAnimationFrame( () => { | |||
There was a problem hiding this comment.
@ellatrix I paused on this for now, as regardless of whether I use RAF or not, a couple of particular E2E tests keep failing. They fail when they attempt to use the toggleBlockHighlight action in a useCallback return.
Edit: Nevermind. I think we may need RAF
ellatrix
added
the
Backport to WP 6.7 Beta/RC
|
@ellatrix Haii! @youknowriad and I hopped on a call to debug this. I think we discovered the issue. The The reason why it's double mounting is because it starts with an accessibility state of To make the tests happy, we opted to use a default accessibility state of cc'ing @diegohaz . Haii! It would be great to get your quick thoughts on the solution we went with. I know you've done extensive and wonderful work on the Toolbar 🙏 Thank you all ❤️ |
* Block Editor: Simplify Selection Reducer * Test selectionStart and selectionEnd reducers instead of internal selection reducer * Add comment when reducer resets state by default
|
@ItsJonQ I agree with defaulting the accessible state of the navigable toolbar to |
| * is true, as it seems to be the most common case. | ||
| * | ||
| * Transitioning from an (initial) false to true state causes the | ||
| * <Toolbar /> component to mount twice, which is causing undesired |
There was a problem hiding this comment.
I wonder if this is still needed after the adjustments I made?
There was a problem hiding this comment.
I think so. From our debugging, the Toolbar was double mounting regardless of the toggleBlockHighlight dispatch action.
| } ); | ||
| }; | ||
| }, [] ); | ||
| return () => toggleBlockHighlight( clientId, false ); |
There was a problem hiding this comment.
Why are we toggling block highlight in an effect rather than at the reducer level? For a SELECT_BLOCK action etc., the highlight state should change immediately. Cc @youknowriad.
There was a problem hiding this comment.
I agree that if we can do it automatically on the reducer, it's better and avoid an extra rerenders for all components.
|
OMG @ellatrix ! That did it! You are amazing 🙏 . Thank you so much!!! |
|
I retested it locally, and it looks like it's working for me! With things working and E2E happy, I'll merge this up! Thank you @ellatrix , @youknowriad , and @diegohaz for all of your help!!!! <3 |
* Editor: Fix block outline rendering after block is moved * Fix rendering for tests. Reduce Timeout from 250 -> 200 to improve UX. * Fix rAF call with toggling highlight * Resolve the E2E test issue, which appears to be caused by a double-mounting Toolbar * Use reducer Co-authored-by: Ella van Durpe <[email protected]>
youknowriad
removed
the
Backport to WP 6.7 Beta/RC
This update resolves the highlight rendering for blocks after they've been dragged and moved. The GIF demo above shows the highlight correctly disappearing after a block is moved, and another block is selected.
The solution involved removing the
requestAnimationFramewrapper that controlled the highlight state. It seemed like this was previously added to help with Toolbar rendering. I've done a bunch on my end. There does not appear to be any behavioural issues (or console errors) from removing thisrAFwrapper.How has this been tested?
npm run devChecklist:
Resolves: #23413