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 Rendering

Assignee:

Kenichi Ishibashi <bashi>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

bashi, dglazkov, hyatt, jamesr, jshin, macpherson, menard, mitz, phiw2, tkent, webkit.review.bot, yusukes

Priority:

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

Patryk Zawadzki

Reported 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.)

Attachments
bold_italic_font_face_v1 (6.53 KB, patch) 2010-02-10 01:38 PST, Yusuke Sato	no flags	Details Formatted Diff Diff
bold_italic_font_face_v2 (82.82 KB, patch) 2010-02-15 21:09 PST, Yusuke Sato	no flags	Details Formatted Diff Diff
Patch (29.64 KB, patch) 2011-10-26 00:47 PDT, Kenichi Ishibashi	no flags	Details Formatted Diff Diff
test expectation1 (18.52 KB, image/png) 2011-10-26 00:49 PDT, Kenichi Ishibashi	no flags	Details
test expectation2 (18.24 KB, image/png) 2011-10-26 00:49 PDT, Kenichi Ishibashi	no flags	Details
Patch (revised Skipped and test_expectations.txt) (26.02 KB, patch) 2011-10-26 02:55 PDT, Kenichi Ishibashi	no flags	Details Formatted Diff Diff
Patch (38.13 KB, patch) 2011-11-01 23:43 PDT, Kenichi Ishibashi	no flags	Details Formatted Diff Diff
Patch (29.18 KB, patch) 2011-11-06 21:30 PST, Kenichi Ishibashi	no flags	Details Formatted Diff Diff
Patch (29.25 KB, patch) 2011-11-28 17:37 PST, Kenichi Ishibashi	no flags	Details Formatted Diff Diff
Patch(revised to ToT) (29.15 KB, patch) 2012-01-06 00:27 PST, Kenichi Ishibashi	no flags	Details Formatted Diff Diff
Patch(fix compile failure) (29.13 KB, patch) 2012-01-06 00:53 PST, Kenichi Ishibashi	no flags	Details Formatted Diff Diff
Patch(just rebased) (29.16 KB, patch) 2012-02-08 16:49 PST, Kenichi Ishibashi	no flags	Details Formatted Diff Diff
Patch for landing (29.09 KB, patch) 2012-02-12 16:24 PST, Kenichi Ishibashi	no flags	Details Formatted Diff Diff
Show Obsolete (10) View All Add attachment proposed patch, testcase, etc.

Patryk Zawadzki

Comment 1 2010-01-25 15:53:30 PST

This happens both in GTK port and current Chromium snapshots. No idea about other ports.

Yusuke Sato

Comment 2 2010-02-09 22:43:17 PST

Chromium bug: http://code.google.com/p/chromium/issues/detail?id=31833

Yusuke Sato

Comment 3 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> abcde abcde abcde abcde abcde </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, ... 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.

Yusuke Sato

Comment 4 2010-02-10 01:38:59 PST

Created attachment 48475 [details] bold_italic_font_face_v1

Kent Tamura

Comment 5 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?

mitz

Comment 6 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.

Yusuke Sato

Comment 7 2010-02-15 21:09:53 PST

Created attachment 48787 [details] bold_italic_font_face_v2

Yusuke Sato

Comment 8 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.

Kent Tamura

Comment 9 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.

Adam Barth

Comment 10 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.

Dave Hyatt

Comment 11 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.

Yusuke Sato

Comment 12 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.

Kenichi Ishibashi

Comment 13 2011-10-26 00:47:26 PDT

Created attachment 112461 [details] Patch

Kenichi Ishibashi

Comment 14 2011-10-26 00:49:15 PDT

Created attachment 112462 [details] test expectation1

Kenichi Ishibashi

Comment 15 2011-10-26 00:49:58 PDT

Created attachment 112463 [details] test expectation2

Kenichi Ishibashi

Comment 16 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?

Kenichi Ishibashi

Comment 17 2011-10-26 02:55:26 PDT

Created attachment 112476 [details] Patch (revised Skipped and test_expectations.txt)

Luke Macpherson

Comment 18 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.

Kenichi Ishibashi

Comment 19 2011-11-01 23:43:27 PDT

Created attachment 113279 [details] Patch

Kenichi Ishibashi

Comment 20 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.

WebKit Review Bot

Comment 21 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

Kenichi Ishibashi

Comment 22 2011-11-06 21:30:37 PST

Created attachment 113822 [details] Patch

Kenichi Ishibashi

Comment 23 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.

Kenichi Ishibashi

Comment 24 2011-11-28 17:37:30 PST

Created attachment 116856 [details] Patch

Kenichi Ishibashi

Comment 25 2011-12-05 18:36:44 PST

(In reply to comment #24) > Created an attachment (id=116856) [details] > Patch Ping, mitz?

Kenichi Ishibashi

Comment 26 2012-01-06 00:27:47 PST

Created attachment 121407 [details] Patch(revised to ToT)

Kenichi Ishibashi

Comment 27 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,

Early Warning System Bot

Comment 28 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

Gyuyoung Kim

Comment 29 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

Kenichi Ishibashi

Comment 30 2012-01-06 00:53:40 PST

Created attachment 121409 [details] Patch(fix compile failure)

Kenichi Ishibashi

Comment 31 2012-02-08 16:49:21 PST

Created attachment 126189 [details] Patch(just rebased)

Kenichi Ishibashi

Comment 32 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)

mitz

Comment 33 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.

Kenichi Ishibashi

Comment 34 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.

Kenichi Ishibashi

Comment 35 2012-02-12 16:24:34 PST

Created attachment 126688 [details] Patch for landing

WebKit Review Bot

Comment 36 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>

WebKit Review Bot

Comment 37 2012-02-12 16:43:52 PST

All reviewed patches have been landed. Closing bug.

mitz

Comment 38 2012-02-22 11:05:58 PST

This broke fast/css/font-face-default-font.html. See bug 79260.

Note You need to log in before you can comment on or make changes to this bug.