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

Add Custom HTML block description and use sandbox for preview #1625

Closed
wants to merge 4 commits into from

Conversation

westonruter
Copy link
Member

For the Custom HTML block, see #1386.

Amends #1597 by adding a block description for the Custom HTML block to align with new Custom HTML widget in 4.8-alpha: https://github.com/WordPress/wordpress-develop/blob/2744e29fd3f0fb17485290d2bf827f21292a92c3/src/wp-includes/widgets/class-wp-widget-custom-html.php#L38

Also adds list of common HTML tags that are disallowed when the user cannot unfiltered_html, similar to the Custom HTML widget: https://github.com/WordPress/wordpress-develop/blob/2744e29fd3f0fb17485290d2bf827f21292a92c3/src/wp-includes/widgets/class-wp-widget-custom-html.php#L124-L135

Custom HTML description

To test, try accessing Gutenberg with a user having an author role.

I'm not certain that exporting the allowed HTML tags and the current user's unfiltered_html capability by amending them to the wp.editor global is the best way to go about this.

Also, and this is probably for a separate issue, I should think that the preview should actually enforce the constraints to remove the disallowed HTML? This is probably also closely related to the need to sandbox the previewed HTML in an iframe so that document.write() calls can be previewed properly.

@westonruter westonruter requested a review from mtias July 1, 2017 06:43
@westonruter westonruter force-pushed the add/custom-html-block-description branch from 98b8959 to 0eee405 Compare July 4, 2017 05:40
@westonruter
Copy link
Member Author

There, now thanks to #1392 there is a proper sandbox to put the custom HTML into, and this means that document.write() will work as expected. It doesn't yet strip out illegal HTML, however.

@westonruter westonruter changed the title Add Custom HTML block description Add Custom HTML block description and use sandbox for preview Jul 4, 2017
@westonruter
Copy link
Member Author

Note the issue of content being clipped in the sandboxed iframe is addressed in #1688.

@westonruter westonruter force-pushed the add/custom-html-block-description branch from c1f14c2 to f1b515c Compare July 12, 2017 19:37
@westonruter
Copy link
Member Author

Rebased to include sandbox improvements from #1688. Former HEAD was c1f14c2.

@westonruter
Copy link
Member Author

@mtias any further changes needed here?

@aduth aduth self-requested a review August 3, 2017 13:32
@gziolo
Copy link
Member

gziolo commented Sep 22, 2017

@westonruter this one needs rebase. @mtias can you check this one?

@gziolo gziolo self-requested a review September 22, 2017 14:43
@jasmussen
Copy link
Contributor

Love this, it seems like a solid improvement over #2917. Perhaps we can take the first paragraph of that ticket, and add the allowed tags below that?

@@ -487,6 +487,10 @@ function gutenberg_editor_scripts_and_styles( $hook ) {
'before'
);

// Export data required by the Custom HTML block.
wp_add_inline_script( 'wp-editor', sprintf( 'wp.editor.canUnfilteredHtml = %s;', wp_json_encode( current_user_can( 'unfiltered_html' ) ) ) );
Copy link
Member

Choose a reason for hiding this comment

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

Can we merge this in with other editor settings? I'm thinking we need a scalable solution here.

$editor_settings = array(
'wideImages' => ! empty( $gutenberg_theme_support[0]['wide-images'] ),
'colors' => $color_palette,
);

The withEditorSettings higher-order component was created to avoid having the component directly reference the window global:

https://github.com/WordPress/gutenberg/tree/master/blocks/with-editor-settings

Copy link
Member Author

Choose a reason for hiding this comment

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

I absolutely agree. The withEditorSettings was not available at the time I wrote this, I believe.

@@ -39,6 +42,9 @@ registerBlockType( 'core/html', {
this.state = {
preview: false,
};
const allowedHtmlTags = new Set( Object.keys( wp.editor.allowedPostHtml ) );
Copy link
Member

Choose a reason for hiding this comment

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

  • Can we be formatting this data from the server to be in a more useful shape? i.e. would we always be expecting to call Object.keys and if so, pass from the server as array_keys ? Not sure how likely it is we'd do anything with the allowed attributes and could avoid increasing the size of the page by including only what's used.
  • The cache assignment here reminds me of a since-removed document about props in state, which is more to the point of: rather than splitting the source of truth, merely generate this rendering on the fly. Since allowedPostHtml is not a prop and is not likely to change, this may not be an issue, but could still be seen as an over-optimization to split the logic as we've done here.

Copy link
Member Author

Choose a reason for hiding this comment

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

Can we be formatting this data from the server to be in a more useful shape?

Actually it is useful as it is. The data consists of keys for the HTML tags, and then the values are objects that indicate the attributes that are allowed for each element. It's the same structure as: https://github.com/WordPress/wordpress-develop/blob/1eda9654da06715798cbf911372c0c13e5baf7ca/src/wp-includes/kses.php#L53-L416

@westonruter
Copy link
Member Author

@jasmussen @aduth @mtias Actually the Custom HTML widget has improved significantly since it was introduced in 4.8.1. Now the widget incorporates CodeMirror and leverages the HTMLHint linter with a custom Kses rule to contextually inform users of illegal tags if they are actually used, preventing them from saving until they fix the illegal (or malformed) HTML. This is naturally way better than listing the tags that are not allowed. It also ensures that the invalid HTML is not silently stripped from the field when they try saving:

image

The list of disallowed tags is now exported from the server as part of the call to wp_enqueue_code_editor(). I'm working on a dev note for this now.

I won't have time until December to work on an enhanced Custom HTML block to make use of the new code editor APIs in 4.9-beta, but if someone wants to work on it now it would be valuable for validation of the new code editor APIs in 4.9. You can refer to the Custom HTML widget JS. I'm available to answer any questions and provide support, but I won't be able to own it due to 4.9 release responsibilities.

@noisysocks
Copy link
Member

I'm taking over this as part of #4348.

@noisysocks noisysocks closed this Jan 8, 2018
@gziolo gziolo deleted the add/custom-html-block-description branch January 8, 2018 07:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants