BUG: Missing inline styles used by CKEditor when adding raw html components

gustavohleal commented 2 years ago

GrapesJS version

[X] I confirm to use the latest version of GrapesJS

What browser are you using?

Chromium v96

Reproducible demo link

https://grapesjs.com/demo-newsletter-editor.html

Describe the bug

How to reproduce the bug?

Open the developer tools at the console tab
Run the code below
Select the text added
Edit something with CKEditor
Select another component

What is the expected behavior? Expected to add inline styles from the spans inside the text component so that the CKEditor understand the text-styling and keep them when opened.

What is the current behavior? Adding the raw HTML by editor.addComponent() do add the styling, but once you edit the text and blur it the style is removed from the text segments where they belong. This happens only with CKEditor because it uses the <span> and inline style to edit the text. I'm putting the newsletter preset demo because it is the only demo from grapesjs that use CKEditor. If is necessary to execute some code in order to reproduce the bug, paste it here below:

editor.DomComponents.clear()
editor.addComponents(`<div data-gjs-type="text"><span style="color: blue;">Teste</span> de <span style="font-family: arial; font-size: 25px"><b>span</b></span> inline</div>`)

I need to add components this way so that legacy HTML models and templates can be opened into grapesjs. Sometimes CKEditor even does not recognizes that spans or <b> elements are text segments and throw an error as it does not work with those text styling elements. This problem is similar to #3069 that was fixed and I believe this has returned once the feature of transforming inline styles into classes was added. Can we somehow keep the elements inside text components as raw html?

Code of Conduct

[X] I agree to follow this project's Code of Conduct

shkhalid commented 1 year ago

+1

pety-dc commented 1 year ago

Hey I've been having similar issues. I think it's quite serious as both newsletter and mjml demo are affected. (Both use CKeditor as RTE)

When I import a content that has a text (mj-text) that has styled elements (span) the imported content gets parsed and the styling elements are recognized as individual components and not as contents of the wrapping text element.

example (This is valid according to mjml.io):

<mjml>
  <mj-body>
    <mj-section>
      <mj-column>
        <mj-image width="100px" src="/assets/img/logo-small.png"></mj-image>
        <mj-divider border-color="#F45E43"></mj-divider>
        <mj-text font-size="20px" font-family="helvetica"><strong>Hello</strong> <span style="color: #F45E43;">World</span></mj-text>
      </mj-column>
    </mj-section>
  </mj-body>
</mjml>

Expected result:

Result:

The problem is that due to nested texts are parsed into components, their style attributes gets stripped and the result code does't contain it anymore:

<mjml>
  <mj-body>
    <mj-section>
      <mj-column>
        <mj-image width="100px" src="/assets/img/logo-small.png">
        </mj-image>
        <mj-divider border-color="#F45E43">
        </mj-divider>
        <mj-text font-size="20px" font-family="helvetica">
          <strong>Hello</strong> 
          <span id="io1ak">World</span>
        </mj-text>
      </mj-column>
    </mj-section>
  </mj-body>
</mjml>

The style must be there under the hood somewhere. Because when I doubleclick on the text, the CKEditor loads and somehow "restores" the style attribute. But unless I open every text for editing on each content edit, the style gets lost.

Not all GrapesJS versions are affected. 0.21.1, and 0.21.1 are But 0.16.3 - 0.19.5 behaves as expected.

I checked changes from 0.19.5 to 0.20.1 and I couldn't clearly trace back what could have casued the change in this behaviour. But probably has something to do with Support textable for extended Text components

Now I know that nested texts are native to grapesJS, since this is the default way of styling them. But is there a way to disable nested text feature?

raymondmakz commented 1 year ago

ckeditor output: <div>some rich text from <span style="color:blue">ckeditor</span></div> after parsing into editor, become like: <div data-gjs-type="text">some rich text from <span style="color:blue" data-gjs-type="default">ckeditor</span></div> Basically every dom become components.

Maybe give a choice to developer to decide whether a component should stop add child nodes as components. Can add a new flag property in component's model, eg: 'stopProcessChildren'

Inside ./parser/mode/ParserHtml.ts::parseNode() function, right before going into the recurisve parseNode() and assigning child components model.components = this.parseNode(node, {/*...*/});

Add an else-if condition: if model's 'stopProcessChildren' is true, do not do the parseNode() on children nodes.
next, need to set the current dom node's innerHtml to model's 'content' or do this in component definition

From the default ./dom_components/model/Component.ts::toHTML() and ./dom_components/model/Component.ts::getInnerHTML(), the html output should return model's 'content' if there are no child components.

Proposed component definition can be like:

editor.DomComponents.addType('my-rich-text', {
  isComponent: el => {
    if (allChildText(el)) {  //check if all text node or tagname in textTags
      return { 
        type: 'my-rich-text',   // or can do 'text' here? not sure.
        editable: true,
        content: el.innerHtml,
        components: false,
        stopProcessChildren: true,      //new property
      };
    }
  },
  // ...
});

Concerns: XXS

pety-dc commented 1 year ago

Last time I said that based on available web.archive versions previous versions of 0.16.3 - 0.19.5 worked properly The funny thing is that I tried downgrading grapesJS to any of those versions and I didn't experience any change in my implementation.

Then I began rolling back the grapesjs-mjml plugin version until 0.5.8 when I finally found a version that somehow works as I expect. (doesn't dive into text components) I don't quite know the reasons since judging by my inexperienced eyes grapesjs-mjml did not change much between 0.5.8 and 0.6.0. Yet still that's the case.

@raymondmakz Thanks for your proposal.

A stopProcessChildren model property that would prevent a componenents contents from further parsing may work. I would imagine that this feature should not only work for custom text components, but CKEditor plugin should naturally override that base Text component to prevent grapesJS from taking control over html elements CKEditor uses for formatting.

artf commented 1 year ago

I'll try to find a way to fix the issue with custom RTE and the remove of styles, but for sure we have to avoid allowing XSS, so I guess the parse of content should not be skipped. Probably it would also be nice to customize what component is selectable inside the text component. Eg. a new option:

// disable select/hover on all inner components except the `link` one
disableInnerTextComponent: innerComponent => !innerComponent.is('link')

I'm open to hear better alternatives

pety-dc commented 1 year ago

Hey @artf

Thanks for your reply.

I'm not an expert on XSS vulnerabilities so could you please explain the nagure of the XSS vulnerability not parsing text contents would introduce? Would it make the GrapesJS editor vulnerable for "trojan horse" third party scripts embeded in texts or the final location of the edited content?

I'm not arguing, I just wish to better understand your concerns.

Maybe it would be enough to prevent XSS if the content of a text components doesn't get parsed for nested components but Githubissues.

Githubissues is a development platform for aggregating issues.

GrapesJS / grapesjs