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

Validate and protect corrupted block content #1929

Merged
merged 5 commits into from
Jul 26, 2017
Merged

Conversation

aduth
Copy link
Member

@aduth aduth commented Jul 17, 2017

Closes #1735
Closes #1736
Related: #391

This pull request seeks to validate blocks parsed from post content. It does so by regenerating the expected save output upon the initial parse and comparing it against the actual content. If there is a mismatch, the block is flagged as invalid and the user is barred from editing the block (aside from moving or deleting) with a visual indicator of the invalid state:

Invalid block

Implementation notes:

  • Yes, there's likely a performance impact of creating an initial serialization of the block
  • Yes, there's likely false positives that can occur, particular on order of attributes or class names (which in reality should not have an impact on behavior of content). To reduce false positives, both contents are run through the same HTML beautifier we use for serialization. False positives from reordering attributes seems acceptable to me, as we should discourage manual markup modifications, or at least transition the user to a "manual mode". Enhancements could be made here. Editing block markup in Text mode should be unaffected.

Testing instructions:

I've left the initial Cover Image block in Demo content as invalid, because I'd like to revisit whether we really want nested section tags in the serialized output. Verify that the first block in post content is flagged as invalid and cannot be edited.

  1. Navigate to Gutenberg > Demo
  2. Note that the first block has a "Corrupt" warning like the one shown above

@aduth aduth added the [Feature] Parsing Related to efforts to improving the parsing of a string of data and converting it into a different f label Jul 17, 2017
@afercia
Copy link
Contributor

afercia commented Jul 17, 2017

Worth noting that, while clicking the block is disabled, using a keyboard it's still possible to focus the cover image title and edit it. Tabbing backwards from the title it's also possible to focus the formatting toolbar buttons and use them:

screen shot 2017-07-17 at 23 31 11

@aduth
Copy link
Member Author

aduth commented Jul 18, 2017

using a keyboard it's still possible to focus the cover image title and edit it

Great point, and a challenging problem to solve if we want to keep the edit render visible in the background (a nice effect I think). A few possibilities that come to mind:

  • Intercept the focus event within the block and "skip" to the next valid focus-able element
  • Query for all editable inputs within the block after render and disable them
  • Wait for something to come of an intertness property, or simulate effect with a component wrapper
  • Show only the warning notice, no edit preview

Other ideas welcome.

<div className="editor-visual-editor__invalid-block-warning">
<Dashicon icon="warning" size={ 40 } />
<p>{ __(
'Uh oh! This block is corrupt and has been locked to protect ' +
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"This block has been modified externally and has been locked to protect against content loss." cc @jasmussen

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, that sounds good!

@mtias
Copy link
Member

mtias commented Jul 24, 2017

We should consider adding these buttons:

[ Convert to Freeform block ] [ Overwrite changes ] [ Do nothing ]

@mtias mtias added [Feature] Blocks Overall functionality of blocks General Interface Parts of the UI which don't fall neatly under other labels. labels Jul 24, 2017
@jasmussen
Copy link
Contributor

We should consider adding these buttons:
[ Convert to Freeform block ] [ Overwrite changes ] [ Do nothing ]

I dig that. Though it beckons yet another name than "Classic Text" for the fallback block. I mean, I do like the "Classic Text" name, I think it's good for when you mean to insert it via the Inserter, and it's more descriptive than "Freeform", but in this context it's obvious that it can contain more than just "text". Should we simply call it "Classic Editor"?

@mtias
Copy link
Member

mtias commented Jul 24, 2017

Yes, any name works for me. Maybe "convert to generic block"? Also we could offer the option to handle as HTML block.

@mtias
Copy link
Member

mtias commented Jul 24, 2017

This looks like a great start to me. @nylen what do you think of measuring performance impact with and without validation on a long-ish post? We may consider making this something you can disable via config or filter.

@aduth aduth force-pushed the try/validate-parse branch from eeb0450 to f452481 Compare July 24, 2017 16:00
@aduth
Copy link
Member Author

aduth commented Jul 24, 2017

Rebased to resolve conflicts and update warning text.

I tried a couple ideas relating to disabling keyboard focus for the preview. Both of which are not particularly ideal from a performance point-of-view. Each implements an Inert component which, when wrapping content, enforces that children cannot receive focus.

  • The approach in 7ad484f intercepts global tab keydown events and, if the key would result in the node receiving focus, prevents the default event behavior and instead shifts focus to the next focusable node it finds
  • The approach in f452481 finds and disables all inputs and contenteditables within the node every time it renders

@mtias
Copy link
Member

mtias commented Jul 25, 2017

@aduth I'm thinking it'd be cool to be able to switch between "old" and "new" renders to visually diff. It could be two tabs next to the warning message.

@aduth
Copy link
Member Author

aduth commented Jul 25, 2017

I'm thinking it'd be cool to be able to switch between "old" and "new" renders to visually diff. It could be two tabs next to the warning message.

Should we do this in a follow-up pull request?

@mtias
Copy link
Member

mtias commented Jul 25, 2017

Should we do this in a follow-up pull request?

Yes, just wondered if it could help with the keyboard issues, because we could try not rendering the default view.

@aduth
Copy link
Member Author

aduth commented Jul 25, 2017

The issue is that if preview uses edit rendering, it's cumbersome to find and disable all inputs within that render.

Alternatively, we could render the save representation for corrupt blocks, which unlike the Edit form should include no editable fields, and is arguably more appropriate to show in case "corrupt" means "user made their own edits that may appear in preview".

@mtias
Copy link
Member

mtias commented Jul 25, 2017

Yes, I was thinking of rendering save and raw as comparison, not edit.

@aduth aduth force-pushed the try/validate-parse branch from f452481 to 5328e23 Compare July 25, 2017 18:12
@aduth
Copy link
Member Author

aduth commented Jul 25, 2017

5328e23 shows the save representation as the preview, which shouldn't have any editable fields, and avoids the need for any special tab behavior (well... now that I think of it, if the block is a contact form, maybe not so much 😬 ).

@mtias
Copy link
Member

mtias commented Jul 25, 2017

If it's a contact form, it is likely the save method is null.

@aduth
Copy link
Member Author

aduth commented Jul 25, 2017

Sure, it's a rare case that the save representation have focusable fields, but still technically possible and not very well handled here.

@jasmussen
Copy link
Contributor

I took the liberty of pushing a little visual polish:

screen shot 2017-07-26 at 09 23 27

Matías points about some action buttons seems really worth addressing, though okay to do it in a separate PR. In the mean time, the message feels like it should be rephrased to be less "this is what's up" and more like "this is what it means". I'm not entirely sure how to phrase this without being overly verbose, as it's a rather complex thing to explain.

Perhaps this begs general help text in some way, or like linking to external documentation. A simple "What does this mean?" link at the end could address it.

@aduth aduth force-pushed the try/validate-parse branch from 5132df5 to feb105e Compare July 26, 2017 16:27
@aduth
Copy link
Member Author

aduth commented Jul 26, 2017

Since this touches many different files, I don't want to let this linger pending finer details. I assume we'll need to do some revisions in subsequent pull requests, notably of course fixing the "corrupt" demo Cover Image block.

In the meantime, I've rebased and force-pushed with intent to merge.

One additional change I made is allow the root node block list item to be focused so that it can be "selected", but only for arrangement / deletion (no toolbar shown). See feb105e.

@aduth
Copy link
Member Author

aduth commented Jul 26, 2017

Actually, I did find one remaining item that should probably be addressed here: If the user tries to edit an invalid block in Text mode, those user edits will not be respected on save.

@aduth
Copy link
Member Author

aduth commented Jul 26, 2017

I believe the behavior described in #1929 (comment) is actually working correctly, and the issue I'm seeing is caused by the wrong URL being assigned when saving a post from the Demo.

Should be: ?page=gutenberg&post_id=568
Actually: ?page=gutenberg-demo&post_id=568

...Resulting in demo content being shown after a refresh, not the saved post's.

Likely a regression introduced in #1967, where $is_demo check no longer accounts for whether a post ID is present in query parameters. Will open a separate pull request shortly.

@aduth
Copy link
Member Author

aduth commented Jul 26, 2017

I've verified that editing an invalid block in Text mode works as expected: Changes will be respected, but switching back to Visual will still show the block as invalid so long as its reserialization doesn't match.

See also #2032 for demo content save fix.

@aduth aduth merged commit a8b39fb into master Jul 26, 2017
@aduth aduth deleted the try/validate-parse branch July 26, 2017 17:17
}
},
"isValid": false,
"originalContent": "<div class=\"wp-block-gallery\">\n\t<figure class=\"blocks-gallery-image\">\n\t\t<img src=\"https://cldup.com/uuUqE_dXzy.jpg\" alt=\"title\" />\n\t</figure>\n\t<figure class=\"blocks-gallery-image\">\n\t\t<img src=\"http://google.com/hi.png\" alt=\"title\" />\n\t</figure>\n</div>"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs to be fixed.

}
},
"isValid": false,
"originalContent": "<figure class=\"wp-block-image\"><img src=\"https://cldup.com/YLYhpou2oq.jpg\" class=\"aligncenter\"/><figcaption>Give it a try. Press the &quot;really wide&quot; button on the image toolbar.</figcaption></figure>"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs to be fixed.

}
},
"isValid": false,
"originalContent": "<ul><li>Text & Headings</li><li>Images & Videos</li><li>Galleries</li><li>Embeds, like YouTube, Tweets, or other WordPress posts.</li><li>Layout blocks, like Buttons, Hero Images, Separators, etc.</li><li>And <em>Lists</em> like this one of course :)</li></ul>"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs to be fixed.

}
},
"isValid": false,
"originalContent": "<pre class=\"wp-block-preformatted\">Some <em>preformatted</em> text...<br>And more!</pre>"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs to be fixed.

}
},
"isValid": false,
"originalContent": "<blockquote class=\"wp-block-pullquote\">\n<p>Testing pullquote block...</p><footer>...with a caption</footer>\n</blockquote>"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs to be fixed.

}
},
"isValid": false,
"originalContent": "<table><thead><tr><th>Version</th><th>Musician</th><th>Date</th></tr></thead><tbody><tr><td><a href=\"https://wordpress.org/news/2003/05/wordpress-now-available/\">.70</a></td><td>No musician chosen.</td><td>May 27, 2003</td></tr><tr><td><a href=\"https://wordpress.org/news/2004/01/wordpress-10/\">1.0</a></td><td>Miles Davis</td><td>January 3, 2004</td></tr><tr><td>Lots of versions skipped, see <a href=\"https://codex.wordpress.org/WordPress_Versions\">the full list</a></td><td>&hellip;</td><td>&hellip;</td></tr><tr><td><a href=\"https://wordpress.org/news/2015/12/clifford/\">4.4</a></td><td>Clifford Brown</td><td>December 8, 2015</td></tr><tr><td><a href=\"https://wordpress.org/news/2016/04/coleman/\">4.5</a></td><td>Coleman Hawkins</td><td>April 12, 2016</td></tr><tr><td><a href=\"https://wordpress.org/news/2016/08/pepper/\">4.6</a></td><td>Pepper Adams</td><td>August 16, 2016</td></tr><tr><td><a href=\"https://wordpress.org/news/2016/12/vaughan/\">4.7</a></td><td>Sarah Vaughan</td><td>December 6, 2016</td></tr></tbody></table>"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs to be fixed.

}
},
"isValid": false,
"originalContent": "<pre class=\"wp-block-verse\">A <em>verse</em>…<br>And more!</pre>"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs to be fixed.

@nylen
Copy link
Member

nylen commented Jul 26, 2017

There are now several examples of block fixtures with "isValid": false in the test suite. All of these should probably be valid, and we need to add test cases for the ways we expect this feature to detect invalid blocks.

@aduth
Copy link
Member Author

aduth commented Jul 26, 2017

See #2038 for updated Cover Image implementation to resolve lingering warning on Demo content screen.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Blocks Overall functionality of blocks [Feature] Parsing Related to efforts to improving the parsing of a string of data and converting it into a different f General Interface Parts of the UI which don't fall neatly under other labels.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants