93725 - 'bolder' and 'lighter' keywords of 'font-weight' don't compound correctly

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect, P3)

Description

[Hixie (not reading bugmail)]

The 'bolder' and 'lighter' keywords don't always work. They are supposed to
guarentee that we pick a lighter or bolder font-weight, but we don't always. 

See: http://www.hixie.ch/tests/adhoc/css/fonts/weight/002.xml for an example.

Tested using Netscape Commercial Trunk build 2001080110 on Windows 2000.

See also bug 77882.

Comment 1

[Hixie (not reading bugmail)]

Note that Opera pass that test (or rather, an HTML equivalent) perfectly.

Comment 2

[Kevin McCluskey (gone)]

Moving to mozilla1.1. Engineers are overloaded!

Comment 3

[Kevin McCluskey (gone)]

Bulk moving from Moz1.1 to future-P1. I will pull from this list when scheduling
work post Mozilla1.0.

Comment 4

[Madhur Bhatia]

cc'ing myself

Comment 5

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Assigning pierre's remaining Style System-related bugs to myself.

Comment 6

[Greg K.]

Reconfirmed using FizzillaCFM/2002070913. Setting All/All.

Comment 7

[Boris Zbarsky [:bzbarsky]]

*** Bug 223550 has been marked as a duplicate of this bug. ***

Comment 8

[Daniel Glazman (:glazou) (not active in Mozilla any more)]

I still see this bug in a FF3 nightly on linux and a xulrunner build.

Comment 9

[Zack Weinberg (:zwol)]

I still see this bug with the very latest FF3 tree on Linux [Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:1.9pre) Gecko/2008050719 Minefield/3.0pre].

Comment 10

[Zack Weinberg (:zwol)]

Attachment: possible patch for this and #77882

This proved to be pretty simple to fix, with some caveats.  The attached patch passes Hixie's test linked from here, + its own reftests, and doesn't break any reftests.

Caveat #1: Thebes does not implement nsIFontMetrics::GetFontHandle, which is the obvious way to tell whether a change to the .weight field of an nsFont has actually resulted in the selection of a different font.  The code in this patch is instead relying on the assumption that changing font weight is likely to change the font's max-advance metric.  In local testing this works great for proportional fonts, but not for monospace fonts (naturally enough) -- this is still an improvement on the present state, but I would be grateful for better ideas.

Caveat #2: The reftests included in this patch are weaker than Hixie's original.    I see no way to make them stronger without relying on a particular font being installed on the testing system, but they did miss a bug in my first attempt at a fix, that Hixie's test caught.

Caveat #3: contra the previous code, I make no use whatsoever of weight values that are not multiples of 100.  This is consistent with my reading of CSS2.1 but not consistent with the discussion in bug 77882.  I put more detailed notes there - see especially my (bug 77882 comment 7) and Hixie's (bug 77882 comment 2).  I'm inclined to say that this caveat, unlike possibly the other two, should not block this bug; rather, if non-multiple-of-100 weights are sometimes appropriate, we can leave bug 77882 open after this lands and fix it there.

Comment 11

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I think caveat #1 is a problem, and you need to expose something from Thebes rather than relying on the max-advance trick.

Comment 12

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

That said, the other thing I'm not sure about is really a spec question.  Consider the following
  <div style="font-weight: bold"><div style="font-weight: lighter">京B</div></div>
and suppose that we further have:
  div { font-family: Bookman, MS Song; }
Now, Bookman has a weight between 400 and 700, but MS Song (let's presume) does not.  Should the 京 be weight 400 or 700?  (Note that I'm not sure whether Bookman Demi is 500 or 600, but I'm guessing that it's 600, which, when absent, should be rounded to 700; if it's actually 500, then replace "bold" above with "normal" and "lighter" above with "bolder".)

In other words, is the lighter / bolder supposed to be applied element-by-element or character-by-character?

Comment 13

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I would further note that I was under the impression that this bug was already fixed on at least some platforms, in the font selection code.

Comment 14

[Zack Weinberg (:zwol)]

Your character-by-character/element-by-element example gets at a deeper problem - every value coming from a Thebes nsIFontMetrics is only true for the first font in the underlying font group.

This being so, I suppose I can just expose mFontGroup->GetFontAt(0) as nsThebesFontMetrics' version of GetFontHandle().  It wouldn't be any more wrong than what we're already doing.

I don't see how this could be fixed in font selection - by the time we get there the request for lighter/bolder has become indistinguishable from a request for some numeric font weight...

Comment 15

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

In the past what we passed down to the font selection code was a number like 402 which could be decomposed into 400 + bolder + bolder, or 699, which means 700 + lighter.

Comment 16

[Zack Weinberg (:zwol)]

I messed around with a GetFontHandle() approach but it doesn't work, for no apparent reason.

Comment 17

[Zack Weinberg (:zwol)]

This is me thinking out loud.

Several, but not all, of the system-specific font selection routines have code to decompose numbers like 402 into 400+bolder+bolder and then do something with that, but the somethings are not consistent with each other and most of them are not consistent with CSS2.1 either, as far as I can tell.  One notable quirk is that some of them, if they can't find a different weight in the specified direction from the base weight, try the opposite direction, which as far as I can tell is just plain wrong.  Also, the font-selection routines that do this decomposition all attempt to do synthetic boldface under some conditions, especially when the font family has no boldface at all.

It seems silly to implement weight selection at the system-specific level, if only because it means the behavior gets coded N times and they aren't necessarily consistent with each other.

Making Thebes' GetFontHandle() return meaningful values is more of a memory management headache than I want to get into.  The low level code (at least on some platforms) seems to deal in names and treat system font handles as transient objects.

Possible way forward: add a nsI(Thebes)FontMetrics interface that returns a nine-element array of numbers, exposing the low-level code's choice of which CSS numeric weights are visually distinct in that font family -- the numbers in the array are just cookies, but visually distinct weights have distinct numbers. StyleUtils uses that to pick a numeric weight when lighter/bolder are used, and we scrap the base/offset weight encoding.  Whether or not synthetic bold is used remains up to the low-level code.

Comment 18

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to comment #17)
> are not consistent with CSS2.1 either, as far as I can tell.  One notable quirk
> is that some of them, if they can't find a different weight in the specified
> direction from the base weight, try the opposite direction, which as far as I
> can tell is just plain wrong.  Also, the font-selection routines that do this

It seems perfectly reasonable if the base weight isn't available either.

Comment 19

[Zack Weinberg (:zwol)]

(In reply to comment #18)
> [Trying the opposite direction] seems perfectly reasonable if 
> the base weight isn't available either.

Yah, but I'm pretty sure it's not limited to that.

For clarity, the thing that seems "just plain wrong" to me is the possibility that "bolder" might under some circumstances produce a lighter weight than unstyled, or "lighter" a bolder weight.

Comment 20

[Zack Weinberg (:zwol)]

Comment on attachment 323226 [details] [diff] [review]
possible patch for this and #77882

retracting review request.

Comment 21

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

It's probably worth asking Stuart and Vlad what they think of the two possible approachs (gfx code, character-by-character; style code, element-by-element).

Comment 22

[Zack Weinberg (:zwol)]

> It's probably worth asking Stuart and Vlad what they think of the two possible
approaches

Ok, I've added them to the cc: list.

Stuart, Vlad - could you please look at this bug, especially comments 12, 14, 17, and 21, and let me and dbaron know what you think?

Comment 23

[Zack Weinberg (:zwol)]

discussion on IRC converged on doing it character-by-character in gfx but changing the way style communicates the lighter/bolder chain so that gfx can get it right.  also of note is that most font backends do implement lighter/bolder (with bugs) but pango (Linux) doesn't, and that gfx needs to communicate its eventual choice back up, for getComputedStyle()'s sake [it is sufficient to communicate the choice for the first font in the element].

Comment 24

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

And see also bug 3512 comment 43.

Comment 26

[John Daggett (:jtd)]

Discussing on IRC with stuart, changes needed in gfx to correctly handle bolder/lighter compounding correctly:

1. Handle a "chain" of bolder/lighter changes instead of a single base weight plus delta value.

Example: 32-bit integer, lower 4 bits = base weight ([1,9]), upper 28 bit = series of 2 bit values 10=lighter, 11=bolder

2. Resolved weight is calculated using array of available weights for a given font family.
  a. resolve initial base weight to available weights using CSS rules (bold weights map to bolder faces, lighter weights map to lighter faces).
  b. resolve each bolder/lighter chain using available weights, set flag when bolder is set but resolved weight < 600, clear this when lighter is set
  c. if flag is set and the resolved weight is < 600, computed weight is set to 600, otherwise to the resolved weight [for synthetic bolding]

Examples:

FontA (400, 700)
300 + bolder ==> 700
800 + bolder ==> 700
700 + bolder ==> 700
700 + bolder + lighter ==> 400
700 + bolder + bolder + lighter ==> 400

FontB (400, 700, 900)
300 + lighter ==> 400
300 + lighter + bolder ==> 700
700 + bolder ==> 900
700 + bolder + lighter ==> 700
700 + bolder + bolder + lighter ==> 700

FontC (400)
700 ==> 600 (synthetic bolding)
700 + bolder ==> 600 (synthetic bolding)
700 + bolder + lighter ==> 400
100 + lighter + bolder ==> 600 (synthetic bolding)

3. Rendering occurs based on <resolved weight, flag>, since the specifics of synthetic bolding is platform-specific, in some cases we need to do it manually.

Comment 27

[John Daggett (:jtd)]

Tweak 2b. set flag when bolder is set but no bolder weight is available and resolved weight < 600

Comment 28

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

So, it actually occurred to me today while writing http://lists.w3.org/Archives/Public/www-style/2008Jun/0151.html that maybe the spec *meant* for this to be a count rather than a sequence, and we don't need to do the chain thing.  See that message and http://lists.w3.org/Archives/Public/www-style/2008Jun/0147.html .

Comment 29

[Zack Weinberg (:zwol)]

Taking this off wanted1.9.1+ list as we are still waiting for a decision from the CSS WG.

Comment 30

[d]

So...? They've decided yet?

Comment 31

[John Daggett (:jtd)]

(In reply to comment #30)
> So...? They've decided yet?

Not yet.  The place for asking this question is the www-style mailing list, not here.  The proposed solution was discussed at the last meeting in June, you can read the discussion in the mailing list archives.

Comment 32

[Zack Weinberg (:zwol)]

I no longer work for Mozilla, I am deassigning myself from bugs I have no intention of working on as a volunteer.

Comment 33

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

The CSS working group did accept the new behavior.  I have a patch for it in my patch queue, since writing the patch was the easiest way to verify that a test was correct.

Comment 34

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

And the patch fixes these pairs of tests in the CSS 2.1 test suite (which we should probably add as reftests):

http://test.csswg.org/suites/css2.1/20101001/html4/font-weight-bolder-001.htm
http://test.csswg.org/suites/css2.1/20101001/html4/font-weight-bolder-001-ref.htm

http://test.csswg.org/suites/css2.1/20101001/html4/font-weight-lighter-001.htm
http://test.csswg.org/suites/css2.1/20101001/html4/font-weight-lighter-001-ref.htm

http://test.csswg.org/suites/css2.1/20101001/xhtml1/font-weight-bolder-001.xht
http://test.csswg.org/suites/css2.1/20101001/xhtml1/font-weight-bolder-001-ref.xht

http://test.csswg.org/suites/css2.1/20101001/xhtml1/font-weight-lighter-001.xht
http://test.csswg.org/suites/css2.1/20101001/xhtml1/font-weight-lighter-001-ref.xht

The patch (no tests yet) is here:
http://hg.mozilla.org/users/dbaron_mozilla.com/patches/raw-file/8d54e6e9abd3/bolder-lighter-style
There might be some additional code in gfx/ that could be removed.

Comment 35

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch

Here's the code; still need to import the reftests, though.

Given the CSS 2.1 status (and the fact that this simplification might have to be undone from the spec if we don't get this in soon), I think we should try and get this in to Firefox 4.

Comment 36

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 488444 [details] [diff] [review]
patch

Requesting approval2.0, for the following reasons:

This is implementing a change in the latest draft of CSS 2.1 that significantly simplifies the spec; it makes it significantly easier to implement 'bolder' and 'lighter' and also means that implementing them requires no platform-specific code, which reduces the burden of porting.  It also provides a more consistent behavior for common uses of 'bolder', like the HTML <b> element, in the presence of font families that have a large number of barely-distinguishable weights, which may start to become more common given downloadable fonts.  (The new text is at http://www.w3.org/Style/css2-updates/css2/fonts.html#propdef-font-weight ; it makes 'bolder' and 'lighter' compute a new value based only on the inherited value.)

I'd like to get this change in ASAP for two reasons:

(1) I'm concerned that if we don't get the change in soon, CSS 2.1 will be forced to revert the change for exiting CR, and the Web might end up permanently stuck with the more complicated rules.

(2) It's fairly low-risk today, but as more people start using high-quality downloadable fonts, it potentially becomes more risky, since it would be more likely to change behavior on Web pages.

Comment 37

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: fix for synthetic-variations reftest

This is the patch for the test I didn't get around to adjusting earlier.

Comment 38

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I also made some mistakes in the adjusting of reftest references in the first patch, which I believe I fixed in:
http://hg.mozilla.org/users/dbaron_mozilla.com/patches/rev/13e8f9ef7b83

Comment 39

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: additional fix for weightmapping reftests

Even with the correct adjustments, weightmapping-458 still fails on Windows due to what looks to be a line-height difference.  I think the problem is that the current structure of the tests is inconsistent between test and reference about whether the final font is set on an inline or a block; making span a block fixes that and means the outer font settings don't influence the line layout.

Comment 40

[John Daggett (:jtd)]

Comment on attachment 489205 [details] [diff] [review]
fix for synthetic-variations reftest

Looks right.  With the elimination of the delta weights (e.g. 698, 402), this test is less interesting.

Comment 41

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

http://hg.mozilla.org/mozilla-central/rev/864ad777dba7
http://hg.mozilla.org/mozilla-central/rev/cf61d18f6210

Comment 42

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: more code removal

I noticed this based on code coverage data from our test suite.

This code should have been removed in the earlier patches on this bug.

Comment 43

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f7940f39c867

Comment 44

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/f7940f39c867