Refactor Insertion API

[jeryj]

Insertion point can mean a lot of different things, and it's very easy to get confused about which action to dispatch or which selector to use. For example, here's what we have in the block editor and editor packages for insertion points:

• showInsertionPoint
• hideInsertionPoint
• getBlockInsertionPoint
• isBlockInsertionPointVisible
• setInserterIsOpened
• __experimentalSetIsInserterOpened
• getInsertionPoint
• __experimentalGetInsertionPoint
• useInsertionPoint

These can be broken down into separate concepts:

• Insertion location
• Insertion cues

There's overlap as they need to communicate with each other.

Insertion Cues: Current state of the API

One of example of these is the blue lines on the canvas that show when you hover around the canvas or hover a block in the inserter. It shows you where you'll be inserting to. To show an insertion point on the canvas, you'd emit the showInsertionPoint action. To hide it, hideInsertionPoint.

If you need to know where this insertion point is showing up on the page, which selector do you use?

If you guessed getInsertionPoint, you'd be wrong. That's for the state of the sidebar inserter. You use getBlockInsertionPoint to access what was set in showInsertionPoint.

But, if there's no data set from showInsertionPoint, then getBlockInsertionPoint will return the next best place to insert.

1. If there's a block selection, it'll return the position after the selection.
2. If there's no block selection, it'll return the position af the end of the block list.

With getBlockInsertionPoint we're starting to blur the canvas insertion cues with where a block will insert.

To check if an insertion cue is showing or not, use isBlockInsertionPointVisible.

For example, if I want to know where the block might insert, I can use getBlockInsertionPoint. If I want to know if the block insertion point is showing and where it's showing, I'd have to use isBlockInsertionPointVisible() && getBlockInsertionPoint().

Insertion Location: Current state of the API

You can insert blocks in plenty of ways, but for this we'll focus on inserting via the quick inserter and the sidebar inserter.

If you hover the canvas to show the blue line and inserter, you can click the + button that shows up to open the inserter. If you insert a block there, then it uses its own internal getInsertionIndex function to determine where to insert. No interaction with the store.

If you click the "Browse all" button, it emits the setInserterIsOpened to the editor store with an object defining where we should insert.

const onBrowseAll = () => {
		setInserterIsOpened( {
			rootClientId,
			insertionIndex,
			filterValue,
			onSelect,
		} );
	};

These insertion points are passed down into the inserter via the editor as getInsertionPoint (which is reading from setInserterIsOpened). This point is passed down as a prop into the inserter sidebar to useInsertionPoint when we click on a block to insert.

When we hover blocks, we see the location they'll insert on the canvas with the inserter cues above. Hovering a block emits the showInsertionPoint which will make the blue line appear on the canvas. However, that point might be different than what is returned from getInsertionPoint, as that's only the location the inserter initialized to.

Not only that, but when you click the block, it goes through logic in useInsertionPoint that may be a spot different than getBlockInsertionPoint OR getInsertionPoint. There isn't a way to know where useInsertionPoint will insert something unless you directly pass it a clientId or root and index to insert into.

Issues with the current API

Naming

The big one here is getInsertionPoint vs getBlockInsertionPoint. getInsertionPoint does not return the values from showInsertionPoint. That's getBlockInsertionPoint (maybe, if the insertion point is set).

Insertion Point is in the editor store

setIsInserterOpened in the editor package opens and sets the insertion point. We can't change this insertion point from the block-editor store, and most of the time that's where we need to access this info. The information around insertion should be stored within the block-editor store, not the editor store. We ran into an issue of not being able to clear the insertion point when selecting a new block, because block selection is in the editor package and insertion point is in the editor package.

getBlockInsertionPoint muddles concepts of insertion cue and insertion point

getBlockInsertionPoint is either returning:

• The state of insertionPoint (which is the state of SHOW/HIDE_INSERTION_POINT, not the actual point of insertion)
• Fallback logic for where it thinks something should get inserted (after selected block or end of the block list)

Neither of these means "the place where a block where will be inserted." It means, "a place a block inserter might be showing or a place it might fallback to being inserted into."

Separating out the actual insertion point vs the insertion cue will be really helpful.

Also, because hovering blocks updates SHOW_INSERTION_POINT, getBlockInsertionPoint is constantly firing. We don't need to update an insertion point, we're just wanting to show a cue at an insertion point.

Potential Changes

While this is all confusing, it worked well enough until we recently allowed the inserter to stay open. This means you can now change your insertion point with the inserter still open. For example, if you:

• click a blue inserter +
• browse all to open the inserter
• click another block on the canvas

we should now be inserting after the selected block, not where you initially opened the inserter.

Here's my potential ideal. I don't know how feasible it is or if this is a good idea. But I'd like it to start the conversation.

Block Editor Store

• setInsertionPoint: Sets where we want block insertion to happen.
• getInsertionPoint: Gets the place we want a block to be inserted. Should this also receive an optional blockName? It will return the closest available place that block could be inserted. Includes the fallbacks if there is no explicit insertion point. This is the source of truth for inserting, and will be used by useInsertionPoint unless it is given a specific point.
• showInsertionCue( index, rootClientId, blockName ): Show blue line at a specific location for an optional blockName. If no insertion point or block is passed, then show it wherever is returned from getInsertionPoint.
• hideInsertionCue: Hide the cue.
• isInsertionCueVisible: Is it showing?

Editor Store

setInserterIsOpened: Open the inserter and optionally specify the open state of the inserter, like do you want the pattern or block tab showing?

Implementation Changes

• useInsertionPoint relies on getInsertionPoint unless passed a specific index rather than having its own fallback logic.
• setInsertionPoint happens when you click the inline insertion cue +, not after "browse all" and relies on getInsertionPoint to reduce duplicated logic.
• Renaming lots of things :)

To Refactor or Not Refactor?

I would not want to move forward with such a refactor unless there were clear benefits. In this case, I think there are. There are have been a lot of bugs recently come up that are traced back to confusion over getInsertionPoint vs getBlockInsertionPoint and making a mistake can cause serious performance issues. For example, to know where a block will be inserted, it makes sense to use getBlockInsertionPoint, however, this value updates rapidly when hovering around the canvas.

[jeryj]

I might prefer just renaming and then removing the getBlockInsertionPoint from returning a fallback.