1614348 - Calling CSSStyleDeclaration.getPropertyValue('font-family') for an unquoted font-family that contains a backslash-escaped space followed by a digit returns unexpected result

Status: RESOLVED FIXED

(Core :: DOM: CSS Object Model, defect)

Description

[ye.sladowsky]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_15_3) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/79.0.3945.130 Safari/537.36

Steps to reproduce:

In Developer Tools select any element and add the following style declaration:
font-family: Font Awesome\ 5 Solid;
(unquoted; this is what my CSS minifier was doing by default to quoted font-family values).
In the console execute the following:
getComputedStyle($0).getPropertyValue('font-family');
or:
$0.style.fontFamily.

Actual results:

"Font Awesome \35 Solid"

Expected results:

"Font Awesome 5 Solid"

Comment 1

[Emilio Cobos Álvarez (:emilio)]

I agree that looks odd, will investigate.

Thanks for reporting it!

Comment 2

[Daniel Holbert [:dholbert]]

I think this is a devtools rules-view bug, perhaps? (Or a bug in an API that devtools uses?)

If you use the console to set the style as well as to get it, then you get expected results. i.e. with some element selected:

$0.style.fontFamily = '"Font Awesome\ 5 Solid"'
$0.style.fontFamily

That ^^ produces this output, which is correct I think:

"\"Font Awesome 5 Solid\""

Comment 3

[Emilio Cobos Álvarez (:emilio)]

No, you're quoting the family which prevents the bug:

$0.style.fontFamily = 'Font Awesome\\ 5 Solid'
$0.style.fontFamily

Comment 4

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1614348 - Make font-family serialization algorithm a bit more conservative. r=heycam,#style

So as to avoid serializing as identifiers font-families with spaces as part of
the identifier. This avoids getting into trouble if the beginning of the stuff
after the space happens to not be a valid identifier start.

This is an slightly more restrictive (and sound) version of the existing logic,
which happens to also match other browsers in my testing.

Comment 5

[Xidorn Quan [:xidorn] UTC+10]

(In reply to ye.sladowsky from comment #0)

Actual results:
"Font Awesome \35 Solid"

This is valid result, and it's identical to your input Font Awesome\ 5 Solid actually.

Expected results:
"Font Awesome 5 Solid"

You would never get this, because it's an invalid value, because family-name requires each part to be a valid identifier if not quoted, and 5 is not an identifier.

So no browser would give you this. Any browser outputting this should consider it as a bug. You would likely get '"Font Awesome 5 Solid"' instead (note the extra quotes), though.

I think emilio's patch is fine, and indeed that makes us match what other browsers do (via quoting in more cases), but I don't really like it. I generally prefer preserving original form in more common cases, like Hello world as Hello world rather than "Hello world", in the expense of having to output something slightly weird in edge cases, like this.

But I don't have strong opinion on that, so either way is fine.

Comment 6

[Emilio Cobos Álvarez (:emilio)]

(In reply to Xidorn Quan [:xidorn] UTC+11 from comment #5)

I think emilio's patch is fine, and indeed that makes us match what other browsers do (via quoting in more cases), but I don't really like it. I generally prefer preserving original form in more common cases, like Hello world as Hello world rather than "Hello world", in the expense of having to output something slightly weird in edge cases, like this.

Note that my patch still keeps that case working, it wouldn't keep Hello\ world though.

Comment 7

[Xidorn Quan [:xidorn] UTC+10]

Oh, so the patch only quotes it when there is any whitespace inside an identifier? That sounds much better then.

Comment 8

[Emilio Cobos Álvarez (:emilio)]

Right

Comment 9

[Emilio Cobos Álvarez (:emilio)]

So to be clear, the serialization we are producing is perfectly fine as-is, because \35 is the escaped sequence for 5. So this is not a correctness fix after all. But I think my patch produces less confusing results over all.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/53904ff83350
Make font-family serialization algorithm a bit more conservative. r=xidorn

Comment 11

[Razvan Maries]

https://hg.mozilla.org/mozilla-central/rev/53904ff83350