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

Serializer: Remove HTML beautification, preserve whitespace #7892

Merged
merged 2 commits into from
Jul 17, 2018

Conversation

aduth
Copy link
Member

@aduth aduth commented Jul 11, 2018

Supersedes #6716
Fixes #4456

This pull request seeks to remove in-block HTML beautification. Per intent with #633, there are still newlines between blocks. Within a block, however, the default serialization will not inject any of its own automated whitespace or tabbing. Existing intentional whitespace will be preserved (e.g. code blocks, shortcode blocks).

This has a few benefits:

  • Performance
    • js-beautify accounts for 48kb (18.1kb gzipped), now removed
    • No longer need to parse / rewrite HTML when saving a post during serialization stage
  • Accuracy
    • children parse would include (often wrongly) text nodes not always intended but automatically injected by beautifier
    • No more collapsing whitespace in shortcode blocks
      • Verified in updated adding-blocks E2E test which would have previously failed in master (see bd78d55)
    • Surfaces potential issues with raw handler not stripping newlines where stripping may be intended (see new test/integration/fixtures output files, e.g. apple-out.html table block)

Implementation notes:

The vast majority of affected file changes are fixture updates. The only meaningful changes are those made within blocks/api/serializer.js, though fixtures verification would be good as well.

Testing instructions:

Verify there are no issues with serializing and restoring block contents.

Examples:

  • Save a Demo post and reload it, ensure no validation errors
  • Save a post including a Columns block and reload it, ensure content preserved and no validation errors

@aduth aduth added the Framework Issues related to broader framework topics, especially as it relates to javascript label Jul 11, 2018
@aduth aduth added this to the 3.3 milestone Jul 11, 2018
@aduth aduth requested review from danielbachhuber, ellatrix and a team July 11, 2018 14:52
Copy link
Member

@danielbachhuber danielbachhuber left a comment

Choose a reason for hiding this comment

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

Can you include a test specifically for #4456? My understanding is that we'd need to change the Shortcode Block <input> to <textarea> to properly display multi-line shortcodes.

Also, it'd be great if you merged in #7846 to incorporate those tests.

@aduth
Copy link
Member Author

aduth commented Jul 11, 2018

Rebased against master.

@danielbachhuber The tests added in 6ae7a11 specifically target multi-line shortcode (the shortcode block is already a textarea field). Is this what you had in mind; or something different?

@danielbachhuber
Copy link
Member

(the shortcode block is already a textarea field)

Ok, great.

Is this what you had in mind; or something different?

Could you add one more test for segmentHTMLToShortcodeBlock?

@aduth
Copy link
Member Author

aduth commented Jul 11, 2018

I don't think that falls within the scope of what this pull request is targeting. Raw handling is a different feature than the serialization beautifier.

@danielbachhuber
Copy link
Member

I don't think that falls within the scope of what this pull request is targeting. Raw handling is a different feature than the serialization beautifier.

Ok. I still think it would be a helpful test to have to truly fix #4456. Would you like me to add the test when I have a moment?

@aduth
Copy link
Member Author

aduth commented Jul 11, 2018

That's fine. I guess I'm unclear about what's specifically not covered already to address #4456 . The classic editor content issue is already fixed (on master), and this one resolves issues with shortcode block (with tests). There's nothing in the original report that touches segmentHTMLToShortcodeBlock .

@danielbachhuber
Copy link
Member

@aduth Ok. I'll defer to your better judgement for this PR, and follow up with a second PR for anything I think of.

@aduth aduth merged commit 6462f08 into master Jul 17, 2018
@aduth aduth deleted the remove/beautify branch July 17, 2018 20:20
@mtias
Copy link
Member

mtias commented Jul 17, 2018

And such was the way beauty left our world...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Framework Issues related to broader framework topics, especially as it relates to javascript
Projects
None yet
Development

Successfully merging this pull request may close these issues.

New lines in shortcode content are ignored
3 participants