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

RichText: Ensure HTML can be created from serialized object #10351

Closed
wants to merge 2 commits into from

Conversation

ellatrix
Copy link
Member

@ellatrix ellatrix commented Oct 4, 2018

Description

We should make sure the format references are created before converting to HTML so it works correctly. This makes it possible to use the value in the HTML comments.

How has this been tested?

This blocks should work correctly:

wp.blocks.registerBlockType( 'test/test', {
    title: 'test',
    description: 'test',
    category: 'common',

    attributes: {
        content: {
            default: wp.richText.create(),
        },
    },

    edit( props ) {
        return wp.element.createElement( wp.editor.RichText, {
            value: props.attributes.content,
            onChange: ( value ) => props.setAttributes( { content: value } ),
        } );
    },

    save() {
        return null;
    },
} );

Screenshots

Types of changes

Checklist:

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

@@ -34,7 +35,7 @@ export function toTree( value, multilineTag, settings ) {
onStartIndex,
onEndIndex,
} = settings;
const { formats, text, start, end } = value;
const { formats, text, start, end } = normaliseFormats( value );
Copy link
Contributor

Choose a reason for hiding this comment

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

I know normaliseFormats is performant at the moment but this is the kind of function that can hurt the editor's performance pretty quickly if we made a not-good-enough change. How can we ensure it doesn't happen? Any ideas.

(don't require an immediate answer)

Copy link
Member Author

Choose a reason for hiding this comment

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

That's an interesting question, but it's valid for any change within this package.

@@ -27,5 +27,7 @@ describe( 'normaliseFormats', () => {
expect( result.formats[ 1 ][ 0 ] ).toBe( result.formats[ 2 ][ 0 ] );
expect( result.formats[ 1 ][ 0 ] ).toBe( result.formats[ 3 ][ 0 ] );
expect( result.formats[ 2 ][ 1 ] ).toBe( result.formats[ 3 ][ 1 ] );
// Should be serializable.
expect( JSON.stringify( record ) ).toBe( JSON.stringify( result ) );
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

Copy link
Contributor

@youknowriad youknowriad left a comment

Choose a reason for hiding this comment

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

Thanks for the updates Ella. Seems fine to me, I'll try to do some tests to approve.

What do you think about adding docs for the format and some higher level docs on how these toTree, toDom, create... fit together.

If we’re to support it for a long term, just make it explicit.

@mcsf mcsf added [Type] Bug An existing feature does not function as intended [Feature] Rich Text Related to the Rich Text component that allows developers to render a contenteditable labels Oct 4, 2018
@youknowriad youknowriad added this to the 4.0 milestone Oct 4, 2018
@ellatrix
Copy link
Member Author

ellatrix commented Oct 5, 2018

What do you think about adding docs for the format and some higher level docs on how these toTree, toDom, create... fit together.

Yes I definitely plan to write some documentation on this and the format in general, just haven't been able to prioritise it so far.

@youknowriad
Copy link
Contributor

While testing, I noticed this being saved in the HTML

<!-- wp:test/test {"content":{"formats":[null,null,null,null,null,[{"type":"strong"}],[{"type":"strong"}],[{"type":"strong"}],[{"type":"strong"}],null,null,null,null,null,null,null,null,null,null],"text":"test test test test"}} /-->

It feels a bit weird to have a format for each character (all these nulls)

It also produces this error:

screen shot 2018-10-05 at 08 52 23

@ellatrix
Copy link
Member Author

ellatrix commented Oct 5, 2018

@youknowriad I fixed the problem.

Explanation:

When a value object is created, the formats are put into a sparse array, meaning that any space where there are no formats is just empty (not null or undefined, but empty). JSON does not have this concept, so JSON.stringify will create null values for the empty spaces. JSON.stringify( [ , [ format ], , ] ) would equal [ null, [ format ], null, null ].

normaliseFormats so far assumed that the array is sparse, expecting only arrays when it loops over the formats. E.g. [ , [ format ], , ].forEach() will skip the empty spaces.

When the serialised object comes back in, the empty places are null, so we have to account for them in the loop: [ null, [ format ], null, null ].forEach() will loop over the null values. So the solution is to check for the null value and delete it.

I hope this makes sense!

It feels a bit weird to have a format for each character (all these nulls)

Yes, so that's because of JSON.stringify. As I said before, this format is not meant for saving, but for manipulation, though I agree it should still work if someone does save the serialised form. For saving it is best to use HTML.

@ellatrix ellatrix force-pushed the fix/rich-text-value-serialize branch from a39c179 to 24a1195 Compare October 5, 2018 21:05
@ellatrix ellatrix force-pushed the fix/rich-text-value-serialize branch from 24a1195 to a776108 Compare October 8, 2018 15:09
@gziolo
Copy link
Member

gziolo commented Oct 10, 2018

Superseded by #10439.

@gziolo gziolo closed this Oct 10, 2018
@gziolo gziolo deleted the fix/rich-text-value-serialize branch October 10, 2018 13:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Rich Text Related to the Rich Text component that allows developers to render a contenteditable [Type] Bug An existing feature does not function as intended
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants