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

Style attributes randomly reorder causing inconsistent styles #154

Closed
zachzurn opened this issue May 14, 2021 · 6 comments
Closed

Style attributes randomly reorder causing inconsistent styles #154

zachzurn opened this issue May 14, 2021 · 6 comments
Assignees
Labels
bug Something isn't working

Comments

@zachzurn
Copy link

zachzurn commented May 14, 2021

Styles take priority based on where they are in the css list. These properties are randomly ordered which causes style shifts when rendering the same mjml.

For example this set of style rules would end up adding a padding-top of 50px:

padding: 0;
padding-top: 50px;

But with random ordering, this happens which makes the padding-top come out as 0:

padding-top: 50px;
padding: 0;

Styles should remain in the same order in which they were defined, while also honoring the apply order that is mentioned in the mjml docs:

Note that the apply order of attributes is: inline attributes, then classes, then default mj-attributes and then defaultMJMLDefinition

  • inline
  • classes
  • mj-attributes
  • default mjml styles
@jdrouet jdrouet self-assigned this May 15, 2021
@jdrouet jdrouet added the bug Something isn't working label May 15, 2021
@jdrouet
Copy link
Owner

jdrouet commented May 15, 2021

This comes from the use of a HashMap to store the styles when building the html. I will fix that this weekend.

@jdrouet
Copy link
Owner

jdrouet commented May 15, 2021

@zachzurn could you try the version 1.3.1 of mrml-cli? It should to be fixed.
If not, could you provide me a failing template?

@zachzurn
Copy link
Author

I’ll give it a run later today.

@zachzurn
Copy link
Author

Ran an initial test and styles seem to be ordering properly with some minor exceptions. I'll be able to do some more testing next week with some real world templates I am working on.

Here are some notes when comparing the output between the two renderers. I am leaving out any whitespace formatting notes since they are irrelevant.

See attached zip here:
output-mjml.zip

  • Test input: styles-test.mjml
  • MJML Output: mjml.html
  • MRML Output: mrml.html

Notes:

Line 58 in the MRML output starts to differ. MRML seems to include the style tag even if it was specified as inline. There is also an erroneous second style tag with a media attribute.

MRML Output

<style type="text/css">
    @media only screen and (min-width:480px) {
      .mj-column-per-100 {
        width: 100% !important;
        max-width: 100%;
      }
    }
</style>
<style media="screen and (min-width:480px)">
    .moz-text-html .mj-column-per-100 {
      width: 100% !important;
      max-width: 100%;
    }
</style>
<style type="text/css">
    .blue {
      color: #0000ff;
      text-decoration: underline;
      line-height: 30px;
    }
</style>

MJML Output

<style type="text/css">
    @media only screen and (min-width:480px) {
      .mj-column-per-100 {
        width: 100% !important;
        max-width: 100%;
      }
    }
</style>
<style type="text/css">
</style>

MRML has a body style and a div with an empty style whereas MJML doesn't. I don't think the empty style tag breaks anything. but the word-spacing tag may have some unknown effects.

<body style="word-spacing:normal;">
    <div style>

vs MJML

<body>
    <div>

The style output looks the same with the exception of styles not being inlined. I don't remember if inlining is currently supported in MRML yet, so this might be working exactly as expected.

One thing that might create unexpected results is the font-size:0px being later on in the definition for MJML (which might even break their own spec).

MRML Output

<td align="right" class="blue" style="font-size:0px;padding:10px 25px;word-break:break-word;">
    <div style="font-family:helvetica;font-size:20px;line-height:1;text-align:right;text-decoration:none;color:#F45E43;">
        Hello World</div>
</td>

MJML Output

<td align="right" class="blue" style="color: #0000ff; text-decoration: underline; line-height: 30px; font-size: 0px; padding: 10px 25px; word-break: break-word;">
    <div style="font-family:helvetica;font-size:20px;line-height:1;text-align:right;text-decoration:none;color:#F45E43;">
        Hello World</div>
</td>

@jdrouet
Copy link
Owner

jdrouet commented May 16, 2021

MRML has a body style and a div with an empty style whereas MJML doesn't. I don't think the empty style tag breaks anything. but the word-spacing tag may have some unknown effects.

If you use mjml 4.9.3 (I made a Docker image here) and the latest version of mjml seems to return the body with word-spacing:normal; so this might be due to the online editor not being up to date (I guess you're using it).

There is also an erroneous second style tag with a media attribute.

Same thing, it seems valid according to the latest mjml version 🤔

The style output looks the same with the exception of styles not being inlined. I don't remember if inlining is currently supported in MRML yet, so this might be working exactly as expected.

Nope, not yet supported. Why? Because the inline style requires to parse the generated html to apply the inline style afterward which will kill the performances and, IMO, using inline style should be avoided (although the generate html is already full of it). But it's on the roadmap.

Thank you for this feedback anyway, if you find a bug, don't hesitate to add the template in the issue and I'll try to fix the bug as soon as I can 😉

@jdrouet
Copy link
Owner

jdrouet commented May 19, 2021

I close this issue for now, don't hesitate to re-open one if you find anything wrong.

@jdrouet jdrouet closed this as completed May 19, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants