Block comparison: show difference between invalid block conversion options #7995

johngodley · 2018-07-17T08:27:52Z

Description

When you have an invalid block the result of the available actions can be unknown.

This PR adds a 'compare conversion' option that shows the actual HTML difference when 'convert to blocks' is applied.

The difference appears in a modal and the user can select to keep the current contents or apply the conversion, with full knowledge of the result of their action.

The modal adjusts to the height of the block:

Overflowing, if too tall, and scrollable:

And adjusts accordingly on smaller screens (using overflow scroll if too tall):

Note: this is based on the discussion in #7604

How has this been tested?

Manually check that the 'compare conversion' option displays correctly for different types of blocks and at different screen sizes
Manually verify that the 'keep as HTML' converts the block to an HTML block
Manually verify that the 'convert to blocks' applies a conversion identical to picking 'convert to blocks' from the invalid block modal

Types of changes

This adds a 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.

johngodley · 2018-09-02T13:27:24Z

Noting that the comparison currently uses code from jsdiff to perform proper diffing. This is a BSD licensed open source library. It works well, but we don't need the entire library. I'm unsure about the best route for proceeding here, and I don't know if it's appropriate to include the library as-is, or the full package.

While Gutenberg does have it's own HTML parsing, and also HTML comparison (via isEquivalentHTML), it's doesn't find the longest common sequences, and would result in a poor diff that would need adapting to a proper diffing algorithm.

An alternative is to re-implement jsdiff ourselves.

Open to suggestions.

johngodley · 2018-09-02T13:59:30Z

Also noting that the style is a best attempt at matching that proposed in #7604, but needs some improving. @jasmussen I'd very much appreciate any suggestions/help you can give.

In particular I assumed that we would want to show deleted characters in the right panel, and I've included these in red. We can not show those at all, if it makes more sense.

jasmussen · 2018-09-03T06:25:30Z

Sorry I completely missed this. Feel free to assign me as a reviewer next time and I'll be sure to get to it. I'll try and take a look today, maybe push code.

jasmussen · 2018-09-05T08:56:18Z

@johngodley This is impressive work. I think it's super duper solid, and it's VERY much good enough to get into master so we can iterate.

I pushed a small fix so it only scrolls when necessary, and I made the menu option titlecase.

Can you rebase this?

Question: does this use the modal API? Assuming yes as it really seems to be, which is great, just verifying.

johngodley · 2018-09-05T12:56:37Z

does this use the modal API

If you mean <Modal /> then yep it does.

Rebased. There's a codecov failure which I don't understand as the unit test coverage has increased.

I've noticed some problems with a few of the more esoteric blocks so I'll look at fixing those.

jasmussen · 2018-09-06T09:08:03Z

If you mean then yep it does.

Yes, awesome!

I'm extending the review range and adding a milestone so we can get this in. Amazing work! This is a KILLER feature.

Shows two blocks side by side with the difference in HTML highlighted

johngodley · 2018-09-06T09:32:37Z

I made a some small tweaks to the CSS regarding line height. If you had a deleted section wrapping over lines it could overlap:

This has now been fixed.

I noticed two other problems:

The Modal component doesn't prevent focus/blur events from propagating through to the page underneath. This means you can trigger tooltips and block dropdown menus, making it a bit twitchy feeling. I've got a fix in place which I'll put in a separate PR
Some blocks don't behave well to 'convert to blocks', and can result in the entire block being deleted. This isn't caused by the code here, but the preview ability makes it more noticeable. I'll also look at this separately

jasmussen · 2018-09-06T09:38:27Z

The Modal component doesn't prevent focus/blur events from propagating through to the page underneath. This means you can trigger tooltips and block dropdown menus, making it a bit twitchy feeling. I've got a fix in place which I'll put in a separate PR

🏅

Yeah this is a separate issue that would be sweet to fix.

Some blocks don't behave well to 'convert to blocks', and can result in the entire block being deleted. This isn't caused by the code here, but the preview ability makes it more noticeable. I'll also look at this separately

You rule the world.

The vertical spacing would become strange if the converted preview was shorter than the original preview

talldan

@johngodley - This is really cool. In fact, I think it's so useful I was wondering if we would want to make it more prominent?

I added some comments, I think the main one that needs to be fixed is the max-height: 70% issue which is visible to the user.

Will probably also need someone more knowledgeable to comment on the library inclusion. Not sure who would be best.

talldan · 2018-09-07T11:04:06Z

packages/editor/src/components/block-compare/index.js

+		const difference = differ.diff( originalContent, newContent );
+
+		return difference.map( ( item, pos ) => {
+			if ( item.added ) {


Could use the classnames utility here to remove the need for the if and shorten to something like:

const className = classnames( { 'editor-block-compare__added': item.added, 'editor-block-compare__removed': item.removed, } ); return <span className={ className } key={ pos }>{ item.value }</span>

Excellent idea!

talldan · 2018-09-07T11:15:52Z

packages/editor/src/components/block-compare/index.js

+
+		return {
+			content: block.originalContent,
+			render: getSaveElement( blockType, block.attributes ),


Would be good to make these property names consistent with the props on the BlockView, either changing them here or there. I think the BlockView names are quite nice.

talldan · 2018-09-07T11:18:45Z

packages/editor/src/components/block-compare/index.js

+	render() {
+		const { block, onKeep, onConvert, convertor, convertButtonText } = this.props;
+		const original = this.getOriginalContent( block );
+		const converted = this.getConvertedContent( convertor( block ), original );


Looking at the definition of getConvertedContent, it seems like the second arg isn't needed.

talldan · 2018-09-07T11:20:39Z

packages/editor/src/components/block-compare/index.js

+
+	getConvertedContent( block ) {
+		// The convertor may return an array of items or a single item
+		const newBlocks = Array.isArray( block ) ? block : [ block ];


I found out recently that castArray is another succinct way to do this:
https://lodash.com/docs/4.17.10#castArray

talldan · 2018-09-07T11:24:15Z

packages/editor/src/components/block-compare/style.scss

+
+// Ensure the modal fits the content, otherwise it could be too big
+.editor-block-compare {
+	max-height: 70%;


At low screen width breakpoints the modal becomes full screen, but still only takes up 70% of screen height, so I think we need a rule to set it back to 100%

talldan · 2018-09-07T11:26:34Z

packages/editor/src/components/block-compare/style.scss

+
+.editor-block-compare__wrapper {
+	display: flex;
+	justify-content: space-between;


Not sure space-between is needed as the content seems to take up 100% of the width.

talldan · 2018-09-07T11:32:54Z

packages/editor/src/components/block-compare/style.scss

+		}
+	}
+
+	.editor-block-compare__converted {


Do we have any thoughts on nesting element selectors inside the block selector @jasmussen? It might introduce unnecessary specificity, but then I'm not sure if we'd expect to have other styles try to override these, so probably ok.

One option is to do &__converted and in the resulting css the parent selector won't be included, but that only works one level deep.

talldan · 2018-09-07T11:38:05Z

packages/editor/src/components/block-compare/style.scss

+	padding-bottom: $panel-padding;
+
+	> div {
+		display: flex;


I don't think flex is needed here, could just rely on normal layout.

It's being used to push the buttons to the same position at the bottom of the modal, otherwise they become vertically misaligned.

talldan · 2018-09-07T11:48:08Z

packages/editor/src/components/block-list/block-invalid-warning.js

+
+		if ( compare ) {
+			return (
+				<Modal


Just some food for thought - I think I would have approached this differently and made BlockCompareModal a component that manages its own open/closed state. I'm not sure there's a right or wrong.

Yeah, it's a good point. My thinking is that the comparison component could (potentially, and currently without any need) be used elsewhere that might be outside of a modal or an on/off situation and so I made the component do it's thing, and left it up to the parent to decide when to show it.

It’s not necessary for anything and makes the modal too small on mobile

johngodley · 2018-09-07T13:00:32Z

I think it's so useful I was wondering if we would want to make it more prominent?

Thanks for the review @talldan. I do wonder if it might make more sense if the 'convert to blocks' button just defaults to showing the comparison component directly, and we remove the option from the dropdown menu?

The effect of 'convert to blocks' is unknown to the user, and sometimes quite arbitrary and destructive. At the point the user is being presented with this warning I don't think it's excessive to add an additional click in the process. It saves having to undo the action and being annoyed at Gutenberg if the conversion isn't as expected.

Without this the button doesnt stick to the bottom

talldan · 2018-09-10T10:33:31Z

packages/editor/src/components/block-compare/index.js

+/**
+ * External dependencies
+ */
+import classname from 'classnames';


Just a typo here, the imported function should be classnames instead of classname

talldan · 2018-09-10T11:04:16Z

@johngodley This is looking good. I've spotted one additional thing here:
https://github.com/WordPress/gutenberg/pull/7995/files#r216273361

I think the only thing we need now is to confirm that the script/dependency inclusion is ok. (edit: worth noting that at least if we use an npm package, the licence is checked by the licence-checker script: #8808).

On the user experience, I feel like it could definitely be iterated on this so that it's part of the workflow when editing a block HTML:

User edits block HTML
The diff is present to them when they've finished editing with the two options.
User decides an option.

There are other times when the user might end up with invalid html (e.g. using the classic editor), so we might present that slightly differently to what I've proposed above. Would be good to get additional input on this though.

johngodley · 2018-09-11T13:47:08Z

Thanks for that link, and it's pushed me towards just bundling the module normally

aduth · 2018-09-11T16:21:26Z

packages/editor/src/components/block-compare/block-view.js

@@ -0,0 +1,28 @@
+/**


I'm a bit hesitant about including a component BlockCompare in the components package, since it's not as general-purpose as intended by the package, but rather domain-specific to the idea of WordPress editors. Was editors considered instead?

I'm not sure what you mean by editors - can you point me in the direction?

Can't speak for Andrew but since he's asleep I'll take an educated guess. Maybe he meant edit-post?

Ah ok. I'm not entirely clear on the difference between editor and WordPress editors components. Are you suggesting it work like the keyboard shortcuts modal and isModalActive?

If the concern is the general purposeness of the component then could it be made local to the invalid warning component? I'm not sure if modals are designed to be used locally like this as the only other one I can find is the keyboard shortcut modal, which is 'global'.

Again I can't but make educated guesses — I think it's something about the packages getting published on NPM but not sure.

I'll wait for Andrew to clarify, but the component is already in the editor package, not in the components package, so maybe it was a misread. Or maybe he did mean edit-post, but I'm not sure we should be implementing components from edit-post in editor like this would be.

Oof, I'm sorry for the trouble, it was definitely a misread on my part.

I had mistaken it as being part of packages/components.

talldan

This is really close, I just spotted one thing in further testing. Will also wait for @aduth to clarify his comment.

talldan · 2018-09-12T11:01:25Z

packages/editor/src/components/block-compare/index.js

+
+		return {
+			rawContent: block.originalContent,
+			renderedContent: getSaveElement( blockType, block.attributes ),


I've just given this another test, and I'm not sure the original content's preview displays quite what I was expecting. My steps:

Add some simple text to a paragraph block

Edit the html and add an inline style style="font-size:25px;"

View the comparison

Note that the original content preview doesn't show the font size at 25px, which is what I'd get if I selected 'Keep as HTML'.

To make the preview display correctly I tried using RawHTML like the HTML block does for its preview, which seemed to do the trick:

getOriginalContent( block ) { return { rawContent: block.originalContent, renderedContent: ( <RawHTML>{ block.originalContent }</RawHTML> ), }; }

Interesting!

RawHTML uses dangerouslySetInnerHTML, which opens up things like <p onclick="alert(1)">fdsfsdfds fd sfds</p>. I'm not sure if this is a problem in this context and what the behaviour is supposed to be so I'll have a poke around.

The 'preview' mode of the HTML block uses the <Sandbox /> component to display arbitrary HTML, which neutralises my above example, and seems to be the way to go. The styling is different so I'll see if it can be made to fit in.

After some playing around it seems <Sandbox /> is an iframe and while it does prevent the JS above, it is also unstyled.

Although I think the scope for mischief is small I'm not sure we should introduce it.

With that in mind we can either accept the unstyled appearance (looks ok for paragraphs, doesnt look ok for images), or keep the current not-quite-full preview (it's displaying the block without the invalid bits, but with the valid changes). There may be another way I'm unaware of, too

Hmm, yep, thanks for looking into it. Lets go with what we've got and then maybe look into this on a separate issue.

johngodley · 2018-09-13T08:51:29Z

Thanks for the feedback and suggestions!

jasmussen · 2018-09-13T09:39:37Z

Amazing work!

aduth · 2018-09-13T14:00:32Z

packages/editor/src/components/block-compare/block-view.js

+	return (
+		<div className={ className }>
+			<div className="editor-block-compare__content">
+				<h1 className="components-modal__header-heading">{ title }</h1>


This is not the modal component, so should not be stealing / inheriting styles. Components should be insulated from one another to avoid maintenance issues with cascading consequences of future changes.

aduth · 2018-09-13T14:02:18Z

packages/editor/src/components/block-compare/block-view.js

+			</div>
+
+			<div className="editor-block-compare__action">
+				<Button isLarge tabindex="0" onClick={ action }>{ actionText }</Button>


I'm guessing a console warning is being logged on this line?

We should pass tabIndex as the DOM property tabIndex={ 0 }, not the attribute name.

Note that you should still use the canonical React naming for known attributes:

// Yes, please <div tabIndex="-1" /> // Warning: Invalid DOM property `tabindex`. Did you mean `tabIndex`? <div tabindex="-1" />

In other words, the way you use DOM components in React hasn’t changed, but now you have some new capabilities.

https://reactjs.org/blog/2017/09/08/dom-attributes-in-react-16.html

Oddly it doesn't log a warning, but both very valid points and I'll get those fixed in another PR

johngodley added [Feature] Blocks Overall functionality of blocks [Status] In Progress Tracking issues with work in progress [Feature] Block Transforms Block transforms from one block to another labels Jul 17, 2018

johngodley force-pushed the add/invalid-block-diff branch 2 times, most recently from 5ffb94b to 27ed264 Compare July 24, 2018 08:54

johngodley force-pushed the add/invalid-block-diff branch 2 times, most recently from 5c9e4b3 to 0ec836a Compare July 29, 2018 14:48

johngodley force-pushed the add/invalid-block-diff branch 4 times, most recently from 06c5849 to f181dce Compare August 14, 2018 05:46

johngodley mentioned this pull request Aug 28, 2018

Block Validation, Deprecation and Migration Experience #7604

Open

johngodley force-pushed the add/invalid-block-diff branch 3 times, most recently from b7196b4 to 8079b91 Compare September 2, 2018 12:12

johngodley force-pushed the add/invalid-block-diff branch from 8079b91 to 2d4e20b Compare September 2, 2018 13:42

jasmussen self-requested a review September 3, 2018 06:25

johngodley force-pushed the add/invalid-block-diff branch from 8d870fc to 58e8bab Compare September 5, 2018 11:53

johngodley changed the title ~~WIP - Block comparison: show difference between invalid block conversion options~~ Block comparison: show difference between invalid block conversion options Sep 5, 2018

johngodley removed the [Status] In Progress Tracking issues with work in progress label Sep 5, 2018

johngodley changed the title ~~Block comparison: show difference between invalid block conversion options~~ WIP: Block comparison: show difference between invalid block conversion options Sep 5, 2018

jasmussen requested a review from a team September 6, 2018 09:08

jasmussen added this to the 3.9 milestone Sep 6, 2018

Add block compare component

7b7744e

Shows two blocks side by side with the difference in HTML highlighted

Fix flexbox spacing when converted preview is smaller

af8459a

The vertical spacing would become strange if the converted preview was shorter than the original preview

johngodley changed the title ~~WIP: Block comparison: show difference between invalid block conversion options~~ Block comparison: show difference between invalid block conversion options Sep 6, 2018

Update snapshot for flexbox fixes

e90c766

johngodley mentioned this pull request Sep 6, 2018

Update invalid block copy #9667

Merged

4 tasks

talldan requested changes Sep 7, 2018

View reviewed changes

johngodley added 4 commits September 7, 2018 13:38

Clean up var naming and class usage

382ec69

Remove unnecessary flexbox

076e01d

Remove max height

b0ed4a5

It’s not necessary for anything and makes the modal too small on mobile

Allow full height on small screens

7694f8d

Restore flex for vertical spacing of elements

d326543

Without this the button doesnt stick to the bottom

talldan reviewed Sep 10, 2018

View reviewed changes

Replace inline diff code with jsdiff module

ddf1862

aduth reviewed Sep 11, 2018

View reviewed changes

talldan reviewed Sep 12, 2018

View reviewed changes

talldan approved these changes Sep 12, 2018

View reviewed changes

johngodley merged commit 5ea0283 into master Sep 13, 2018

johngodley deleted the add/invalid-block-diff branch September 13, 2018 08:51

aduth reviewed Sep 13, 2018

View reviewed changes

johngodley mentioned this pull request Sep 14, 2018

Fix camelCase and cross-component class name #9895

Merged

4 tasks

talldan mentioned this pull request Sep 25, 2018

Add UI for unregistered block types #8274

Merged

gziolo mentioned this pull request Oct 12, 2018

Extending blocks - validation fail #6856

Closed

oandregal mentioned this pull request Nov 21, 2018

Leverage revisions in convert to blocks action #12162

Closed

Block comparison: show difference between invalid block conversion options #7995

Block comparison: show difference between invalid block conversion options #7995

Conversation

johngodley commented Jul 17, 2018 • edited Loading

Description

How has this been tested?

Types of changes

Checklist:

johngodley commented Sep 2, 2018

johngodley commented Sep 2, 2018

jasmussen commented Sep 3, 2018

jasmussen commented Sep 5, 2018

johngodley commented Sep 5, 2018

jasmussen commented Sep 6, 2018

johngodley commented Sep 6, 2018

jasmussen commented Sep 6, 2018

talldan left a 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

talldan Sep 7, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johngodley commented Sep 7, 2018

Choose a reason for hiding this comment

talldan commented Sep 10, 2018 • edited Loading

johngodley commented Sep 11, 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

talldan Sep 12, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

talldan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johngodley Sep 12, 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

johngodley commented Sep 13, 2018

jasmussen commented Sep 13, 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

johngodley commented Jul 17, 2018 •

edited

Loading

talldan Sep 7, 2018 •

edited

Loading

talldan commented Sep 10, 2018 •

edited

Loading

talldan Sep 12, 2018 •

edited

Loading

johngodley Sep 12, 2018 •

edited

Loading