[BUG ]Mj-image and other components not closing properly

navewi commented 4 years ago

@artf I made a video to reproduce this, maybe it helps.

Originally posted by @navewi in https://github.com/artf/grapesjs-mjml/issues/139#issuecomment-543575604

I made a new thread for this because I think this is one of the biggest problems the editor has. Everytime i safe and/or reload all content after an image in the same column is not visible and I have to fix it manually. I think the screenplay says it all, but feel free to ask for more information. @artf

DRoet commented 4 years ago

just going to copy my response from the other issue:

it seems as if the void option is being ignored by the renderer:

<mj-spacer/>
<mj-spacer/>
<mj-spacer/>

turns into:

<td id="spacer">
    <td id="spacer">
        <td id="spacer">
        </td>
     </td>
</td>

instead of

<td id="spacer">
</td>
<td id="spacer">
</td>
<td id="spacer">
</td>

achirous commented 4 years ago

You can edit the component type you are having issues with by doing the following:

editor.DomComponents.addType('mj-image', {
        model: {
            defaults: {
                void: false
            }
        }
    });

This solved the issue for me.

DRoet commented 4 years ago

@achirous that doesn't seem to solve the issue

navewi commented 4 years ago

@achirous Hi! Yes that helped, thank you.

But then I still wonder why theres an option to set this to true and screw everything up 😂

DRoet commented 4 years ago

that option turns <mj-image/> into <mj-image></mj-image> when exporting, however <mj-image/> is still valid mjml syntax and should be supported when loading an existing template.

navewi commented 4 years ago

@DRoet Okay but then this is still a bug, because with the void option set to false and <mj-image/> the content after the image in the same section is still missing after import. I don't think this is how it's supposed to work.

achirous commented 4 years ago

@DRoet Oh yeah I agree that it <mj-image/> should be supported by the renderer. This is just a quick workaround until the issue gets fixed.

marklumancas commented 4 years ago

@navewi I modified the toHTML of mj-image model to fix the issue encountered when importing mj-image that are self enclosed. When it is set to void === false, somehow the closing tag of mj-image is added right before its parent's closing tag.

Hope it helps.

editor.DomComponents.addType('mj-image', {
        model: {
            defaults: {
                void: false
            },
            toHTML() {
                let code = ''
                let model = this
                let tag = model.get('tagName')
                let sTag = model.get('void')

                // Build the string of attributes
                let strAttr = ''
                let attr = this.getAttrToHTML()
                for (let prop in attr) {
                let val = attr[prop]
                strAttr +=
                  typeof val !== 'undefined' && val !== ''
                    ? ' ' + prop + '="' + val + '"'
                    : ''
              }

              code += `<${tag}${strAttr}${sTag ? '/' : ''}>` + model.get('content')

              if (!sTag) code += `</${tag}>`

              // Add the components after the closing tag.
              //
              // This will also fix an issue caused by changing void property value
              // where an imported template has an mj-image that is not enclosed by a closing tag,
              // the editor adds the mj-image closing tag right before its parent's closing tag.
              // Ultimately making all its sibling as its children.
              model.get('components').each(model => {
                code += model.toHTML()
              })

              return code
            }
        }
    });

emilsedgh commented 4 years ago

The reason this is happening is this:

Grapes, in order to parse HTML into a DOM tree, creates a new div element and does this:

div.innerHTML = <html code>

And the browser will take care of creating a DOM tree from the html code.

Browsers seem to completely understand self closing tags on predefined html elements where the element is self-closing per spec. Like the <br /> or <img /> tags.

However, when browsers are supposed to parse an html tag which is unknown to them, they won't parse self-closing ones properly.

So basically, because <mj-font> is not defined as self-closing, browser doesn't treat it as such.

I think the only solution to this is to use DOMParser API to parse the elements.

I thought another noble approach would've been to register mjml tags as custom elements (using browser.customElements) but they don't support self-closing/void elements.

artf commented 4 years ago

I think the only solution to this is to use DOMParser API to parse the elements.

I think DOMParser API (as it follows the HTML5 specs) has the same issue. But, in any case, we'd need to implement the possibility to pass custom HTML parsers to GrapesJS

DRoet commented 4 years ago

Since there is not an easy fix at the moment, maybe we should avoid using self-closing tags for now when dragging in new components? mjml doesn't seem to mind it when additional closing tags are provided.

pcorpet commented 3 years ago

@artf, without using a custom HTML parser, maybe we could update the one in grapejs: in parseNode https://github.com/artf/grapesjs/blob/dev/src/parser/model/ParserHtml.js#L261 we already detect the component type. If we could access the defaults.void property, we could decide to take the inner components and push them as other results (i.e. sibling components).

        if (componentType && componentType.get('void') && comps) {
          model.components = []
          result.push(model)
          comps.forEach(function(comp) {
            result.push(comp)
          })
          continue;
        }

        result.push(model);

This is not working because I cannot access the void property here, but I think it would do what we want. Could you help me understand how to reach the void property from this code? Or do you believe it shouldn't work like this?

alex621 commented 3 years ago

Just in case you need a temp fix, while none of the above solutions work for you, you may try the following. What it does is, search for existing mj-image and set "void" to false. Then for any newly added mj-image, set "void" to false.

/*
To fix the problem that mj-image is rendered as <mj-image /> instead of <mj-image></mj-image>,
which will make the next sibling be treated as its child instead of sibling
*/
let root = editor.DomComponents.getWrapper();
let searchAndUpdate = function (component, layer = 0){
    if (component.get("tagName") === 'mj-image'){
        component.set('void', false);
    }

    let children = component.get('components');
    if (children){
        children.each(child => searchAndUpdate(child, layer + 1));
    }
};
searchAndUpdate(root);
editor.on("component:add", function (model){
    model.set('void', false);
});

ThomasPof commented 3 years ago

Supposed to be fixed in https://github.com/artf/grapesjs-mjml/pull/240

marcus-at-localhost commented 3 years ago

I finally found out that I can set global formatting option in <mj-head> but unfortunately the self-closing tags are getting nested and throws this approach to set global styles out of the window.

This:

<mjml>
    <mj-head>
        <!-- https://documentation.mjml.io/#mj-attributes -->
        <mj-attributes>
            <mj-all font-size="16px" />
            <mj-all font-family="Arial" />
            <mj-all line-height="1.8" />
            <mj-text font-size="16px" font-family="Helvetica,Arial,sans-serif" line-height="1.8" />
        </mj-attributes>
    </mj-head>
</mjml>

becomes this:

<mjml>
  <mj-head>
    <!-- https://documentation.mjml.io/#mj-attributes -->
    <mj-attributes>
      <mj-all font-size="16px">
        <mj-all font-family="Arial">
          <mj-all line-height="1.8">
            <mj-text font-size="16px" font-family="Helvetica,Arial,sans-serif" line-height="1.8">
            </mj-text>
          </mj-all>
        </mj-all>
      </mj-all>
    </mj-attributes>
  </mj-head>

and so it doesn't work anymore.

Edit: I think I got around by doing this:

            editor.on('update', () => {
                console.log('update');
                editor.DomComponents.getWrapper().find('mj-head mj-attributes *').forEach((item) => {
                    item.set('void',true);
                })
            });

not sure if that's the best way, but good to know I can find nodes with find() :)

artf commented 1 month ago

Closing this as there is useXmlParser option that solves the import of custom void elements.

GrapesJS / mjml

[BUG ]Mj-image and other components not closing properly #149