Bug 6484

Summary: font-weight does not properly support graded weights
Product: WebKit Reporter: Oliver Hunt <oliver>
Component: TextAssignee: mitz
Severity: Normal CC: ap, bugmail, darwin, ian, mitz, nickshanks, phiw2
Priority: P2    
Version: 420+   
Hardware: Mac   
OS: OS X 10.4   
URL: http://www.w3.org/Graphics/SVG/Test/20030813/htmlframe/full-text-fonts-02-t.html
Bug Depends on:    
Bug Blocks: 18306    
Description Flags
Attaching previously mentioned testcase
Adds numerical and basic keyword support for graded font-weights
oliver: review-
nickshanks' work-in-progress patch
darin: review-
Expanded test case. Uses fonts which may not be on all systems.
Sample rendering of expanded test page
patch v 0.9
patch v 1.0
patch v 1.0 alternative
patch v 1.0.1
demonstration of choices with Helvetica Neue
Updated patch
Updated patch hyatt: review+

Description Oliver Hunt 2006-01-10 19:08:31 PST
Intensities of font-weight seem unnecessarily discontinuous (only two apparent levels in referenced test 
case).  Test output in this test case seems to have little variation in weights -- see numbers 100-900 on 
the right of svg image
Comment 1 Maciej Stachowiak 2006-03-31 15:10:12 PST
This applies to HTML as well as SVG.
Comment 2 Alexey Proskuryakov 2006-04-01 01:16:12 PST
Isn't this expected behavior? Times and Times New Roman only have two weights (regular and bold), and the SVG expected result also shows two font variations.
Comment 3 Mike Bremford 2006-08-09 03:19:33 PDT
This happens with "Helvetica Neue" as well. Weights 100 to 500 are regular and 600 to 900 are bold, even though the font has an Ultralight, Light, Regular and Bold weighting.

<head><style>p { font:"Helvetica Neue"; margin:0 } </style>
<p style="font-weight:100">100</p>
<p style="font-weight:200">200</p>
<p style="font-weight:300">300</p>
<p style="font-weight:400">400</p>
<p style="font-weight:500">500</p>
<p style="font-weight:600">600</p>
<p style="font-weight:700">700</p>
<p style="font-weight:800">800</p>
<p style="font-weight:900">900</p>
Comment 4 Alex Taylor 2006-11-24 19:12:27 PST
Created attachment 11627 [details]
Attaching previously mentioned testcase

Test case using Helvetica Neue which has 4 standard weights.
Comment 5 Alex Taylor 2006-11-25 13:29:50 PST
Created attachment 11629 [details]
Adds numerical and basic keyword support for graded font-weights

This patch adds support for font-weights 100 through 900 to map to the weight in font-faces. Keyword support (bolder, lighter) is only basic and only does numerical adjustments whereas the spec suggests that lighter should map to a font-variant lighter and bolder should map to a font-variant bolder.
However it also says that behavious in is not guaranteed and the only guarantee is that "bolder" will result in a face that is "not lighter than the current" and vice-versa for "lighter".

LayoutTests/css2.1/t1506-c525-font-wt-00-b.html is already a testcase for this behaviour, the only better behaviour would be to have it render with a font-family with more weight-variants (like Helvetica Neue) and it would then the fix would be apparent in the pixel-test case.
Comment 6 Oliver Hunt 2006-11-25 13:56:10 PST
Comment on attachment 11629 [details]
Adds numerical and basic keyword support for graded font-weights

I don't like the VAL_BOLDER handling -- possibly talk to hyatt.

Reorder the switch cases, so that they're in order

Why does FontDescription: :m_weight become signed?  You lose a bit of precision for very little gain.  Why do you use -1 to indicate badness -- why not 0?

+    // Discourage Extra Black weight fonts
+    if (candidateWeight > 10)
+        return NO;

Is there a reason for 
-#define MIN_BOLD_WEIGHT 9
+#define MIN_BOLD_WEIGHT 6

Could this change rendering of many pages?

Have you run the layout tests? 
That'll be an absolute necessity for this patch.
Comment 7 Alex Taylor 2006-11-25 20:19:42 PST
(In reply to comment #6)
> I don't like the VAL_BOLDER handling -- possibly talk to hyatt.
Indeed, it is gross, I'll talk to hyatt about implementing "bolder" and "lighter" properly.

> Reorder the switch cases, so that they're in order
Done, I'll upload a new patch once I've had a chat to hyatt about the keywords

> Why does FontDescription: :m_weight become signed?  You lose a bit of precision
> for very little gain.  Why do you use -1 to indicate badness -- why not 0?

As discussed in IRC, more code was already written referencing weight as an int (in WebFontCache) and as the values are always lower than 15 (1 - 14 in AppKit and using 1 - 9 for css) there isn't much difference. Can change it if need be. The -1 to indicate badness was to match the text size case in the same line. It uses 0xFFFFFFFFU to indicate "deleted" and 0 to indicate "empty" so I used -1 to indicate deleted and 0 to indicate empty. Not sure if it actually matters at all though.

> Wha?
> +
> +    // Discourage Extra Black weight fonts
> +    if (candidateWeight > 10)
> +        return NO;

The mapping between NSFont weights (1-14) maps to CSS font weights such that an Font's Regular weight is usually "5" (from the fonts properties) and in css, normal is defined as 4 (or 400 to be specific). Also, a Font's Bold face from the font properties is usually weight 9 which needs to map to CSS's 7 (= 700). The mapping I used was to scale the NSFont's weight by 9/12 to match them up. This leaves "Extra Black" variant fonts (which have a font weight of around 14) mapping to 10/11. 

CSS2.1 says that a user-agent can decide not to map these extreme values. Those two lines simply mean that an Extra Black font (which would be off the CSS weight scale (10/11 is > 9) and as such, are not ever returned as a "better choice" than the previous candidate. This is similar to the Unwanted Traits matching except there isn't an ExtraBlack font trait so I rely on the weight being very large.

> Is there a reason for 
> -#define STANDARD_WEIGHT 5
> -#define MIN_BOLD_WEIGHT 9
> +#define STANDARD_WEIGHT 4
> +#define MIN_BOLD_WEIGHT 6
These values were artibutary and not used in the destination function, they now use the same scale as the weight values used elsewhere, 4 = 400 in css etc.

> Could this change rendering of many pages?
Not really. Before, font faces were either "normal" or "bold" and these constants were never taken into account for that, only the NSFontBold trait was.

> Have you run the layout tests? 
> That'll be an absolute necessity for this patch.
There weren't any standard layout test regressions, hard to tell from the svg pixel tests, no more test failed than are currently failing in buildbot though.
Comment 8 Alex Taylor 2006-11-26 01:30:17 PST
From talk with dhyatt on IRC:
* Mapping between NSFont weights and CSS weights needs to be more specific. 
* We should probably try and remove "isBold" functions because that question is rather more complex now.
* The arbituary VAR_BOLDER = weight + 3 hack would be acceptable (to him) for this patch with a followup bug filed for improvements.
* Extra Black fonts should be a lower preference but not excluded
* A big fat LayoutTest would be required:
  > dhyatt - "and we'll want lots of fun tests using some of these  fonts like Baskerville, Gill Sans, and some with the super heavy or ultralight variants"
Comment 9 Nicholas Shanks 2007-01-31 06:08:26 PST
I have just been pointed to this bug by MacDome. I have an independently written patch which looks almost identical, though I use values 1-14 for weight, and convert the CSS property values to these when reading the CSS. Ironically I also used a +3 hack for bolder (and -2 for lighter) :-)
In addition I removed the isBold/setBold functions, and all references to NSFontBoldMask trait now use fontDescription.weight() instead, so I am glad to see hyatt has already okayed these changes.

One thing I will steal from Alex's patch is the matching down for lighter and up for heavier weights if two closest matches are equidistant from the desired weight, something that I didn't like the look of when I saw it in the code.

I will review Oliver's comments and Alex's responses to his patch before posting mine, but you can get a taste from http://web.nickshanks.com/font-weight2.png
Comment 10 Nicholas Shanks 2007-01-31 10:56:35 PST
Created attachment 12829 [details]
nickshanks' work-in-progress patch

This is my working diff, submitted for comment. It does not attempt to fix CSSComputedStyleDeclaration.cpp nor WebHTMLView.mm correctly yet (which were already marked FIXME). Please ignore these issues for the time being.

As noted in my previous comment, it uses a fourteen-point graded weight system on Mac to allow ease of comparison to NSFontManager values. cssstyleselector.cpp contains the conversion values. Qt keeps the existing system, and other platforms get a 9-point one based on CSS.

The CSS_PROP_FONT_WEIGHT and _STRETCH switch statements are so ordered for speed reasons in common case usage. Use of cDemiBoldWeight as a value for minimum weight considered '"bold" is a temporary hack.

Also includes changes submitted as bug #12481 and bug #12482 because it would be hassle to remove them from my tree.

I have not ran existing regression tests yet, as I expect quite a few will require new expected results. New tests and a changelog are forthcoming. Please don't r- the patch until several people have had a chance to see it and comment.
Comment 11 Nicholas Shanks 2007-01-31 11:05:02 PST
The patch I just attached had a logic bug in narrowerStretch() and widerStretch() which I have fixed locally.
Comment 12 Nicholas Shanks 2007-01-31 16:18:46 PST
Created attachment 12838 [details]
Expanded test case. Uses fonts which may not be on all systems.
Comment 13 Nicholas Shanks 2007-01-31 16:21:49 PST
Created attachment 12839 [details]
Sample rendering of expanded test page
Comment 14 Darin Adler 2007-01-31 21:14:35 PST
Comment on attachment 12829 [details]
nickshanks' work-in-progress patch

Thanks for tackling this. The most important issue is testing and not breaking websites. This slightly more advanced capability is not nearly as important as working properly with existing websites.

Here's a quick pass of review:

+                    case CSS_VAL_BOLDER:
+                        fontDescription.setWeight(fontDescription.bolderWeight());
+                        break;

Why is this code in #if PLATFORM_MAC? I think the platform-specific code should lower level.

+	{

Brace should be on same line with property name.

+			switch (primitiveValue->getIdent()) {

Incorrect indenting here using tabs. Need to indent with spaces. I saw tabs elsewhere.

+    FontPlatformDataCacheKey key(familyName, fontDescription.computedPixelSize(), fontDescription.weight(),  fontDescription.stretch(), fontDescription.italic());

Extra space here.

+const unsigned cUltraLightWeight = 1;  // CSS 100

This is not the way this should work. The whole idea of these classes is to make a cross-platform abstraction. Instead, the platform-specific code should go a bit lower level.

+    // FIXME: Does not necessarilly return the immediate next weight up or down.
+    //  The font cache will return the lowest weight if the two closest available weights are equidistant from the
+    //	requested weight, potentially the same weight as was used before. Using +3/-2 decreases the liklyhood of that.
+    unsigned bolderWeight() const { return (m_weight +3 < cUltraBoldWeight)? m_weight +3 : cUltraBoldWeight; }
+    unsigned lighterWeight() const { return (m_weight -2 > cUltraLightWeight)? m_weight -2 : cUltraLightWeight; }

This code does not belong in a header. Please don't make it all inlined like this. Also some typos here.

+        unsigned actualWeight = (unsigned) [fontManager weightOfFont:substituteFont];

Please use static_cast, not C-style cast.

+                                       (actualWeight +3) < font.fontDescription().weight(),

We use spaces before and after operators like "+".

This code needs a comment. The magic number +3 makes no sense.

+    desiredTraits &= ~NSBoldFontMask;	// NOTE: you shouldn't be using this trait anymore; make sure you don't pass it in

Who is "you" here? I think this should just be an assertion. This is not general purpose code, so it doens't have to be passed in.

You made a change to the small caps rule. We need a test case that covers that.

Does this affect any layout test results? Which ones?

+    // FIXME: cannot represent weights accuratly
+    // FIXME: cannot represent stretches accuratly

How can these be fixed? FIXME is supposed to be for bugs that can be fixed.

We'll need a test case that uses default-installed fonts if possible. We may need to rig a test mode where some fonts are intentionally installed so we can regression-test this properly.
Comment 15 Dave Hyatt 2007-01-31 21:25:39 PST
Comment on attachment 12829 [details]
nickshanks' work-in-progress patch

Please break font-stretch out into its own patch.  Weights and stretch are unrelated and should be the subject of two separate bugs.
Comment 16 Nicholas Shanks 2007-02-02 11:12:39 PST
Created attachment 12882 [details]
patch v 0.9

working patch for feedback. addresses every issue mentioned on either my or alex's patches.
does not include tests or changelog yet

1) I am not sure if all my "using namespace" and "@namespace" changes are necessary
2) The FontManager <=> CSS weight conversion table needs tweaking (or something else does) since "normal" gives a Light weight and "bold" gives an Extra Bold weight. This is not considered significant for the purposes of the review request.
3) Does not assert on NSBoldFontMask being passed in. Trait is ignored anyway, and no longer used elsewhere so I don't see it necessary to assert (other ignored traits don't assert, for example).
Comment 17 Nicholas Shanks 2007-02-02 12:39:46 PST
I have fixed issue (2) above. It will be in the final patch when that gets submitted. I am going to be away from now until monday so there's no rush.
Comment 18 Nicholas Shanks 2007-02-05 11:08:26 PST
Created attachment 12937 [details]
patch v 1.0

Fixes previously mentioned issue (2) above; adds test case and change log. Would like this patch to get into trunk before it's locked, so review by hyatt or darin welcomed.
Comment 19 Nicholas Shanks 2007-02-05 11:52:12 PST
Created attachment 12939 [details]
patch v 1.0 alternative

a version of the above patch with new switch statements using non-indented cases, for the sadists amongst you. Existing switch statements unmodified.
Comment 20 Adele Peterson 2007-02-05 16:51:52 PST
Comment on attachment 12937 [details]
patch v 1.0

I'd like to see some more explanation in the ChangeLog
Comment 21 Nicholas Shanks 2007-02-06 02:46:59 PST
Created attachment 12965 [details]
patch v 1.0.1

per adele's request, more verbose changelog, includes bug id
Comment 22 Nicholas Shanks 2007-02-06 07:05:12 PST
John Sullivan raised concerns that this patch might 'introduce' traits, which it can do, however WebKit has no knowledge of 'introduced' traits, only a desired result and an algorithm to find the closest match. Consider this, bearing in mind that Helvetica has no Condensed face of regular weight, so #a will be rendered with the Regular face:

p { font-family: 'Helvetica Neue'; }
#a { font-weight: normal; font-stretch: condensed; }
#a span { font-weight: bold; }
#b { font-weight: bold; font-stretch: condensed; }
#b span { font-weight: normal; }

<p id="a">A condensed run with <span>bolder</span> text.</p>
<!-- rendered as Regular with a Bold Condensed span -->
<p id="b">A bold condensed run with <span>lighter</span> text.</p>
<!-- rendered as Bold Condensed with a Regular span -->

When it encounters the span, should WebKit choose Regular or Bold Condensed? If it choses Regular, then it 'introduces' a wider stretch into #b where there was none before; if it choses Bold Condensed, then it introduces a condensed stretch into #a where none was being used before (because Regular was used in the absence of Condensed). in other words, both choices require the introduction of two properties into the rendered text.

The only solution I see to this is for new computed styles to be based on the actual font used for the parent, rather than the computed style of the parent. This contravenes the CSS specifications, but would mean '#a span' used the regular-stretch Bold face despite having font-stretch: condensed and there being a Bold Condensed face available. In this case it would only embolden the text.

I conclude that without re-designing large parts of the font selection process and breaking the CSS specs in the process, introducing styles is inevitable when there is a mis-match between available faces and desired result (and where synthesizing the missing styles is not possible, such as for lighter weights).

This patch chooses to prefer font-weight over other properties because weight has the most visual impact, and because font-stretch is a likely candidate for future synthesis (allowing Light + synth Condensed to be preferred over Condensed of a heavier weight with no synth'd properties, for example). In the above example I gave, '#a' and '#b span' would both get a synthetic condensed based on the regular weight, and no properties would be introduced to either.

I hope I have proved to sufficient extent that there is no simple solution without compromises, and justified my choices in which compromises to make with a forward-thinking viewpoint.
I have attached a demonstration page showing the above example, hopefully it will help if you don't understand.
Comment 23 Nicholas Shanks 2007-02-06 07:06:34 PST
Created attachment 12970 [details]
demonstration of choices with Helvetica Neue
Comment 24 Dave Hyatt 2007-02-07 02:17:01 PST
Comment on attachment 12965 [details]
patch v 1.0.1

This constitutes a feature, and we're not accepting any new features at this time.  Once the tree is open again, this can be reflagged and looked at again.

Clearing the bit rather than minusing.
Comment 25 Greg K. 2007-04-17 01:30:04 PDT
Will this patch also utilize the AAT weight variations of Skia?
Comment 26 Nicholas Shanks 2007-04-17 03:12:39 PDT
No, that feature is listed seperatly as bug 5168.
Comment 27 mitz 2008-04-02 14:12:43 PDT
Created attachment 20298 [details]
Updated patch

Updated for TOT and Windows. Some notable problems with testing:
- AppKit gives Helvetica Neue Light and Ultralight the same weight.
- Windows considers Helvetica Neue Light a distinct family.
Comment 28 mitz 2008-04-02 16:14:41 PDT
Comment on attachment 20298 [details]
Updated patch

I would like to get this reviewed while I test it and think of way to test automatically despite the aforementioned limitations.
Comment 29 Philippe Wittenbergh 2008-04-02 17:10:13 PDT
(In reply to comment #27)

> - AppKit gives Helvetica Neue Light and Ultralight the same weight.

There is a Apple bug 5781372 on that.
See this Gecko bug
Comment 30 mitz 2008-04-02 21:45:43 PDT
Comment on attachment 20298 [details]
Updated patch

I think I didn't get the logic in matchImprovingEnumProc completely right: it should also rule out a candidate that adds undesird italics.
Comment 31 mitz 2008-04-03 00:47:38 PDT
Created attachment 20309 [details]
Updated patch

Updated to work around <rdar://problem/5781372> and fixed the handling of undesired italics on Windows.
Comment 32 Nicholas Shanks 2008-04-03 01:21:59 PDT
Mitz: I think you should take ownership of this bug now, as I haven't the time to work on it.
Comment 33 Dave Hyatt 2008-04-03 11:40:12 PDT
Comment on attachment 20309 [details]
Updated patch


Make sure to file a followup bug (if one does not exist already) about doing better bolder/lighter support.
Comment 34 mitz 2008-04-03 20:48:50 PDT
Landed in <http://trac.webkit.org/projects/webkit/changeset/31620>.
Comment 35 Nicholas Shanks 2008-08-27 09:27:27 PDT
follow-up bug #18323 filed