1434802 - A backslash (\) is inserted before the space in the font family of the style when the font name contains spaces.

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[dapsdh]

Attachment: ff_font_family.png

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:58.0) Gecko/20100101 Firefox/58.0
Build ID: 20180128191252

Steps to reproduce:

1. In the Developer Tools console, type the following line of code.
var p = document.createElement("p"); p.style.fontFamily = "맑은 고딕"; console.log(p.outerHTML);
2. You can see that the style value has a backslash (\).
<p style="font-family: 맑은\ 고딕;"></p>


Actual results:

In version 58, if there is a space in the font-family value, a backslash is placed before the space. In version 57, it had a value without a backslash. (57.0.3)

When comparing fonts in my application, I get a problem that is recognized as a different font because of the backslash.


Expected results:

Do not insert a backslash in the font-family style.

Comment 1

[Gingerbread Man]

Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:60.0) Gecko/20100101 Firefox/60.0
20180131220303

https://hg.mozilla.org/integration/autoland/json-pushes?changeset=d49cf4aed3350dff668d64a2cf86ce64e6c7a2df&full=1

NI :xidorn because :heycam is away at the moment.

Comment 2

[Andrew Overholt [:overholt]]

Is it possible for you to use mozregression to figure out when this breakage happened? http://mozilla.github.io/mozregression/

Comment 3

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

This sounds similar to bug 1384398 comment 5, so potentially a Stylo issue?

Comment 4

[Emilio Cobos Álvarez (:emilio)]

There's a regression range in comment 1

Comment 5

[Emilio Cobos Álvarez (:emilio)]

I see why this happens, though I don't have an immediate fix just yet...

Comment 6

[Emilio Cobos Álvarez (:emilio)]

Also, to be clear, this is not a correctness bug. Both font-family names are effectively equivalent. I'm going to do a simple change that also makes us match how chrome deals with these cases, but just because it also simplifies surrounding code.

Comment 7

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1434802: Tweak specified font family serialization in Gecko so that it is simpler.

In particular, every time that there's at least more than one identifier, switch
to quoted family name, since the reconstruction of the serialization will be
lossy anyway.

This allows us to avoid copies and all that.

What Chrome implements doesn't make much sense in the sense that they always
serialize:

  font-family: "foo";  -> font-family: foo;
  font-family: foo bar;  -> font-family: "foo bar";
  font-family: foo\ bar; -> font-family: "foo bar";

This patch makes us match on the second case, but not on the rest, because I
think Gecko's behavior is preferable in those cases.

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

Comment 8

[Emilio Cobos Álvarez (:emilio)]

Hmm, also... What does your application do exactly?

I think you can't expect to get back the exact same serialization as you put in, and instead you should roundtrip it. If only because other browsers can hand out that in a quoted form. Do you deal with that? If so, what's the problem with dealing with CSS escaping?

What you should do to compare two font families is effectively check what the browser gives you back, and compare with that value _after_ having gone through the browser. For example:

  let family = "foo bar";
  let div = document.createElement('div');
  div.style.fontFamily = family;
  let familyHandedOutByTheBrowser = div.style.fontfamily;
  if (whateverElement.style.fontFamily == familyHandedOutByTheBrowser) {
    ...
  }

Comment 9

[Emilio Cobos Álvarez (:emilio)]

See https://bugzilla.mozilla.org/show_bug.cgi?id=1384398#c15 for the hard cases. This gets them right afaict, but worth checking.

Comment 10

[Xidorn Quan [:xidorn] UTC+11]

Comment on attachment 8949080 [details]
Bug 1434802: Tweak specified font family serialization in Gecko so that it is simpler.

https://reviewboard.mozilla.org/r/218482/#review224404

Comment 11

[Emilio Cobos Álvarez (:emilio)]

Thanks for the review!

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

Comment 12

[Xidorn Quan [:xidorn] UTC+11]

Attachment: Bug 1434802 - Output unquoted family name as a series of identifiers.

This implements what Gecko's nsStyleUtil::AppendEscapedCSSFontFamilyList
does, which should bring us to Gecko's original behavior.

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

Comment 13

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8949315 [details]
Bug 1434802 - Output unquoted family name as a series of identifiers.

https://reviewboard.mozilla.org/r/218702/#review224506

::: servo/components/style/values/computed/font.rs:291
(Diff revision 1)
>                  write!(CssStringWriter::new(dest), "{}", self.name)?;
>                  dest.write_char('"')
>              }
> -            FamilyNameSyntax::Identifiers(ref serialization) => {
> -                // Note that `serialization` is already escaped/
> -                // serialized appropriately.
> +            FamilyNameSyntax::Identifiers => {
> +                let mut first = true;
> +                for ident in self.name.to_string().split_whitespace() {

Hmm, does this work with stuff like:

```
foo\ \ bar
```

?

I guess `split_whitespace` produces three slices?

::: servo/components/style/values/computed/font.rs:414
(Diff revision 1)
>              "unset" => css_wide_keyword = true,
>              "default" => css_wide_keyword = true,
>              _ => {}
>          }
>  
>          let mut value = first_ident.as_ref().to_owned();

It'd be nice to not unconditionally call to_owned unless there's a secon identifier, though don't worry too much if you don't think it's worth.

Comment 14

[Xidorn Quan [:xidorn] UTC+11]

Comment on attachment 8949315 [details]
Bug 1434802 - Output unquoted family name as a series of identifiers.

https://reviewboard.mozilla.org/r/218702/#review224506

> Hmm, does this work with stuff like:
> 
> ```
> foo\ \ bar
> ```
> 
> ?
> 
> I guess `split_whitespace` produces three slices?

It produce two, which is... probably incorrect in terms of spec conformance.

The old Gecko style system produces `foo \ bar` for this case, and Blink does `"foo  bar"`, which both have two whitespaces in the font family name...

There are lots of edge cases around an escaped whitespace, actually...

Comment 15

[Xidorn Quan [:xidorn] UTC+11]

Comment on attachment 8949315 [details]
Bug 1434802 - Output unquoted family name as a series of identifiers.

https://reviewboard.mozilla.org/r/218702/#review224506

> It'd be nice to not unconditionally call to_owned unless there's a secon identifier, though don't worry too much if you don't think it's worth.

I... don't think it's worth that additional complexity... unless profiling shows this can be a problem at some point.

Comment 16

[Xidorn Quan [:xidorn] UTC+11]

Comment on attachment 8949315 [details]
Bug 1434802 - Output unquoted family name as a series of identifiers.

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

Comment 17

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8949315 [details]
Bug 1434802 - Output unquoted family name as a series of identifiers.

https://reviewboard.mozilla.org/r/218702/#review224696

::: commit-message-c7bd4:4
(Diff revision 2)
> +Bug 1434802 - Output unquoted family name as a series of identifiers. r?emilio
> +
> +This implements what Gecko's nsStyleUtil::AppendEscapedCSSFontFamilyList
> +does, which should bring us to Gecko's original behavior.

This is no longer true, right?

Mind describing this behavior a bit more detailedly on the commit message?

::: servo/components/style/values/computed/font.rs:430
(Diff revision 2)
>              value.push(' ');
>              value.push_str(&ident);
> -            serialization.push(' ');
> -            serialize_identifier(&ident, &mut serialization).unwrap();
>          }
> +        let syntax = if value.starts_with(' ') || value.ends_with(' ') || value.contains("  ") {

Maybe put an example of something that starts with a space due to an scape sequence or something?

Ideally an example for all the three cases this tries to handle.

::: servo/components/style/values/computed/font.rs:440
(Diff revision 2)
> +            FamilyNameSyntax::Quoted
> +        } else {
> +            FamilyNameSyntax::Identifiers
> +        };
>          Ok(SingleFontFamily::FamilyName(FamilyName {
> -            name: Atom::from(value),
> +            name: Atom::from(value), syntax

nit: field to its own line? no big deal anyway.

Comment 18

[Xidorn Quan [:xidorn] UTC+11]

Attachment: Servo PR

Comment 19

[Xidorn Quan [:xidorn] UTC+11]

https://hg.mozilla.org/integration/autoland/rev/f740058b4be0

Comment 20

[Xidorn Quan [:xidorn] UTC+11]

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

Comment 21

[Ryan VanderMeulen [:RyanVM]]

Is it possible to land a test for this? Also, can you comment on the severity of this issue and whether it warrants backport consideration for 59?

Comment 22

[Emilio Cobos Álvarez (:emilio)]

301 Xidorn, who ended up doing the final fix.

Testing this is not terrible, but I'm not sure we want to commit to this behavior forever, given no browser agrees. Maybe it'd be worth to file a CSSWG issue.

Comment 23

[Xidorn Quan [:xidorn] UTC+11]

The spec is simple that it just wants us to serialize all font family as string... which should probably be updated anyway.

Comment 24

[Ryan VanderMeulen [:RyanVM]]

From IRC discussion with Xidorn, this can ride with Fx60.

Comment 25

[Xidorn Quan [:xidorn] UTC+11]

Adding test in bug 1446232.