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.

Sign up for GitHub

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

Jump to bottom

Paste: Omit <style> tags #10019

Merged
merged 3 commits into from
Sep 20, 2018
Merged

Paste: Omit <style> tags #10019

merged 3 commits into from
Sep 20, 2018

Conversation

mcsf
Copy link
Contributor

@mcsf mcsf commented Sep 18, 2018 •
edited
Loading

Description

Fixes #9719

How has this been tested?

  • Verify all block-conversion integration tests pass, including newly added ms-word-styled.
  • Try reproducing parent issue.
  • Stress-test pasting or classic-to-blocks conversion with whatever input.

Questions

  • Properly investigate the regression (this was seemingly working in 2.0), rather than "just fix" as I did.
  • Figure out if it's worth keeping a dedicated HTML.replace call to strip meta tags instead of relying on proper node processing.

Checklist:

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

@mcsf mcsf added [Type] Bug An existing feature does not function as intended [Feature] Paste labels Sep 18, 2018
@mcsf mcsf added this to the 4.0 milestone Sep 18, 2018
@mcsf mcsf requested a review from ellatrix September 18, 2018 23:31
@mcsf mcsf mentioned this pull request Sep 18, 2018
Closed
@mcsf mcsf force-pushed the fix/raw-handling-word-doc-style branch from cf080d9 to 2467448 Compare September 18, 2018 23:34
ellatrix
ellatrix reviewed Sep 19, 2018
View reviewed changes
packages/blocks/src/api/raw-handling/index.js Outdated
@@ -79,7 +80,7 @@ function getRawTransformations() {
*/
export default function rawHandler( { HTML = '', plainText = '', mode = 'AUTO', tagName, canUserUseUnfilteredHTML = false } ) {
// First of all, strip any meta tags.
HTML = HTML.replace( /<meta[^>]+>/, '' );
// HTML = HTML.replace( /<meta[^>]+>/, '' );
Copy link
Member

Choose a reason for hiding this comment

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

This is still needed for internal paste (pasting blocks, which is handled right below).

Copy link
Member

Choose a reason for hiding this comment

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

Maybe needs more comment.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did imagine it would affect the following pieces, but wanted to try this anyway. The tests still pass, so I think we need both comments and also some new tests that fail if this line is commented. Would you have some?

Copy link
Member

Choose a reason for hiding this comment

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

Sure, I'll have a look.

Copy link
Member

Choose a reason for hiding this comment

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

Pushed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks!

ellatrix
ellatrix reviewed Sep 19, 2018
View reviewed changes
packages/blocks/src/api/raw-handling/head-remover/index.js Outdated
export default function( node ) {
if (
node.nodeName !== 'META' &&
node.nodeName !== 'STYLE'
Copy link
Member

Choose a reason for hiding this comment

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

Interesting that we stopped stripping these. Maybe this happened with the paste rewrite, but that was ages ago... If we want to continue with this fix, maybe it's not a bad idea to think about all HTML tags that should be stripped entirely (as opposed to unwrapped), like script etc.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, script would be another one. I can’t see a use for just unwrapping them.

Copy link
Member

Choose a reason for hiding this comment

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

I'd say script, style, noscript and template. meta is fine I think, it cannot have any child nodes. I wouldn't expect it in pasted HTML, but maybe it's prudent to strip head as well.

ellatrix
ellatrix reviewed Sep 20, 2018
View reviewed changes
packages/blocks/src/api/raw-handling/head-remover/index.js Outdated
@@ -0,0 +1,12 @@
export default function( node ) {
Copy link
Member

Choose a reason for hiding this comment

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

Just noting that, unlike the other filters, this has its own folder. Any reason why?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Habit. :) I'll change for consistency.

mcsf and others added 3 commits September 20, 2018 23:30
@mcsf
Raw handling: remove <style> tags
6f1827f 
- Input file for integration test `ms-word-styled` is a truncated
version of file provided by issue reporter. See
#9719 (comment)
@ellatrix @mcsf
Add Gutenberg integration test
03b42f3
@mcsf
Make head-remover a standalone file
46c377a
@mcsf mcsf force-pushed the fix/raw-handling-word-doc-style branch from e6ed53e to 46c377a Compare September 20, 2018 22:30
@mcsf mcsf merged commit 640eb98 into master Sep 20, 2018
@ellatrix ellatrix deleted the fix/raw-handling-word-doc-style branch September 21, 2018 07:28
@Haldaug Haldaug mentioned this pull request Nov 12, 2018
Closed
@designsimply designsimply mentioned this pull request Feb 8, 2019
Closed
@ellatrix
Copy link
Member

ellatrix commented Dec 8, 2022

@mcsf Looks like this wasn't added to the inline filter. #46402 fixes this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ellatrix ellatrix ellatrix approved these changes

Assignees
No one assigned
Labels
[Feature] Paste [Type] Bug An existing feature does not function as intended
Projects
None yet
Milestone
4.0
Development

Successfully merging this pull request may close these issues.

Paste from Word or LibreOffice produces block of CSS first
2 participants
@mcsf @ellatrix