1384398 - stylo: serialization of font-family doesn't match Gecko

Status: RESOLVED FIXED

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

Description

[Cameron McCormack (:heycam)]

It looks like there are a couple of differences in how font-family values are serialized.  Compare the attached test with stylo enabled and disabled:

1. Stylo serializes the space separated font names as a single identifier with backslashed-escaped spaces, but Gecko serializes them as three separate identifiers.

2. Stylo puts a space after the comma, and Gecko doesn't.  (Stylo's behaviour seems preferable.)

3. If you open devtools, the two `:root { font-family: ... }` rules are displayed differently.  In the style sheet without the following @supports rule, backslashes are not displayed, and in the style sheet with the following @supports rule, backslashes are displayed.  That's pretty weird.

Comment 1

[Emilio Cobos Álvarez (:emilio)]

Did you meant to attach a test case?

Comment 2

[Cameron McCormack (:heycam)]

Attachment: test

Comment 3

[j.j.]

(In reply to Cameron McCormack (:heycam) from comment #0)

> 2. Stylo puts a space after the comma, and Gecko doesn't.  (Stylo's
> behaviour seems preferable.)

Space after comma is speced, if I'm not mistaken.
https://drafts.csswg.org/cssom/#serialize-a-string

Comment 4

[Cameron McCormack (:heycam)]

(In reply to Cameron McCormack (:heycam) from comment #0)
> 3. If you open devtools, the two `:root { font-family: ... }` rules are
> displayed differently.  In the style sheet without the following @supports
> rule, backslashes are not displayed, and in the style sheet with the
> following @supports rule, backslashes are displayed.  That's pretty weird.

I can't reproduce this one any more.

Comment 5

[Cameron McCormack (:heycam)]

(In reply to Cameron McCormack (:heycam) from comment #0)
> 1. Stylo serializes the space separated font names as a single identifier
> with backslashed-escaped spaces, but Gecko serializes them as three separate
> identifiers.

For this I think the issue is that Servo's FamilyName type cannot distinguish between a sequence of identifiers (each of which individually must be escaped individually) and a single identifier.

Comment 6

[Cameron McCormack (:heycam)]

(In reply to Cameron McCormack (:heycam) from comment #0)
> 2. Stylo puts a space after the comma, and Gecko doesn't.  (Stylo's
> behaviour seems preferable.)

This is a regression from bug 280443.

Comment 7

[Cameron McCormack (:heycam)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=5fcfff8ce506ae00305be86bb0f1c6cbbda65da2

Comment 8

[Cameron McCormack (:heycam)]

Attachment: Bug 1384398 - style: Preserve font-family identifier sequence when serializing.

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

Comment 9

[Cameron McCormack (:heycam)]

Attachment: Bug 1384398 - Separate family names in font-family serialization with ", ".

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

Comment 10

[Cameron McCormack (:heycam)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=74396928648fd9150db1adc0c2b6f476dce77011

Comment 11

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8891724 [details]
Bug 1384398 - style: Preserve font-family identifier sequence when serializing.

https://reviewboard.mozilla.org/r/162786/#review168124

::: servo/components/style/properties/gecko.mako.rs:1825
(Diff revision 1)
>                      FontFamilyType::eFamily_moz_fixed => FontFamily::Generic(Atom::from("-moz-fixed")),
> -                    FontFamilyType::eFamily_named => FontFamily::FamilyName(FamilyName {
> -                        name: (&*gecko_font_family_name.mName).into(),
> -                        quoted: false
> -                    }),
> +                    FontFamilyType::eFamily_named => {
> +                        let name = Atom::from(&*gecko_font_family_name.mName);
> +                        FontFamily::FamilyName(FamilyName {
> +                            name: name.clone(),
> +                            syntax: FamilyNameSyntax::Identifiers(vec![name]),

hmm... So I don't really think this is correct... Why is it? Couldn't it be a sequence of identifiers?

::: servo/components/style/properties/longhand/font.mako.rs:106
(Diff revision 1)
> +
> +        #[derive(Debug, PartialEq, Eq, Clone, Hash)]
> +        #[cfg_attr(feature = "servo", derive(HeapSizeOf, Deserialize, Serialize))]
> +        pub enum FamilyNameSyntax {
> +            Quoted,
> +            Identifiers(Vec<Atom>),

Instead of `Identifiers(Vec<Atom>)`, why not just `SpaceSeparatedIdentifiers`, and given we need to convert the name to a `String` anyway, we can split it during serialisation instead of keeping track of it this way.

Would that work? If not, why not?

::: servo/components/style/properties/longhand/font.mako.rs:251
(Diff revision 1)
> -                    write!(CssStringWriter::new(dest), "{}", self.name)?;
> +                        write!(CssStringWriter::new(dest), "{}", self.name)?;
> -                    dest.write_char('"')
> +                        dest.write_char('"')
> -                } else {
> -                    serialize_identifier(&*self.name.to_string(), dest)
> -                }
> +                    }
> +                    FamilyNameSyntax::Identifiers(ref identifiers) => {

And if that works, a comment to mention that it's intentional and what problem it prevents would be quite on point.

Comment 12

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8891725 [details]
Bug 1384398 - Separate family names in font-family serialization with ", ".

https://reviewboard.mozilla.org/r/162788/#review168126

r=me

Apparently we had tests for this, but not the previous patch, any chance to write one?

Comment 13

[Cameron McCormack (:heycam)]

Comment on attachment 8891724 [details]
Bug 1384398 - style: Preserve font-family identifier sequence when serializing.

https://reviewboard.mozilla.org/r/162786/#review168124

> hmm... So I don't really think this is correct... Why is it? Couldn't it be a sequence of identifiers?

The spec says (if I read it correctly) that sequences of identifiers should compute to a single string with all of the identifiers concatened with a single space in between.  Gecko doesn't do that currently, and it just stores a single string with a quoted flag.  So I figured I'd do the smallest change here and just worry about the specified value.  (And yes, ideally we would have a separate specified and computed type for a family name list.)

> Instead of `Identifiers(Vec<Atom>)`, why not just `SpaceSeparatedIdentifiers`, and given we need to convert the name to a `String` anyway, we can split it during serialisation instead of keeping track of it this way.
> 
> Would that work? If not, why not?

By this do you mean store a single String that is the serialization of the sequence of identifiers, and so would already take into account any backslash-escaping of individual identifiers?

Comment 14

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8891724 [details]
Bug 1384398 - style: Preserve font-family identifier sequence when serializing.

https://reviewboard.mozilla.org/r/162786/#review168124

> The spec says (if I read it correctly) that sequences of identifiers should compute to a single string with all of the identifiers concatened with a single space in between.  Gecko doesn't do that currently, and it just stores a single string with a quoted flag.  So I figured I'd do the smallest change here and just worry about the specified value.  (And yes, ideally we would have a separate specified and computed type for a family name list.)

Right, but this effectively makes a value of:

```
Identifiers([foo, bar]),
```

Go to gecko as:

```
{ name: "foo bar", quoted: false }
```

And return as:

```
Identifiers([foo bar])
```

Which at least deserves a comment I think.

> By this do you mean store a single String that is the serialization of the sequence of identifiers, and so would already take into account any backslash-escaping of individual identifiers?

Nope, I mean the raw string as Gecko does, just instead of passing it down to serialize_identifier entirely, which would escape the spaces, do it ourselves splitting... I think it may work because it doesn't matter whether the original had any escaped space or not, I think.

Basically, I was suggesting something like:


```
let name = self.name.to_string(); // We already do this conversion.

let mut first = true;
for ident in name.split(' ') { // This doesn't allocate or anything like that.
    if !first {
        dest.write_char(' ')?;
    }
    first = false;
    serialize_identifier(ident, dest)?;
}
````

Comment 15

[Cameron McCormack (:heycam)]

(In reply to Emilio Cobos Álvarez [:emilio] from comment #14)
> > By this do you mean store a single String that is the serialization of the sequence of identifiers, and so would already take into account any backslash-escaping of individual identifiers?
> 
> Nope, I mean the raw string as Gecko does, just instead of passing it down
> to serialize_identifier entirely, which would escape the spaces, do it
> ourselves splitting... I think it may work because it doesn't matter whether
> the original had any escaped space or not, I think.
> 
> Basically, I was suggesting something like:
> 
> 
> ```
> let name = self.name.to_string(); // We already do this conversion.
> 
> let mut first = true;
> for ident in name.split(' ') { // This doesn't allocate or anything like
> that.
>     if !first {
>         dest.write_char(' ')?;
>     }
>     first = false;
>     serialize_identifier(ident, dest)?;
> }
> ````

I don't think that would work when you have identifiers with backslash-escaped spaces in them, e.g. if you had

  p { font-family: A B\ \ C D; }

then it should represent the family name "A B  C D" but it would get incorrectly serialized as "A B   C D" (which is equivalent to "A B C D").

If you like I would be happy to store the white space concatenated identifier-serialized identifiers in one Atom, and pass that to Gecko_FontFamilyList_AppendNamed with aQuoted = false.  I guess that is closer to what Gecko does itself.  Then serialization is just writing that Atom out.

Comment 16

[Emilio Cobos Álvarez (:emilio)]

(In reply to Cameron McCormack (:heycam) from comment #15)
> I don't think that would work when you have identifiers with
> backslash-escaped spaces in them, e.g. if you had
> 
>   p { font-family: A B\ \ C D; }
> 
> then it should represent the family name "A B  C D" but it would get
> incorrectly serialized as "A B   C D" (which is equivalent to "A B C D").

Hmm... Yeah, that's fair.

> If you like I would be happy to store the white space concatenated
> identifier-serialized identifiers in one Atom, and pass that to
> Gecko_FontFamilyList_AppendNamed with aQuoted = false.  I guess that is
> closer to what Gecko does itself.  Then serialization is just writing that
> Atom out.

That sounds great :)

Comment 17

[Cameron McCormack (:heycam)]

Reworked patch still needs some work: https://treeherder.mozilla.org/#/jobs?repo=try&revision=24e6d23b7603663552938cfb0d8803dec5faa2bd

Comment 18

[Cameron McCormack (:heycam)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=cb11b27e054737edd362e1508a9d03b5e00d2b55

Comment 19

[Cameron McCormack (:heycam)]

Comment on attachment 8891724 [details]
Bug 1384398 - style: Preserve font-family identifier sequence when serializing.

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

Comment 20

[Cameron McCormack (:heycam)]

Comment on attachment 8891725 [details]
Bug 1384398 - Separate family names in font-family serialization with ", ".

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

Comment 21

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8891724 [details]
Bug 1384398 - style: Preserve font-family identifier sequence when serializing.

https://reviewboard.mozilla.org/r/162786/#review170528

::: servo/components/style/properties/longhand/font.mako.rs:260
(Diff revision 2)
> -                    dest.write_char('"')
> +                        dest.write_char('"')
> -                } else {
> -                    serialize_identifier(&*self.name.to_string(), dest)
> -                }
> +                    }
> +                    FamilyNameSyntax::Identifiers(ref serialization) => {
> +                        dest.write_str(&*serialization)

nit: Maybe add a comment on here to note they're escaped already.

Comment 22

[Cameron McCormack (:heycam)]

Comment on attachment 8891725 [details]
Bug 1384398 - Separate family names in font-family serialization with ", ".

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

Comment 23

[Cameron McCormack (:heycam)]

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

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/df230b4a8eb5
Separate family names in font-family serialization with ", ". r=emilio

Comment 25

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

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

Comment 26

[:gerard-majax]

Is it expected that this patch produces a lot of stdout messages about fonts? It is polluting my system logs (linux).

Comment 27

[Emilio Cobos Álvarez (:emilio)]

Yeah, Cameron accidentally left a debugging println! there, but that was removed in https://github.com/servo/servo/pull/17987 which should be in the next nightly.

Sorry for the hassle!