72164 - font-size:smaller doesn't extrapolate values

Status: RESOLVED FIXED

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

Description

[Hixie (not reading bugmail)]

Per CSS2, section 15.2.4:
#
# <relative-size> 
#   A <relative-size> keyword is interpreted relative to the table of font 
#   sizes and the font size of the parent element. Possible values are: 
#     [ larger | smaller ] 
#
#   For example, if the parent element has a font size of 'medium', a value 
#   of 'larger' will make the font size of the current element be 'large'. If 
#   the parent element's size is not close to a table entry, the user agent is 
#   free to interpolate between table entries or round off to the closest one. 
#   The user agent may have to extrapolate table values if the numerical value 
#   goes beyond the keywords. 
 -- http://www.w3.org/TR/REC-CSS2/fonts.html#value-def-relative-size

We currently clip to the absolute font-size keyword list, so 
   parent { font-size: 10em; }
   child { font-size: smaller; }
...will result in unexpectedly small text, instead of just slightly smaller.

What we do isn't wrong, but certainly could be improved. As is, we are probably
discouraging use of 'smaller' (and 'larger').

Comment 1

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

Do we do this differently in quirks mode and strict mode?  I can't remember the
whole discussion with Todd Fahrner sometime around a year ago...

Comment 2

[basic]

Hixie: how does your testcase work? I don't see red (no I'm not colorblind).

Comment 3

[basic]

spam, grr forgot to cc self

Comment 4

[Hixie (not reading bugmail)]

The testcase works by first drawing some red at the 'medium' (1em) font size,
and then on top of it drawing some green at a font size 'smaller' that 10em.
(The colour is actually from backgrounds on inline elements.) If the smaller 
text ends up smaller than the medium text, then there is a bug and you can see
it because stuff will be red.

However, that's quite a poor test really, as demonstrated by the fact that we 
currently pass it. :-/

Here's a better test:
   http://www.hixie.ch/tests/adhoc/css/fonts/size/002.xml
It works in a similar way, except this time I replaced 'medium' with 'xx-large'
and made the xx-large text bold, making it visible even if the 'smaller' text
is the same size as the 'xx-large' text. Sorry about that; thanks for pointing
it out to me.

Comment 5

[Hixie (not reading bugmail)]

See also: http://www.hixie.ch/tests/adhoc/css/fonts/size/004.xml

Comment 6

[rbs]

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

Comment 7

[rbs]

Another troublesome manifestation of this bug is described in bug 93843. 
Updating priority and severity to reflect the wider nature of this bug. The 
mapping code needs to be revisited so as to take into account the current parent 
font.

Also clearing TM to trigger re-evaluation.

Comment 8

[rbs]

pierre, this bug is odd. I had a look at the code and I am wondering what
is going on in the mapping function. (To be precise, I am wondering why
it is coded the way it is coded. I would have though that it is an
interpolation problem with predefined coefficients).

Looking at the code, I see:
  static PRInt32 sFontSizeFactors[8] = { 60,75,89,100,120,150,200,300 };
and
  static PRInt32 sStrictFontSizeTable[...][8] =
  {
      { 9,    9,     9,     9,    11,    14,    18,    27},
      { 9,    9,     9,    10,    12,    15,    20,    30},
      { 9,    9,    10,    11,    13,    17,    22,    33},
      { 9,    9,    10,    12,    14,    18,    24,    36},
      { 9,   10,    12,    13,    16,    20,    26,    39},
      { 9,   10,    12,    14,    17,    21,    28,    42},
      { 9,   10,    13,    15,    18,    23,    30,    45},
      { 9,   10,    13,    16,    18,    24,    32,    48}
  };

And from the description at:
http://style.cleverchimp.com/font_size_intervals/altintervals.html
the intention seems to be to get:

 index  mapping value                 scaling factor
======  ==============                ==============
xxs  0  size(0) =  9px                60%

 xs  1  size(1) = 10px                75%

  s  2  size(2) = 13px                89%

  m  3  size(3) = 16px  <-- default   100%

  l  4  size(4) = 18px                120%

 xl  5  size(5) = 24px                150%

xxl  6  size(6) = 32px                200%

     7  size(7) = 48px                300%


=================
Are these what are needed (in pseudo-markup for simplicity):

1) <font size="a-predefined-edge-value"> <!-- e.g., 9px, 10px, 16px -->
      <font size="+1">
         <!-- gives the next edge value -->

2) a) if the current size isn't an edge value, e.g.,
   <font size="33px">
      <font size="+1">
         <!-- what is the current size here ... -->
   or

   <font size="17px">
      <font size="+1">
         <!-- what is the current size here ... -->

   b) Should the scaling factor corresponding to the sub-interval in which
   the current size falls be used? (The current scaling factors look odd
   for such a purpose. How are they supposed to be used?)


3) is it intended that <font size="+k"> should be equivalent to:

   <font size="+1">
     ... <!-- k-times -->
       <font size="+1"> 
   (so that <font size="+1"> and <font size="-1"> balance to no-op.)

4) if the current size is an edge value, then should
   <span style="font-size:a-certain-percentage"> cause a perfect hop
   to the next egde value for percentages that match scaling factors.
   (almost the same oddity as in 2b comes into mind)

4) any thing else?

Note: I am not that much interested in the discussion that led to
choosing the current numbers -- these are just parameters. If the code
can just do what it is supposed to be doing, that will be fine with me.

Comment 9

[rbs]

I got more understanding into what is happening (I added comments in bug 93843 
to that effect). <font size="+/-k"> works fine, as per the spec. What is missing 
is the extrapolation of 'larger' and 'smaller' as originally reported when 
opening this bug. And with pixel rounding operations and with the fact that the 
spec says that "the user agent is free to interpolate between table entries or 
round off to the closest one", there is room for different implementations and 
roundtrips (larger <-> smaller) is not something to expect across browsers.

In fact, although extrapolation will look more glamorous to the end user, a 
possible spec compliant fix for this bug could be to pass the parent size _as_ 
the 'base size' in FindNext[Smaller|Larger]FontSize(). This will cause the 
resulting size to "round off to the closest one" -- the visual effect 
being less glamorous than extrapolation, e.g., 'larger' of 18px..23px will all 
resolve to 24px (c.f. the table above).

Comment 10

[rbs]

Attachment: possible patch

Comment 11

[Pierre Saslawsky]

The values are correctly extrapolated above xx-large but not below xx-small.

Comment 12

[Pierre Saslawsky]

Attachment: testcase

Comment 13

[Pierre Saslawsky]

Note: the testcase has a html mimetype instead of xml but it works too...

Comment 14

[Kevin McCluskey (gone)]

Moving to mozilla1.1. Engineers are overloaded!

Comment 15

[Kevin McCluskey (gone)]

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

Comment 16

[Madhur Bhatia]

cc'ing myself

Comment 17

[Peter 'Rufus' Nelson]

This looks to be fixed to me on Linux (2002051921, trunk), but leaving as is for
testing on other platforms.  I see no red text in the link, and in the other 2
testcases each X is bigger (or smaller in the 2nd test case) than the previous
for the first 7 at least; after that they're all the same size, but that would
be a different bug.

Comment 18

[Hixie (not reading bugmail)]

No, that would in fact be this very bug.

Comment 19

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

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

Comment 20

[Eric Lawrence]

Attachment: interpolate/extrapolate font sizes in FindNext[Smaller|Larger]FontSize

This should make the relative keywords more consistent across different DPI
settings (and missettings), parent font sizes, and smaller/larger combos (ie,
smaller+larger=noop, as long as 'smaller' was not previously applied to a base
font size of < 2px).

I will post a test page in a second, but can anybody find sites in the wild
that use a lot of relative keywords?

Comment 21

[Eric Lawrence]

Attachment: another test page

Comment 22

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 121157 [details] [diff] [review]
interpolate/extrapolate font sizes in FindNext[Smaller|Larger]FontSize

>Index: content/base/src/nsRuleNode.cpp
>+        aFont->mSize = nsStyleUtil::FindNextLargerFontSize(parentSize, 

Add an 

NS_ASSERTION(aFont->mSize > aParentFont->mSize, "FindNextLargerFontSize didn't
work");

perhaps?

>+        aFont->mSize = nsStyleUtil::FindNextSmallerFontSize(parentSize, (PRInt32)aDefaultFont.size,

Same here, but aFont->mSize <= aParentFont->mSize (since there is the
degenerate "both are 1px" case.....

>Index: content/html/style/src/nsStyleUtil.cpp
>+    else {  // larger than HTML table, drop by 150%

"drop by a factor of 1.5" (dividing by 1.5 is dropping by 33%).

>+    smallerSize = (aFontSize >= (2*onePx)) ? aFontSize - onePx : onePx; 

smallerSize = PR_MAX(aFontSize - onePx, onePx);

>+    else {  // larger than HTML table, increase by 150%
>+      largerSize = NSToCoordRound((float) aFontSize * 1.5);

Again, that's "increase by a factor of 1.5" of "increase by 50%".

r=me with those minor changes.

Comment 23

[Eric Lawrence]

Attachment: new patch with bz's fixes

Fixed the percentages (duh; thanks) and made the rest of bz's changes, with one
minor change of my own:

+	 NS_ASSERTION(aFont->mSize < parentSize, 
+	     "FindNextSmallerFontSize failed; this is expected if parentFont
size <= 1px");

If we're going to make this check, we should do a strict less-than.  In the
rare case that a page has microscopic fonts, it is clear that it's ok.

Comment 24

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

Comment on attachment 122074 [details] [diff] [review]
new patch with bz's fixes

>+  if (aFontSize > CalcFontPointSize(indexMin+1, aBasePointSize, aScalingFactor, aPresContext, aFontSizeType)) {
>+    if (aFontSize <= CalcFontPointSize(indexMax, aBasePointSize, aScalingFactor, aPresContext, aFontSizeType)) { // in HTML table
>+      for (index = indexMax; index > indexMin; index--) { // find largest index smaller than current
>+        indexFontSize = CalcFontPointSize(index, aBasePointSize, aScalingFactor, aPresContext, aFontSizeType);
>+        if (indexFontSize < aFontSize)

You know this isn't going to be true for the first iteration of the loop
(thanks to the second |if| above), so why bother with that first iteration?

>           break;
>+      } 
>+      largerIndexFontSize = CalcFontPointSize(index+1, aBasePointSize, aScalingFactor, aPresContext, aFontSizeType);
>+      smallerIndexFontSize = CalcFontPointSize(index-1, aBasePointSize, aScalingFactor, aPresContext, aFontSizeType);
>+      if (largerIndexFontSize != indexFontSize) {
>+        fontSizeRatio = (float) (aFontSize - indexFontSize) / (largerIndexFontSize - indexFontSize);            
>+        smallerSize = smallerIndexFontSize + NSToCoordRound(fontSizeRatio * (indexFontSize - smallerIndexFontSize));      
>+      } else {
>+        smallerSize = smallerIndexFontSize;
>+      }

This wasn't quite the algorithm I was expecting.  A standard extrapolation
algorithm would, I think, be something like (note the casts to double before
the division, so that floating point division is performed):

      smallerIndexFontSize = CalcFontPointSize(index-1, aBasePointSize,
aScalingFactor, aPresContext, aFontSizeType);
      if (smallerIndexFontSize != indexFontSize) {
	smallerSize = NSToCoordRound(aFontSize * (double(smallerIndexFontSize)
/ double(indexFontSize)));
      } else {
	// XXX Should we ever get here?
	smallerSize = smallerIndexFontSize;
      }


Roughly the same comments apply to FindNextLarger...

Comment 25

[Eric Lawrence]

Yes, that loop has an unnecessary iteration.  My bad.

>      smallerIndexFontSize = CalcFontPointSize(index-1, aBasePointSize,
>aScalingFactor, aPresContext, aFontSizeType); 
>      if (smallerIndexFontSize != indexFontSize) { 
>	smallerSize = NSToCoordRound(aFontSize * (double(smallerIndexFontSize) 
>/double(indexFontSize))); 

This assumes the scaling is linear, but it's not.  You'll get (what I think) 
are inconsistent and unwanted results, especially for larger sizes.  For 
example, the strict mode size table for a 16px base is

9,   10,    13,    16,    18,    24,    32,    48

If the parent font is 47px, then the computed smaller size is
(24/32)*47=35.3px, which is *larger* than what you'd get if the parent
font size were 48px (->32px).

Even if you changed it to extrapolate from the 'current' interval (in the 
example above, take 32/48 as your shrinking ratio), you'd have problems with 
parent font sizes that are slightly larger than an index size (except it would 
overshoot instead of undershoot). 

The idea behind my implementation is to treat the 'curve' as piecewise linear, 
and interpolate on it, instead of taking only two points and extrapolating from 
them.

>      } else { 
>	// XXX Should we ever get here?
>	smallerSize = smallerIndexFontSize;
>      }

Yes, see

http://lxr.mozilla.org/seamonkey/source/content/html/style/src/nsStyleUtil.cpp#265

and note how in the first few rows, '9' is repeated.

Please reconsider the sr.

Comment 26

[Eric Lawrence]

ok, that was an exaggerated example since the last column is not used for CSS
computations, but there are other spots where the problem would crop up:

17,    21,    28  (use parent=27)

9,     10,    13  (use parent=12)

etc...

Comment 27

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

> and note how in the first few rows, '9' is repeated.

I see how that could cause indexFontSize and smallerIndexFontSize to be the
same, but not indexFontSize and largerIndexFontSize.


The algorithm makes sense now that you describe it.  Could you attach a revised
patch with a comment in the code explaining the key features of the algorithm
(probably a bit more concisely than above)?  (i.e., explain that the
'fontSizeRatio' variable is the proportion of the way the current size is from
one entry in the table to the next, and that your goal is to end up that same
ratio between the adjacent pair of entries)

Also, I prefer explicit, construction-style, casts on both operands when doing
floating point division of integers.  (Then I don't have to think about the
rules for dividing a float by an integer (in addition to order of operations,
with the C-style casts).)

Comment 28

[Eric Lawrence]

Attachment: better handling of boundary cases +dbaron's fixes

After reflecting on dbaron's previous comment, I realized my patch doesn't
treat the boundary cases correctly (you could get slightly inconsistent results
similar to those described in comment 26 with certain base font sizes).  Here
is a new patch that addresses this, and should hopefully be a bit clearer.

Do I need a new r=, since I added a fair bit of code?

Comment 29

[Asa Dotzler [:asa]]

Comment on attachment 122645 [details] [diff] [review]
better handling of boundary cases +dbaron's fixes

a=asa (on behalf of drivers) for checkin to 1.4

Comment 30

[Boris Zbarsky [:bzbarsky]]

Checked in for 1.4 final.  Thanks for the patch, Eric!

Comment 31

[Aleksey Nogin]

This check-in have added two "may be used uninitialized" warning on brad TBox:

+content/html/style/src/nsStyleUtil.cpp:419
+ `nscoord indexFontSize' might be used uninitialized in this function
+
+content/html/style/src/nsStyleUtil.cpp:487
+ `nscoord indexFontSize' might be used uninitialized in this function

Comment 32

[Boris Zbarsky [:bzbarsky]]

It's nice to know that gcc cannot tell that a loop of the form:

+      for (index = indexMax; index >= indexMin; index--) {

will run at least once given the possible assignments of indexMin and indexMax
in this function.  Wanna report that as a bug to the gcc people?

Comment 33

[Eric Lawrence]

Should we initialize |indexFontSize| up front just to quiet gcc?  I wouldn't
want to be responsible for creating more warnings, even if they are stupid.

fwiw, apple gcc 3.1 doesn't complain.  

Comment 34

[Boris Zbarsky [:bzbarsky]]

Brad is actually building using gcc 3.2, so we can't even hope that it'll get
better.... :(

If you have time to do a patch to init those up front, I'll review...

Comment 35

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

Yeah, thanks for the patch.  (I was planning to check it in today, but Boris
clearly beat me to it.)

Comment 36

[Eric Lawrence]

Attachment: patch to get rid of gcc warning

This should stop the silly gcc warnings, but I don't have any way of testing it
with Linux gcc.  In any case, it won't hurt anything since indexFontSize will
get set in the previously mentioned loop.

If cvs-mirror ever comes back up, I'll post a patch against cvs.  Until then,
here is one against nsStyleUtil.cpp in lxr.

bz: I thought you were on vacation. ;)

Comment 37

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 123723 [details] [diff] [review]
patch to get rid of gcc warning

r+sr=me, and I'm trying to be on vacation, yes...  :(

Comment 38

[Eric Lawrence]

Attachment: same patch as previous, now diffed against cvs

Comment 39

[Eric Lawrence]

Attachment: same patch as previous, now diffed against cvs, and with a linebreak fixed

Comment 40

[Eric Lawrence]

Comment on attachment 123799 [details] [diff] [review]
same patch as previous, now diffed against cvs, and with a linebreak fixed

This should fix an unnecessary warning from Linux gcc.	Has no effect on the
code.

It is the same as attachment 123723 [details] [diff] [review], which has r+sr=bzbarsky.

Comment 41

[Asa Dotzler [:asa]]

Comment on attachment 123799 [details] [diff] [review]
same patch as previous, now diffed against cvs, and with a linebreak fixed

a=asa (on behalf of drivers) for checkin to 1.4

Comment 42

[Boris Zbarsky [:bzbarsky]]

Warning fix checked in.

Comment 43

[Aleksey Nogin]

Comment on attachment 123799 [details] [diff] [review]
same patch as previous, now diffed against cvs, and with a linebreak fixed

Confirming that the warnings went away. Thanks!