search

TokamakUI / Tokamak

SwiftUI-compatible framework for building browser apps with WebAssembly and native apps for other platforms
Apache License 2.0
2.62k stars 111 forks source link

Add support for custom fonts #421

Closed carson-katri closed 3 years ago

carson-katri commented 3 years ago

Adds a .custom member to Font, which will pass the value to font-family in the DOM renderer.

ezraberch commented 3 years ago
  1. Certain characters in the font name, such as ", can break the HTML. It's probably best to sanitize the font names. Not doing so could potentially cause security issues as well.
  2. From https://developer.mozilla.org/en-US/docs/Web/CSS/font-family:

    Font family names containing whitespace should be quoted.

You should always include at least one generic family name in a font-family list, since there's no guarantee that any given font is available. This lets the browser select an acceptable fallback font when necessary.

carson-katri commented 3 years ago

I added a Sanitizer protocol to TokamakStaticHTML, and created a sanitizer for CSS strings. Please let me know if I overlooked anything in the sanitizer implementation.

ezraberch commented 3 years ago

Generic family names are keywords and must not be quoted.

Do we want to allow custom selection of the fallback, or multiple fonts in general? There are several ways this could be done (such as allowing the font name string to contain multiple fonts, allowing an array of font names to be used, or allow chaining of .font() to add fonts rather than replacing).

carson-katri commented 3 years ago

I liked the idea of chaining font modifiers, because it's compatible with SwiftUI.

Now a .font modifier on any View/Text will add to a stack of fonts, which will then be translated into CSS.

Renderers can still choose to ignore the _fontStack environment value and use font instead

Text("Hello, world!")
.font(.custom("Marker Felt", size: 17))
.font(.system(.body, design: .serif))

The above snippet will use Marker Felt if available in the UA, or the default serif stack if not:

font-family: 'Marker Felt', Cambria, 'Hoefler Text', Utopia, ...;

I also improved the sanitizer to sanitize identifiers and strings separately.

ezraberch commented 3 years ago
  1. Identifier validation is still not properly handling when the first character is a number. Try adding this test: XCTAssertFalse(Sanitizers.CSS.validate(identifier: "1hey-there"))
  2. String sanitize truncates rather than removing the offending characters. For example Sanitizers.CSS.sanitize(string: "'hey'there'") -> "\'hey\'"
  3. Is identifier-sanitize still necessary? It only happens if identifier-validate has already succeeded, so I don't think it does anything.
carson-katri commented 3 years ago

Resolved 3. Do you have suggestions on how to fix the other 2? I can't figure out why its matching numbers in the identifier parsing.

ezraberch commented 3 years ago

  1. The reason why it's matching some numbers is because it's matching this part: /// '[\240-\377]' static let nonAscii: RegularExpression = #"[\240-\377]"# The digits which match are 0,1,2,3,4, and 7, so it looks like it's ignoring the backslashes and interpreting that as "2, 4, 0-3, 7, or 7". In other words, swift is not recognizing that syntax as a way to denote a range of characters.

  2. What your filter is doing is finding all matches to the regex and merging them together, and ignoring the rest. You've defined a match as including start and end quotes. So 'ab'ed' has one valid match ('ab') so that's what you get. On the other hand, 'ab'c'de' has two valid matches ('ab' and 'de'), so you get 'ab''de'. Anything not part of a match is lost. If you remove the requirement that each match includes the start and end quotes, what you're doing should work.

carson-katri commented 3 years ago

Thank you for tracking those issues down! Turns out NSRegularExpression uses \0xxx to specify an Octal character, so that fixed it.

MaxDesiatov commented 3 years ago

@ezraberch would you have a moment to have another quick look? I personally don't have any objections for this code to be merged, but I appreciate very detailed reviews from you!

carson-katri commented 3 years ago

Yeah it might be unnecessary. I just thought the functionality may be useful in the future when we are sanitizing more areas.

MaxDesiatov commented 3 years ago

@ezraberch thanks again for your feedback on this and other PRs and your previous contributions! Would you be interested in joining our maintainers team? This would allow you to give review approvals for example.

ezraberch commented 3 years ago

Sure.