Bug 35583 - if font-family lists several generic families, the last one takes precedence
Summary: if font-family lists several generic families, the last one takes precedence
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Linux
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-03-02 09:05 PST by Olivier Tilloy
Modified: 2010-06-10 21:13 PDT (History)
5 users (show)

See Also:


Attachments
layout test case (847 bytes, text/html)
2010-03-02 09:22 PST, Olivier Tilloy
no flags Details
[patch] set the generic family only once (2.31 KB, patch)
2010-03-02 09:39 PST, Olivier Tilloy
no flags Details | Formatted Diff | Diff
Updated patch with Changelog entry (3.02 KB, patch)
2010-03-03 10:14 PST, Olivier Tilloy
darin: review-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Olivier Tilloy 2010-03-02 09:05:58 PST
According to the CSS fonts specification (http://www.w3.org/TR/css3-fonts/#font-family-the-font-family-property), "[the font-family property] specifies a prioritized list of font family names or generic family names", and "authors are encouraged to append a generic font family as a last alternative for improved robustness".

It doesn't make much sense to list several generic families in a font-family property, since all generic font families are guaranteed to be always available by the user agent. However if one does, the current behaviour of webkit is to override the fallback family with the last generic family encountered in the list.
Consider the following example:

    font-family: ThisFontIsInvalid, monospace, sans-serif

If the first font family is not found on the system, the font selected should be the UA's default monospace font. Instead, it is the default sans-serif font.

A real life example is at http://svn.debian.org/wsvn/secure-testing/doc/narrative_introduction.
The text of the page is intended to be displayed with a fixed width, and the the style is defined like this:

    font-family:'Consolas',monospace,sans-serif;

(in http://svn.debian.org/websvn/templates/calm/styles.css)
Comment 1 Olivier Tilloy 2010-03-02 09:22:50 PST
Created attachment 49808 [details]
layout test case

This is a layout test case that highlights the issue.
I used the offsetWidth property of inline <p> elements to determine whether the font used for the two paragraphs is the same, if there is a more elegant solution it is welcome.
I guess this test could go in LayoutTests/fast/css.
Comment 2 Olivier Tilloy 2010-03-02 09:39:30 PST
Created attachment 49810 [details]
[patch] set the generic family only once

This patch fixes the issue by setting the generic family only once.
If another generic family is encountered in the list, setGenericFamily() is not called again.
Comment 3 Alexey Proskuryakov 2010-03-02 16:47:01 PST
The test says "PASS" for me. I was testing in Safari on Mac OS X.

OIf the behavior is indeed correct in some ports, it's likely that a proper fix wouldn't need to touch cross-platform code.

Please see <https://webkit.org/coding/contributing.html> for detailed information about contributing code to WebKit - we do require creating a single patch with tests and ChangeLogs.
Comment 4 Olivier Tilloy 2010-03-03 00:59:16 PST
Thanks for your feedback Alexey.
It looks like the bug I'm trying to fix is indeed platform-specific. Since I could reproduce it on the GTK and QT ports on linux, I assumed a bit too fast it was general.

Still, I believe the first part of my analysis and the patch are valid, as far as I can interpret the specification and understand the CSSStyleSelector code correctly. I can't think of an obvious layout test for it though.

If anyone who knows better this part of the code could take a look into it, it would be much appreciated! If my change makes sense I'll gladly generate a suitable patch with the Changelog.


As far as the layout test I attached is concerned, my guess is that ports that do the right thing (like the mac one) iterate over the list of families, discarding the first one as invalid and falling back on the second one ("-webkit-monospace"), whereas the GTK port seems to always generate a font from the first family, using the family name and falling back to the font description's generic family (which explains why this patch seems to fix it).
I will file a separate bug for the GTK port.
Comment 5 Alexey Proskuryakov 2010-03-03 09:54:32 PST
> If anyone who knows better this part of the code could take a look into it, it
> would be much appreciated!

A bug without any patches marked for review has pretty low visibility. Please consider either submitting a patch for review (as documented at https://webkit.org/coding/contributing.html), or e-mailing webkit-dev.
Comment 6 Olivier Tilloy 2010-03-03 10:14:05 PST
Created attachment 49919 [details]
Updated patch with Changelog entry
Comment 7 Darin Adler 2010-03-03 10:18:51 PST
Comment on attachment 49919 [details]
Updated patch with Changelog entry

Even if you can't test with DumpRenderTree cross-platform you can do the following:

    1) Add a DumpRenderTree test that requires per-platform results.

    2) Create a manual test with instructions for how to interpret the results and put it in WebCore/manual-tests.

    3) Add some new feature to DumpRenderTree to make it possible to test this across platforms.

As another possibility, is there perhaps some way to detect which family is used with computed style?

It's not OK to make the change without a test at all, so you need to do at least one of the things above.
Comment 8 Olivier Tilloy 2010-03-03 10:29:05 PST
(In reply to comment #7)
> (From update of attachment 49919 [details])
> Even if you can't test with DumpRenderTree cross-platform you can do the
> following:
> 
>     1) Add a DumpRenderTree test that requires per-platform results.
> 
>     2) Create a manual test with instructions for how to interpret the results
> and put it in WebCore/manual-tests.
> 
>     3) Add some new feature to DumpRenderTree to make it possible to test this
> across platforms.

#3 looks like the best option, but also the most complex to implement (given my
current knowledge of the code). Also, wouldn't changing the output of
DumpRenderTree break a lot of existing tests?

> As another possibility, is there perhaps some way to detect which family is
> used with computed style?

No, I already tried that, but the computed style only contains an ordered list
of font families as encountered by the CSS parser, no hint on which of them is
actually used.
The fact that webkit chooses to remember only one generic family instead of a
list of them is an implementation detail that's unknown to the CSS specification.
Comment 9 mitz 2010-03-03 11:40:14 PST
So I noticed that

platform/graphics/chromium/FontCacheChromiumWin.cpp
platform/graphics/chromium/FontCacheLinux.cpp
platform/graphics/gtk/FontPlatformDataGtk.cpp
platform/graphics/gtk/FontPlatformDataPango.cpp
platform/graphics/wx/FontPlatformDataWx.cpp

are the only files in platform/graphics that refer to the generic family in the font description. The Mac, Windows and Qt implementations don’t do this. I think this is the problem. The mapping from generic family to concrete family happes through the -webkit-* family names (e.g. -webkit-serif) and the Settings object in CSSFontSelector. I don’t know why the aforementioned ports don’t take advantage of that and do platform-level mapping.