Visit each HTML element and apply inline CSS

Question

I need to apply some css styles to an IMarkupElement. The generated HTML is only for emails so all styles are inline styles and the selectors are very simple, just element names, ids or class names can be used. I'm not sure whether a Visitor is the right tool to apply styles to elements but it seemed to fit for the task.

This is the IMarkupElment:

public interface IMarkupElement : ICollection<object>, IFormattable
{
    string Name { get; }
    IDictionary<string, string> Attributes { get; }
    IMarkupElement Parent { get; set; }
    int Depth { get; }
}

The StyleVisitor works recursively and finds all IMarkupElement nodes and redirects the style creations to the Element method. Styles are provided via a Dictionary<string, string> where the key is a single selector like element name, #id or .class. I have another module that parses a very simple css file into such a dictionary.

public class StyleVisitor
{
    private readonly IDictionary<string, string> _styles;

    public StyleVisitor([NotNull] IDictionary<string, string> styles)
    {
        _styles = styles ?? throw new ArgumentNullException(nameof(styles));
    }

    public IMarkupElement Visit([NotNull] IMarkupElement element)
    {
        if (element == null) throw new ArgumentNullException(nameof(element));

        var selectors = GetSelectors(element).Distinct(StringComparer.OrdinalIgnoreCase);
        var style = Element(selectors);

        if (string.IsNullOrEmpty(style))
        {
            element.Attributes.Remove("style");
        }
        else
        {
            element.Attributes["style"] = style;
        }

        foreach (var child in element)
        {
            if (child is IMarkupElement e)
            {
                Visit(e);
            }
        }

        return element;
    }

    // Gets element, id and class selectors.
    private static IEnumerable<string> GetSelectors(IMarkupElement element)
    {
        yield return element.Name;

        if (element.Attributes.TryGetValue("id", out var id))
        {
            yield return $"#{id}";
        }

        if (element.Attributes.TryGetValue("class", out var classes))
        {
            foreach (var className in Regex.Split(classes, @"\s+").Select(className => className.Trim()))
            {
                yield return $".{className}";
            }
        }
    }

    private string Element(IEnumerable<string> selectors)
    {
        var styles = new StringBuilder();
        foreach (var selector in selectors)
        {
            if (_styles.TryGetValue(selector, out var style))
            {
                // Fix the ";" but trim it first in case there is already one to avoid an "if".
                styles
                    .Append(style.Trim().TrimEnd(';'))
                    .Append(";");
            }
        }
        return styles.ToString();
    }
}

This is the test that passes that I wrote for it so far:

[TestClass]
public class StyleVisitorTest
{
    private static readonly IMarkupElement Html = MarkupElement.Builder;

    [TestMethod]
    public void Visit_WithStyles_Applied()
    {
        var html = Html
            .Element("p", p => p
                .Append("foo ")
                .Element("span", span => span
                    .Attribute("class", "qux")
                    .Append("bar"))
                .Append(" baz"));

        Assert.AreEqual(@"<p>foo <span class=""qux"">bar</span> baz</p>", html.ToHtml());

        var styleVisitor = new StyleVisitor(new Dictionary<string, string>(StringComparer.OrdinalIgnoreCase)
        {
            [".qux"] = "font-family: sans-serif;"
        });

        html = styleVisitor.Visit(html);

        Assert.AreEqual(@"<p>foo <span class=""qux"" style=""font-family: sans-serif;"">bar</span> baz</p>", html.ToHtml());
    }
}

Before I write more complex html and tests I wanted to ask you what do you think about the StyleVisitor?

---

(Just for reference: this is build on top of an improved version of the Functional Html builder)

Answer 1

A quick review:

1. You shouldn't name classes/methods after patterns (except when pattern name makes sense semantically, like factory). StyleVisitor.Visit does not tell me anything, about what this class does. But, say, CssManager.ApplyStyle does.
2. Its hard to tell what Element method does at first glance. I mean it is called Element (add a verb?) but it returns styles? Weird.
3. Visit_WithStyles_Applied contains what looks like two separate tests. One tests your parser, another tests StyleVisitor. Separate them?

P.S. There is also quite a lot of strings in your code. Selector is a string, attribute is a string, style is a string, etc. Indeed all of them are technically strings but they all have different domain meaning. Maybe you should consider adding more classes to your domain model to represent those entities. You will gain the safety of strong types, but you will have to write more boilerplate code. So there is a trade off.

Answer 2

I don't have any strong objections to your code and the other answer does a good job of addressing some slight naming issues. My problem is on a much larger scale - I think your domain model is far too simplistic.

Consider this CSS:

.i-should-be-applied-first {
    position: relative;
} 

#i-should-be-applied-second {
    position: static;
}

Now, look at your code: which rule will be in-lined first? Now, I haven't actually compiled your code, let alone run it, but I'm sure you'll apply the styles in the wrong order.

And another example:

.class-1 {
}

.class-2 {
}

With the html:

<p class="class-2 class-1"></p>

By modelling your stylesheet as a simple IDictionary<string, string> you lose the order of the rules. You have to know which rule came first to in-line the rules in the right order! That's why it's called a cascading style sheet.

It looks like you're only doing a simple thing so I think you can reasonably ignore specificity and media queries.

Build up the domain!