Closed
Bug 72164
Opened 24 years ago
Closed 22 years ago
font-size:smaller doesn't extrapolate values
Categories
(Core :: CSS Parsing and Computation, defect, P3)
Core
CSS Parsing and Computation
Tracking
()
RESOLVED
FIXED
Future
People
(Reporter: ian, Assigned: dbaron)
References
()
Details
(Keywords: css1, fonts, testcase, Whiteboard: [Hixie-P2][CSS1-5.2.6][patch])
Attachments
(5 files, 4 obsolete files)
(deleted),
text/plain
|
Details | |
(deleted),
text/html
|
Details | |
(deleted),
text/html
|
Details | |
(deleted),
patch
|
dbaron
:
review+
dbaron
:
superreview+
asa
:
approval1.4+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
asa
:
approval1.4+
|
Details | Diff | Splinter Review |
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').
Reporter | ||
Updated•24 years ago
|
Priority: -- → P5
Whiteboard: (py8ieh: need better test case)
Target Milestone: --- → Future
Assignee | ||
Comment 1•24 years ago
|
||
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...
Hixie: how does your testcase work? I don't see red (no I'm not colorblind).
Reporter | ||
Comment 4•24 years ago
|
||
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.
Whiteboard: (py8ieh: need better test case)
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.
Severity: enhancement → normal
OS: Windows 2000 → All
Priority: P5 → P3
Hardware: PC → All
Target Milestone: Future → ---
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.
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•23 years ago
|
||
Comment 11•23 years ago
|
||
The values are correctly extrapolated above xx-large but not below xx-small.
Comment 12•23 years ago
|
||
Updated•23 years ago
|
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla0.9.9
Comment 14•23 years ago
|
||
Moving to mozilla1.1. Engineers are overloaded!
Target Milestone: mozilla0.9.9 → mozilla1.1
Comment 15•23 years ago
|
||
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
Updated•23 years ago
|
Whiteboard: [Hixie-P2] → [Hixie-P2][CSS1-5.2.6]
Comment 17•23 years ago
|
||
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.
Assignee | ||
Comment 19•23 years ago
|
||
Assigning pierre's remaining Style System-related bugs to myself.
Assignee: pierre → dbaron
Status: ASSIGNED → NEW
Priority: P1 → P3
Comment 20•22 years ago
|
||
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•22 years ago
|
||
Updated•22 years ago
|
Attachment #121157 -
Flags: superreview?(dbaron)
Attachment #121157 -
Flags: review?(bzbarsky)
![]() |
||
Comment 22•22 years ago
|
||
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.
Attachment #121157 -
Flags: review?(bzbarsky) → review+
Updated•22 years ago
|
Attachment #121157 -
Attachment is obsolete: true
Attachment #121157 -
Flags: superreview?(dbaron)
Comment 23•22 years ago
|
||
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.
Updated•22 years ago
|
Attachment #122074 -
Flags: superreview?(dbaron)
![]() |
||
Updated•22 years ago
|
Attachment #122074 -
Flags: review+
Assignee | ||
Comment 24•22 years ago
|
||
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...
Attachment #122074 -
Flags: superreview?(dbaron) → superreview-
Comment 25•22 years ago
|
||
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•22 years ago
|
||
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...
Assignee | ||
Comment 27•22 years ago
|
||
> 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•22 years ago
|
||
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?
Attachment #122074 -
Attachment is obsolete: true
Updated•22 years ago
|
Attachment #122645 -
Flags: superreview?(dbaron)
Assignee | ||
Updated•22 years ago
|
Attachment #122645 -
Flags: superreview?(dbaron)
Attachment #122645 -
Flags: superreview+
Attachment #122645 -
Flags: review+
Assignee | ||
Updated•22 years ago
|
Whiteboard: [Hixie-P2][CSS1-5.2.6] → [Hixie-P2][CSS1-5.2.6][patch]
Assignee | ||
Updated•22 years ago
|
Attachment #122645 -
Flags: approval1.4?
Comment 29•22 years ago
|
||
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
Attachment #122645 -
Flags: approval1.4? → approval1.4+
![]() |
||
Comment 30•22 years ago
|
||
Checked in for 1.4 final. Thanks for the patch, Eric!
Status: NEW → RESOLVED
Closed: 22 years ago
Resolution: --- → FIXED
Comment 31•22 years ago
|
||
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•22 years ago
|
||
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•22 years ago
|
||
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•22 years ago
|
||
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...
Assignee | ||
Comment 35•22 years ago
|
||
Yeah, thanks for the patch. (I was planning to check it in today, but Boris clearly beat me to it.)
Comment 36•22 years ago
|
||
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. ;)
Updated•22 years ago
|
Attachment #123723 -
Flags: superreview?(bz-bugspam)
Attachment #123723 -
Flags: review?(bz-bugspam)
![]() |
||
Comment 37•22 years ago
|
||
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... :(
Attachment #123723 -
Flags: superreview?(bz-bugspam)
Attachment #123723 -
Flags: superreview+
Attachment #123723 -
Flags: review?(bz-bugspam)
Attachment #123723 -
Flags: review+
Comment 40•22 years ago
|
||
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.
Attachment #123799 -
Flags: approval1.4?
Comment 41•22 years ago
|
||
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
Attachment #123799 -
Flags: approval1.4? → approval1.4+
Comment 43•22 years ago
|
||
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!
You need to log in
before you can comment on or make changes to this bug.
Description
•