Bug 20181 - font shorthand with inherit keyword incorrectly parsed and rendered
Summary: font shorthand with inherit keyword incorrectly parsed and rendered
Status: VERIFIED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Windows XP
: P2 Normal
Assignee: Alexis Menard (darktears)
URL: http://www.gtalbot.org/BrowserBugsSec...
Keywords: HasReduction
Depends on:
Blocks:
 
Reported: 2008-07-26 16:21 PDT by Gérard Talbot (no longer involved)
Modified: 2012-11-28 12:38 PST (History)
8 users (show)

See Also:


Attachments
minimal test case (245 bytes, text/html)
2008-07-29 05:29 PDT, Robert Blaut
no flags Details
minimal test case (fixed) (257 bytes, text/html)
2009-06-14 22:19 PDT, Robert Blaut
no flags Details
Patch (7.05 KB, patch)
2012-02-13 12:50 PST, Alexis Menard (darktears)
no flags Details | Formatted Diff | Diff
Patch (7.69 KB, patch)
2012-02-14 12:01 PST, Alexis Menard (darktears)
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Gérard Talbot (no longer involved) 2008-07-26 16:21:45 PDT
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
Comment 1 Gérard Talbot (no longer involved) 2008-07-26 17:05:36 PDT
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+
Comment 2 Gérard Talbot (no longer involved) 2008-07-28 10:59:53 PDT
[ [ <'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.
Comment 3 Robert Blaut 2008-07-29 05:29:35 PDT
Created attachment 22535 [details]
minimal test case
Comment 4 Gérard Talbot (no longer involved) 2008-11-15 10:11:30 PST
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
Comment 5 Gérard Talbot (no longer involved) 2009-05-02 11:05:21 PDT
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
Comment 6 Gérard Talbot (no longer involved) 2009-06-14 13:10:20 PDT
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
Comment 7 Robert Blaut 2009-06-14 22:19:23 PDT
Created attachment 31279 [details]
minimal test case (fixed)
Comment 8 Robert Blaut 2009-06-14 22:21:07 PDT
(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.
Comment 9 Alexis Menard (darktears) 2012-02-13 12:50:42 PST
Created attachment 126809 [details]
Patch
Comment 10 Alexis Menard (darktears) 2012-02-13 12:51:51 PST
(In reply to comment #9)
> Created an attachment (id=126809) [details]
> Patch

Though in a following patch I think the parseFont() could be improved.
Comment 11 Tony Chang 2012-02-14 11:29:45 PST
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.
Comment 12 Alexis Menard (darktears) 2012-02-14 12:01:31 PST
Created attachment 127012 [details]
Patch
Comment 13 WebKit Review Bot 2012-02-14 13:00:40 PST
Comment on attachment 127012 [details]
Patch

Clearing flags on attachment: 127012

Committed r107728: <http://trac.webkit.org/changeset/107728>
Comment 14 WebKit Review Bot 2012-02-14 13:00:45 PST
All reviewed patches have been landed.  Closing bug.
Comment 15 Gérard Talbot (no longer involved) 2012-03-14 23:39:33 PDT
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
Comment 16 Alexis Menard (darktears) 2012-03-15 01:53:46 PDT
(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
Comment 17 Gérard Talbot (no longer involved) 2012-11-28 10:58:40 PST
With Chrome 23.0.1271.91, I get expected results. Thank you Alexis!

Marking as VERIFIED
Comment 18 Alexis Menard (darktears) 2012-11-28 12:38:37 PST
(In reply to comment #17)
> With Chrome 23.0.1271.91, I get expected results. Thank you Alexis!
> 
> Marking as VERIFIED

You're welcome!