Text with multiple formatting does not render space

FengWei-Plasmatic commented 2 years ago

Decision Table

[X] My issue does not look like “The HTML attribute 'xxx' is ignored” (unless we claim support for it)
[X] My issue does not look like “The HTML element <yyy> is not rendered”

Good Faith Declaration

[X] I have read the HELP document here: https://git.io/JBi6R
[X] I have read the CONTRIBUTING document here: https://git.io/JJ0Pg
[X] I have confirmed that this bug has not been reported yet

Description

When rendering html with text inside two or more combinations of  (bold),  (italics), or  (underline), the text's preceding and following spaces are not rendered.

ex. Rendering Bold Italics Normal will show Bold ItalicsNormal instead of Bold Italics Normal (see snack)

React Native Information

confer snack

RNRH Version

v6.3.4

Tested Platforms

[X] Android
[X] iOS
[ ] Web
[ ] MacOS
[ ] Windows

Reproduction Platforms

[X] Android
[X] iOS
[ ] Web
[ ] MacOS
[ ] Windows

Minimal, Reproducible Example

https://snack.expo.dev/@fengwei/render-html-formatted-text-bug

Additional Notes

No response

jsamr commented 2 years ago

@FengWei-Plasmatic Good call; thanks for your great report. Will take a look.

got2golfxx commented 2 years ago

I had the same issue. The spaces are being collapsed on span, I, b. Solved by using tag styles and adding whitespace: 'pre" to bold, italics and span. Not optimal but it worked.

sladkoff commented 1 year ago

@jsamr Did you have a chance to look into this? Can you give some pointers for first-time contributors on where to start searching for the cause of this bug? Thanks.

jsamr commented 1 year ago

@sladkoff I did not! There are two possibilities. Either the problem happens at React Native level, or in the transient-render-engine package. Best is to set the debug prop to true and look a the output. If the whitespace is missing in the debug log tree, then the bug is from this package: https://github.com/native-html/core/tree/master/packages/transient-render-engine

You're then welcome to contribute to this other repo directly; I would suggest adding unit tests to cover this case.

CDBridger commented 11 months ago

Just commenting that I'm getting this issue too, and adding whitespace: 'pre' isn't really a solution that I can use.

CDBridger commented 10 months ago

Just as a specific update,

the html

<p><span> conference, has seen a </span><span>21-page global</span></p>

correctly gets rendered in the browser but using this library visually produces "conference, has seen a21-page global". (notice the missing space)

With the debug on this is the tree it produces

<TBlock tagName="p">
  <TPhrasing anonymous>
    <TPhrasing tagName="span">
      <TText anonymous data=" conference, has seen a" />
    </TPhrasing>
    <TText tagName="span" data="21-page global" />
  </TPhrasing>
</TBlock>

so we can see that the first TText node has an incorrectly trimmed string so assuming what @jsamr has said, it must be coming from the transient-render-engine.

Ideally we wouldn't have a weird span in the middle of the sentence like this example, but this is getting auto generated from a WYSIWYG editor.

EDIT: Further investigation, something is calling the trim right function on the TNodeCtor in the transient-render-engine, so the TText gets intialised with the correct string, and then at a later point gets trimmed via the trimRight function which trims the last child in the node.

2nd EDIT: Ah I see, By default when the tree is built, trim right is (and trim left) is called on the root (and something called collapse, unsure what that does), this works itself down the tree which eventually causes the string to be trimmed. I can see there is a prop called dangerouslyDisableWhitespaceCollapsing which you can pass which actually fixes this issue (by skipping the two trims and the collapse).

buildTTreeFromDoc(document: Document | Element): TDocument {
    const tdoc = translateDocument(document, this.dataFlowParams);
    const hoistedTDoc = this.hoistingEnabled ? hoist(tdoc) : tdoc;
    const collapsedTDoc = this.whitespaceCollapsingEnabled
      ? collapse(hoistedTDoc) // <- this can be skipped by passing dangerouslyDisableWhitespaceCollapsing on the renderer
      : tdoc;
    return collapsedTDoc as unknown as TDocument;
  }

  export function collapse(root: TNodeImpl): TNodeImpl {
  root.collapse();
  root.trimLeft();
  root.trimRight(); // <- this causes the incorrectly trimmed string
  return root;
}

My question to @jsamr is, why is this dangerous? and why is trimming the desired behavior (as this is making a visual change that is not present in the source html)? I don't want to go flipping this flag on in our production builds without knowing the reasoning behind the default behavior, and all the consequences of flipping the flag.

Cheers

CDBridger commented 10 months ago

Also as a follow up to my above comment, another way of solving it would be if the html

<p>
   <span />
    <span />
</p>

would get turned into a

<TBlock tagName="p">
  <TPhrasing anonymous>
    <TText tagName="span"data=" conference, has seen a " />
    <TText tagName="span" data="21-page global" />
  </TPhrasing>
</TBlock>

so then the first TText isn't the last child of its parent and then wouldn't get trimmed, currently for some reason it gets put into another TPhrasing (with the span applied) which then makes it the last child (with no span applied), causing it to get trimmed.

any thoughts @jsamr ?

EDIT: A further look into the translate code, it looks like this transient-engine-library doesn't support having more than one text element as a child of a node?

  if (elementModel.isTranslatableTextual()) {
    if (node.children.length === 1) {
      const child = node.children[0] as Node;
      if (isDomText(child)) {
        return new TTextCtor({
          ...sharedProps,
          elementModel,
          textNode: child,
          domNode: node
        });
      }
    } else if (node.children.length === 0) {
      return new TTextCtor({
        ...sharedProps,
        elementModel,
        domNode: node,
        textNode: new Text('')
      });
    }
    const phrasing = new TPhrasingCtor({
      ...sharedProps,
      domNode: node,
      elementModel
    });
    bindChildren(phrasing, node.children, params); //< -- this is effectively recursive, working its way down the tree
    return phrasing;
  }

based on this snippet which is called inside translateDocument it looks like if there are more than 1 siblings, it wraps everything but the first in a Phrase before then generating a TextCtor as a child of that phrase. That means something like what ive suggested,

<Phrase><Text /> <Text /> </Phrase> is just not possible with the current way this library works? (as any siblings would get put into another child)

CDBridger commented 10 months ago

Hmm I understand it a little better now, my second suggestion would never really work, as it interprets it chunk by hcunk, e.g

tag -> text -> close tag -> open tag -> text -> close tag. So it would never get two sibling text childs.

for instance here is the object it is parsing in thr above HTML

 {
    "type": "tag",
    "startIndex": null,
    "endIndex": null,
    "children": [
      {
        "type": "tag",
        "startIndex": null,
        "endIndex": null,
        "children": [
          {
            "type": "text",
            "startIndex": null,
            "endIndex": null,
            "data": "A dramatic turnaround in "
          },
          {
            "type": "tag",
            "startIndex": null,
            "endIndex": null,
            "children": [
              {
                "type": "text",
                "startIndex": null,
                "endIndex": null,
                "data": "Dubai"
              }
            ],
            "name": "strong",
            "attribs": {}
          },
          {
            "type": "text",
            "startIndex": null,
            "endIndex": null,
            "data": ", at the "
          },
          {
            "type": "tag",
            "startIndex": null,
            "endIndex": null,
            "children": [
              {
                "type": "text",
                "startIndex": null,
                "endIndex": null,
                "data": "COP28"
              }
            ],
            "name": "strong",
            "attribs": {}
          },
          {
            "type": "text",
            "startIndex": null,
            "endIndex": null,
            "data": " conference, has seen a "
          }
        ],
        "name": "span",
        "attribs": {}
      },
      {
        "type": "tag",
        "startIndex": null,
        "endIndex": null,
        "children": [
          {
            "type": "text",
            "startIndex": null,
            "endIndex": null,
            "data": "21-page global deal on reducing climate change produced, which for the first time ever, takes explicit aim at the use of fossil fuels. "
          }
        ],
        "name": "span",
        "attribs": {}
      }
    ],
    "name": "p",
    "attribs": {}
  },

so aside from setting the dangerous flag (which I'm not sure i want to do), i'm out of ideas

meliorence / react-native-render-html