Overlapping styles and entities in convertToHTML not working when intersecting no style

[jebarjonet]

I just noticed the release of #43 which is super cool. Unfortunately, it does not work for intersections situations, which is problematic.

Here is my code:

convertToHTML({
    blockToHTML: { // I put this here to have <div> in my output instead of <p>
        unstyled: {
            start: '<div>',
            end: '</div>',
            empty: '<br>',
        }
    },
    entityToHTML: (entity, originalText) => {
        if (entity.type === 'link') {
            return {
                start: `<a href="${entity.data.url}" target="_blank">`,
                end: '</a>',
            }
        }
        return originalText
    }
})(contentState)

Overlapping styles

1. Add link

2. Add bold intersecting link

3. Result

<div>t<strong>e<</strong>a href="http://google.com" target="_blank">st test</a></div>

4. Render

Situation

1. Style strictly inside link ✅

2. Style over link ✅

3. Style outside link ✅

4. Style intersecting link 🔴

Intersect start

Result:

Code:

<div>t<strong>e<</strong>a href="http://google.com" target="_blank">st test te</a>st</div>

Intersect end

Result:

Code:

<div>te<a href="http://google.com" target="_blank">st test t<strong>e<</strong>/a>st</div>

[noahlemen]

Should resulting tags overlap, or be close/opened around the intersect?

In the Intersect start example, for example, which of these possibilities would be desired?

• <div>t<strong>e<a href="http://google.com" target="_blank">s</strong>t test te</a>st</div>
• <div>t<strong>e</strong><a href="http://google.com" target="_blank"><strong>s</strong>t test te</a>st</div>

For HTML5, either one should be valid. Is there any preference here?

[benbriggs]

I think the latter is what we want here - it's how overlapping style ranges currently work and I'm not so sure the first is valid.

[benbriggs]

closed via #57 🎉

[liyantang]

@benbriggs Found a breaking case:

Given that you style text not overlapping to links, and have more that one link in text block, style is left on stack and error is thrown.

Example:

testing testing testing testing testing

Uncaught Error: Character 73: -1 styles left on stack that should no longer be there

Error is thrown in blockInlineStyles: https://github.com/HubSpot/draft-convert/blob/master/src/blockInlineStyles.js#L119

[benbriggs]

Hmm I can't seem to reproduce that @liyantang, can you provide a failing test? Here's one I tried to mimic your example that passes:

    const contentState = buildContentState([
      {
        type: 'unstyled',
        text: 'testing testing testing testing testing',
        styleRanges: [
          {
            style: 'BOLD',
            offset: 8,
            length: 7
          }
        ],
        entityRanges: [
          {
            key: 0,
            offset: 16,
            length: 7
          },
          {
            key: 1,
            offset: 24,
            length: 7
          }
        ]
      }
    ], {
      0: {
        type: 'LINK',
        mutability: 'MUTABLE',
        data: {
          url: 'asdf'
        }
      },
      1: {
        type: 'LINK',
        mutability: 'MUTABLE',
        data: {
          url: 'asdf'
        }
      }
    });

    const html = convertToHTML({
      entityToHTML: (entity, originalText) => {
        if (entity.type === 'LINK') {
          return <a>{originalText}</a>;
        }
      }
    })(contentState);

    expect(html).toBe('<p>testing <strong>testing</strong> <a>testing</a> <a>testing</a> testing</p>');

[liyantang]

@benbriggs

Example:

    it('failing test', () => {
      const contentState = buildContentState([
        {
          type: 'unstyled',
          text: 'overlapping test Hello World',
          styleRanges: [
            {
              offset: 0,
              length: 11,
              style: 'BOLD'
            },
            {
              offset: 23,
              length: 5,
              style: 'ITALIC'
            }
          ],
          entityRanges: [
            {
              key: 0,
              offset: 17,
              length: 5,
              prefixLength: '<a href="http://google.com">'.length,
              suffixLength: '</a>'.length
            },
            {
              key: 1,
              offset: 23,
              length: 5,
              prefixLength: '<a href="http://google.com">'.length,
              suffixLength: '</a>'.length
            }
          ],
        },
      ], {
        0: {
          type: 'LINK',
          mutability: 'IMMUTABLE',
          data: {
            href: 'http://google.com',
          }
        },
        1: {
          type: 'LINK',
          mutability: 'IMMUTABLE',
          data: {
            href: 'http://google.com',
          }
        }
      });

      const result = convertToHTML(convertToHTMLProps)(contentState);

      expect(result).toBe('<strong>overlapping</strong> test <a href="http://google.com">Hello</a> <em><a href="http://google.com">World</a></em>');
    });

[benbriggs]

Hmm...that still passes for me as well (after adding wrapping <p> tags and defining convertToHTMLProps). Is your local fork/clone up to date to draft-convert@1.4.5?

[liyantang]

I am on the latest of master branch of 1.4.5. I added this test to the existing tests in converToHTML.js and it throws the error for me.

[benbriggs]

Strange! Could you try submitting it as a PR so that the test runs in Travis and we can have an isolated place to verify that it can be fixed? Thanks for digging into this!

[liyantang]

#66

https://travis-ci.org/HubSpot/draft-convert/builds/226509786

[benbriggs]

Perfect, thanks! I'll dig into it now.

[liyantang]

Great, thanks!

[benbriggs]

Fix is landed in 1.4.6 - thanks so much for your help @liyantang!