font shorthand with inherit keyword incorrectly parsed and rendered

• Component: CSS
• Status: VERIFIED FIXED

[Gérard Talbot]

The following consecutive CSS rules

 h1 {font-size: 32px;}

 h1 {font: 96px inherit;}

should be parsed and only the 
h1 {font-size: 32px;} 
rule should apply. A parsing error should be found for 
h1 {font: 96px inherit;}
rule.

Steps to reproduce:
1- Load provided URL

Actual results in Safari 3.1.2 build 525.21
The rendered text will be using a font-size of 96px 

Expected results
The rendered text should be using a font-size of 32px

Notes:
- Section 15.8 of CSS 2.1
http://www.w3.org/TR/CSS21/fonts.html#font-shorthand
only allows 
font: inherit 
as correct
- CSS validator will report
h1       Value Error : font  Too many values or values are not recognized : 
96px inherit 
- I searched for a duplicate and did not find any

[Gérard Talbot]

This bug, it seems, also occurs in Safari 4.0 / Mac OS X 10.5 (Leopard) 	AppleWebKit build 526.11.
Therefore, setting version to 526+

[Gérard Talbot]

[ [ <'font-style'> || <'font-variant'> || <'font-weight'> ]? <'font-size'> [ / <'line-height'> ]? <'font-family'> ] | caption | icon | menu | message-box | small-caption | status-bar | inherit

"A bar (|) separates two or more alternatives: exactly one of them must occur."

http://www.w3.org/TR/CSS21/about.html#value-defs

Worth noting is that "System fonts may only be set as a whole; that is, the font family, size, weight, style, etc. are all set at the same time."
The same should have been said about inherit to avoid possible confusion, to be explicit.

[Robert Blaut]

Created attachment 22535 [details]
minimal test case

[Gérard Talbot]

Bug 169610: font shorthand and inherit incorrectly parsed 
https://bugs.kde.org/show_bug.cgi?id=169610
has more explanations on this bug.

Also filed for Opera:
http://www.gtalbot.org/BrowserBugsSection/Opera9Bugs/#bug35

[Gérard Talbot]

From CSS 2.1, Candidate Recommendation 23 April 2009, Appendix C. Changes

{
C.3.1 Shorthand properties

Shorthand properties take a list of subproperty values or the value 'inherit'.
One cannot mix 'inherit' with other subproperty values as it would not be
possible to specify the subproperty to which 'inherit' applied. The definitions
of a number of shorthand properties did not enforce this rule: 'border-top',
'border-right', 'border-bottom', 'border-left', 'border', 'background', 'font',
'list-style', 'cue', and 'outline'. 
}
http://www.w3.org/TR/CSS21/changes.html#q142

regards, Gérard

[Gérard Talbot]

Robert, 
your minimal testcase, attachment 22535 [details] , raises a problem, at least a question. Font shorthand requires font-size and font-family if one of the 2 properties are defined. In other words:

div {font: 1px; }

should be parsed as an error and be rejected and ignored. Was this intentional/deliberate on your part? Just asking..

regards, Gérard

[Robert Blaut]

Created attachment 31279 [details]
minimal test case (fixed)

[Robert Blaut]

(In reply to comment #6)

> your minimal testcase, attachment 22535 [details] [review] , raises a problem, at least a
> question. Font shorthand requires font-size and font-family if one of the 2
> properties are defined. 

I fixed the test case to avoid confusion.

[Alexis Menard (darktears)]

Created attachment 126809 [details]
Patch

[Alexis Menard (darktears)]

(In reply to comment #9)
> Created an attachment (id=126809) [details]
> Patch

Though in a following patch I think the parseFont() could be improved.

[Tony Chang]

Comment on attachment 126809 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=126809&action=review

> Source/WebCore/css/CSSParser.cpp:4303
> +        if (value->id == CSSValueInitial || value->id == CSSValueInherit)
> +            return list.release();

Is this correct?  It looks like it's possible for list to have values appended to it that are returned.  Can you add some tests that just set font family directly?

> LayoutTests/fast/css/font-shorthand-mix-inherit.html:29
> +    test.style.font = "12pt/14pt inherit";
> +    shouldBe("test.style.getPropertyValue('font')", "''");

Can you also add a test for "inherit sans-serif"?

> LayoutTests/fast/css/font-shorthand-mix-inherit.html:36
> +    test.style.font = "x-large/110% \"new century schoolbook\", serif, inherit";

Nit: Use '' so you don't have to escape the "s.

[Alexis Menard (darktears)]

Created attachment 127012 [details]
Patch

[WebKit Review Bot]

Comment on attachment 127012 [details]
Patch

Clearing flags on attachment: 127012

Committed r107728: <http://trac.webkit.org/changeset/107728>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Gérard Talbot]

Hello,

As far as I can see, it appears that this bug has NOT been fixed.

Under Linux, when using Chrome 17.0.963.79, the tested sentence uses a font-size of 96px and not 32px.

Under XP Pro SP3, when using Safari 5.1.4 (build 7534.54.16), the tested sentence uses a font-size of 96px and not 32px.

Is this bug supposed to be fixed with next release of Safari and Chrome? Just asking...

regards, Gérard

[Alexis Menard (darktears)]

(In reply to comment #15)
> Hello,

Hi,

> 
> As far as I can see, it appears that this bug has NOT been fixed.
> 

It has been fixed and is covered by a test.

> Under Linux, when using Chrome 17.0.963.79, the tested sentence uses a font-size of 96px and not 32px.
> 
> Under XP Pro SP3, when using Safari 5.1.4 (build 7534.54.16), the tested sentence uses a font-size of 96px and not 32px.
> 
> Is this bug supposed to be fixed with next release of Safari and Chrome? Just asking...
> 

There is delay, you may not see yet the fix. It's there you just need to wait that Google or Apple rebase the webkit shipping with their browsers.

> regards, Gérard

[Gérard Talbot]

With Chrome 23.0.1271.91, I get expected results. Thank you Alexis!

Marking as VERIFIED

[Alexis Menard (darktears)]

(In reply to comment #17)
> With Chrome 23.0.1271.91, I get expected results. Thank you Alexis!
> 
> Marking as VERIFIED

You're welcome!