1355345 - stylo: Support font-display descriptor in @font-face rule

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, enhancement, P2)

Description

[Xidorn Quan [:xidorn] UTC+10]

font-display is currently defined unofficially in https://tabatkins.github.io/specs/css-font-display/

Given bug 1157064 and bug 1157064, we probably want to add it for stylo.

Comment 1

[Fernando Jiménez Moreno [:ferjm]]

Attachment: Bug 1355345 - stylo: Support font-display descriptor in @font-face rule.

Review commit: https://reviewboard.mozilla.org/r/140236/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/140236/

Comment 2

[Anthony Ramine [:nox]]

This is now specified in https://drafts.csswg.org/css-fonts-4/#font-display-desc, btw.

And while at it, drive-by reviewing: Fernando, define_css_keyword_enum! is your friend.

Comment 3

[Emilio Cobos Álvarez [:emilio]]

Comment on attachment 8868652 [details]
Bug 1355345 - stylo: Support font-display descriptor in @font-face rule.

https://reviewboard.mozilla.org/r/140236/#review143996

r=me with those two, thanks!

::: servo/components/style/font_face.rs:80
(Diff revision 1)
> +/// A font-display value for a @font-face rule.
> +/// The font-display descriptor determines how a font face is displayed based
> +/// on whether and when it is downloaded and ready to use.
> +#[cfg(feature = "gecko")]
> +#[derive(Clone, Debug, PartialEq, Eq)]
> +pub enum FontDisplay {

yes, you want to use `define_css_keyword_enum` for this, as nox said.

Also, any reason this _needs_ to be `cfg`'d, if it doesn't I'd rather leave it compiling for Servo too.

::: servo/components/style/gecko/rules.rs:125
(Diff revision 1)
> +        nscssvalue.set_enum(match self {
> +            FontDisplay::Auto => structs::NS_FONT_DISPLAY_AUTO as i32,
> +            FontDisplay::Block => structs::NS_FONT_DISPLAY_BLOCK as i32,
> +            FontDisplay::Swap => structs::NS_FONT_DISPLAY_SWAP as i32,
> +            FontDisplay::Fallback => structs::NS_FONT_DISPLAY_FALLBACK as i32,
> +            FontDisplay::Optional => structs::NS_FONT_DISPLAY_OPTIONAL as i32,

nit: You can put the cast after the `match`, I think.

match self {
    // ...
} as i32

Comment 4

[Anthony Ramine [:nox]]

https://github.com/servo/servo/pull/16931

Comment 5

[Fernando Jiménez Moreno [:ferjm]]

Comment on attachment 8868652 [details]
Bug 1355345 - stylo: Support font-display descriptor in @font-face rule.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/140236/diff/1-2/

Comment 6

[Fernando Jiménez Moreno [:ferjm]]

Comment on attachment 8868652 [details]
Bug 1355345 - stylo: Support font-display descriptor in @font-face rule.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/140236/diff/2-3/

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e486cf1ce1b0
stylo: Support font-display descriptor in @font-face rule. r=emilio

Comment 8

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/e486cf1ce1b0