Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix: when enter is pressed on title unallowed blocks may be inserted #7265

Closed

Conversation

jorgefilipecosta
Copy link
Member

Description

When pressing enter in the title, the default block was always inserted. Even if it should not be possible to insert the default block because of a template lock or an allowed blocks change.

How has this been tested?

I verified that in normal post/situations pressing enter on the title still inserts the default block (the paragraph).
I verified that if a template lock exists pressing enter on the title does not insert blocks.
Template lock code used:
e.g:

function myplugin_register_book_lock_post_type() {
    $args = array(
        'public' => true,
	  	'template_lock' => 'all',
        'label'  => 'Books Lock',
        'show_in_rest' => true,
        'template' => array(
            array( 'core/image', array(
            ) ),
            array( 'core/heading', array(
                'placeholder' => 'Add Author...',
            ) ),
            array( 'core/paragraph', array(
                'placeholder' => 'Add Description...',
            ) ),
        ),
    );
    register_post_type( 'book_lock', $args );
}
add_action( 'init', 'myplugin_register_book_lock_post_type' );

I verified that if the default block is not allowed because of a filter in the allowed blocks, pressing when on the titles does nothing.
Block filter code used.

add_filter( 'allowed_block_types', function( $allowed_block_types, $post ) {
	if ( $post->post_type === 'post' ) {
	    return $allowed_block_types;
	}
	return [ 'core/image', 'core/button' ];
}, 10, 2 );

@jorgefilipecosta jorgefilipecosta added the [Type] Bug An existing feature does not function as intended label Jun 11, 2018
@jorgefilipecosta jorgefilipecosta self-assigned this Jun 11, 2018
@aduth
Copy link
Member

aduth commented Jun 11, 2018

Should we be restricting this globally from the store, not the UI? It all feels rather ad hoc, and it's not surprising that there's many cracks in unintentionally allowing additions.

@jorgefilipecosta
Copy link
Member Author

jorgefilipecosta commented Jun 11, 2018

Should we be restricting this globally from the store, not the UI? It all feels rather ad hoc, and it's not surprising that there's many cracks in unintentionally allowing additions.

I'm thinking about that too. But the problem is that the information needed to make this kind of restrictions is span across state trees e.g: nested block settings, editor settings, and even different stores because default block is part of the block store, not the editor store.

@aduth
Copy link
Member

aduth commented Jun 11, 2018

Makes me think again about something like the idea we'd discussed previously and commented in #6067 (comment), where insertion is managed by a top-level component and descendents integrate with it via context.

It'd also be fine for a middleware to access the disparate parts of the state tree (and other stores) and prevent it from being processed if it doesn't meet all conditions.

Finally, it could highlight that our store is more complicated than it needs to be if it touches so many parts to decide whether an insertion is possible, and should be simplified in general.

I don't think we're helping ourselves move in a positive direction by adding these conditions all over the place.

@jorgefilipecosta
Copy link
Member Author

Hi @aduth,

An inserter context that provides the allowedBlocks is not very different from querying the state to check if a given block is allowed, since the discussion, we have more easy to use selectors like canInsertBlockType. The context could provide an onInsert that does nothing if the insert is not possible but that would probably not be possible to use in this case because the post title would probably be out of the context.

It'd also be fine for a middleware to access the disparate parts of the state tree (and other stores) and prevent it from being processed if it doesn't meet all conditions.

I like this idea, and I think in cases like the one in this PR where if the insert is not possible we do nothing it may simplify the code. It may also simplify #7230. I remember that before we had some limitations on the usage of middlewares in our stores, I'm not sure if the situation changed, but I will give it a try.
With an inserter middleware at least we know that we will never insert unallowed blocks (provided the middleware works correctly).

That said I think we will still have some UI code with inserter logic on the UI for cases where we don't even want to allow the user to even try the insert. E.g., hide something if it is not possible to insert, don't allow drag to start, etc...

@jorgefilipecosta jorgefilipecosta changed the title Fix: when enter is pressed on title block unallowed blocks may be inserted Fix: when enter is pressed on title unallowed blocks may be inserted Jun 13, 2018
@jorgefilipecosta jorgefilipecosta added the [Status] Blocked Used to indicate that a current effort isn't able to move forward label Jun 18, 2018
@jorgefilipecosta
Copy link
Member Author

jorgefilipecosta commented Feb 19, 2019

Closing this PR, I will soon open a new PR that will address the same problem in a simpler way using controls, in a similar way to what was done in #13924.

@jorgefilipecosta jorgefilipecosta deleted the fix/respect-allowed-block-on-title-enter branch February 19, 2019 18:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Status] Blocked Used to indicate that a current effort isn't able to move forward [Type] Bug An existing feature does not function as intended
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants