Rendering depends on thread culture

[mcliment]

Hi! It's me again!

If I run the tests on my machine (with "es-ES" locale), they won't work as the decimal separator in this locale is , instead of ., that is the invariant.

For example the first error for amario.mjml is:

 * Actual: 'vertical-align:middle;width:399,99999999999994px;'.
 * Should: 'vertical-align:middle;width:399.99999999999994px;'.

If I set the thread to use invariant culture the tests pass but I can imagine that it's not a good solution.

I tried to check where "${width}px" and similar stuff is done but putting {width.ToInvariantString()} everywhere is not very fool proof... may be the thread culture should be temporarily set to invariant when rendering and then restored.

I may add a PR if you want to.

[SebastianStehle]

Thanks for reporting this. We use a trick for fast performance. We leverage the interpolation handler of the string builder.

I always assumed that it uses the Invariant culture by default, but it seems it uses null (aka CurrentCulture). There is an overload which accepts the culture. I guess we have to use that. It is this line: https://github.com/SebastianStehle/mjml-net/blob/main/Mjml.Net/IHtmlStyleRenderer.cs#L38

There are a 3-4 lines like this.

PR is very welcome.

[SebastianStehle]

Have you seen my answer above. It should work if you just pass the CultureInfo.InvariantCulture to the interpolation handler. I am going to test that.

[LiamRiddell]

Have you seen my answer above. It should work if you just pass the CultureInfo.InvariantCulture to the interpolation handler. I am going to test that.

Apologies, I didn't see the comment above. Ignore my rambling :D

[mcliment]

Awesome @SebastianStehle!

[mcliment]

@SebastianStehle I see that it still happens in some cases, although most of them were fixed.

For example, for tempalte arturia.mjml, using es-ES culture shows some errors. This is the first:

1 The text in style(7) > #text(0) is different.
 * Actual: '
    @media only screen and (min-width:480px) {
      .mj-column-per-33-333333333333336 {
width:33,333333333333336% !important;
max-width: 33,333333333333336%;
}
      .mj-column-per-100 {
width:100% !important;
max-width: 100%;
}
      .mj-column-per-25 {
width:25% !important;
max-width: 25%;
}
    }
  '.
 * Should: '
    @media only screen and (min-width:480px) {
      .mj-column-per-33-333333333333336 {
        width: 33.333333333333336% !important;
        max-width: 33.333333333333336%;
      }

      .mj-column-per-100 {
        width: 100% !important;
        max-width: 100%;
      }

      .mj-column-per-25 {
        width: 25% !important;
        max-width: 25%;
      }
    }

  '.

It seems that not all paths go though the interpolator.

I can take a look tomorrow if you need some help. The easiest way I found to reproduce is setting the culture before rendering in the ComplexTests class.

[LiamRiddell]

Issue

I believe this is the offending line: https://github.com/SebastianStehle/mjml-net/blob/main/Mjml.Net/Components/Body/ColumnComponent.cs#L284.

private string GetColumnClass (GlobalContext context) {
    string className;

    // Correctly uses ToInvariantString();
    if (CurrentWidth.Unit == Unit.Percent) {
        className = $"mj-column-per-{CurrentWidth.Value.ToInvariantString().Replace('.', '-')}";
    } else {
        className = $"mj-column-px-{CurrentWidth.Value.ToInvariantString().Replace('.', '-')}";
    }

    // CurrentWidth.WidthString lacks the ToInvariantString() usage when set in Measure().
    context.SetGlobalData (className, MediaQuery.Width (className, CurrentWidth.WidthString));

    return className;
}

However, CurrentWidth is set by the Measure() function before the component is rendered: https://github.com/SebastianStehle/mjml-net/blob/main/Mjml.Net/Components/Body/ColumnComponent.cs#L91-L132

            if (Width != null)
            {
                (widthValue, widthUnit) = UnitParser.Parse(Width);

                widthString = Width;
            }
            else
            {
                widthValue = 100d / Math.Max(1, numNonRawSiblings);
                widthUnit = Unit.Percent;
                widthString = $"{widthValue}%";
            }

The above lacks usage of ToInvariantString() when using string interpolation.

Proposed Fix

Change the following line:

widthString = $"{widthValue}%";

to

widthString = $"{widthValue.ToInvariantString()}%";

We can also check other occurrences of Measure() to find the same issue. Quick search suggests the following components are affected:

• Column
• Group

Update ComplexTests

I believe we should update the ComplexTests to follow the same approach as the original multi-culture tests we implemented for MjmlRenderer:

[Theory]
[MemberData (nameof (Cultures))]
public void Should_render_interolated_content_with_invariant_culture (CultureInfo culture) {
    var currentCulture = CultureInfo.CurrentCulture;
    var currentUICulture = CultureInfo.CurrentUICulture;

    try {
        CultureInfo.CurrentCulture = culture;
        CultureInfo.CurrentUICulture = culture;

        sut.Content ($"<div class=\"{0.3333}px\">");

        AssertText (new [] {
            "<div class=\"0.3333px\">"
        });
    } finally {
        CultureInfo.CurrentCulture = currentCulture;
        CultureInfo.CurrentUICulture = currentUICulture;
    }
}

[SebastianStehle]

Yes, I can update that.

[mcliment]

Just ran the tests locally and works like a charm, nice! Thanks a lot @LiamRiddell @SebastianStehle 🎉 💯

[SebastianStehle]

Thanks for your feedback :)

[SebastianStehle]

What have you used before?

[mcliment]

What have you used before?

More or less what you use in the ComplexTests, launch an external node process to do the job and in case of failure, call the API, but nothing can compare to a native .NET library.

[SebastianStehle]

Our implementation is also 30times faster :)