mjmlio / mjml

MJML: the only framework that makes responsive-email easy
https://mjml.io
MIT License
17.08k stars 960 forks source link

inline styles are parsed right-to-left #1991

Closed willhertz closed 4 years ago

willhertz commented 4 years ago

Describe the bug

When using in-line styles over mj elements. If there are 2 conflicting styles the one to the left takes precedence.

To Reproduce

using the standard boiler plate in Live Editor <mj-text font-size="20px" font-size="40px" font-size="60" color="#F45E43" font-family="helvetica">Hello World</mj-text> The text will be 20px. If the font-size=40 and 60 styles are removed the template won't be rendered. If the font-size=20 is removed, then the text will be 40px, and if this one is also removed then the text will finally become 60px

Expected behavior The expected behavior in CSS and in-line styles is that the last one to be mentioned is the one that overrides all the previously mentioned styles.

MJML environment (please complete the following information):

Email sending environment(for rendering issues): N/A

Affected email clients (for rendering issues): N/A Screenshots N/A

Additional context Found out while porting mjml/email-templates. Then tested with more simpler code and got the same behaviour. Is this a bug? If not, is it documented? Fixing it will definitely break lots of templates that might be based on the mjml templates website because many of them have a lot of redundant inline styles (Christmas is a good example, and there's a big change many users left them as-is and only changed content)

iRyusa commented 4 years ago

Christmas is a good example, and there's a big change many users left them as-is and only changed content)

If you're talking about https://mjml.io/try-it-live/templates/christmas then I don't see a single node with a double font-size declaration ?

This is not really a bug or a feature, it's more a limitation from htmlparser2 where redundant attributes are simply ignored.

I believe most browser behave the same as <input type="text" type="password" /> will be interpreted as a text input not a password in Chrome.

I don't really see why this would need documentation as there's no proper usage for this.

willhertz commented 4 years ago

Yes I'm talking about https://mjml.io/try-it-live/templates/christmas

Sorry that I didn't explain well. I used the font-sizing as an example because of simplicity. Not because font-sizing is the reall issue (at least in the aforementioned template)

<input type="text" type="password" />

Yes definitely is the way that elements work. are some kind of set once and it's done.

But styles are meant to be cascaded and no, should not be used like that in real world. There's no need for it, but this template is cluttered with lines that go like this (I'm trying to port it, among others to non inline styles and V4 syntax, that's how found he issue)

<mj-section background-color="#ffffff" background-repeat="repeat" padding-bottom="0px" padding-top="30px" padding="20px 0" text-align="center" vertical-align="top">

in html the padding-top set to 20px 0 will override padding-top 30, making padding-top=20. but mjml makes the first one the most-significant and that is counterintuitive.

I don't really see why this would need documentation as there's no proper usage for this.

Definitely agree that there's no proper usage for this. But:

If a template provided by mailjet (at least "Black Friday" and "Christmas" suffer from this) , then someone may think that is the right way to do things, definitely must be documented. or at least warned against it. Because is not the expected behavior for cascaded styles

IMHO mjml has the potential to become the actual industry standard for email templating and as such the limitations of the underlying tehcnologies should be acknowledged and documented for at least:

Of published templates

Specially since is an unexpected behavior (for an unexpected, undesired but very possible and actual missuse of the features). in a vendor(mailjet/mjml) provided example. That might save a lot of time debugging and promote best practices among users.

iRyusa commented 4 years ago

But styles are meant to be cascaded and no, should not be used like that in real world. There's no need for it, but this template is cluttered with lines that go like this (I'm trying to port it, among others to non inline styles and V4 syntax, that's how found he issue)

<mj-section background-color="#ffffff" background-repeat="repeat" padding-bottom="0px" padding-top="30px" padding="20px 0" text-align="center" vertical-align="top">

in html the padding-top set to 20px 0 will override padding-top 30, making padding-top=20. but mjml makes the first one the most-significant and that is counterintuitive.

You're totally wrong about this, padding-top will take over padding. You can just see the simplified exemple here : https://mjml.io/try-it-live/Hk1JicgZw

Again that's not an issue padding+direction and padding are differents attributes, padding + direction will take over padding at any time (regarding of the placement on mj-tag).

I think you totally misunderstand here how padding & padding+direction works that's all. ( padding-bottom="0px" padding-top="30px" padding="20px 0" there's no direction on the last one so that's a different attribute)

edit: Just to clarify, mj-attributes order doesn't do anything at all. If you defined padding top after or before padding, the output will be the same. The reason is fairly simple, you mostly use padding+direction to override a single value from the shortcut version. Mailjet template aren't sanitize they're taken directly from Mailjet template gallery where their API store extra default attributes to ensure proper migration over the time. It's more like a showcase rather than example here.

GarryFlemings commented 4 years ago

@willhertz : Maybe I can drop an idea that'll help. Nothing here contradicts @iRyusa.

I note that we all agree that cascading and overriding is working properly in the examples you and iRyusa discussed involving two attribute lists. For example, in "Christmas", padding is specified with different values, one in mj-section and one in an mj-image within it. We all agree the mj-image value should apply to mj-image. Yay! Common ground.

I understand your question to apply to multiple attributes within a single list. From your first post:

To Reproduce

using the standard boiler plate in Live Editor

Hello World

The text will be 20px. If the font-size=40 and 60 styles are removed the template won't be rendered. If the font-size=20 is removed, then the text will be 40px, and if this one is also removed then the text will finally become 60px

I put that code in Try-it-Live. We all agree the rendered text is 20px. Yay! Common ground.

More common ground: We all agree that using the same attribute twice in an attribute list is improper. Yay! (I don't find that point in any document, but I'm no expert on all the documents, either.)

Maybe you feel that since padding in two attribute lists demonstrates override behavior, a preferred way to deal with the improper syntax is to use the last-listed attribute above (font-size=60).

Perhaps we can all agree that (1) the "preferred way" above might be an available choice to browser developers, and (2) that's not the choice demonstrated by any of the browsers witnessed here.

I kinda take it (and it's possible it says it this way in the browser requirement document that W3 makes publicly available) that the browser simply ignores repeat attributes. In the example above, the 20px comes along; it's not improper and the browser accepts it. Then the 40px and then the 60; both are improper, so it ignores each.

Does the browser ignore other improper syntax? It ignores misspelled attributes.

The "right [or last] has precedence" idea requires a new and unannounced rule. I find "ignoring misspelled and repeated attributes" is more comfortable. (Full disclosure: I haven't found that in any document, either.)

Maybe that comfort is just me. All the best. 🍀