Edit Post: Add block management modal

[aduth]

Closes #14139

This pull request seeks to add a new "Manage Blocks" option to the editor menu which, when clicked, will present the user with an options modal allowing them to enable and disable blocks from being made available for use in the editor.

Open Questions:

Usability Questions:

• How should we handle block types which are either contextually available in the inserter, or are never made available? For example, child block types like "Column", deprecated block types like "Subheading", or inline block types like "Inline Image".
• If a theme or plugin filters allowed block types, how should this interoperate with the disabled block types? Should those excluded blocks be shown and made "perma-disabled" (i.e. the user can see the block type as disabled, but cannot enable it), should they not be present at all, or should they serve as a default where the user could re-enable them?
  • Opinion: I don't think it would be advisable to allow a user to re-enable them, as plugins may make assumptions around block content structure around e.g. a custom post type.
• I may anticipate some feedback around whether the block disabled appearance in the manager is too subtle; if it needs to be changed, should we apply those changes consistently to the Inserter menu as well for disabled inserter blocks?
  • Similarly, there is a departure from design mockups in Block Management: Add the possibility to hide/show blocks from the inserter #14139 in the placement of the "Disable all" checkbox, where my current implementation chose to inherit UI from the Inserter/Transform for consistency's sake. Or would we prefer to break from those expectations for the "Disable all" button placement?
• How sticky should the setting be, and for whom should it apply? It is currently implemented as a user preference which persists across post editor sessions. Notably, this means the disabled blocks would not be remembered for a future implementation of a block editor embedded in the widgets screen.
  • Responding to @mapk 's remark at Block Management: Add the possibility to hide/show blocks from the inserter #14139 (comment) "I do love that if a block is in use, when clicking 'Disable all,' that particular block doesn't disable." : A problem is that this behavior is post-specific, which would conflict with the idea of the disabled block setting persisting across post editing sessions. Phrased differently, if I'm seeking to disable a particular block type, should I be conscious of the current post or not? It feels separate from the editing experience of a specific post.
  • Is it correct to think about this as a user preference, or would it be more appropriate as a site option? For example, a site administrator could foreseeably want to disable a particular block type for all users on a site. This could be considered as an additional enhancement, and if so, should it be deferred for a future implementation?

Technical Questions:

• Relating to the last top-level usability question above, this is implemented as a combination of preferences and components within edit-post. Depending how broadly this should apply, I think it could also be appropriate instead in editor or block-editor, or perhaps even its own separate module.
• While there's a good amount of reuse of existing components made possible, there's also some repetition and some questionable interfaces:
  • BlockTypesList:
    • Seems like a useful component, but I don't like how it's tied to the "inserter item" object shape, especially given the name of the component
      • Should the component be refactored to accept blockTypes as a prop? The inserter-unique behaviors are showing the "stacked" appearance for block types which can have children inserted within, and being able to make an item appear/act as disabled. Perhaps a pattern of function children could be leveraged to customize the behavior for each context?
    • Search behavior is duplicated from InserterMenu (normalizing search terms)
      • Should search implementation be embedded in the BlockTypesList component?
      • Should search be exposed as a selector(s) within e.g. core/blocks ?
    • While "disabled" could be a shared characteristic, the behavior is not the same. In the Inserter / Transforms menu, it disallows selection. In the Block Manager, it should act more as an active/inactive toggle

Testing Instructions:

Verify that you can disable/enable individual blocks/whole categories of blocks from the Block Manager, found under More Options > Manage Blocks.

Disabling a block should prevent it from being inserted within the post. (Note: It does not currently have an impact on blocks already within a post, even if those blocks become disabled by the user)

[youknowriad]

Can you clarify what does an enabled block type mean in this context and how it relates with other disabling mechanisms like "allowed_block_types" filter?

• If the block is already in the content, is it rendered properly?
• Can I transform to this kind of block?
• Should we make exceptions for the "default block" and the fallback one? it doesn't feel right to allow disabling these blocks.

I think at the moment, the various ways of disabling blocks are confusing and we should have this documented somewhere in the handbook to allow us to have all the same understanding of these mechanisms.

[aduth]

Can you clarify what does an enabled block type mean in this context and how it relates with other disabling mechanisms like "allowed_block_types" filter?

There's more detail in the original comment. Essentially, everything here builds upon allowed_block_types. The preference acts as a blacklist atop the allowed_block_types whitelist, where the difference is calculated when rendering the <EditorProvider> settings from within edit-post. An "enabled" block type, at least in the sense of this store action, is the act of omitting a block type from this blacklist.

[youknowriad]

The behavior of allowed_block_types is still unclear to me. Originally it was meant to be used to hide things from the inserter but I think it evolved into hiding the block entirely (transforms, inserter, default blocks ...) but still keep it registered for posts already using it.

[aduth]

As far as I can tell, the intent is to omit is as an option everywhere where it can result in it being inserted, as you point out. They're always still registered. For posts which already contain those blocks, I'm not sure we'd care to be destructive with those "invalid" blocks anyways.

[mapk]

Really quick turnaround on a PR, @aduth! Thanks!

How should we handle block types which are either contextually available in the inserter, or are never made available?

If the block is visible in the Inserter, it should be included here. I don't think child-blocks are necessary to be displayed in this modal.

If a theme or plugin filters allowed block types, how should this interoperate with the disabled block types?

This and so many other good questions, Andrew, make me think that maybe this just becomes a "Toggle Inserter Visibility" option. In this case, the Block Manager would just hide blocks from the Inserter instead of actually removing anything from the pages/posts. This would mean that "disabled" or "hidden" blocks can still be used in the post or page, just not evident in the Inserter. I'm just throwing this idea out there for some conversation. If we say "disabled block" then I'd assume this block would not be used going forward, but if used anywhere in the past, it would remain functional.

How sticky should the setting be, and for whom should it apply?

I can see this being a user preference at this point, and persisting across all that user's experiences (ie. widgets screen too).

Similarly, there is a departure from design mockups in #14139 in the placement of the "Disable all" checkbox, where my current implementation chose to inherit UI from the Inserter/Transform for consistency's sake.

I think the inclusion of the accordion here works well, but the toggle will need to be redesigned or tied in a bit better.

[mapk]

I may anticipate some feedback around whether the block disabled appearance in the manager is too subtle; if it needs to be changed, should we apply those changes consistently to the Inserter menu as well for disabled inserter blocks?

A great question around a11y of disabled blocks. I've been thinking this through a bit. We should probably display a "disabled" block in another way rather than dimming them out into a non-accessible state. And adding a background to identify a disabled state doesn't seem quite right b/c it makes it more obvious as if selected. Still thinking about this...

@LukePettway If you have any thoughts about this please share.

Here's a screencast of what's in question here.

[talldan]

Looking good. I noticed a slightly unusual behaviour in testing when disabling the paragraph block (likely unrelated to this PR, but it's definitely more noticeable):

1. Start a new post
2. Disable paragraph block from the modal
3. Add a block using the alternative button-style appender
4. Remove that block.

Expected: block is removed and the the alternative appender is shown in its place
Actual: The default paragraph style block appender is shown in place of the removed block, even though it's disabled.

Happy to make this a separate issue.

[talldan]

autoFocus doesn't seem to have the desired effect, at least in my testing.

I think this is because the modal has a focusOnMount prop that defaults to true and it overrides this input. Setting that to false seems to fix it.

[gziolo]

Expected: block is removed and the the alternative appender is shown in its place
Actual: The default paragraph style block appender is shown in place of the removed block, even though it's disabled.
Happy to make this a separate issue.

Yes, it's unrelated to this PR. This new feature makes it only more prominent :)

Should we make exceptions for the "default block" and the fallback one? it doesn't feel right to allow disabling these blocks.

It only was something that Riad anticipated when sharing his feedback.

It makes me wonder whether we should lock some blocks from being removed in the first iteration? @mapk do you have some thoughts on how this could look visually? I'm picturing it myself as something grayed out with a lock icon on top of it.

[Luciaisacomputer]

@mapk I see what you mean. WCAG requirements for contrast are less strict on disabled elements but in this situation the real concern isn't the disabled state necessarily but how close the color is between active and inactive. (This is is actually a situation where I disagree strongly with WCAG 💯).

When there are extras on buttons like borders, background colors, and shadows it's a lot more obvious when you remove those and I think thats what makes these a little harder to tell the difference between states. We are essentially relying on color alone which isn't helpful.

Visually you could have text within each button like Paragraph (disabled) where the disabled text is wrapped in a visual only class so that screen readers don't duplicated that when focusing the control since the button will most likely be disabled at an attribute level.

Just a few thoughts, I'll keep thinking on this one a bit more.

[MDWolinski]

Had a couple of thoughts on this (new here, so...). My apologizes if overstepping previous discussion.

What's the value of the having a Disable All toggle in the interface? It seems to me that it's incredibly small the number of people who would want to disable all blocks at once. The one exception being that it's a short cut to remove all then add in the few that you would prefer to show. [Added comment] Did note from prior mockups that the disable all would be on each section.

I agree that disabled blocks are very subtle compared to enabled ones, Would it make more sense to have a Disabled block section so when blocks are disabled, they move to that section. This would have the added benefit of users with proper roles/capabilities being able to see those blocks at the bottom of the list to use if they wanted to without the need to go in, reenable it.

[bph]

As a content creator the primary goal for a block manager would be to declutter my inserter, so I am in favor for the semi-disable option to 'hide in inserter’ before actually disabling a block. On the surface that might just be semantics, but I worry about the ramification of a disable vs hide decision when it impairs using that block in a block template from my theme or a custom post type. I would not think that my decision would also render things unusable, and would be a major and heart stopping “oops” when this happens.

So if you are aiming for a small first win with this feature, low key, high impact the “hide” block might get more mileage. And maybe add a toggle to show hidden blocks, show active blocks for fast overview

[richtabor]

What's the value of the having a Disable All toggle in the interface?

The toggle is not for all blocks, but for all blocks within a particular category.

I added it because it's much easier if folks want to only use one or two blocks from a category (or third-party block collection). They simply toggle them all off with a click, then turn on the blocks they'd like to use.

[MDWolinski]

What's the value of the having a Disable All toggle in the interface?

The toggle is not for all blocks, but for all blocks within a particular category.

I added it because it's much easier if folks want to only use one or two blocks from a category (or third-party block collection). They simply toggle them all off with a click, then turn on the blocks they'd like to use.

Thanks.

From my perspective, this control is, at best, going to be used by a user once, maybe twice, so the value it offers is limited. By that, I mean, in a section with 10 blocks, I'd have to want to hide 6 blocks to make it more efficient (1 click to hide all, 4 clicks to unhide those I want), anything less makes it more efficient for me as a user to just click the ones I want to hide. And once I do that, I have no use for the functionality in the future, in that section.

For most sections, it doesn't make much value for the toggle to be there. For the embed section, that's a slightly different story, so perhaps this toggle is beneficial for sections with over X blocks.

[richtabor]

From my perspective, this control is, at best, going to be used by a user once, maybe twice, so the value it offers is limited.

Valid point, though we are only going to have more and more blocks available as Gutenberg (and developers) continue to mature.

[mapk]

One thing I noticed is the modal should change height when the content inside is less than the height of the modal.

[aduth]

One thing I noticed is the modal should change height when the content inside is less than the height of the modal.

This would be the natural behavior of a modal. The issue, however, is that modals are pinned to the center of the screen. When entering text into the search field, then, the modal rapidly resizes and shifts its position; a very jarring effect.

You can test it yourself by removing the following line:

gutenberg/packages/edit-post/src/components/manage-blocks-modal/style.scss

Line 4 in e43f953

height: calc(100% - #{ $header-height } - #{ $header-height });

I'd guess then, if we want it, we might want to consider either (a) changing the default pinned position for modals to the top, not center of the screen or (b) at least allow it as an option.

[mapk]

Visually you could have text within each button like Paragraph (disabled)

I was thinking about this too. Here's a take on it.

Option 1

Building off of that, I went a bit more obvious on this one.

Option 2

And Hi there, @MDWolinski thanks for commenting!

I agree that disabled blocks are very subtle compared to enabled ones, Would it make more sense to have a Disabled block section so when blocks are disabled, they move to that section.

While thinking this solution through, I imagine there needs be some user feedback to let the user know where these went in case they want to turn them back on again. I'm worried users might not remember this section and if they're just fiddling around to see what happens, if that block disappears from their immediate view, this could cause panic.

[youknowriad]

@chrisvanpatten These are good questions (indetermined state), but there's precedent with a lot of software like mentioned by @boemedia and like a lot of things if we don't try it, we won't know for sure.

The PR at the moment don't have such state, I'd be fine landing with or without it, that was my point.

[aduth]

What would be the "expected" behaviour here? How does the PR currently behave? I can imagine users — heck, even myself — forgetting they had hidden a category, and getting confused when they try to find a newly installed block.

As implemented today, the "All" checked state is determined by the filtered results under each category. The toggle is unchecked if and only if all filtered results in that category are unchecked.

As a result:

• A different search query (or no search query) can have an impact on whether it's checked or not, per the same behavior.
• By the fact it's not persisted as associated with the category specifically, blocks added in the future will be enabled by default, even if in a previous section I had toggled the checkbox off.

[chrisvanpatten]

Oh man. I find that even more confusing; the checkbox isn't reflecting the state of the category, it's reflecting the state of the visible items within it?

So this is just a visible indicator/shortcut for taking an action within the category?

I guess I don't really have a better idea, but I think as a casual user that would confuse the heck out of me.

[aduth]

So this is just a visible indicator/shortcut for taking an action within the category?

Yes, within the view filtered by the search term (if exists).

I guess I don't really have a better idea

Well, let's examine the options:

• We leave it as it is, all caveats remain.
• We eliminate searching as an option, so at least it renders moot the first point of my previous comment.
• We have the checkbox always align to the value of the category, which runs the risk of confusion in the inverse: That the checked state changes outside the view of my current applied search filter.

Are there others? How do we proceed?

[mapk]

I'd like to acknowledge that a11y is important in everything we do within Gutenberg. That being said, some of us aren't as knowledgeable as others in this area, me being one of those with less knowledge. I rely heavily on those who know more to help provide insight but to also help provide recommendations and potential solutions. When there is an a11y problem, it's important to engage in productive conversations with the realization that we'd all love a more accessible product. I personally don't respond well to attacks and criticisms that are meant to belittle one's contributions. We should strive to maintain positive communication and productive solutions.

I'd like to see us move toward this mockup below. I've addressed some of the major concerns about toggles, checkboxes, labels, etc. Yes, I may not have addressed everything perfectly, but we can iterate... even after a merge. I'll remind everyone that this is due to merge by tomorrow.

Notes on this mockup:

1. Using checkboxes instead of toggles for the Category title.
2. The checkbox for the category in these mockups reflects the state of the blocks, no longer the category visibility. And it will include the indeterminate state as well if only a portion of blocks are checked.
3. The checkboxes are moved to the left.
4. Focus loss will not be a blocker for this PR. It needs to be accounted for in it's own issue.
5. Indentation of the blocks within the category (slightly decreased) will remain for further visual clarity of children/parent relationships.

@aduth can we get these changes in today?

[aduth]

@aduth can we get these changes in today?

I'll turn my attention to it now 👍

[aduth]

I've pushed another round of updates per the latest mockup.

Specific notes:

• The title for each category serves both as the describer for the fieldset of checkboxes, and as the label for the mixed-state checkbox.
• "Mixed" is applied as aria-checked="mixed", which applies the attribute, which in turn applies the visual flourish. Per prior findings, environments which do not consider aria-checked="mixed" would announce this as unchecked. As best I can determine, this is consistent with expectations for post listing screen behavior as a baseline; considering aria-checked="mixed" an enhancement only as supported.
• The checkbox shows as unchecked if all items are unchecked, aria-mixed="true" (with visual cue) if a non-zero number of items are checked, and checked if all items are checked. If the bulk operation checkbox is clicked while not all its child checkboxes are checked, it has the result of checking all remaining checkboxes.

Here's a short animation with Voiceover + Chrome visible:

[aduth]

Focus loss pull request at #14444

[mapk]

Just ran through the latest PR. It looks and works great from my perspective.

I even tried hiding a Category completely, then installed a 3rd party plugin that included a block in that category. The result was as expected. The Category was again visible in the Inserter and the new block was available, but none of the others which I previously hid.

Thanks for the work on this @aduth! And for everyone's feedback. It's come a long ways and will continue to be improved.

LGTM!

[youknowriad]

Thanks everyone, nice to see this collaboration. Let's focus on the focus loss PR now.