1290237 - stylo: @font-face support

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect, P1)

Description

[Bobby Holley (:bholley)]

Support for downloadable fonts is another pieces of medium-large property integration work for stylo.

Comment 1

[Bobby Holley (:bholley)]

Chris, can you track this and bug 1290228?

Comment 2

[Chris Peterson [:cpeterson]]

OK

Comment 3

[Xidorn Quan [:xidorn] UTC+11]

Integration of @font-face may not be very complicated. It seems to me except parsing, other part of style system in Gecko basically depends on the CSSOM interface of font-face rule. That says, as far as we add a CSSFontFaceRule impl backed by Servo and returns the requested nsCSSValue like what current Gecko impl does, then everything should just work.

A challenge is that in Rust we generally don't preserve strings, so we may need to re-serialize values of descriptors to feed Gecko. But I guess that may not be a big issue as I don't think that would usually be on a hot path.

It seems the most complicated descriptor font-variant is not supported in Gecko at the moment, fortunately :)

Comment 4

[Bobby Holley (:bholley)]

(In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #3)
> Integration of @font-face may not be very complicated. It seems to me except
> parsing, other part of style system in Gecko basically depends on the CSSOM
> interface of font-face rule. That says, as far as we add a CSSFontFaceRule
> impl backed by Servo and returns the requested nsCSSValue like what current
> Gecko impl does, then everything should just work.

That's great news!

> A challenge is that in Rust we generally don't preserve strings, so we may
> need to re-serialize values of descriptors to feed Gecko. But I guess that
> may not be a big issue as I don't think that would usually be on a hot path.

Yeah sounds good.
 
> It seems the most complicated descriptor font-variant is not supported in
> Gecko at the moment, fortunately :)

:-)

So I guess the next step here is to implement the CSSOM bits - do we have a bug on file that we can block this on?

Comment 5

[Xidorn Quan [:xidorn] UTC+11]

Also, I think this bug can probably have a higher priority, because I believe quite a number of reftests depend on @font-face due to wide usage of testing fonts, especially Ahem.

Probably Simon can help implementing the parser part of @font-face in Servo?

Comment 6

[Simon Sapin (:SimonSapin)]

Servo already supports @font-face, though currently only with the 'src' and 'font-family' descriptors. I can work on adding parsing and serialization for other descriptors. Although since there is no specified/computed value split like for properties, maybe we want to parse directly to FFI types (nsCSSValue?) rather than Rust/Servo types?

Comment 7

[Simon Sapin (:SimonSapin)]

Note to myself: http://searchfox.org/mozilla-central/rev/225ab0637ed51b8b3f9f4ee2f9c339a37a65b626/layout/style/nsCSSParser.cpp#12188

Comment 8

[Xidorn Quan [:xidorn] UTC+11]

(In reply to Simon Sapin (:SimonSapin) from comment #6)
> Servo already supports @font-face, though currently only with the 'src' and
> 'font-family' descriptors. I can work on adding parsing and serialization
> for other descriptors. Although since there is no specified/computed value
> split like for properties, maybe we want to parse directly to FFI types
> (nsCSSValue?) rather than Rust/Servo types?

If that is not too hard for Servo side, I think it would be very helpful.

It seems to me the current binding functions is not enough for that. It seems we need the additional functions to set string value, normal value, pair list value, url value, and local font value to nsCSSValue.

Ah, this makes me a bit hesitant. Probably we should make bindgen generate methods for us rather than adding so many binding functions :/ (But method generating requires build-time bindgen to be enabled first, because otherwise MSVC has different mangled name for methods.)

Comment 9

[Simon Sapin (:SimonSapin)]

https://github.com/servo/servo/pull/15356 add support for more descriptors inside @font-face. I’ll add conversions to nsCSSValue separately since that requires new C++ functions.

Compared to Gecko’s CSSParserImpl::ParseFontDescriptorValue:

* eCSSFontDesc_FontLanguageOverride and eCSSFontDesc_Display are in Gecko but not in the spec. I don’t know what they are, but if they’re needed in stylo I can reverse-engineer Gecko to find out, and implement equivalent parsing and serialization.

* font-variant is supported inside @font-face in the spec but not in Gecko. Servo currently only supports "normal | small-caps" out of the many values specified in CSS Fonts 3. Also in level 3 the font-variant property became a shorthand, so for @font-face we’ll likely want a Rust type in font_face.rs that holds values of all of the corresponding longhands.

* The font-feature-settings property already has parsing and serialization code in Servo, but with products="none". Is it because the bindings are not there yet? Once that’s done, adding support for it in @font-face should be a matter of a couple new lines in a macro.


[1] http://searchfox.org/mozilla-central/rev/d20e4431d0cf40311afa797868bc5c58c54790a2/layout/style/nsCSSParser.cpp#12186

Comment 10

[Cameron McCormack (:heycam)]

I'm not sure if it has made it into an official spec yet, but font-display is defined here: https://tabatkins.github.io/specs/css-font-display/#font-display-desc

font-language-override is defined here, but is marked as at risk: https://drafts.csswg.org/css-fonts-3/#propdef-font-language-override

Comment 11

[Simon Sapin (:SimonSapin)]

(In reply to Cameron McCormack (:heycam) from comment #10)
> font-language-override is defined here, but is marked as at risk:
> https://drafts.csswg.org/css-fonts-3/#propdef-font-language-override

It’s a property (that applies to elements) though.

Jonathan, looks like you added support for font-language-override as an @font-face descriptor in bug 511339. How does it interact with the property? Could you write to CSSWG about specifying that?

Comment 12

[Jonathan Kew [:jfkthame]]

Huh, I thought it was specified alongside font-feature-settings, but apparently it's not there.

It was always intended to parallel font-feature-settings, which exists as both a property on elements and a descriptor in @font-face rules, with the descriptor providing a default that the property can override.

I'll raise it with the WG...

Comment 13

[Bobby Holley (:bholley)]

Shing mentioned that fonts were a major reason why a lot of sites don't work.

Simon, how much work is left on the servo side? Are we at the point where we just need to do the work in comment 3?

Comment 14

[Simon Sapin (:SimonSapin)]

As discussed over video I added parsing and serialization code for @font-face descriptors, except a couple per Comment 9. Nazım added some nsCSSValue constructors in Bug 1340728. What’s left is converting the Rust types for descriptor values to nsCSSValue (maybe implementation of the`std::ops::From` trait?), glue to call this from Gecko, and integration with Gecko’s font subsystem.

Comment 15

[Bobby Holley (:bholley)]

Jonathan said he'd have a look here - thanks Jonathan!

Comment 16

[Shing Lyu [:shinglyu]]

Attachment: Related reftest failures

Many reftests are failing because they 1) tests font-loading itself 2) use custom font or Ahem font. Here is a list for them.

Comment 17

[Xidorn Quan [:xidorn] UTC+11]

I think this would be done with my work in bug 1345696.

Comment 18

[Bobby Holley (:bholley)]

Xidorn, what's left to do here?

Comment 19

[Xidorn Quan [:xidorn] UTC+11]

I think majority of work has been done in bug 1345696. Many (read: most) mochitests related to @font-face still don't work because of servo/servo#16165 which I guess SimonSapin would take care of. There are three descriptors (font-feature-settings, font-language-override, and font-display) not implemented for stylo, which is also mostly just Servo-side work.

We probably want to check reftest failures which are supposed to be fixed and see why they're still failing.

There are still a few reftests (some of them are skipped) marked "# bug 1290237" which I failed to re-enable (mainly in layout/reftests/font-face), which are probably the most worthwhile target to check.

Failures on layout/reftests/reftest-sanity/font-download.html (there are four tests on this file with different pref) may be worth checking as well, but I guess it might be more a reftest harness issue for us.

I don't think there are too much left to do here, but checking the reftests may reveal something new.

Comment 20

[Bobby Holley (:bholley)]

(In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #19)
> I think majority of work has been done in bug 1345696. Many (read: most)
> mochitests related to @font-face still don't work because of
> servo/servo#16165 which I guess SimonSapin would take care of. There are
> three descriptors (font-feature-settings, font-language-override, and
> font-display) not implemented for stylo, which is also mostly just
> Servo-side work.

Filed bug 1352514 about this.
 
> We probably want to check reftest failures which are supposed to be fixed
> and see why they're still failing.
> 
> There are still a few reftests (some of them are skipped) marked "# bug
> 1290237" which I failed to re-enable (mainly in layout/reftests/font-face),
> which are probably the most worthwhile target to check.
> 
> Failures on layout/reftests/reftest-sanity/font-download.html (there are
> four tests on this file with different pref) may be worth checking as well,
> but I guess it might be more a reftest harness issue for us.
> 
> I don't think there are too much left to do here, but checking the reftests
> may reveal something new.

Ok, flagging shing to have a look into the above and make sure it's all on his radar.

I think we can close this issue now, and deal with the rest in followups.