ClassName can be invalid

[Skyfold]

Several of the ToClassName instances return non-valid class names by starting with a number:

instance ToClassName Int

instance ToColor HexColor where
  colorValue c = c
  colorName (HexColor a) = T.dropWhile (== '#') a

We can fix this by adding a "_" or escaping the number if the class name start with a number and writing the semigroup instance to ensure invalid class names are not created. It is also possible to remove unnecessary escaped characters with the semigroup instance. Valid class names are as follows:

In CSS, identifiers (including element names, classes, and IDs in selectors) can contain only the characters [a-zA-Z0-9] and ISO 10646 characters U+00A0 and higher, plus the hyphen (-) and the underscore (_); they cannot start with a digit, two hyphens, or a hyphen followed by a digit. Identifiers can also contain escaped characters and any ISO 10646 character as a numeric code (see next item). For instance, the identifier "B&W?" may be written as "B&W?" or "B\26 W\3F".

[seanhess]

ToClassName is a bit of a misnomer, it's really a segment of a classname. Maybe we coiuld have the cls helper clean it up?

[Skyfold]

Is the purpose of ToClassName to create the unique name of the attribute and cls to turn that ClassName into a valid Class? In other words ClassName is really the class name components and Class is where the class name is constructed.

In that case, yes, ToClassName does not need to return valid class names, I can put some escaping logic in cls. This is simpler to implement since the components of a class name do not need to be valid class names.

[seanhess]

Yeah, that's about right. A ClassName is a monoid of valid class name segments. They're really just designed to make generating unique classnames for different values easy. See Web.View.Style

minWidth :: Length -> Mod c
minWidth n =
  addClass $
    cls ("mw" -. n)
      & prop "min-width" n

It uses the ToClassName instance to get (-.) append segments. The final class name will be "mw-100" or something similar.

Maybe the right place to put escaping is in Web.View.Render.renderCSS?

Maybe it might make sense to rename a few things to clear this up? ClassName could be ClassNameSegment or something.

[Skyfold]

I think ClassName and ToClassName are correct since our goal is to create a class name. We can just add some comments explaining that the user does not have to create valid class names, since we will handle the escaping.

The next question I have is related to using Builder to replace the underlying representation. I'll explain how I think we can introduce blaze-builder in #14. The part I want to address here is that escaping the class name cannot happen once the Type has been converted to a Builder, since the only operations on a Builder are concatenation. We could store the class name as a Text, but then we are back to O(n) for concatenation. Two solutions:

1. Hash the values to create a unique class name. The class name Type would be an Int and we would use hashWithSalt to combine class names. Upside, you will always have a unique name and good performance. Downside, name tells you nothing about what is in the class.

2. Escape the ClassName while creating the Builder. In other words, ensure all created ClassName are valid class names such that concatenating them will also be valid. Upside, the class name contains all the values (with minor changes). Downside, this does complicate writing ToClassName instances and can get very long.

What do you think?

[Skyfold]

I realised I made an assumption about how we are using classes that is incorrect. We are not combining classes, just creating unique class names. In other words, we currently create a class once and never update the values of that class. Interestingly, I think the fix for #17 is changing how we handle classes.

To explain:

Currently we can have multiple classes for each element where those classes are shared among multiple elements. Each class can have multiple attributes and those attributes can clash #17.

The fix is either:

1. Combine all classes for an element into a single uniquely named class where later attributes take precedence. This solves Bug: undefined style precedence when set more than once #17, but does mean reusing classes becomes mostly impossible.

2. Require that all classes can only have one attribute, then delete older classes when attributes clash. This also solves Bug: undefined style precedence when set more than once #17, but means flexRow would create two uniquely named classes, "display-flex" and "flex-direction-row". We would have class reuse, but there would be a lot of classes.

[Skyfold]

I spent some more time looking at the blaze-markup source and realised I have a misconception on how to use Builder effectively. You should store your values as Text, String, etc. For example, blaze-markup uses this:

data ChoiceString
    -- | Static data
    = Static {-# UNPACK #-} !StaticString
    -- | A Haskell String
    | String String
    -- | A Text value
    | Text Text
    -- | An encoded bytestring
    | ByteString B.ByteString
    -- | A pre-escaped string
    | PreEscaped ChoiceString
    -- | External data in style/script tags, should be checked for validity
    | External ChoiceString
    -- | Concatenation
    | AppendChoiceString ChoiceString ChoiceString
    -- | Empty string
    | EmptyChoiceString

instance Semigroup ChoiceString where
    (<>) = AppendChoiceString
    {-# INLINE (<>) #-}

instance IsString ChoiceString where
    fromString = String
    {-# INLINE fromString #-}

The key is their Semigroup instance, instead of doing the concatenation when (<>) is applied, you store the values as is. Later on in the render function they convert everything to a Builder before combining:

fromChoiceString :: ChoiceString  -- ^ String to render
                 -> Builder       -- ^ Resulting builder
fromChoiceString (Static s)     = B.copyByteString $ getUtf8ByteString s
fromChoiceString (String s)     = B.fromHtmlEscapedString s
fromChoiceString (Text s)       = B.fromHtmlEscapedText s
fromChoiceString (ByteString s) = B.fromByteString s
fromChoiceString (PreEscaped x) = case x of
    String s -> B.fromString s
    Text   s -> B.fromText s
    s        -> fromChoiceString s
fromChoiceString (External x) = case x of
    -- Check that the sequence "</" is *not* in the external data.
    String s     -> if "</" `isInfixOf` s then mempty else B.fromString s
    Text   s     -> if "</" `T.isInfixOf` s then mempty else B.fromText s
    ByteString s -> if "</" `S.isInfixOf` s then mempty else B.fromByteString s
    s            -> fromChoiceString s
fromChoiceString (AppendChoiceString x y) =
    fromChoiceString x `mappend` fromChoiceString y
fromChoiceString EmptyChoiceString = mempty
{-# INLINE fromChoiceString #-}

When you look at the definition of ByteString's Builder (which blaze-builder is based on), this makes sense:

newtype Builder = Builder (forall r. BuildStep r -> BuildStep r)

It is a function to combine Builder, not a storage medium. You can ignore my comment about needing to change ClassName be defined as a Builder. Instead we want to have a data structure like ChoiceString, which would make editing the class names after the fact, easy.