Bug 34147

Summary: If @font-face does not provide an explicit italic/bold variant, regular is used
Product: WebKit Reporter: Patryk Zawadzki <patrys>
Component: Layout and RenderingAssignee: Kenichi Ishibashi <bashi>
Status: RESOLVED FIXED    
Severity: Normal CC: bashi, dglazkov, hyatt, jamesr, jshin, macpherson, menard, mitz, phiw2, tkent, webkit.review.bot, yusukes
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Attachments:
Description Flags
bold_italic_font_face_v1
none
bold_italic_font_face_v2
none
Patch
none
test expectation1
none
test expectation2
none
Patch (revised Skipped and test_expectations.txt)
none
Patch
none
Patch
none
Patch
none
Patch(revised to ToT)
none
Patch(fix compile failure)
none
Patch(just rebased)
none
Patch for landing none

Description Patryk Zawadzki 2010-01-25 14:48:41 PST
Some families, for example M+ (http://mplus-fonts.sourceforge.jp/mplus-outline-fonts/design/index-en.html) do not provide an explicit italic/oblique font. Firefox behaves like the system does, by emulating italic using slanted Roman (more likely Pango does that automatically), in WebKit, regular roman is used instead.

(Sorry if this is reported against an incorrect element.)
Comment 1 Patryk Zawadzki 2010-01-25 15:53:30 PST
This happens both in GTK port and current Chromium snapshots. No idea about other ports.
Comment 2 Yusuke Sato 2010-02-09 22:43:17 PST
Chromium bug: http://code.google.com/p/chromium/issues/detail?id=31833
Comment 3 Yusuke Sato 2010-02-10 01:28:14 PST
Here is a small html that could reproduce the problem.

<html><head>
<style type="text/css">
@font-face {
     font-family: "A";
     src: url("http://HOST.NAME/PATH/TO/FONT.ttf");
}
.test {
     font-family: 'A';
}
</style></head>
<body><div>
<strong><p class="test">abcde</p></strong>
<i><p class="test">abcde</p></i>
<strong><i><p class="test">abcde</p></i></strong>
<p class="test">abcde</p>
<p>abcde</p>
</div></body></html>

When we don't specify font-weight: in a @font-face rule, WebCore/css/CSSFontSelector.cpp treat the font as if it has 100|200|300|400|...|900 weight. Not 400 (i.e., normal weight).
And when a @font-face font has the 100|...|900 weight mask, current WebCore/css/CSSSegmentedFontFace.cpp implementation never synthesize a bold font for the font. That seems to be the problem.
The same is true for italic. Unusual mask is used when font-style: is missing from @font-face.

I think we could fix the problem by giving special treatment to the 100|...|900 mask in CSSSegmentedFontFace.cpp.

(And I think we shouldn't modify CSSFontSelector.cpp side, since the 100|...|900 mask is necessary for proper handling of this case: https://bugs.webkit.org/show_bug.cgi?id=16348
 Without the 100|...|900 mask, <i>...</i> text in https://bugs.webkit.org/attachment.cgi?id=28455&action=prettypatch will be displayed in Times, not Arial.)

I'll attach a patch with tests shortly.
Comment 4 Yusuke Sato 2010-02-10 01:38:59 PST
Created attachment 48475 [details]
bold_italic_font_face_v1
Comment 5 Kent Tamura 2010-02-10 08:48:51 PST
Comment on attachment 48475 [details]
bold_italic_font_face_v1

> +++ b/LayoutTests/platform/mac/fast/css/font-face-synthetic-bold-italic-expected.txt
> @@ -0,0 +1,48 @@

What part of this result represents the correctness of this patch?
Comment 6 mitz 2010-02-10 08:49:59 PST
Comment on attachment 48475 [details]
bold_italic_font_face_v1

WebKit’s current behavior matches the working draft that allowed specifying multiple values for the font-weight and font-style descriptors: <http://www.w3.org/TR/2002/WD-css3-webfonts-20020802/#select>. The latest working draft does not allow it <http://www.w3.org/TR/2009/WD-css3-fonts-20090618/#font-property-descriptors-the-font-style>. I think it’s reasonable to change the behavior to match the newer draft—which I think will have the same effect on the test case in this patch. However, this patch introduces new behavior that doesn’t match either version of the spec. That seems like a bad idea.
Comment 7 Yusuke Sato 2010-02-15 21:09:53 PST
Created attachment 48787 [details]
bold_italic_font_face_v2
Comment 8 Yusuke Sato 2010-02-15 21:14:44 PST
(In reply to comment #6)
Thanks for the comment. I've updated the patch so that it follows the latest 2009 draft. Can you please take another look?

> What part of this result represents the correctness of this patch?

Kent-san, I modified the html and added a png result so developers can see if the test passes or not.
Comment 9 Kent Tamura 2010-02-15 21:29:15 PST
> > What part of this result represents the correctness of this patch?
> Kent-san, I modified the html and added a png result so developers can see if
> the test passes or not.

ok, the updated test result looks good.
Comment 10 Adam Barth 2010-03-22 08:47:55 PDT
Comment on attachment 48787 [details]
bold_italic_font_face_v2

The actual code change in this patch is somewhat mysterious to me, but according to the comments on this bug, this patch brings our behavior closer to Firefox and the specification.  Yusuke has also addressed comments from mitz.

Please wait a few days before landing this patch to give mitz and others a chance to object to this patch.

@reviewers: If you'd rather patches like this one got more expert review, please be more agressive at r-ing or r+ing them.  I'd rather not review this far outside my area, but we also don't want patches waiting for review forever.
Comment 11 Dave Hyatt 2010-03-22 12:14:16 PDT
Comment on attachment 48787 [details]
bold_italic_font_face_v2

Dan says, "If we’re making this change, shouldn’t we entirely drop support for 'all' and for multiple values in @font-face descriptors?"

I'm a bit confused by this change.  Is it supported by the latest spec draft?

Clearing flag for discussion.
Comment 12 Yusuke Sato 2010-03-23 03:31:38 PDT
(In reply to comment #11)

Thanks for the comment. 'all' is no longer supported by the latest draft. I'll modify the CSS parser and attach a new patch later.
Comment 13 Kenichi Ishibashi 2011-10-26 00:47:26 PDT
Created attachment 112461 [details]
Patch
Comment 14 Kenichi Ishibashi 2011-10-26 00:49:15 PDT
Created attachment 112462 [details]
test expectation1
Comment 15 Kenichi Ishibashi 2011-10-26 00:49:58 PDT
Created attachment 112463 [details]
test expectation2
Comment 16 Kenichi Ishibashi 2011-10-26 00:53:36 PDT
(In reply to comment #13)
> Created an attachment (id=112461) [details]
> Patch

I've revised Yusuke-san's patch so that font selection logic matches the current draft (http://www.w3.org/TR/2011/WD-css3-fonts-20111004/#font-matching-algorithm). Attached expectations were generated by using chromium Linux port. Could you take another look?
Comment 17 Kenichi Ishibashi 2011-10-26 02:55:26 PDT
Created attachment 112476 [details]
Patch (revised Skipped and test_expectations.txt)
Comment 18 Luke Macpherson 2011-10-31 14:46:01 PDT
Comment on attachment 112476 [details]
Patch (revised Skipped and test_expectations.txt)

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

> Source/WebCore/css/CSSFontSelector.cpp:120
> +        if (styleList->length() != 1)

Does it make sense for this to ever be anything other than a single CSSPrimitiveValue here? I would remove handling of lists and remove generation of lists from the parser.
Comment 19 Kenichi Ishibashi 2011-11-01 23:43:27 PDT
Created attachment 113279 [details]
Patch
Comment 20 Kenichi Ishibashi 2011-11-01 23:44:46 PDT
Comment on attachment 112476 [details]
Patch (revised Skipped and test_expectations.txt)

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

Thanks for the comments!

>> Source/WebCore/css/CSSFontSelector.cpp:120
>> +        if (styleList->length() != 1)
> 
> Does it make sense for this to ever be anything other than a single CSSPrimitiveValue here? I would remove handling of lists and remove generation of lists from the parser.

Done.
Comment 21 WebKit Review Bot 2011-11-02 00:31:36 PDT
Comment on attachment 113279 [details]
Patch

Attachment 113279 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10258166

New failing tests:
svg/W3C-SVG-1.1/fonts-desc-02-t.svg
Comment 22 Kenichi Ishibashi 2011-11-06 21:30:37 PST
Created attachment 113822 [details]
Patch
Comment 23 Kenichi Ishibashi 2011-11-06 21:32:58 PST
(In reply to comment #22)
> Created an attachment (id=113822) [details]
> Patch

Seems like we need to leave font-variant related code in font selection algorithm for CSS2.1 compatibility.
Comment 24 Kenichi Ishibashi 2011-11-28 17:37:30 PST
Created attachment 116856 [details]
Patch
Comment 25 Kenichi Ishibashi 2011-12-05 18:36:44 PST
(In reply to comment #24)
> Created an attachment (id=116856) [details]
> Patch

Ping, mitz?
Comment 26 Kenichi Ishibashi 2012-01-06 00:27:47 PST
Created attachment 121407 [details]
Patch(revised to ToT)
Comment 27 Kenichi Ishibashi 2012-01-06 00:32:14 PST
(In reply to comment #26)
> Created an attachment (id=121407) [details]
> Patch(revised to ToT)

Hi mitz,

Could you please review this patch? If you have no time to review the patch, could you suggest another reviewers?

Thanks,
Comment 28 Early Warning System Bot 2012-01-06 00:44:10 PST
Comment on attachment 121407 [details]
Patch(revised to ToT)

Attachment 121407 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/11153006
Comment 29 Gyuyoung Kim 2012-01-06 00:48:39 PST
Comment on attachment 121407 [details]
Patch(revised to ToT)

Attachment 121407 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/11141294
Comment 30 Kenichi Ishibashi 2012-01-06 00:53:40 PST
Created attachment 121409 [details]
Patch(fix compile failure)
Comment 31 Kenichi Ishibashi 2012-02-08 16:49:21 PST
Created attachment 126189 [details]
Patch(just rebased)
Comment 32 Kenichi Ishibashi 2012-02-08 17:06:26 PST
Mitz and other reviewers,

Could you please review the patch? We have been received several bug report related to this problem.
- http://crbug.com/31833 (the original bug report)
- http://crbug.com/101577
- http://code.google.com/p/chromium/issues/detail?id=39017#c66
- https://groups.google.com/a/googleproductforums.com/d/topic/chrome-ja/dRcBObfLh1c/discussion (in Japanese)
Comment 33 mitz 2012-02-12 08:44:23 PST
Comment on attachment 126189 [details]
Patch(just rebased)

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

> Source/WebCore/css/CSSFontSelector.cpp:432
> +    //   - If the desired weight is 500, 400 is checked first and then the rule for desired weights less than 400 is used.

This almost seems like a typo in the spec. If you specify 500, you get 100 before you get 600.
Comment 34 Kenichi Ishibashi 2012-02-12 16:22:33 PST
Comment on attachment 126189 [details]
Patch(just rebased)

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

Hi mitz,

Thank you so much for review.

>> Source/WebCore/css/CSSFontSelector.cpp:432
>> +    //   - If the desired weight is 500, 400 is checked first and then the rule for desired weights less than 400 is used.
> 
> This almost seems like a typo in the spec. If you specify 500, you get 100 before you get 600.

I'm not sure whether it is a typo. I'd like to keep it at this time. I'll change the behavior if the spec is wrong.
Comment 35 Kenichi Ishibashi 2012-02-12 16:24:34 PST
Created attachment 126688 [details]
Patch for landing
Comment 36 WebKit Review Bot 2012-02-12 16:43:45 PST
Comment on attachment 126688 [details]
Patch for landing

Clearing flags on attachment: 126688

Committed r107516: <http://trac.webkit.org/changeset/107516>
Comment 37 WebKit Review Bot 2012-02-12 16:43:52 PST
All reviewed patches have been landed.  Closing bug.
Comment 38 mitz 2012-02-22 11:05:58 PST
This broke fast/css/font-face-default-font.html. See bug 79260.