605231 - unquoted 'initial' and 'default' should not match <family-name> when parsing 'font-family'

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

http://www.w3.org/TR/CSS21/fonts.html#propdef-font-family says:

The keywords 'initial' and 'default' are reserved for future use and must also be quoted when used as font names. UAs must not consider these keywords as matching the '<family-name>' type. 

We should implement this (though perhaps at the same time as doing bug 280443).

This makes us fail:
http://test.csswg.org/suites/css2.1/20101001/html4/font-family-rule-011.htm
http://test.csswg.org/suites/css2.1/20101001/xhtml1/font-family-rule-011.xht

Comment 1

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

http://test.csswg.org/suites/css2.1/20101001/html4/font-family-rule-010.htm
http://test.csswg.org/suites/css2.1/20101001/xhtml1/font-family-rule-010.xht

Comment 2

[John Daggett (:jtd)]

There's another subtle ripple here and that is the way CSSParserImpl::ParseFamily checks for reserved keywords is also incorrect, the check needs to be against the first *sequence* of idents, not simply the first ident in the token stream.  And additional names *also* need to be checked:

Examples:

  font-family: default; /* invalid */
  font-family: blah, default; /* invalid */
  font-family: default blah, blah default; /* valid */
  font-family: blah inherit, inherit blah; /* valid */
  font-family: blah initial; /* valid */
  font-family: blah -moz-initial; /* valid - matches a family called "blah -moz-initial" */

So I think the logic needs (1) handle valid keywords when the value is a single ident and (2) reject reserved keywords used as a single name in a list of names.

Comment 3

[John Daggett (:jtd)]

Attachment: add font family parsing tests

Add a bunch of parsing tests for various font family names, including names with various escape sequences and escaped quotes in them (see bug 475216 and 660397) and names that include reserved keywords explicitly disallowed for use as unquoted family names by the spec.

Structured in W3C testharness format, included as a mochitest.

Passes with current versions:

Google Chrome dev channel  2152/2488
Firefox Nightly            1706/2488
IE9                        1292/2488

Comment 4

[John Daggett (:jtd)]

Attachment: add font family parsing tests (failing tests commented out)

Added additional tests and commented out the failing tests (these fail due to quote and escape handling, with bug numbers noted).

Comment 5

[John Daggett (:jtd)]

Attachment: don't parse invalid and default as unquoted family names

This adjusts the logic used with ParseFamily to check for the use of invalid keywords within font family name lists.  The logic specifically handles the case where keywords are used *within* family names (e.g. "default blah") rather than as the entire name.

Comment 6

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 678646 [details] [diff] [review]
add font family parsing tests (failing tests commented out)

>+  { namelist: "default", invalid: true, single: true },
>+  { namelist: "initial", invalid: true, fontonly: true, single: true },
>+  { namelist: "inherit", invalid: true, fontonly: true, single: true },

Once we implement css3-values 'default' you'll also need 'fontonly'
on 'default'.  It might be better to add it there now to avoid
confusion later.

>+function fontProp(n, size, s1, s2) { return (s1 ? s1 : "") + " " + (s2 ? s2 : "") + " " + size + " " + n; }

Maybe put the + " " inside the (), twice, so you're substituting:
  (s1 ? s1 : "") + " "
with:
  (s1 ? s1 + " " : "")
so that you don't have so many leading spaces?

>+  while(sheet.cssRules.length > 0)
>+    sheet.deleteRule(0);

Braces would be nice.

>+  try {
>+    sheet.insertRule(rule, 0);
>+  } catch (e) {
>+    assert_true((e.name == "SyntaxError" || e.name == "SYNTAX_ERR")
>+      && e instanceof DOMException
>+      && e.code == DOMException.SYNTAX_ERR
>+      && invalid,
>+      "unexpected syntax error with " + decl + " ==> " + e.name);
>+    return;
>+  }

You shouldn't ever get this exception, since you shouldn't get a syntax
error for the rule for having an invalid declaration inside it.
Declaration parsing captures errors.  You should probably just remove
the whole try-catch bit.

>+    try {
>+      sheet.insertRule(r, 0);
>+    } catch (e) {
>+      assert_true(false, "exception occurred parsing serialized form of rule - " + rule + " ==> " + r + " " + e.name);
>+      return;
>+    }

Same here.

I'm not sure the splitting of where the test() function is and where the
asserts are really matches the intent of how testharness.js is supposed
to be used -- I think the intent is that the test() calls be
finer-grained.  But I think what you've done here is fine for now; just
something to consider when writing more testharness.js tests.

>+    assert_true(el.style.fontFamily != def, "fontFamily setter should parse serialized form - " + parsed);

You should make the stronger assertion here that el.style.fontFamily == parsed.
(Maybe as a separate assert_true, or maybe just by dropping the existing
one.)

(Same for el.style.font in the next function.)


I think the test names passed in the first half and the second
half of testFamilyNameParsing should probably be more different.  Your
current names are:
  t
  t + " (setter)"
  t
  t + " (setter)"
Maybe instead you could use:
  "font-family: " + t
  "font-family: " + t + " (setter)"
  "font: " + t
  "font: " + t + " (setter)"


r=dbaron with that

Comment 7

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 678647 [details] [diff] [review]
don't parse invalid and default as unquoted family names

Instead of just moving ParseOneFamily across the transform
parsing code, maybe instead have a separate patch to move the
transform parsing code out of the middle of the font parsing
code?  I'd suggest moving it to in between ParseTextOverflow
and ParseTransitionProperty.  (r=dbaron on that in advance,
if it's just a simple move of that chunk of transform parsing
code)


This patch needs a little adjustment for the fact that we've
added support for unprefixed 'initial', but it should be
straightforward:  just:
 (a) no need to add initial to the keyword list anymore
 (b) initial and -moz-initial both get the SetInitialValue()
     treatment; only default is the "for the future" case.

>+      switch (keyword) {
>+        case eCSSKeyword_inherit:
>+        case eCSSKeyword_initial:
>+        case eCSSKeyword_default:
>+        case eCSSKeyword__moz_initial:
>+        case eCSSKeyword__moz_use_system_font:
>+          return false;
>+          break;

This 'break;' is unreachable and should be removed; the return
is sufficient to exit from those cases.

r=dbaron with that

Comment 8

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 678646 [details] [diff] [review]
add font family parsing tests (failing tests commented out)

Review of attachment 678646 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/style/test/test_font_family_parsing.html
@@ +163,5 @@
> +      && e.code == DOMException.SYNTAX_ERR
> +      && invalid,
> +      "unexpected syntax error with " + decl + " ==> " + e.name);
> +    return;
> +  }

Better written as

assert_throws("SyntaxError", function() {
  sheet.insertRule(rule, 0);
});

@@ +165,5 @@
> +      "unexpected syntax error with " + decl + " ==> " + e.name);
> +    return;
> +  }
> +
> +  assert_true(sheet.cssRules.length == 1, "strange number of rules (" + sheet.cssRules.length + ") with " + decl);

assert_equals(sheet.cssRules.length, 1, ...)

@@ +169,5 @@
> +  assert_true(sheet.cssRules.length == 1, "strange number of rules (" + sheet.cssRules.length + ") with " + decl);
> +  var s = extractDecl(sheet.cssRules[0].cssText);
> +
> +  if (invalid) {
> +    assert_true(s == "", "rule declaration shouldn't parse - " + rule + " ==> " + s);

assert_equals(s, "", ...)

@@ +173,5 @@
> +    assert_true(s == "", "rule declaration shouldn't parse - " + rule + " ==> " + s);
> +  } else {
> +    assert_true(s != "", "rule declaration should parse - " + rule);
> +    if (s == "")
> +      return;

An assert_* failure will throw an exception, so no need for this check

@@ +183,5 @@
> +    try {
> +      sheet.insertRule(r, 0);
> +    } catch (e) {
> +      assert_true(false, "exception occurred parsing serialized form of rule - " + rule + " ==> " + r + " " + e.name);
> +      return;

assert_unreached, and drop the return

Comment 9

[John Daggett (:jtd)]

(In reply to David Baron [:dbaron] from comment #6)
> >+  try {
> >+    sheet.insertRule(rule, 0);
> >+  } catch (e) {
> >+    assert_true((e.name == "SyntaxError" || e.name == "SYNTAX_ERR")
> >+      && e instanceof DOMException
> >+      && e.code == DOMException.SYNTAX_ERR
> >+      && invalid,
> >+      "unexpected syntax error with " + decl + " ==> " + e.name);
> >+    return;
> >+  }
> 
> You shouldn't ever get this exception, since you shouldn't get a
> syntax error for the rule for having an invalid declaration inside
> it. Declaration parsing captures errors.  You should probably just
> remove the whole try-catch bit.

Some of the commented-out tests include punctuation characters that if
not handled correctly may cause a syntax error.  But your point is
taken, switching this to assert_unreached (this fires in Webkit for
some of the commented out tests).

Comment 10

[John Daggett (:jtd)]

(In reply to David Baron [:dbaron] from comment #6)
> I think the test names passed in the first half and the second
> half of testFamilyNameParsing should probably be more different.  Your
> current names are:
>   t
>   t + " (setter)"
>   t
>   t + " (setter)"
> Maybe instead you could use:
>   "font-family: " + t
>   "font-family: " + t + " (setter)"
>   "font: " + t
>   "font: " + t + " (setter)"

Sorry but I'm not quite sure what you want changed here...  The 't'
variable is set to 'font-family: ...' in the first half, 'font: ...'
in the second.

Comment 11

[John Daggett (:jtd)]

Attachment: add font family parsing tests (failing tests commented out)

Adjusted for review comments.  Carrying forward r=dbaron.

Comment 12

[John Daggett (:jtd)]

Attachment: move transform parsing code within nsCSSParser.cpp

As suggested in comment 7, move transform parsing code out of font property parsing code.  r=dbaron

Comment 13

[John Daggett (:jtd)]

Attachment: don't parse invalid and default as unquoted family names

Changes based on review comments.  Carrying forward r=dbaron

Comment 14

[John Daggett (:jtd)]

Pushed to mozilla-inbound:

https://hg.mozilla.org/integration/mozilla-inbound/rev/a9fafac7049f
https://hg.mozilla.org/integration/mozilla-inbound/rev/b33c80a5c3be
https://hg.mozilla.org/integration/mozilla-inbound/rev/3fcfb2fe7d98

If additional changes are needed in response to comment 10, I'll write an additional supplementary patch.

Comment 15

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to John Daggett (:jtd) from comment #10)
> (In reply to David Baron [:dbaron] from comment #6)
> > I think the test names passed in the first half and the second
> > half of testFamilyNameParsing should probably be more different.  Your
> > current names are:
> >   t
> >   t + " (setter)"
> >   t
> >   t + " (setter)"
> > Maybe instead you could use:
> >   "font-family: " + t
> >   "font-family: " + t + " (setter)"
> >   "font: " + t
> >   "font: " + t + " (setter)"
> 
> Sorry but I'm not quite sure what you want changed here...  The 't'
> variable is set to 'font-family: ...' in the first half, 'font: ...'
> in the second.

I just wanted something to distinguish the tests that were testing the font-family longhand from those that were testing the font shorthand.

Comment 16

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

... which means it's fine as it is.

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/a9fafac7049f
https://hg.mozilla.org/mozilla-central/rev/b33c80a5c3be
https://hg.mozilla.org/mozilla-central/rev/3fcfb2fe7d98