Move insertionPoint state to block-editor store/rename existing insertionPoint to insertionCue #65098
Conversation
|
Size Change: +607 B (+0.03%) Total Size: 1.77 MB
ℹ️ View Unchanged
|
|
I was also trying to find the right solution for this issue and came to a similar conclusion. We should probably fully revert #64048 and work on an alternative fix for #63866. Why revert? Zoom-out mode is still experimental (behind the settings flag); experimental features should hinder stable ones. |
|
@Mamaduka - I'm fine reverting #64048 as well. I've been looking at this most of the day and feel like there's a deeper issue at play. The insertion point is bound to the editor, and gets set when the inserter opens. The idea of the inserter staying open and the insertion point changing wasn’t a thing when this was designed, because the inserter was supposed to close and reset when focus was moved out of it. Now that it stays open and you can interact with the canvas, we have this muddling of states where the insertion point can change while the inserter is open. |
|
I worked more on this today to move the insertion point into the block editor package. It's working correctly for the zoom out inserters. I really dislike the naming of all the things as it's so hard to untangle which insertion point means what :/ |
|
cc @WordPress/gutenberg-core |
| * @param {string} value.filterValue A query to filter the inserter results. | ||
| * @param {Function} value.onSelect A callback when an item is selected. | ||
| * @param {string} value.tab The tab to open in the inserter. | ||
| * @param {string} value.category The category to initialize in the inserter. |
There was a problem hiding this comment.
These are not new, just adding the documentation for things already in use.
| - _value.filterValue_ `string`: A query to filter the inserter results. | ||
| - _value.onSelect_ `Function`: A callback when an item is selected. | ||
| - _value.tab_ `string`: The tab to open in the inserter. | ||
| - _value.category_ `string`: The category to initialize in the inserter. |
There was a problem hiding this comment.
These are not new, just adding the documentation for things already in use.
| } else if ( | ||
| insertionPoint?.insertionIndex && | ||
| insertionPoint?.rootClientId | ||
| ) { | ||
| _destinationRootClientId = insertionPoint.rootClientId; | ||
| _destinationIndex = insertionPoint.insertionIndex; |
There was a problem hiding this comment.
Putting this after the insertionIndex or clientId to preserve the existing API. This lets anyone who passes these values in via the props to still have full control.
| export function setInserterInsertionPoint( value ) { | ||
| return { | ||
| type: 'SET_INSERTER_INSERTION_POINT', | ||
| value, | ||
| }; | ||
| } | ||
|
|
There was a problem hiding this comment.
I don't like this naming, but it's at least clear 🤷🏻 I'm concerned that this assumes only one instance of the inserter, while the editor package way passes it in. That said, it's still possible to manually pass the inserter the root block and index and it will be respected.
| case 'SELECT_BLOCK': | ||
| return null; |
There was a problem hiding this comment.
Clear the insertion point after a new block selection.
There was a problem hiding this comment.
In my testing, the vertical displacement is still visible after selecting a different block.
jeryj
requested review from
ellatrix,
ajitbohra,
juanmaguitar,
fabiankaegy and
ndiego
as code owners
|
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. |
jeryj
changed the title
jeryj
force-pushed
the
fix/insertion-point-hover
branch
from
392038a
to
a90bb7f
Compare
jeryj
changed the title
|
Flaky tests detected in 98cf472. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/11055136565
|
|
Performance charts after the revert PR was merged. We should monitor "Inserter Hovering Items" results while working on the fix. Screenshot
|
There was a problem hiding this comment.
I noticed something weird that I can't reproduce on trunk. I'm not sure if it's related to this PR nor what's going on. The testing environment is Playground.
Kapture.2024-09-11.at.16.29.35.mp4
Notice how sometimes selecting the pattern will insert it at the wrong place. And some other times hover over sections will rerender the patterns inserter.
| _destinationRootClientId = insertionPoint?.rootClientId | ||
| ? insertionPoint.rootClientId | ||
| : rootClientId; | ||
| _destinationIndex = insertionPoint.insertionIndex; |
There was a problem hiding this comment.
The other conditions have comments to explain them. Could we add one here?
| setInserterIsOpened( { | ||
| rootClientId, | ||
| insertionIndex, | ||
| filterValue, | ||
| onSelect, | ||
| } ); | ||
| setInserterInsertionPoint( { rootClientId, insertionIndex } ); |
There was a problem hiding this comment.
We could have the editor stores setInserterIsOpened dispatch the setInserterInsertionPoint to the block editor store as part of the action. That said it might be clearer this way as the "set inserter is opened" communicates "toggling" rather than "setting an insertion point".
There was a problem hiding this comment.
I think we might want to setInserterInsertionPoint without setInserterIsOpened as well - for example if you're using the quick inserter.
There was a problem hiding this comment.
But what about backwards compat on the setInserterIsOpened API?
There was a problem hiding this comment.
What I mean is that I think its good to have the flexibility of calling these independently rather than tying one to the other.
packages/editor/src/store/actions.js
Outdated
| * @param {string} value.rootClientId The root client ID to insert at. | ||
| * @param {number} value.insertionIndex The index to insert at. |
There was a problem hiding this comment.
We could continue to make this backwards compatible by dispatching the new action to the block editor store if the rootClientId or the insertionIndex are provided as part of value. You could also fire a deprecated notice in the console.
There was a problem hiding this comment.
This API has changed. It's a public API. We have to ensure it's backwards compatible.
There was a problem hiding this comment.
You can still pass these things, they just don't do anything within our implementation of the block editor. I'll add a deprecation and fire that though.
There was a problem hiding this comment.
...they just don't do anything
That was the bit I was concerned about because if a public API used to do something it needs to continue to do that.
Looks like you fixed it up now 👍
There was a problem hiding this comment.
We can’t deprecate a public API/feature and suggest to use a private API instead.
It would be non-actionable for consumers.
There was a problem hiding this comment.
I think it would also be fine to not deprecate any of it. You can still pass it those props, it just won't do anything in our codebase 🤷🏻
There was a problem hiding this comment.
Yeh actually we don't need to deprecate it do we? @Mamaduka is right that we can't point to a private API - great spot there.
|
I have a couple of surface notes:
Screenshot
|
|
@jeryj, I appreciate all the work done here, but I think getting broader feedback from the core team would have been nice. |
|
@Mamaduka - I'd be love to have gotten more feedback on this as well. It had been open long enough that I didn't think I was going to hear anything else, and it was also blocking follow-up work on insertion point work with the zoom out view. Very happy to iterate or follow-up with any of it! Fortunately, I ended up finding a way to not change any public APIs, so everything is possible to iterate or undo. |
| * | ||
| * @return {Object} Action object. | ||
| */ | ||
| export function setInsertionPoint( value ) { |
There was a problem hiding this comment.
How is this different than showInsertionPoint?
There was a problem hiding this comment.
Ok, so If I'm understand properly we have:
showInsertionPointwhich is the exact location where the block will be inserted. So the true insertion point.- and we have
setInsertionPointwhich is about the top level "inserter" component. IT's about setting the "default" insertion point (if blocks are not allowed to be inserted there, we compute the true insertion point and show it using showInsertionPoint)
is that correct?
There was a problem hiding this comment.
Clarifying my understanding more:
The "inserter insertion point" state was being set in the "editor" package because the "Inserter" component has a "rootClientId" and "clientId" props, so this one is controlled externally, I guess the idea is that at the same time, you can potentially render different inserters with different default insertion points. The current PR seems to move the state to "block-editor" but the props remain so it's still "externally" controlled right? So why the move to "block-editor" state.
There was a problem hiding this comment.
How is this different than showInsertionPoint?
showInsertionPointmakes the insertion cue visible. It is not code-wise related to the insertion point, just a visual indicator.
showInsertionPoint which is the exact location where the block will be inserted. So the true insertion point.
No, this only shows where the blue line will get shown (what I will now refer to as the insertion cue for clarity). We don't have a true insertion point at the moment other than passing an insertion point to useInsertionPoint (what the sidebar inserter was doing) or via the new setInsertionPoint added in this PR.
The current PR seems to move the state to "block-editor" but the props remain so it's still "externally" controlled right? So why the move to "block-editor" state.
Exactly. No props were removed and a manually passed insertion point to the inserter will still be respected. We needed to move it to the block-editor state to clear the insertion point when a block is selected.
This reverted PR demos more about why we need this. Code-wise, the inserter was built with a static insertion point in mind. Now the inserter can be open with selection on the block canvas happening, meaning the insertion point can be updated while the inserter is still open.
The problem with the PR above is that it relied on getBlockInsertionPoint which updates constantly on hover, so we had to revert it.
I wrote an overview of the Insertion API current state here: #65290
| * @param {Object} state | ||
| * @return {Object} where the insertion point in the block editor is or null if none is set. | ||
| */ | ||
| export function getInsertionPoint( state ) { |
There was a problem hiding this comment.
How is this different than getBlockInsertionPoint?
There was a problem hiding this comment.
getBlockInsertionPoint is really about the blue insertion cue that gets shown. getBlockInsertionPoint is constantly updated based on where you're hovering the canvas. Maybe originally getBlockInsertionPoint was about where the insertion would happen, but its code now is tied to where the insertion cue shows.
There was a problem hiding this comment.
What's the difference for you between "insertion cue" and "insertion position". If we're showing a different insertion cue than the actual insertion point, then we have problem.
There was a problem hiding this comment.
The difference is that one is an index and the other one is an UI element?
There was a problem hiding this comment.
Both are state for me. The UI element is just a reflexion of a state.
There was a problem hiding this comment.
I think it gets tricky when you include drag and drop. You’re showing an insertion cue at a location that may be different than what is set via selection.
If you update that point on hover via drag and drop, this is the correct behavior for the insertion cue/insertion point, but a lot of the things built off using it were not intended to be updating their insertion point that often.
So, I think insertion cue of where insertion will happen and should rely on insertion point if set, but the insertion cue should be able to show separate from the insertion point on instances like drag and drop. Hovering the canvas should not be updating the insertion point, it should be updating a cue about where you might be able to insert.
There was a problem hiding this comment.
So, I think insertion cue of where insertion will happen and should rely on insertion point if set, but the insertion cue should be able to show separate from the insertion point on instances like drag and drop. Hovering the canvas should not be updating the insertion point, it should be updating a cue about where you might be able to insert.
I agree with this, but it doesn't really explain the PR to me. we were using showInsertionPoint for me before this PR to do this and this was already different than the "default" insertion point used in the inserter component.
So I'm still not clear what this PR is trying to accomplish aside moving things around and potentially making them more complicated.
|
One thing I'm not sure I understand is how is this blocking things in zoom-out? |
We need to be able to update the insertion point with the inserter panel still open. This isn't zoom-out specific, as it's useful for the main canvas as well. We also have work on the zoom out separators that is behaving in a buggy way, and being able to get a clear insertion point will be very helpful. |
Can you expand more, I'm not sure how the current change is related to that. Because the previous |
You could change the insertion point via the old There very well may be better way to handle this, and I'm very open to that. The way I went with here seemed like the best first step towards clarifying and increasing flexibility of the insertion methods. |
There are two ways to look at it and both are valid: 1- We want for the "block-editor" itself to control the state "default insertion point" of the global inserter. That is definitely a valid approach, in which case, you'd need a selector like this PR adds and also you'd need to use that selector consistently and remove the Before this PR, we used to have 2. With this PR, we tried to move towards 1 but instead we endup with a mix of both. In my opinion, this PR is creating more confusion than solving it. We still didn't achieve 1 fully and IMO we have no reason to move towards 1 at the moment. (adding selectors, changing APIs...). I'm not sure I see a "conceptual" bug in approach 2 that forces us to reconsider it and move towards approach 1. |
|
@youknowriad @jeryj isn't this part of Refactor Insertion API? Because if so it should link to it and explain if it closes that or advances it. If it advances it maybe this is just an intermediary state we need, and pherhaps the discussion should be moved there instead of here? |
|
@youknowriad - without moving the insertion point to the block-editor, how would you recommend solving the issue of blocks being inserted in the wrong place when selecting a different block? This was the cleanest way I could see to do it, but I’m very open to there being a better way. @draganescu - moving the conversation to the issue be great, and it does move things forwards. |
Can you clarify which inserter is that? I don't know zoom-out mode enough. |
|
It happens with any click that sets the insertion point. Here's a video reproducing the bug that this PR fixes. Screen.Recording.2024-09-30.at.10.22.33.AM.mov
Here's the same flow with this PR merged: Screen.Recording.2024-09-30.at.10.27.49.AM.mov
|
|
@jeryj Thanks for sharing the video, and the issue, this is a more convincing story and I do see why you decided to move the state to block-editor to achieve that second flow. That said, I wonder if actually both approaches are flawed here :). Let me explain: Previously, the inserter was open like a modal and closes when you insert, which means, if you open it using Now, in the second video, you're trying to argue that now that the inserter stays open, if you select another block, you should also change the "insertion point" of the inserter. But for me there's an ambiguity here. When I click the in-between inserter and "browse all", that block is not necessarily selected but the inserter still insert into that position that differs from the selected block. Here's my take: what if clicking the "in-between inserter" actually selected the block instead of "setting the insertion point". For me that would be the best of two worlds:
Am I missing anything? |
While that would simplify the code, I have some concerns about unintentional side effects. The inserter responds to the selected block to filter results and allowed blocks, etc. However, maybe it all works well and simplifies it! I'll open up a PR and we can find out :) |
|
I don't think we can rely on only block selection because of instances where we could validly insert inside of or after a selected block. The group block is the best example of this. We need to be able to insert inside of and after it. If I use the group block inner appender, block selection would land on the group block. One is intended to insert inside of the group and one is intended to insert after the group:
In the above two scenarios as the codebase currently is, I don't think we could infer where to insert the block based only on block selection. The only way I could see this working via block selection is if we changed the insertion process to:
I think that might be similar to how the navigation block works, but I don't see that as simpler. Lmk if you see a different route here! |
|
you're right in the case of the appender within an empty container block, there's no easy way to translate "selection" to "insertion point". It's weird though, because let's say you clicked an "in between inserter" -> "browse all" to open the inserter and then you went on with your life and clicked around while the inserter is open. It's very unclear where should the block be inserted in that case. |
I think bringing over the idea of the zoom out separators to the non-zoomed out view could work. 
|
What?
In the block editor store:
insertionPointreducer toinsertionCue(as it is where the blue line shows up not where insertion is guaranteed to happen).insertionPointto be used to manually pass an insertion point (such as when clicking an inline inserter button like the blue zoom out inserter buttons)setInsertionPointactiongetInsertionPointselectorIn the editor store:
setIsInserterOpeneddispatchessetInsertionPointto the block editor storegetInsertionPointtogetInserterto more clearly communicate this is the state of the inserter, not solely about the insertion point.Why?/How
Now that the inserter can remain open while interacting with the block canvas, it is expected that when you select a new block, the insertion will occur after the selected block.
The insertion point is handled by the editor package via
setIsInserterOpened( value ), which can take an object to open the inserter to a certain tab panel and also provide the place where the insertion should happen. We want to change this insertion point when selecting a new block on the canvas. Block selection is handled via the block editor package. Since the block editor should not know about the editor, and we can't rely on theSELECTION_CHANGEaction to clear the insertion point within the editor store, I've moved the insertion point data to be within the block editor package.In order to clear up confusion, I've also renamed some state and selectors:
insertionPointstate becomesinsertionCue, as this more accurately names it. It was not the insertion point, but the place where we want to show the blue line on the canvas.insertionPointto be used for manually setting an insertion point, if needed, such as when opening the inserter via one of the inline inserter buttons.getInsertionPointis renamed togetInserter, as this was a point of confusion. It is only the insertion point if the inserter is open and becomes stale data as soon as another interaction on the canvas happens. This is about the sate of the inserter when it is opened, and if it should still be open.getInserteris a better name, and it could still be better.getInserterPanelmaybe?How?
state.insertionPointtostate.insertionCuein the block editor store and related action/reducers.state.insertionPointto store when an insertion point is manually passed.setInsertionPointaction in the block editor store whensetIsInserterOpenhas an root index and insertion point set.insertionPointstate onSELECT_BLOCKgetInsertionPointtogetInserterto more accurately communicate that this is the state the inserter initialized to rather than the final insertion point.Testing Instructions
For pattern insertion points in Zoom Out
Default Editing
Testing Instructions for Keyboard
Screenshots or screencast