Add UI for unregistered block types #8274

brandonpayton · 2018-07-30T00:37:18Z

Description

This PR adds UX for unregistered block types found in the post content.

A notice is shown in the place of each unregistered block type, offering the ability to remove the block or leave its content intact. If the core Custom HTML block is present and the unregistered block has HTML content, the notice also offers the opportunity to keep the block HTML as a Custom HTML block.

⚠️ Child blocks are not preserved. This is consistent with today's fallback behavior. Parsed blocks do not yet contain information about where their child blocks were found, so it is not possible to reconstitute child blocks within an unregistered parent. This is important to fix but should probably be done after a separate PR that updates the parser to provide such information. See #8214 for what appears to be a related issue.

Contributes to #7811.

How has this been tested?

Unit tests. It has also been manually tested by:

Creating custom block types.
Removing those types and reloading to observe missing block warnings.
Changing other content, saving, re-enabling the custom block types, and reloading to observe the original block content was preserved and loaded correctly again.

Screenshots

Types of changes

Today, we have an "unknown fallback" that handles both non-block content and unregistered block types. This PR splits the fallback in two, a freeform content fallback and an unregistered type fallback, and deprecates the "unknown fallback".

In discussion with @karmatosed, we want the unregistered block fallback to appear more like a warning than a block, so we hide its block outline and override its block sidebar to appear as if no block is selected.

karmatosed · 2018-07-30T19:14:15Z

Design wise could we go more with this please?

This was outlined in comments and be great to have consistency. Ideally greying out the background - is that possible?

karmatosed

Design needs to if possible reflect the proposed design. This seems to add a different style.

brandonpayton · 2018-08-03T07:18:04Z

Thanks for taking a look, @karmatosed.

Design needs to if possible reflect the proposed design. This seems to add a different style.

Yeah, I used the same Warning component we use for block validation errors and noticed it didn't match your design, probably due to multiple lines. It was one of things I needed to follow up on. (The point of mentioning this is to let you know I saw and had regard for your designs but hadn't gotten to fixing this as it was a WIP).

In a previous comment, you mentioned:

I will also note this new notice style has a bit of a problem of scaling to multiple lines but that would need to be tackled in the other issue.

Was this referring to our other notices like this one, and if so, would you be up for pointing me to the other issue that deals with the notice style so I can be sensitive to what we're doing there?

Regarding the design you shared above (and in the originating issue):

Do I understand correctly that our message lines should wrap to the left of the button while the button remains in the upper right?
Which background should be greyed out? I see there is a bit of grey background on the left hand side of your image, but I don't think I'm fully understanding your intent.

brandonpayton · 2018-08-28T01:22:37Z

I made some updates in response to another design review with @karmatosed on Friday. We'd like to emphasize that this is a warning and reduce the presence of block UI around the warning. To this end, the latest:

Hides the block menu (visually but not from a11y tooling)
Hides the block outline and label that are shown when the block is hovered or selected.
Updates the block sidebar to show "No block selected" for unregistered blocks.

Here's what the latest looks like:

Note that the action buttons now run under the bottom of the text as they do for our other warnings when the text is long enough to cause the buttons to move to another flex row.

What do you think, @karmatosed?

brandonpayton · 2018-08-28T01:29:49Z

There is a CSS outline transition bug that needs fixed and a related !important (added during troubleshooting) that will be removed.

brandonpayton · 2018-08-28T01:43:38Z

The outline transition is now fixed.

karmatosed · 2018-08-28T21:20:05Z

@brandonpayton thanks for this iteration, it seems to be getting close! Is there a reason for the uneven spacing around the block?

Just going back to the designs, I wonder if we can align more with those? For example do we need ellipsis to remove? Also could we have that layer over the content?

brandonpayton · 2018-09-07T02:45:19Z

@karmatosed and I met out of band and discussed her latest feedback. Here's my understanding of the outcome:

We should should use the block menu for removing an unregistered block rather than providing a "Remove block" button. The addition of the button was due to a misunderstanding from an earlier conversation.
The bottom location of the "Keep as HTML" button is OK because it how our Warning component currently behaves for longer text.
We won't try to display the warning as a content overlay. The reason is that styles are missing for unregistered blocks, making it unlikely we'll render them well.

brandonpayton · 2018-09-07T02:48:23Z

Hi Karmatosed, I've rebased and updated the UI based on our discussion. Here's how the latest looks:

What do you think?

Recently, the location of the block menu changed to the block toolbar, and that looks a bit odd since we hid the block outline here. I'm wondering whether this is still OK to merge, and we can adjust that in a follow up PR.

mtias · 2018-09-07T10:58:12Z

There has to be an option about not doing anything. If we can map a block with a plugin, we can offer to activate it it.

mtias · 2018-09-07T11:00:46Z

Also, if the block has HTML content we should somehow show it as it'd help the user determine what to do — should be similar to the invalid block preview.

brandonpayton · 2018-09-10T18:22:14Z

Hi @mtias, thanks for your feedback!

There has to be an option about not doing anything. If we can map a block with a plugin, we can offer to activate it it.

What do you think about looking at this in a follow up PR? This PR contains a change to the blocks API and would be good to get settled soon.

Regarding implementation, it seems impossible to reliably map registered blocks to a plugin since blocks may be registered with arbitrary names, and there is no guarantee a namespace/block name will have a namespace that matches a plugin basename. Perhaps...

There's something I'm missing.
A best effort at mapping to a plugin is better than nothing and would encourage plugin authors to comply with block naming conventions.

Do you have any thoughts here? Either way, I'm glad to explore this.

Also, if the block has HTML content we should somehow show it as it'd help the user determine what to do — should be similar to the invalid block preview.

Which preview are you referring to? Is this the invalid block message you're talking about?

On a related note, I see how the block settings menu nicely aligns with the message above and will look at fixing that for this PR.

mcsf · 2018-10-12T11:37:58Z

Thanks for the work, @brandonpayton. So far this looks pretty good.

I've rebased against master.
I've lowered the deprecation version to 4.2 (bf89150), given our core merge timeline.

mcsf · 2018-10-12T11:40:21Z

Noting a bug that is not introduced by this PR, merely highlighted by it:

It's an issue in the HTML block and other users of iframe, in which editor styles aren't respected. Tracked in #10547. It doesn't block this PR.

mcsf · 2018-10-12T13:12:58Z

packages/blocks/src/api/parser.js

 		attrs: attributes,
 		innerBlocks = [],
 		innerHTML,
 	} = blockNode;
+	const freeformContentFallbackBlock = getFreeformContentHandlerName();
+	const unregisteredFallbackBlock = getUnregisteredTypeHandlerName() || freeformContentFallbackBlock;


This initially struck me as odd, but upon testing the actual behavior, it's not bad to default to Freeform if Unregistered is not present. 👍 (Although, who'd have one and not the other?)

aduth · 2018-10-12T17:17:26Z

packages/block-library/src/missing/index.js

+	return (
+		<Fragment>
+			<Warning actions={ actions }>
+				<span dangerouslySetInnerHTML={ { __html: messageHTML } } />


In this case, dangerous is actually dangerous. Localized strings should be considered equivalent to user input for sanitization purposes.

since translations should not be considered trusted strings, be sure to sanitize the result before echoing.

https://codex.wordpress.org/I18n_for_WordPress_Developers#HTML

Related: #9846

Dang, I missed this one. Yes.

Since the only markup in the string is the <code> tag, we can just drop it. I'll start a branch.

Thanks very much for catching this, @aduth, and for alerting me to the need for care here.

@mcsf, thank you for the fix. <3

since translations should not be considered trusted strings, be sure to sanitize the result before echoing.

FWIW this isn't entirely accurate. WordPress.org treats stranslations as trusted strings.

FWIW this isn't entirely accurate. WordPress.org treats stranslations as trusted strings.

Is the Codex wrong then? Or is the article not applicable for core development?

Both I guess :-)

aduth · 2018-10-15T17:52:11Z

packages/blocks/src/api/parser.js


 	attributes = attributes || {};

 	// Trim content to avoid creation of intermediary freeform segments.
-	innerHTML = innerHTML.trim();
+	const originalUndelimitedContent = innerHTML = innerHTML.trim();


This is too clever. I'm still not totally sure I understand why / what we're doing with this line.

Thanks, @aduth. The intent wasn't cleverness, but I see what you mean. We want innerHTML to be trimmed, and saving its value at that point is a separate concern.

The missing block needs the original undelimited content for determining whether the unregistered block has actual HTML content, for rendering a preview of the block HTML, and for converting to an HTML block. originalUndelimitedContent exists because this pre-existing line changes the value of innerHTML to be the delimited HTML content. The delimited content is used in saving the value of the block.

The above approach, which I am responsible for, is a bit weird IMO. It would be more straightforward to:

Get rid of originalUndelimitedContent.

Stop overwriting innerHTML with the delimited content.

Add an originalAttributes attribute to core/missing.

Update core/missing to call getCommentDelimitedContent with originalName, originalAttributes and originalContent to create the save content.

If you agree this is cleaner, I can create a follow-up PR to make the change.

I actually don't have much of a problem with originalUndelimitedContent, I just think we could be clearer / more verbose with how we assign it / document its intent.

Also, I think with this type of assignment we're inadvertently assigning innerHTML as global outside the scope of the function:

I actually don't have much of a problem with originalUndelimitedContent, I just think we could be clearer / more verbose with how we assign it / document its intent.

✅ thanks
I don't love originalUndelimitedContent but plan to leave it after documenting its purpose and separating the assignment.

I don't believe this assignment should be creating a global since innerHTML is declared in the function scope here.

aduth · 2018-10-15T17:55:15Z

packages/block-library/src/missing/index.js

+			<Warning actions={ actions }>
+				<span dangerouslySetInnerHTML={ { __html: messageHTML } } />
+			</Warning>
+			<div>


For what reason do we need the div?

It isn't necessary. I initially misunderstood how <RawHTML> worked, but see it adds its own div. I can fix this as part of the PR mentioned above or independently.

mcsf · 2018-10-17T08:15:47Z

One thing I'm noticing in master, @brandonpayton, is how Unregistered blocks are not selectable:

If I focus one such block, the Block sidebar reads No block selected.
If I try to multi-select with the keyboard, I can't start the process on such a block. Arrow-based selection seems fine, even though the visual cues for selected state could be clearer.

brandonpayton · 2018-10-17T23:25:38Z

Hi @mcsf, thanks for mentioning these.

If I focus one such block, the Block sidebar reads No block selected.

This behavior was requested by @karmatosed so that the warning block wouldn't feel like a block to the user, but perhaps we can show something other than "No block selected". Do you have any thoughts on this, @karmatosed?

If I try to multi-select with the keyboard, I can't start the process on such a block. Arrow-based selection seems fine, even though the visual cues for selected state could be clearer.

I am able to select both with the mouse and keyboard and can start the selection from a warning, but I see an issue with warning styles. There's a rule that applies a white background color to warnings, and that's overlapping the highlight background we've set for .is-selected and .is-multi-selected blocks.

Here's a screen cap of my experience:

This is with Chrome. In what browser are you seeing this issue?

brandonpayton · 2018-10-17T23:44:14Z

I created #10712 to track the selection highlight bug.

gAllegr · 2018-10-18T13:47:18Z

Here's a screen cap of my experience:

Speaking as a translator, there are two strings with Resolve word (String 1 and String 2 ) which are not so self-explicative.

It is possible to add a comment to these strings?

brandonpayton · 2018-10-19T07:23:35Z

Speaking as a translator, there are two strings with Resolve word (String 1 and String 2 ) which are not so self-explicative.

It is possible to add a comment to these strings?

Created #10779 to address this.

mcsf · 2018-10-23T18:14:28Z

This is with Chrome. In what browser are you seeing this issue?

It would've either been Firefox Developer Edition or Safari, my more commonly used browsers.

aduth added [Status] In Progress Tracking issues with work in progress Needs Design Feedback Needs general design feedback. labels Jul 30, 2018

karmatosed self-requested a review July 30, 2018 19:14

karmatosed requested changes Jul 30, 2018

View reviewed changes

karmatosed removed the Needs Design Feedback Needs general design feedback. label Jul 30, 2018

brandonpayton force-pushed the add/ui-for-missing-blocks-2 branch 2 times, most recently from d276d2d to 59811a0 Compare August 3, 2018 07:04

diegoreymendez mentioned this pull request Aug 22, 2018

Unsupported Block wordpress-mobile/gutenberg-mobile#106

Merged

brandonpayton force-pushed the add/ui-for-missing-blocks-2 branch 2 times, most recently from 4518c16 to d7d1b34 Compare August 27, 2018 23:19

brandonpayton requested a review from youknowriad August 28, 2018 14:07

brandonpayton added Needs Design Feedback Needs general design feedback. [Feature] Block API API that allows to express the block paradigm. [Feature] Blocks Overall functionality of blocks and removed [Status] In Progress Tracking issues with work in progress labels Aug 28, 2018

brandonpayton mentioned this pull request Aug 31, 2018

WIP/Try: Make pullquote a variation #8821

Closed

brandonpayton force-pushed the add/ui-for-missing-blocks-2 branch 2 times, most recently from 035b790 to 0064e21 Compare September 6, 2018 19:36

brandonpayton requested a review from a team September 7, 2018 02:48

brandonpayton added 3 commits October 12, 2018 11:42

Clarify missing block case in block inspector

dec455b

Treat unregistered block case as a warning

8207914

Update unit tests

2712de7

mcsf force-pushed the add/ui-for-missing-blocks-2 branch from fb979f4 to 2712de7 Compare October 12, 2018 11:19

Lower deprecation version to 4.2

bf89150

mcsf added 2 commits October 12, 2018 12:50

chore: Build docs

2cd0f8e

Minor refactor (isFallbackBlock); wrap lines

79222f4

mcsf approved these changes Oct 12, 2018

View reviewed changes

mcsf merged commit 4f33ba8 into master Oct 12, 2018

mcsf deleted the add/ui-for-missing-blocks-2 branch October 12, 2018 14:35

aduth reviewed Oct 12, 2018

View reviewed changes

mtias mentioned this pull request Oct 15, 2018

Special Invalid Case for Missing Blocks #7811

Closed

koke mentioned this pull request Oct 15, 2018

Differentiate unsupported blocks from unavailable wordpress-mobile/gutenberg-mobile#51

Open

4 tasks

aduth reviewed Oct 15, 2018

View reviewed changes

rahulsprajapati mentioned this pull request Oct 18, 2018

Stop trying to autosave when title and classic block content both are empty. #10404

Merged

4 tasks

This was referenced Oct 19, 2018

Address further review of unregistered type handler #10776

Merged

Add i18n context for resolving invalid blocks #10779

Merged

dimadin mentioned this pull request Nov 10, 2018

Use apostrophe instead of single-quote character. #11710

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add UI for unregistered block types #8274

Add UI for unregistered block types #8274

brandonpayton commented Jul 30, 2018 •

edited

Loading

karmatosed commented Jul 30, 2018

karmatosed left a comment

brandonpayton commented Aug 3, 2018

brandonpayton commented Aug 28, 2018

brandonpayton commented Aug 28, 2018

brandonpayton commented Aug 28, 2018

karmatosed commented Aug 28, 2018

brandonpayton commented Sep 7, 2018

brandonpayton commented Sep 7, 2018

mtias commented Sep 7, 2018

mtias commented Sep 7, 2018

brandonpayton commented Sep 10, 2018

mcsf commented Oct 12, 2018

mcsf commented Oct 12, 2018

mcsf Oct 12, 2018

aduth Oct 12, 2018

mcsf Oct 15, 2018

mcsf Oct 15, 2018

brandonpayton Oct 15, 2018

swissspidy Oct 25, 2018

aduth Oct 25, 2018

swissspidy Oct 27, 2018

aduth Oct 15, 2018

brandonpayton Oct 16, 2018

aduth Oct 16, 2018 •

edited

Loading

brandonpayton Oct 16, 2018

aduth Oct 15, 2018

brandonpayton Oct 16, 2018

mcsf commented Oct 17, 2018

brandonpayton commented Oct 17, 2018 •

edited

Loading

brandonpayton commented Oct 17, 2018

gAllegr commented Oct 18, 2018

brandonpayton commented Oct 19, 2018

mcsf commented Oct 23, 2018

Add UI for unregistered block types #8274

Add UI for unregistered block types #8274

Conversation

brandonpayton commented Jul 30, 2018 • edited Loading

Description

How has this been tested?

Screenshots

Types of changes

karmatosed commented Jul 30, 2018

karmatosed left a comment

Choose a reason for hiding this comment

brandonpayton commented Aug 3, 2018

brandonpayton commented Aug 28, 2018

brandonpayton commented Aug 28, 2018

brandonpayton commented Aug 28, 2018

karmatosed commented Aug 28, 2018

brandonpayton commented Sep 7, 2018

brandonpayton commented Sep 7, 2018

mtias commented Sep 7, 2018

mtias commented Sep 7, 2018

brandonpayton commented Sep 10, 2018

mcsf commented Oct 12, 2018

mcsf commented Oct 12, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aduth Oct 16, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mcsf commented Oct 17, 2018

brandonpayton commented Oct 17, 2018 • edited Loading

brandonpayton commented Oct 17, 2018

gAllegr commented Oct 18, 2018

brandonpayton commented Oct 19, 2018

mcsf commented Oct 23, 2018

brandonpayton commented Jul 30, 2018 •

edited

Loading

aduth Oct 16, 2018 •

edited

Loading

brandonpayton commented Oct 17, 2018 •

edited

Loading