Block list appender: use prevent default to prevent block selection

[ellatrix]

Description

In #19397, I replaced <IgnoreNestedEvents /> in the block list appender with event.stopPropagation() on the focus event. After thinking about the problem a bit, I realised that maybe we could use event.preventDefault() to do the same thing, where the default behaviour would be to select the block. Normally event.preventDefault() is used to prevent native default behaviour, but nothing stops an application from using the mechanism for its own default behaviours. The focus event is not cancellable, so normally event.preventDefault() is not used on a focus event. It exclusively cancels only our own selection behaviour.

It's worth noting that the event will now bubble further. event.stopPropagation() would prevent this from happening.

A cool side effect of this is that any block could now prevent blocks selection if it wanted to, but I wouldn't advertise this as an official API.

I think either we should do something like this, or find a way to both focus the appender and select the block in the same click.

How has this been tested?

Screenshots

Types of changes

Checklist:

• My code is tested.
• My code follows the WordPress code style.
• My code follows the accessibility standards.
• My code has proper inline documentation.
• I've included developer documentation if appropriate.
• I've updated all React Native files affected by any refactorings/renamings in this PR. .

[aduth]

It's an interesting idea. It raises the question: In what way is this practically any different from stopping the event propagation? Are there cases where we would want to capture a propagated event and ignore the fact that its default behavior was prevented? Especially when we're trying to expand what it means for default behavior to include some of our own custom use-cases.

I might worry as well that it could muddy what it means for the default behavior of an event to be prevented, if a developer has some prior expectation around those effects (e.g. I might expect that event.preventDefault() in respond to a keydown event in an input might prevent the character from being added, but I wouldn't necessarily expect it to have some ramification on the editor application).

In these cases where we have control over both ends of the operation (here, we both preventDefault and check defaultPrevented), I wonder if it's something where we might be comfortable to add custom properties to the event. IgnoreNestedEvents did this with its _blockHandled property. An advantage here is we can assign some semantic meaning between the property and its expected effects. Possible disadvantages are that its an invalid property for an event (a possible complication for future TypeScript efforts), interoperability with React (see also event pooling and use of nativeEvent in the IgnoreNestedEvents example), and possible developer-facing confusion stemming from standard-vs.-non-standard event properties.

[ellatrix]

In what way is this practically any different from stopping the event propagation?

It is different, yes. The event now propagates so it can be listened to elsewhere. The difference is that now the block explicitly decides to ignore it rather than the appender deciding that is should be ignored everywhere.

Are there cases where we would want to capture a propagated event and ignore the fact that its default behavior was prevented?

Perhaps, I'm not sure. I'm just trying to avoid using stopPropagation as I'm think it's something that should be avoided. Something higher up the DOM tree could be generally listening for focus events for an entirely different purpose, and then there would be silence if you click on the appender.

I might worry as well that it could muddy what it means for the default behavior of an event to be prevented, if a developer has some prior expectation around those effects (e.g. I might expect that event.preventDefault() in respond to a keydown event in an input might prevent the character from being added, but I wouldn't necessarily expect it to have some ramification on the editor application).

I think it's different for the focus event. You can't prevent default behaviour here. Focus will always be set by the browser. So event.preventDefault() normally doesn't do anything, so it's "available".

In these cases where we have control over both ends of the operation (here, we both preventDefault and check defaultPrevented), I wonder if it's something where we might be comfortable to add custom properties to the event.

That's a solution too, and it pretty much the same thing to set a property or use preventDefault. In fact, IgnoreNestedEvents inspired me to use preventDefault. I'm fine with either approach.

[aduth]

I suppose I should clarify that I'm approaching this from a perspective that this might be a pattern we'd want to consider leveraging for other use-cases as well.

So for something like:

The difference is that now the block explicitly decides to ignore it rather than the appender deciding that is should be ignored everywhere.

... in asking about the practical difference, it is specifically about whether we would actually want this to be a decision made for others. I think a potential risk is that if we don't, are we assuming that those event handlers should always be wary check the defaultPrevented ? As you note, I think it could depend.

That's a solution too, and it pretty much the same thing to set a property or use preventDefault. In fact, IgnoreNestedEvents inspired me to use preventDefault. I'm fine with either approach.

I agree with you on this point. I think the main difference for me is that it provides some opportunity to assign some meaning through the property naming and to distinguish it from the browser-native default behavior.

[ellatrix]

it is specifically about whether we would actually want this to be a decision made for others.

Right, either way, we are making a decision for others. Ignore the event by default, or not. I'm leaning towards not ignoring the event by default and explicitly opting in to ignoring it.

Say I have a little tool on the page that tells me which element is currently focussed (just a stupid example). This tool cannot even opt-in to receiving the event when clicking on the appender if we stop propagation (if my thinking is correct).

I think the main difference for me is that it provides some opportunity to assign some meaning through the property naming and to distinguish it from the browser-native default behavior.

I like either approach, I think we should just try one of them. Do you see big problems with a custom property? Otherwise I can swap it out for a custom property.

[ellatrix]

To be honest, I think ideally we should avoid doing anything at all. Maybe it's ok for the block to get selected. Something is preventing the appender to also be activated. Maybe that's the issue that should be fixed instead.

[aduth]

To be honest, I think ideally we should avoid doing anything at all. Maybe it's ok for the block to get selected. Something is preventing the appender to also be activated. Maybe that's the issue that should be fixed instead.

I think this ties in to some of what I was sharing at #19397 (comment) and #19515 (comment), where it could be argued either way whether pressing the Inserter here should select the block. I think it could be argued (especially evidenced by the fact that we have to go out of our way to prevent the default behavior) that clicking the Inserter should select the block, and I would agree with you that we should at least understand what it is about this behavior which conflicts with the default appender behavior.

Something we might want to be considerate about: In the case that we click an appender in multiple levels of nesting, which block should be selected? In fact, the Column example should be sufficient for consideration: Should the Column parent or the Columns grandparent become the selected block? Is this prone to some conflict about which would "win" ?

[ellatrix]

@aduth I found the problem, I think, but I'm not sure how to fix it.

gutenberg/packages/block-editor/src/components/inner-blocks/button-block-appender.js

Lines 7 to 13 in 0db5ebb

	export const ButtonBlockAppender = ( { clientId, showSeparator } ) => {
	return (
	<BaseButtonBlockAppender rootClientId={ clientId } showSeparator={ showSeparator } />
	);
	};
	export default withClientId( ButtonBlockAppender );

This component is rerendering, which down the line rerender the callback before it can execute. I tried passing consistently the same root client ID, but for some reason the component keeps rerendering.

[aduth]

I tried passing consistently the same root client ID

By this do you mean testing with a static rootClientId ? I kinda wonder if there's some issue here with how withClientId, seeing that it operates from context (maybe updating too frequently?) and that at least its implementation with pick would yield new objects each time (though based on how they're applied by spread I wouldn't necessarily think this would be an issue).

[ellatrix]

@aduth, right, currently the root client ID changes when you move selection. I fixed that by making it always the same, which also removes withClientId, yet the component is still rerendering even though it haves the same props.