stylo: Parsing for @font-feature-values

ASSIGNED
Assigned to

Status

()

Core
CSS Parsing and Computation
P2
normal
ASSIGNED
2 months ago
11 hours ago

People

(Reporter: jeremychen, Assigned: canaltinova)

Tracking

(Blocks: 2 bugs)

Firefox Tracking Flags

(Not tracked)

Details

MozReview Requests

()

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

Attachments

(3 attachments, 1 obsolete attachment)

(Reporter)

Description

2 months ago
Separate the parsing part to here.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Reporter)

Comment 6

2 months ago
Comment on attachment 8868976 [details]
Bug 1365900 - (wip) add initial style system support for @font-feature-falues rule.

The most different part of @font-feature-values rule is that it may contain nested @rules blocks, such like:

@font-feature-values font1 {
  @swash {
    ident1: 2;
    ident2: 4;
  }
}

I tried to reuse the existing parsing codes for 1-level @rule parser and wrote this patch. Since I'm not very familiar with Rust and macro_rules stuff, so I think it would be better to gather some feedback before going further.

Xidorn, could you take a look and give me some feedback? Do you think this patch is toward the right direction? If you're not comfortable with giving feedback on this pure Rust patch, please feel free to pass the request to Emilio or Simon, thank you.
Attachment #8868976 - Flags: feedback?(xidorn+moz)
Comment hidden (mozreview-request)
(Reporter)

Updated

2 months ago
Attachment #8868976 - Flags: feedback?(xidorn+moz)
Comment on attachment 8868976 [details]
Bug 1365900 - (wip) add initial style system support for @font-feature-falues rule.

I myself don't write lots of parser stuff. Most of parser bits of my previous work were written by Simon, so I redirect f? to him.
Attachment #8868976 - Flags: feedback?(xidorn+moz) → feedback?(simon.sapin)
Comment on attachment 8868976 [details]
Bug 1365900 - (wip) add initial style system support for @font-feature-falues rule.

@font-feature-values is different enough from @font-face that I don’t think it makes sense to try to re-use the macros. I recommend doing this without macros at first, even if that means duplicating some code. Once it is clear where duplication exist consider reducing it first with (possibly generic) functions, and then (if necessary) with macros.

Now, a good starting point for new code is data structures. Once you have the correct data structures, the code around it tends to be easier to shape. For CSS stuff data structures are dictated by two things: what input syntax has to be accepted/rejected by the parser, and how it is accessed through CSSOM.

Syntax of CSS things is often specified with a grammar. In this case it is https://drafts.csswg.org/css-fonts/#basic-syntax, but feature_value_definition in particular is more general than what is really accepted: it defines one or more integers, but then https://drafts.csswg.org/css-fonts/#multi-valued-feature-value-definitions specified that depending on the feature type the value may be limited to one integers or a pair of integers.

The WebIDL interface at https://drafts.csswg.org/css-fonts/#om-fontfeaturevalues show that CSSOM always has a (string -> sequence of integers) map for each feature type, even if that map is empty.

Now, you have:

    pub struct FontFeatureValuesRuleData {
        pub family_name: Option<FamilyName>,
        pub swash: Option<FontFeatureRuleData>,
        // TODO: [ @stylistic, @styleset, @character-variant, @ornaments, @annotation ]
    }

    pub struct FontFeatureRuleData {
        name: String,
        features: Vec<FeatureValuePair>,
    }

    pub struct FeatureValuePair {
        pub name: Option<String>,
        pub value: Option<i32>,
    }

This doesn’t look right.

Option<FamilyName> encodes zero or one family name, but font_family_name_list in the grammar is `font_family_name [ S* ',' S* font_family_name ]*` which is is one or more family names. So Option<FamilyName> should be replaced Vec<FamilyName>. (Vec is "zero or more" rather than "one or more", but it’s often not worth enforcing the "at least one" part with the type system.)

CSSOM always has a "swash" attribute in CSSFontFeatureValuesRule, which is a map, even it is empty. So Option<FontFeatureRuleData> should not be optional.

FontFeatureRuleData contains a name string, which seems redundant with the name of the "swash" field which is know statically. It seems like that name doesn’t need to be stored in memory at all. This leaves only a Vec inside FontFeatureRuleData, so we probably can remove FontFeatureRuleData entirely and store the Vec directly. ("FontFeatureRuleData" isn’t a great name anyway.)

I assume that FeatureValuePair is named after "'name: value' pair". In other CSS contexts we call these declarations, so this type could be FontFeatureValueDeclaration.

Its name is optional, which doesn’t match the feature_value_definition grammar or the CSSFontFeatureValuesMap WebIDL interface. Atom should be used instead of String for faster string comparison. This makes the field 'pub name: Atom,'

The spec says "Values associated with a given identifier are limited to integer values 0 or greater.", so i32 should be u32.

Option encodes "zero or one" value but the feature_value_definition grammar defines "one or more". But as mentioned before the number of integers actually depends on the feature type. I think it would be nice encode that in the type system.


All in all, here is a data structure that I think matches the spec better:

    pub struct FontFeatureValuesRuleData {
        pub family_names: Vec<FamilyName>,
        pub swash: Vec<(Atom, u32)>,
        pub stylistic Vec<(Atom, u32)>,
        pub ornaments: Vec<(Atom, u32)>,
        pub annotation: Vec<(Atom, u32)>,
        pub character_variant: Vec<(Atom, u32, Option<u32>)>,  // One or two integers
        pub styleset: Vec<(Atom, Vec<u32>)>,  // One or more integers
    }

(Tuples can be replaced with structs with named fields, if desired.)
Attachment #8868976 - Flags: feedback?(simon.sapin)
(Reporter)

Comment 10

a month ago
(In reply to Simon Sapin (:SimonSapin) from comment #9)
> 
> All in all, here is a data structure that I think matches the spec better:
> 
>     pub struct FontFeatureValuesRuleData {
>         pub family_names: Vec<FamilyName>,
>         pub swash: Vec<(Atom, u32)>,
>         pub stylistic Vec<(Atom, u32)>,
>         pub ornaments: Vec<(Atom, u32)>,
>         pub annotation: Vec<(Atom, u32)>,
>         pub character_variant: Vec<(Atom, u32, Option<u32>)>,  // One or two
> integers
>         pub styleset: Vec<(Atom, Vec<u32>)>,  // One or more integers
>     }
> 
> (Tuples can be replaced with structs with named fields, if desired.)

Simon, very appreciate your kind feedback and guidance!!

I'll rework based on this data structure.
By the way, instead of Vec<(Atom, …)> we’ll probably eventually want some kind of hash map to make lookup by name faster in CSSOM and layout. But this might be in the C++ side depending on how Gecko integration is done, so it’s fine to use Vec for now and revisit later.
(Reporter)

Comment 12

a month ago
Per discussed with Nazım offline, he might be able to take a look at this while taking Bug 1356124. They're not highly related though, but the parsing/data storage might be similar I think.

Un-assign myself, and cc Nazım.
Assignee: jeremychen → nobody
Status: ASSIGNED → NEW
(Reporter)

Comment 13

a month ago
Comment on attachment 8868976 [details]
Bug 1365900 - (wip) add initial style system support for @font-feature-falues rule.

I think this patch is not helpful for going forward, obsolete this.
Attachment #8868976 - Attachment is obsolete: true
(Assignee)

Comment 14

a month ago
Yeah, I'll work on this next.
Assignee: nobody → canaltinova
Status: NEW → ASSIGNED
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 21

a day ago
mozreview-review
Comment on attachment 8890235 [details]
Bug 1365900 - Implement parsing/serialization for @font-feature-values rule

https://reviewboard.mozilla.org/r/161348/#review166748

::: servo/components/style/stylesheets/font_feature_values_rule.rs:26
(Diff revision 2)
> +/// A @font-feature-values block declaration.
> +/// It is `<custom-ident>: <integer>+`.
> +/// This struct can take 3 value types.
> +/// - `SingleValue` is to keep just one unsigned integer value.
> +/// - `PairValues` is to keep one or two unsigned integer values.
> +/// - `VectorValues` is to keep a list of unsigned integer vaslues.

typo: vaslues

::: servo/components/style/stylesheets/font_feature_values_rule.rs:170
(Diff revision 2)
> +
> +    fn parse_value<'t>(&mut self, name: CompactCowStr<'i>, input: &mut Parser<'i, 't>)
> +                       -> Result<(), ParseError<'i>> {
> +        let value = input.parse_entirely(|i| T::parse(self.context, i))?;
> +        self.declarations.push(FFVDeclaration {
> +            name: CustomIdent::from_ident(name, &[""])?,

I think using CustomIdent is not appropriate here. The spec does not refer to `<custom-ident>`. Please change the the field type to Atom and use `Atom::from(&*name)` here.

::: servo/components/style/stylesheets/font_feature_values_rule.rs:339
(Diff revision 2)
> +                                    let error = ContextualParseError::UnsupportedKeyframePropertyDeclaration(
> +                                        iter.input.slice(err.span), err.error);
> +                                    log_css_error(iter.input, pos, error, &context);
> +                                }
> +                            }
> +                            self.rule.$ident.append(&mut iter.parser.declarations);

This should deduplicate potential multiple declarations for the same identifier:

https://drafts.csswg.org/css-fonts/#basic-syntax
>  If the same identifier is defined mulitple times for a given feature type and font family, the last defined value is used.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Thanks for the review Simon! Addressed your comments.

Comment 26

22 hours ago
mozreview-review
Comment on attachment 8890235 [details]
Bug 1365900 - Implement parsing/serialization for @font-feature-values rule

https://reviewboard.mozilla.org/r/161348/#review166976

::: servo/components/style/stylesheets/font_feature_values_rule.rs:339
(Diff revisions 1 - 3)
>                                      let error = ContextualParseError::UnsupportedKeyframePropertyDeclaration(
>                                          iter.input.slice(err.span), err.error);
>                                      log_css_error(iter.input, pos, error, &context);
>                                  }
>                              }
> -                            self.rule.$ident.append(&mut iter.parser.declarations);
> +                            let _ = mem::replace(&mut self.rule.$ident, iter.parser.declarations);

Individual declarations should be de-duplicated, not entire `Vec`s. For example, this:

@swash {
  foo: 1;
  bar: 2;
  foo: 3;
}
@swash {
  bar: 4;
}

Needs to parse into:

@swash {
  foo: 3;
  bar: 4;
}
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 30

21 hours ago
mozreview-review-reply
Comment on attachment 8890235 [details]
Bug 1365900 - Implement parsing/serialization for @font-feature-values rule

https://reviewboard.mozilla.org/r/161348/#review166976

> Individual declarations should be de-duplicated, not entire `Vec`s. For example, this:
> 
> @swash {
>   foo: 1;
>   bar: 2;
>   foo: 3;
> }
> @swash {
>   bar: 4;
> }
> 
> Needs to parse into:
> 
> @swash {
>   foo: 3;
>   bar: 4;
> }

Oh, thanks for clarification. I thought we had to overwrite all @swash declaration list. Converted Vec into a HasMap instead and added a test for it. That should work.

Comment 31

21 hours ago
mozreview-review
Comment on attachment 8890235 [details]
Bug 1365900 - Implement parsing/serialization for @font-feature-values rule

https://reviewboard.mozilla.org/r/161348/#review167050

::: servo/components/style/stylesheets/font_feature_values_rule.rs:24
(Diff revisions 1 - 4)
>  use stylesheets::CssRuleType;
> -use values::CustomIdent;
>  
> -/// A @font-feature-values block declaration.
> -/// It is `<custom-ident>: <integer>+`.
> -/// This struct can take 3 value types.
> +/// A list of @font-feature-values block declarations.
> +/// One declaration is `<custom-ident>: <integer>+`.
> +/// Key is a `<custom-ident>` for declaration name.

This comment still refers to `<custom-ident>`.

::: servo/components/style/stylesheets/font_feature_values_rule.rs:330
(Diff revisions 1 - 4)
>                  match prelude {
>                      $(
>                          BlockType::$ident_camel => {
> -                            let parser = FFVDeclarationParser {
> +                            let parser = FFVDeclarationsParser {
>                                  context: &context,
> -                                declarations: vec![] as Vec<FFVDeclaration<$ty>>,
> +                                declarations: FFVDeclarations(HashMap::new() as HashMap<Atom, $ty>),

Are these intermediate hash maps necessary? Couldn’t FFVDeclarationsParser contains a mutable reference to the rule’s hash map instead? (This would also make the `append` method unnecessary.)

::: servo/components/style/stylesheets/font_feature_values_rule.rs:330
(Diff revisions 1 - 4)
>                  match prelude {
>                      $(
>                          BlockType::$ident_camel => {
> -                            let parser = FFVDeclarationParser {
> +                            let parser = FFVDeclarationsParser {
>                                  context: &context,
> -                                declarations: vec![] as Vec<FFVDeclaration<$ty>>,
> +                                declarations: FFVDeclarations(HashMap::new() as HashMap<Atom, $ty>),

`as` looks like a conversion. If specifying the type is necessary, consider `HashMap::<Atom, $ty>::new()`
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 35

20 hours ago
mozreview-review
Comment on attachment 8890235 [details]
Bug 1365900 - Implement parsing/serialization for @font-feature-values rule

https://reviewboard.mozilla.org/r/161348/#review167072
Attachment #8890235 - Flags: review?(simon.sapin) → review+

Comment 36

19 hours ago
mozreview-review
Comment on attachment 8890236 [details]
Bug 1365900 - Extract CSSFontFeatureValuesRule base class

https://reviewboard.mozilla.org/r/161350/#review167084

::: layout/style/PostTraversalTask.h:10
(Diff revision 4)
> +#include "nsError.h"
> +

Why is this?
Attachment #8890236 - Flags: review?(xidorn+moz) → review+

Comment 37

19 hours ago
mozreview-review
Comment on attachment 8890236 [details]
Bug 1365900 - Extract CSSFontFeatureValuesRule base class

https://reviewboard.mozilla.org/r/161350/#review167086

::: layout/style/CSSFontFeatureValuesRule.h:25
(Diff revision 5)
> +                               , public nsIDOMCSSFontFeatureValuesRule
> +{
> +public:
> +  NS_DECL_ISUPPORTS_INHERITED
> +
> +  virtual bool IsCCLeaf() const override = 0;

I believe you can move this method from `nsFontFeatureValuesRule` to this class as well.
(Assignee)

Comment 38

19 hours ago
mozreview-review-reply
Comment on attachment 8890236 [details]
Bug 1365900 - Extract CSSFontFeatureValuesRule base class

https://reviewboard.mozilla.org/r/161350/#review167084

> Why is this?

Oh, after adding new files to UNIFIED_SOURCES, this file started to give error about `nsresult`. I guess this file started to unify with other files during compilation and `nsError.h` was not imported there.

Comment 39

19 hours ago
mozreview-review
Comment on attachment 8890237 [details]
Bug 1365900 - Create ServoFontFeatureValuesRule and bind servo data

https://reviewboard.mozilla.org/r/161352/#review167090
Attachment #8890237 - Flags: review?(xidorn+moz) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 45

14 hours ago
Comment on attachment 8890235 [details]
Bug 1365900 - Implement parsing/serialization for @font-feature-values rule

I had to change back to the Vec from HashMap since it doesn't hold the order and makes tests fail :( Also I had to add a new function named `update_or_push` to overwrite old value. Could you review again Simon?
Attachment #8890235 - Flags: review+ → review?(simon.sapin)

Comment 46

11 hours ago
mozreview-review
Comment on attachment 8890235 [details]
Bug 1365900 - Implement parsing/serialization for @font-feature-values rule

https://reviewboard.mozilla.org/r/161348/#review167232
Attachment #8890235 - Flags: review?(simon.sapin) → review+
You need to log in before you can comment on or make changes to this bug.