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

RESOLVED FIXED in mozilla2.0b8

Status

()

Core
CSS Parsing and Computation
P3
normal
RESOLVED FIXED
16 years ago
3 years ago

People

(Reporter: Hixie (not reading bugmail), Assigned: dbaron)

Tracking

(Blocks: 1 bug, {css1, fonts, testcase})

Trunk
mozilla2.0b8
css1, fonts, testcase
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [Hixie-P2][CSS1-5.2.5], URL)

Attachments

(5 attachments)

(Reporter)

Description

16 years ago
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.
(Reporter)

Updated

16 years ago
Blocks: 77882
Keywords: css1, fonts, testcase
Whiteboard: [Hixie-P2]
(Reporter)

Comment 1

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

Updated

16 years ago
Status: NEW → ASSIGNED
Priority: -- → P3
Target Milestone: --- → mozilla1.0

Updated

16 years ago
Keywords: mozilla1.0
Moving to mozilla1.1. Engineers are overloaded!
Target Milestone: mozilla1.0 → mozilla1.1
Bulk moving from Moz1.1 to future-P1. I will pull from this list when scheduling
work post Mozilla1.0.
Priority: P3 → P1
Target Milestone: mozilla1.1 → Future
Whiteboard: [Hixie-P2] → [Hixie-P2][CSS1-5.2.5]

Comment 4

15 years ago
cc'ing myself
(Assignee)

Comment 5

15 years ago
Assigning pierre's remaining Style System-related bugs to myself.
Assignee: pierre → dbaron
Status: ASSIGNED → NEW
Priority: P1 → P3

Comment 6

15 years ago
Reconfirmed using FizzillaCFM/2002070913. Setting All/All.
OS: Windows 2000 → All
Hardware: PC → All
*** Bug 223550 has been marked as a duplicate of this bug. ***
(Assignee)

Updated

10 years ago
Assignee: dbaron → nobody
QA Contact: ian → style-system
I still see this bug in a FF3 nightly on linux and a xulrunner build.
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].
Created attachment 323226 [details] [diff] [review]
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.
Assignee: nobody → zweinberg
Status: NEW → ASSIGNED
Attachment #323226 - Flags: superreview?(dbaron)
Attachment #323226 - Flags: review?(dbaron)
(Assignee)

Comment 11

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

Comment 12

9 years ago
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?
(Assignee)

Comment 13

9 years ago
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.
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...
(Assignee)

Comment 15

9 years ago
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.
I messed around with a GetFontHandle() approach but it doesn't work, for no apparent reason.
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.
(Assignee)

Comment 18

9 years ago
(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.
(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 on attachment 323226 [details] [diff] [review]
possible patch for this and #77882

retracting review request.
Attachment #323226 - Flags: superreview?(dbaron)
Attachment #323226 - Flags: review?(dbaron)
(Assignee)

Comment 21

9 years ago
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).
> 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?

Updated

9 years ago
Flags: wanted1.9.1?
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].
(Assignee)

Comment 24

9 years ago
And see also bug 3512 comment 43.
Duplicate of this bug: 419962

Comment 26

9 years ago
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.


Summary: 'bolder' and 'lighter' keywords of 'font-weight' don't work → 'bolder' and 'lighter' keywords of 'font-weight' don't compound correctly

Comment 27

9 years ago
Tweak 2b. set flag when bolder is set but no bolder weight is available and resolved weight < 600
(Assignee)

Comment 28

9 years ago
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 .
Flags: wanted1.9.1? → wanted1.9.1+
Taking this off wanted1.9.1+ list as we are still waiting for a decision from the CSS WG.
Flags: wanted1.9.1+

Comment 30

8 years ago
So...? They've decided yet?

Comment 31

8 years ago
(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.
I no longer work for Mozilla, I am deassigning myself from bugs I have no intention of working on as a volunteer.
Assignee: zackw → nobody
Status: ASSIGNED → NEW
(Assignee)

Comment 33

7 years ago
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.
Assignee: nobody → dbaron
Status: NEW → ASSIGNED
(Assignee)

Comment 34

7 years ago
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.
(Assignee)

Updated

7 years ago
Blocks: 605520
(Assignee)

Comment 35

7 years ago
Created attachment 488444 [details] [diff] [review]
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.
Attachment #488444 - Flags: review?(jdaggett)

Updated

7 years ago
Attachment #488444 - Flags: review?(jdaggett) → review+
(Assignee)

Comment 36

7 years ago
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.
Attachment #488444 - Flags: approval2.0?
(Assignee)

Comment 37

7 years ago
Created attachment 489205 [details] [diff] [review]
fix for synthetic-variations reftest

This is the patch for the test I didn't get around to adjusting earlier.
Attachment #489205 - Flags: review?(jdaggett)
Attachment #488444 - Flags: approval2.0? → approval2.0+
(Assignee)

Comment 38

7 years ago
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
(Assignee)

Comment 39

7 years ago
Created attachment 489406 [details] [diff] [review]
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.
Attachment #489406 - Flags: review?(jdaggett)
(Assignee)

Updated

7 years ago
Attachment #489406 - Attachment is patch: true
Attachment #489406 - Attachment mime type: application/octet-stream → text/plain

Updated

7 years ago
Attachment #489406 - Flags: review?(jdaggett) → review+

Comment 40

7 years ago
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.
Attachment #489205 - Flags: review?(jdaggett) → review+
(Assignee)

Comment 41

7 years ago
http://hg.mozilla.org/mozilla-central/rev/864ad777dba7
http://hg.mozilla.org/mozilla-central/rev/cf61d18f6210
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
Target Milestone: Future → mozilla2.0b8
(Assignee)

Comment 42

5 years ago
Created attachment 655762 [details] [diff] [review]
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.
Attachment #655762 - Flags: review?(jdaggett)

Updated

5 years ago
Attachment #655762 - Flags: review?(jdaggett) → review+
(Assignee)

Comment 43

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/f7940f39c867
https://hg.mozilla.org/mozilla-central/rev/f7940f39c867

Updated

3 years ago
Depends on: 1116751
You need to log in before you can comment on or make changes to this bug.