Bug 6484

Summary:

font-weight does not properly support graded weights

Product:

WebKit

Reporter:

Oliver Hunt <oliver>

Component:

Text

Assignee:

mitz

Status:

RESOLVED FIXED

Severity:

Normal

CC:

ap, bugmail, darwin, ian, mitz, nickshanks, phiw2

Priority:

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

Attachments:

Description	Flags
Attaching previously mentioned testcase	none
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.	none
Sample rendering of expanded test page	none
patch v 0.9	none
patch v 1.0	none
patch v 1.0 alternative	none
patch v 1.0.1	none
demonstration of choices with Helvetica Neue	none
Updated patch	none
Updated patch	hyatt: review+

Oliver Hunt

Reported 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

Attachments
Attaching previously mentioned testcase (449 bytes, text/html) 2006-11-24 19:12 PST, Alex Taylor	no flags	Details
Adds numerical and basic keyword support for graded font-weights (18.80 KB, patch) 2006-11-25 13:29 PST, Alex Taylor	oliver: review-	Details Formatted Diff Diff
nickshanks' work-in-progress patch (38.47 KB, patch) 2007-01-31 10:56 PST, Nicholas Shanks	darin: review-	Details Formatted Diff Diff
Expanded test case. Uses fonts which may not be on all systems. (10.70 KB, text/html) 2007-01-31 16:18 PST, Nicholas Shanks	no flags	Details
Sample rendering of expanded test page (718.84 KB, image/pdf) 2007-01-31 16:21 PST, Nicholas Shanks	no flags	Details
patch v 0.9 (44.68 KB, patch) 2007-02-02 11:12 PST, Nicholas Shanks	no flags	Details Formatted Diff Diff
patch v 1.0 (191.97 KB, patch) 2007-02-05 11:08 PST, Nicholas Shanks	no flags	Details Formatted Diff Diff
patch v 1.0 alternative (191.69 KB, patch) 2007-02-05 11:52 PST, Nicholas Shanks	no flags	Details Formatted Diff Diff
patch v 1.0.1 (192.27 KB, patch) 2007-02-06 02:46 PST, Nicholas Shanks	no flags	Details Formatted Diff Diff
demonstration of choices with Helvetica Neue (3.15 KB, text/html) 2007-02-06 07:06 PST, Nicholas Shanks	no flags	Details
Updated patch (80.88 KB, patch) 2008-04-02 14:12 PDT, mitz	no flags	Details Formatted Diff Diff
Updated patch (82.37 KB, patch) 2008-04-03 00:47 PDT, mitz	hyatt: review+	Details Formatted Diff Diff
Show Obsolete (7) View All Add attachment proposed patch, testcase, etc.

Maciej Stachowiak

Comment 1 2006-03-31 15:10:12 PST

This applies to HTML as well as SVG.

Alexey Proskuryakov

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

Mike Bremford

Comment 3 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. <html> <head><style>p { font:"Helvetica Neue"; margin:0 } </style> </head><body> 100 200 300 400 500 600 700 800 900 </body></html>

Alex Taylor

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

Alex Taylor

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

Oliver Hunt

Comment 6 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? Wha? + + // Discourage Extra Black weight fonts + if (candidateWeight > 10) + return NO; Is there a reason for -#define STANDARD_WEIGHT 5 -#define MIN_BOLD_WEIGHT 9 -#define STANDARD_BOLD_WEIGHT 10 +#define STANDARD_WEIGHT 4 +#define MIN_BOLD_WEIGHT 6 +#define STANDARD_BOLD_WEIGHT 8 Could this change rendering of many pages? Have you run the layout tests? That'll be an absolute necessity for this patch.

Alex Taylor

Comment 7 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_BOLD_WEIGHT 10 > +#define STANDARD_WEIGHT 4 > +#define MIN_BOLD_WEIGHT 6 > +#define STANDARD_BOLD_WEIGHT 8 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.

Alex Taylor

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

Nicholas Shanks

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

Nicholas Shanks

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

Nicholas Shanks

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

Nicholas Shanks

Comment 12 2007-01-31 16:18:46 PST

Created attachment 12838 [details] Expanded test case. Uses fonts which may not be on all systems.

Nicholas Shanks

Comment 13 2007-01-31 16:21:49 PST

Created attachment 12839 [details] Sample rendering of expanded test page

Darin Adler

Comment 14 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: +#if PLATFORM(MAC) + 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. + case CSS_PROP_FONT_STRETCH: + { 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. +#if PLATFORM(MAC) +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.

Dave Hyatt

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

Nicholas Shanks

Comment 16 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 Issues: 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).

Nicholas Shanks

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

Nicholas Shanks

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

Nicholas Shanks

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

Adele Peterson

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

Nicholas Shanks

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

Nicholas Shanks

Comment 22 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; } A condensed run with bolder text.  A bold condensed run with lighter text.  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.

Nicholas Shanks

Comment 23 2007-02-06 07:06:34 PST

Created attachment 12970 [details] demonstration of choices with Helvetica Neue

Dave Hyatt

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

Greg K.

Comment 25 2007-04-17 01:30:04 PDT

Will this patch also utilize the AAT weight variations of Skia?

Nicholas Shanks

Comment 26 2007-04-17 03:12:39 PDT

No, that feature is listed seperatly as bug 5168.

mitz

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

mitz

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

Philippe Wittenbergh

Comment 29 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 https://bugzilla.mozilla.org/show_bug.cgi?id=420981#c3

mitz

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

mitz

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

Nicholas Shanks

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

Dave Hyatt

Comment 33 2008-04-03 11:40:12 PDT

Comment on attachment 20309 [details] Updated patch r=me. Make sure to file a followup bug (if one does not exist already) about doing better bolder/lighter support.

mitz

Comment 34 2008-04-03 20:48:50 PDT

Landed in <http://trac.webkit.org/projects/webkit/changeset/31620>.

Nicholas Shanks

Comment 35 2008-08-27 09:27:27 PDT

follow-up bug #18323 filed

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