Closed Bug 1614348 Opened 2 years ago Closed 2 years ago

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

Categories

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

72 Branch
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla75
Tracking Status
firefox75 --- fixed

People

(Reporter: ye.sladowsky, Assigned: emilio)

Details

Attachments

(1 file)

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"

Component: Untriaged → DOM: CSS Object Model
Product: Firefox → Core

I agree that looks odd, will investigate.

Thanks for reporting it!

Assignee: nobody → emilio
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: needinfo?(emilio)

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\""

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

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

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.

Flags: needinfo?(emilio)

(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.

(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.

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

Right

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.

Pushed by ealvarez@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/53904ff83350
Make font-family serialization algorithm a bit more conservative. r=xidorn
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla75
You need to log in before you can comment on or make changes to this bug.