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

RESOLVED FIXED in Firefox 55

Status

()

Core
CSS Parsing and Computation
P2
normal
RESOLVED FIXED
2 months ago
8 days ago

People

(Reporter: xidorn, Assigned: ferjm)

Tracking

(Blocks: 2 bugs)

53 Branch
mozilla55
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox55 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Reporter)

Description

2 months ago
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.
Priority: -- → P2
(Assignee)

Updated

24 days ago
Assignee: nobody → ferjmoreno
Comment hidden (mozreview-request)
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

10 days ago
mozreview-review
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
Attachment #8868652 - Flags: review?(emilio+bugs) → review+
https://github.com/servo/servo/pull/16931
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 7

9 days ago
Pushed by ferjmoreno@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/e486cf1ce1b0
stylo: Support font-display descriptor in @font-face rule. r=emilio
https://hg.mozilla.org/mozilla-central/rev/e486cf1ce1b0
Status: NEW → RESOLVED
Last Resolved: 8 days ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.