Skip to content

Commit 08fa649

Browse files
emilioemilio
committed
Bug 1986533 - Quote invalid font-family descriptors in FontFace constructor and family setter. r=layout-reviewers,firefox-style-system-reviewers,dshin
Existing tests will be modified as needed for this, and a WPT added for this edge case. Differential Revision: https://phabricator.services.mozilla.com/D265128
1 parent 893348a commit 08fa649

File tree

5 files changed

+64
-24
lines changed

5 files changed

+64
-24
lines changed

‎layout/style/FontFaceImpl.cpp‎

Lines changed: 17 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -442,17 +442,29 @@ bool FontFaceImpl::SetDescriptor(nsCSSFontDesc aFontDesc,
442442

443443
// FIXME(heycam): Should not allow modification of FontFaces that are
444444
// CSS-connected and whose rule is read only.
445-
bool changed;
446-
if (!Servo_FontFaceRule_SetDescriptor(GetData(), aFontDesc, &aValue, url,
447-
&changed)) {
445+
bool changed = false;
446+
const bool valid = [&] {
447+
if (Servo_FontFaceRule_SetDescriptor(GetData(), aFontDesc, &aValue, url,
448+
&changed)) {
449+
return true;
450+
}
451+
if (aFontDesc == eCSSFontDesc_Family) {
452+
// TODO: Warn to the console?
453+
nsAutoCString quoted;
454+
nsStyleUtil::AppendEscapedCSSString(aValue, quoted, '"');
455+
if (Servo_FontFaceRule_SetDescriptor(GetData(), aFontDesc, &quoted, url,
456+
&changed)) {
457+
return true;
458+
}
459+
}
448460
aRv.ThrowSyntaxError(
449461
nsPrintfCString("Invalid font descriptor %s: %s",
450462
nsCSSProps::GetStringValue(aFontDesc).get(),
451463
PromiseFlatCString(aValue).get()));
452464
return false;
453-
}
465+
}();
454466

455-
if (!changed) {
467+
if (!valid || !changed) {
456468
return false;
457469
}
458470

‎layout/style/nsStyleUtil.cpp‎

Lines changed: 9 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -140,16 +140,16 @@ bool nsStyleUtil::ValueIncludes(const nsAString& aValueList,
140140
return false;
141141
}
142142

143-
void nsStyleUtil::AppendEscapedCSSString(const nsAString& aString,
144-
nsAString& aReturn,
145-
char16_t quoteChar) {
146-
MOZ_ASSERT(quoteChar == '\'' || quoteChar == '"',
143+
void nsStyleUtil::AppendEscapedCSSString(const nsACString& aString,
144+
nsACString& aReturn,
145+
char aQuoteChar) {
146+
MOZ_ASSERT(aQuoteChar == '\'' || aQuoteChar == '"',
147147
"CSS strings must be quoted with ' or \"");
148148

149-
aReturn.Append(quoteChar);
149+
aReturn.Append(aQuoteChar);
150150

151-
const char16_t* in = aString.BeginReading();
152-
const char16_t* const end = aString.EndReading();
151+
const char* in = aString.BeginReading();
152+
const char* const end = aString.EndReading();
153153
for (; in != end; in++) {
154154
if (*in < 0x20 || *in == 0x7F) {
155155
// Escape U+0000 through U+001F and U+007F numerically.
@@ -160,13 +160,12 @@ void nsStyleUtil::AppendEscapedCSSString(const nsAString& aString,
160160
// It's not technically necessary to escape the quote
161161
// character that isn't being used to delimit the string,
162162
// but we do it anyway because that makes testing simpler.
163-
aReturn.Append(char16_t('\\'));
163+
aReturn.Append('\\');
164164
}
165165
aReturn.Append(*in);
166166
}
167167
}
168-
169-
aReturn.Append(quoteChar);
168+
aReturn.Append(aQuoteChar);
170169
}
171170

172171
/* static */

‎layout/style/nsStyleUtil.h‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -44,10 +44,10 @@ class nsStyleUtil {
4444
const nsStringComparator& aComparator);
4545

4646
// Append a quoted (with 'quoteChar') and escaped version of aString
47-
// to aResult. 'quoteChar' must be ' or ".
48-
static void AppendEscapedCSSString(const nsAString& aString,
49-
nsAString& aResult,
50-
char16_t quoteChar = '"');
47+
// to aResult. 'aQuoteChar' must be ' or ".
48+
static void AppendEscapedCSSString(const nsACString& aString,
49+
nsACString& aResult,
50+
char aQuoteChar = '"');
5151

5252
// Append the identifier given by |aIdent| to |aResult|, with
5353
// appropriate escaping so that it can be reparsed to the same

‎layout/style/test/test_font_loading_api.html‎

Lines changed: 4 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -384,13 +384,12 @@
384384
invalidFontFamilyNames.forEach(function(aFamilyName) {
385385
familyTests = familyTests.then(function() {
386386
var face = new win.FontFace(aFamilyName, fontData);
387-
is(face.status, "error", "FontFace should be error immediately after construction with invalid family name " + aFamilyName + " (TEST 17) (" + what + ")");
388-
is(face.family, "", "FontFace.family should be the empty string after construction with invalid family name " + aFamilyName + " (TEST 17) (" + what + ")");
387+
isnot(face.status, "error", "FontFace should be quoted after construction with invalid family name " + aFamilyName + " (TEST 17) (" + what + ")");
388+
is(face.family, `"${aFamilyName}"`, "FontFace.family should be the quoted string after construction with invalid family name " + aFamilyName + " (TEST 17) (" + what + ")");
389389
return face.loaded.then(function() {
390-
ok(false, "FontFace should not load after invalid family name " + aFamilyName + " specified (TEST 17) (" + what + ")");
390+
ok(true, "FontFace should load after invalid family name " + aFamilyName + " specified (TEST 17) (" + what + ")");
391391
}, function(aError) {
392-
ok(true, "FontFace should not load after invalid family name " + aFamilyName + " specified (TEST 17) (" + what + ")");
393-
is(aError.name, "SyntaxError", "FontFace.loaded with invalid family name " + aFamilyName + " should be rejected with a SyntaxError (TEST 17) (" + what + ")");
392+
ok(false, "FontFace should load after invalid family name " + aFamilyName + " specified (TEST 17) (" + what + ")");
394393
});
395394
});
396395
});

‎testing/web-platform/tests/css/css-font-loading/fontface-invalid-family.tentative.html‎

Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,30 @@
1+
<!doctype html>
2+
<meta charset="utf-8">
3+
<link rel="author" title="Emilio Cobos Álvarez" href="mailto:emilio@crisal.io">
4+
<link rel="author" title="Mozilla" href="https://mozilla.com">
5+
<link rel="help" href="https://drafts.csswg.org/css-font-loading-3/">
6+
<link rel="help" href="https://bugzilla.mozilla.org/show_bug.cgi?id=1986533">
7+
<link rel="help" href="https://github.com/w3c/csswg-drafts/issues/6236">
8+
<script src="/resources/testharness.js"></script>
9+
<script src="/resources/testharnessreport.js"></script>
10+
<script>
11+
// These are values that are invalid as a family-name, or CSS generics.
12+
const kInvalidValues = [
13+
"content:Segoe UI",
14+
"sans-serif",
15+
"A, B",
16+
"inherit",
17+
"a 1",
18+
"",
19+
];
20+
21+
for (let familyName of kInvalidValues) {
22+
promise_test(async function() {
23+
let face = new FontFace(familyName, "url('resources/Rochester.otf')");
24+
assert_not_equals(face.status, "error", `FontFace should be quoted after construction with invalid family name ${familyName}`);
25+
assert_equals(face.family, `"${familyName}"`, `FontFace.family should be the quoted string after construction with invalid family name ${familyName}`);
26+
await face.load();
27+
assert_true(true, `FontFace should load after invalid family name ${familyName} specified`);
28+
}, `family: ${familyName}`);
29+
}
30+
</script>

0 commit comments

Comments
 (0)