tangrams / tangram

WebGL map rendering engine for creative cartography
https://tangram.city
MIT License
2.21k stars 290 forks source link

Support variable font weight ranges + JS function weights #757

Closed bcamper closed 4 years ago

bcamper commented 4 years ago

Variable fonts allow multiple variants of a font to be stored in a single file.

Supporting these has several benefits:

In CSS, the syntax for variable font weight looks like: font-weight: 125 950;. Tangram font syntax is similarly extended to support this as weight: 125 950 for a given font within the fonts block.

Note that while the variable font spec supports variation for other properties including font stretch and even custom, per-typeface-defined properties, Tangram relies on Canvas text rendering for labels, which unfortunately does not support these properties (font stretch cannot be set even for non-variable fonts).

This PR also enables JS function-based font weight, e.g. an example mapping font weight to building heights:

weight: |
  function() {
    return (feature.height||0) * 2 + 400;
}

tangram-1591501269395

bcamper commented 4 years ago

cc @bdon @meetar @matteblair @tallytalwar @nvkelso

bdon commented 4 years ago

I'm trying to think of a concrete use case for why I would specify a desired range of my variable font other than the entire valid range (1-1000) - it's not validated against when setting explicit weights in font draw styles.

One case might be where a user wants to swap out a font in the YAML without changing anything else, but also wants to clamp the allowed weights to an acceptably legible range based on the design of that specific font...

LGTM

bcamper commented 4 years ago

@bdon that is a good question, and I was about to say maybe we should just drop it entirely and assume the full range. However... I would need to check again, but I think that the font may not report as loaded at all unless a valid weight is provided, e.g. in this syntax, if the first provided value in the range exists in the font. I'm not 100% sure, and the behavior may vary across browsers as well. If we want to add support for this in Tangram ES, we also may need explicit values as it will be using a different font mechanism.

We need to care about the font being loaded and with correct settings because if we render too soon then the first tile may render with the wrong font, but FontFaceObserver also has a 3 second timeout for font loading failures, so it is not a "fast failure" if the provided values are wrong, and instead causes a long delay for map load/display.

bdon commented 4 years ago

I did some testing across browsers (safari, firefox, chrome) with the 1-1000 range specified. It works fine so far, but I did discover that setting style: italic may not work for variable fonts in Chrome.

Chromium ticket: Issue 1064756: font-style: italic doesn't activate the ital axis of variable fonts Font: Inter

I don't think this a blocker for merging this though.

bcamper commented 4 years ago

@bdon that's interesting to know about the italic axis in Chrome. I didn't explicitly point out italic, but it's covered by this comment from the description:

Note that while the variable font spec supports variation for other properties including font stretch and even custom, per-typeface-defined properties, Tangram relies on Canvas text rendering for labels, which unfortunately does not support these properties (font stretch cannot be set even for non-variable fonts).

I discovered this while trying to support additional axes beyond weight, but as far as I can tell, the short-hand CSS syntax -- which is what Canvas uses for text rendering, exposed as the font property on the canvas context -- doesn't support using numbers for these properties, it appears to confuse the parsing. I thought I found something explicitly stating this somewhere in one of the specs, but I'm not recalling where at the moment...

bdon commented 4 years ago

I did some more digging into the italics issue, and it's only Chrome, when using the "100 900" weight specification, that italics stop working. It works fine on Safari and Firefox; it also works fine on Chrome when using the multiple single-weight definitions workaround e.g.

        - weight: 100
          url: https://example.com/fonts/woff2/Inter.var.woff2
        - weight: 200
          url: https://example.com/fonts/woff2/Inter.var.woff2
bdon commented 4 years ago

A workaround I discovered: If you only ever use style:italic with font weight 200, you can have this set of fonts:

        - weight: 100 900
          url: https://example.com/fonts/woff2/Inter.var.woff2
        - weight: 200
          url: https://example.com/fonts/woff2/Inter.var.woff2

And it will work properly in chrome. So any weight that might be italic needs to be explicitly loaded by FontFace.

bcamper commented 4 years ago

@bdon ahhh OK thank you for the further info, I had misunderstood, I only thought that variable italics didn't work, not that all italics on variable fonts are broken (in Chrome)?? Do you still think this makes sense to merge and perhaps just document the Chrome bug?

I guess the alternative would be to have Tangram be smart enough to register additional fonts w/FontFace on the fly... right now it relies on parsing all the font definitions once upfront. Some refactoring would be needed to change that, though it would be generally more flexible. Maybe we just create an issue for that work, and proceed with merging this.

bdon commented 4 years ago

I think it does make sense to merge, because I don't see anything wrong with the implementation as-is. (BTW, I tried changing it so that it observes for the '900' weight load event instead of the '100' in case this was some implementation detail about the order in which italics are loaded, but that didn't fix anything)

The chrome issue has the workaround I described above, so maybe best to document that.

bcamper commented 4 years ago

Thanks. Going to merge this one then and give some thought to adapting font loading to address this, which would bring other performance benefits (deferred loading).

bcamper commented 4 years ago

Note, fixed a regression in 28afd161100344cdb8418664174495b9be142f72