feat: Show sidebar in HTML edit mode for blocks

[tofumatt]

Description

Fix #4413.

When editing a block in HTML, the sidebar inspector controls would disappear because BlockEdit was not rendered–and BlockEdit contains the InspectorControls slot fill content for a block. This changes BlockEdit to always render, even in HTML mode, but be hidden when editing via HTML.

I tried for a better way but it wasn't immediately apparent how to do that.

I also tried for a more fluid updating of the block during HTML content editing, but this would cause issues with block validation mid-change. It might take some time to edit a block's HTML and during the edit it might appear externally modified even if the eventual change would be valid. So we don't update the core/editor/Redux state until onBlur, as we did before. This means things like an image's alt text property, being edited in HTML, won't be reflected in real-time in the sidebar inspector controls, though the sidebar's changes will be reflected in real-time in HTML. Once you click over to the sidebar though, the changes will appear, so there are no race conditions/missing content. It just doesn't look as fluid as I'd like, but the alternative is a very buggy experience.

---

It would be great to test a fair number of blocks with this to make sure it doesn't break anything or have a really janky UX anywhere.

How has this been tested?

Wrote e2e tests to ensure they're visible and tested this for a fair few blocks in Firefox, Chrome, and IE 11. I observed some weirdness with paragraph blocks but it's unrelated to this change and I think caused by #8950.

Screenshots

Before

After

Types of changes

New feature.

Checklist:

• My code is tested.
• My code follows the WordPress code style.
• My code follows the accessibility standards.
• My code has proper inline documentation.

[youknowriad]

Just a question:

A block author might have selection dependent controls in the inspector (imagine a "bold" button in the inspector) and in that case it will break I guess? I think that's the reason we hide the toolbar. Granted that this use-case is probably less important but conceptually speaking, there's no difference between BlockControls and BlockInspector.

[tofumatt]

That's a good question, I'll look through blocks that do that. Do you know of any blocks that do that?

[youknowriad]

@tofumatt I don't think we have any in Core blocks.

[tofumatt]

So should we maybe have an option/API that explicitly disables blocks from having inspector controls appear?

[youknowriad]

Yes, or maybe. An option would be to not render it :). If you're in the global code mode, there's no toolbar/inspector. 🤷‍♂️ At least that has my vote.

[tofumatt]

If there aren't any controls that modify only selected content in the inspector I don't see the issue, but I suppose if that's a thing we:

1. allow to happen technically
2. don't explicitly discourage in docs

then this is a dangerous idea to implement, yeah. The issue didn't really delve into the edge cases around it.

For what it's worth I found it, even in my testing, to be a nice UX to see the HTML instantly update from the inspector and to see what each control did to the HTML. That said, it's a fairly edge case usage.

[jasmussen]

It seems like blocks that treat inline elements would all somehow touch the Editable no? Can we detect this in any way?

[aduth]

IMO anything in the sidebar should affect the block as a whole, not an active selection. I don't know that we need to detect this, but it could be something worth documenting.

[tofumatt]

That makes sense to me, and certainly feels like the implication made by the way all of the core blocks have been designed.

[jasmussen]

If we document that and someone builds it regardless, presumably they can then respond to feedback when their users report a thing not working.

[ZebulanStanphill]

@jasmussen @tofumatt I know of one plugin that uses the inspector controls for inline formatting:

https://wordpress.org/plugins/advanced-gutenberg/

This plugin uses the inspector for single-cell formatting in the Advanced Table block. I would not say it is the most intuitive experience, but that is how they implemented it.

[youknowriad]

We need a decision to get this in for 3.6 or not. It could be just a documentation addition.

[tofumatt]

Sounds like there's agreement that it's not the right way to do things, so I'm happy to document that… seems like it should go in https://github.com/WordPress/gutenberg/blob/master/docs/blocks/block-controls-toolbars-and-inspector.md, but should it be anywhere else?

[aduth]

@tofumatt That document appears a fine place.

If we had documentation here, I'd also make mention of it:

https://github.com/WordPress/gutenberg/tree/master/packages/editor/src/components/inspector-controls

[tofumatt]

Okay, I've added some documentation to both the component (sparse) and to the existing guidelines on creating a block.

[aduth]

I have a feeling that this link will break if / when we delete the branch (standard practice).

[tofumatt]

It will, but I wanted it in here until we merged so people could see it 😄

I plan to change it to master before merge, but you're right: I should've mentioned it 😄

[aduth]

I think we should be consistent with ternary fragment placements: Either all on one line, or Fragment and 'div' separately on their own lines.

[aduth]

We duplicate this condition from before. Makes me wonder if there's another way we could apply the wrapper to keep the logic contained:

Component:

function BlockEditWrapper( { isVisual, children } ) {
	if ( isVisual ) {
		return children;
	}

	return <div style={ { display: 'none' } }>{ children }</div>;
}

Wrapping in an if condition:

let edit = <BlockEdit { /* ... */ } />;
if ( mode !== 'visual' ) {
	edit = <div style={ { display: 'none' } }>{ edit }</div>;
}

[tofumatt]

Oh, good point, that'd work I'd think.

[aduth]

Can you explain this change?

[tofumatt]

Hah, no. It was a mistake from something else I think. Will remove! 🔥

[tofumatt]

Changes made, including changing image URL to master… which won't work in this branch, but the image is this:

Ready for another look.

[youknowriad]

LGTM 👍

[aduth]

Cool 😃

[youknowriad]

I just "squashed" your PR @tofumatt :P

[mtias]

Nice to see this one in!