83658 - font size=7 broken

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

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

Right now <font size=7> is broken.  The HTML font size scale runs from 1-7, but
it doesn't correspond to CSS's font size scale (since the default is at 3 rather
than at 4).

Steps to reproduce:
 * load attached test case
 * look at "font size=7" line.  It should be bigger than the size=6 line rather
than the normal size.

Comment 1

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

Attachment: testcase showing HTML and CSS font sizes

Comment 2

[Aaron Kaluszka]

It appears that size=1=x-small, not xx-small

Comment 3

[David Hyatt]

Yes, the comments in the testcase for font size=1 seem wrong.  Looking at the 
enumeration in nsStyleConsts.h, it seems that <font size="1"> went to x-small.

At first glance, I don't see why this is broken.  I am using 
NS_STYLE_FONT_SIZE_LARGER for <font size="7">.

Oh, I have a theory.  Perhaps that const was only examined in the font element, 
and now it needs to be examined over in nsRuleNode.cpp code.  I may have lost 
the code that checked for that case.

Comment 4

[David Hyatt]

Oh hell.  I see it now.  I have an idea for how to fix this, but I need to 
ensure I'm not doing anything incorrect from a CSS perspective.  It looks like 
blindly trying to use a value of "7" ends up corresponding to the enumerated 
value of LARGER, which is not correct.

I think I want to introduce a new styleconst with a value of 7 that represents 
an even larger point size (we'll call it XXXLARGE).  If I do this, I think I'll 
be ok as long as nobody can specify this size via CSS.  In CSS, do you have to 
use a keyword rather than an integer to specify values like xxsmall etc.?  

If so, then what I'm proposing should work just fine.

Comment 5

[David Hyatt]

Attachment: Patch to styleconsts

Comment 6

[David Hyatt]

Attachment: Patch to rulenode code.

Comment 7

[David Hyatt]

dbaron, attinasi, r and sr?

Comment 8

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

r=dbaron (although I thought this used to be more flexible)

Comment 9

[David Hyatt]

What do you mean by more flexible?  

I may be wrong, but I think this should be exactly equivalent to what was there 
before.  In the old code, a font size of <= 0 turned into 1 and a font size of 
>=8 turned into 7.  All values were treated as though they were the enumerated 
values 1-7 and CalcPointSize was called on those values.

I preserved the boundary-checking code in fontelement and just rely on the 
rulenode code that does font computation.  

In fact, I reduced code bloat by consolidating all the font computation into 
one place (the rule node).  Before, a bunch of font computation code was 
duplicated (in nsHTMLFontElement and in nsCSSStyleRule).

Comment 10

[David Hyatt]

It just happens that the font computation code that I cut and pasted came from 
nsCSSStyleRule.cpp, and that only dealt with the enumerated values 1-6.  Now 
that both font element and rule node use the same computation function, the 
code needs to deal with a value of 7 as well.

Comment 11

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

I had thought we were leaving a gap in the mapping so <font-size=1> was
xx-small, but maybe not...

Comment 12

[David Hyatt]

Nope.  xx-small has an enumerated value of 0.  We just turned all values into 
the values 1-7, which means we treated <font size=1> as x-small.  Is this 
incorrect?  It's trivial to make <font size="1"> be xx-small.

Comment 13

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

No, just as long as it's how it used to be.

Comment 14

[David Hyatt]

Fixed.

Comment 15

[John Morrison]

(It's not the end of the world, but ...) in current 06/02 trunk builds (checked 
on Linux on w2k), <font size=7> is still the same size as <font size=6>. 

Comment 16

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

Reopening.  The problem is that the code in nsRuleNode is always using
eFontSize_CSS, and if it's going to work the way it does now (which is
admittedly less flexible -- see the code in nsStyleUtil::NewCalcFontPointSize),
NewCalcFontPointSize needs to be changed.  But as I said before, that gives us a
little less flexibility.

Comment 17

[David Hyatt]

Attachment: This fixes it, right?

Comment 18

[David Hyatt]

I think this will fix it.  r/sr/a again? :)

Comment 19

[Peter Trudelle]

This doesn't really fit the profile for 0.9.2, although if the attached patch
fixes it you'd probably be able to get approval for checkin during 'limbo'. ->0.9.3

Comment 20

[David Hyatt]

This is a regression from the style system landing.  We can't ship a product 
with this regressed.  The bug has a patch.

Comment 21

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

r=dbaron

Comment 22

[rbs]

BTW, I integrated this patch to what I am doing over at bug 84398.

Comment 23

[Chris Waterson]

Righteous. sr=waterson, le review super!

Comment 24

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

a=dbaron for trunk checkin (on behalf of drivers)

Comment 25

[David Hyatt]

Fixed.