Closed Bug 158868 Opened 22 years ago Closed 21 years ago

Bad interline spacing when minimum font size is set (need to scale line-height)

Categories

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

x86
Windows 2000
defect

Tracking

()

RESOLVED FIXED
mozilla1.5beta

People

(Reporter: mics, Assigned: dbaron)

References

(Depends on 1 open bug, )

Details

(Keywords: testcase)

Attachments

(6 files)

When viewing page http://www.atptennis.com/en/default.asp, with minimum font size for "Western" set to anything except "None", text is shown with incorrect interline spacing. When manually decreasing font size (Ctrl '-'), lines begin to be shown on top of the previous lines, i.e. font size is not decreased but interline spacing is decreased. The same happens with some other sites as well: http://www.cnn.com/ (left sidebar) http://www.gazeta.ru/ http://websearch.about.com/ (left sidebar) I'm using Mozilla 1.1b Win2000
Attached image Normal text (deleted) —
Attached image Same text when min font size is set (deleted) —
The page explicitly sets an interline spacing of 12px and a font size of 10px. Any font size bigger than 12px will cause the lines to overlap as you see. Text zoom scales both font sizes and line heights, so if your initial font size was bigger than 10px and you scale down eventually you will end up with overlapping text.....
The question is why it allows scaling down interline spacing when font size is not scaled down (because of minimum font size set)? Shouldn't setting minimum font size also limit interline spacing of the text to which this minimum font size is applied?
QA Contact: petersen → moied
I've seen the same behaviour in both the bulleted and numbered list in http://support.microsoft.com/default.aspx?scid=kb;en-us;Q235356 Viewing the page directly in the browser shows the page in a small font, with too little spacing so you cannot read it. If I copy the source into the composer, the preview shows a larger font (Times New Roman) and I have no spacing problems. My preferences settings are serif: Times New Roman 16. Sans Serif Arial and Allow documents to use other fonts is checked. Setting this to a large font does display the large fonts, but does not eliminate the spacing problem.
Taking bug. I think the solution to this is to do something similar to the minimum font size as I did for text zoom in bug 41847. However, for the line height, I think the solution is to scale the line height proportionally when it's specified in physical or 'px' units and we're applying a minimum font size to the font.
Assignee: attinasi → dbaron
Status: UNCONFIRMED → NEW
Component: Layout → Style System
Ever confirmed: true
Priority: -- → P1
QA Contact: moied → ian
What makes this difficult is that the font min size is per lang-group. (font.min-size.{fixed,variable}.<lang>)
The reason for having different minimum sizes for different languages is that different languages have different glyph complexity. Latin glyphs are fairly simple and are often readable down to 6-8px. Kanji (Chinese/Japanese/Korean) can be much more complex and typically become unreadable below 10-12px. There is no number that is suitable for both.
Perhaps there's a way we can propagate the information from font selection out to the layout code and then consider the line height in terms of half-leading (half the computed 'line-height' minus the computed 'font-size').
Actually, the value being modified on the Fonts preference dialog is: font.minimum-size.<lang>. It is handled/propagated by the Style System directly, and so further action can be taken there if necessary. http://lxr.mozilla.org/seamonkey/search?string=minimumFontSize (The old one, font.min-size.{fixed,variable}.<lang>, is something that bstell seems attach to.)
I particularly attached to either one as long as we don't break things too much for users.
Attached file reduced testcase (deleted) —
steps: 1. display testcase with 100% original size 2. use "ctrl + -" to reduce text size the text size is reduced without any distortion till 7% text size. Text starts getting distorted from 4% text size onwards. noted that list items do not get affected with the size reduction
Keywords: testcase
Severity: normal → major
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla1.3alpha
There are also other related cases where using a minimum font setting creates problems because Mozilla changes only the font size and does not scale any other component in the page to reflect the overridden font size value. For example, currently there are search fields and drop-down menus on http://www.bn.com/ the height of which the webmaster has fixed to 18px using CSS. When viewing this page with a relatively large minimum font size, say 20pt, the form controls are illegible because the fonts used in them are too large for the text to be completely visible within the control. Mozilla should override the CSS rule that sets the form control size and enlarge the form controls so that the text fits into them.
Even text zoom has troubles with http://www.bn.com/. The comprehensive fix is going to come from the full zoom bug 4821.
In phoenix, I have "Always use my fonts" checked and Minimum font size set to 18. I sometimes get the bad interline spaceing problem. To fix, I've inserted the following lines into userContent.css: a { line-height: normal !important; } font { line-height: normal !important; } p { line-height: normal !important; } li { line-height: normal !important; } td { line-height: normal !important; } body { line-height: normal !important; }
Summary: Bad interline spacing when minimum font size is set → Bad interline spacing when minimum font size is set (need to scale line-height)
Target Milestone: mozilla1.3alpha → mozilla1.5beta
Blocks: 203888
*** Bug 217791 has been marked as a duplicate of this bug. ***
*** Bug 219631 has been marked as a duplicate of this bug. ***
moving nomination from duped bug.
Flags: blocking1.5?
Flags: blocking1.5? → blocking1.5-
Would it be possible to add a "Minimum Line Height" preference to go along with the "Minimum Font Size"?
There's no need for an additional preference -- we just need to scale the line-height when the minimum font size applies.
Attached patch patch (deleted) — Splinter Review
This testcase shows the bug fixed by the typo the patch fixes (changing aInherited to inherited).
Attachment #135745 - Flags: superreview?(bz-vacation)
Attachment #135745 - Flags: review?(bz-vacation)
Attachment #135744 - Flags: superreview?(bz-vacation)
Attachment #135744 - Flags: review?(bz-vacation)
Attachment #135745 - Flags: superreview?(bz-vacation)
Attachment #135745 - Flags: review?(bz-vacation)
Comment on attachment 135744 [details] [diff] [review] patch >Index: nsRuleNode.cpp >+ mPresContext->GetCachedIntPref(kPresContext_MinimumFontSize, >+ minimumFontSize); >+ if (minimumFontSize > 0 && !IsChrome(mPresContext)) { >+ // If we applied a minimum font size, scale the line height by >+ // the same ratio. (If we *might* have applied a minimum font >+ // size, we can't cache in the rule tree.) >+ inherited = PR_TRUE; >+ const nsStyleFont *font = aContext->GetStyleFont(); >+ lh = float(lh) * float(font->mFont.size) / float(font->mSize); Wouldn't it make sense to do this scaling any time font->mFont.size is not the same as font->mSize, though? Alternately, is there any way we can end up with the min font size passed into this function? That business about getting the cached pref and then checking for chrome is not all that fast.... Other than this bit, looks great.
I think the only way the two can be different is if there's a min font size. See |SetFont|, which does: // We want to zoom the cascaded size so that em-based measurements, // line-heights, etc., work. if (zoom) aFont->mSize = nsStyleFont::ZoomText(aPresContext, aFont->mSize); // enforce the user' specified minimum font-size on the value that we expose aFont->mFont.size = PR_MAX(aFont->mSize, aMinFontSize); Getting the cached pref from the pres context is quick. This will be a tiny performance hit for when the font is set -- but no worse than what ComputeFontData already does. (Perhaps the solution to both problems is a GetMinFontSize on the pres context that already has the equivalent of |IsChrome| done, but I'd rather do one thing at a time.)
> I think the only way the two can be different is if there's a min font size. Then why bother doing that check, if multiplying by that ratio is a no-op whenever the check is false? Or rather, why not make that check be a simple if (font->mFont.size != font->mSize) ? One other question, out of curiousity.... What is font->mSize if I do "font-size: 0" in a stylesheet?
The reason to do the minimum font size pref check is to know whether we can cache in the rule tree -- although another possibility would be to just explicitly say that we can never cache the text struct in the rule tree.
I would just go with: if (font->mFont.size != font->mSize) { // these two are different is if there's a min font size inherited = true; // don't cache ... }
The suggestion in comment 28 is incorrect, since it could cause us to cache a struct in the rule tree in a case where the font size is large (no difference) and then use that cached node when the font size is small rather than recomputing. (That is somewhat unlikely to happen, but considering that this whole thing is to work around when authors do things they shouldn't, I wouldn't be surprised to see it.)
Comment on attachment 135744 [details] [diff] [review] patch Oh, I see. You want to avoid caching no matter whether the min font size actually affected the font size in this case... r+sr=bzbarsky, then; I finally understand the comment. ;)
Attachment #135744 - Flags: superreview?(bz-vacation)
Attachment #135744 - Flags: superreview+
Attachment #135744 - Flags: review?(bz-vacation)
Attachment #135744 - Flags: review+
Well, the reason I was curious about a preference for minimum line-height is for cases where the size is fine but the author has spaced things too closely for my taste. I seem to see that a lot.
* { line-height: normal ! important } in userContent.css will handle that, no?
Fix checked in to trunk, 2003-11-17 21:53 -0800.
Status: ASSIGNED → RESOLVED
Closed: 21 years ago
Resolution: --- → FIXED
No longer blocks: 203888
*** Bug 203888 has been marked as a duplicate of this bug. ***
*** Bug 252458 has been marked as a duplicate of this bug. ***
Depends on: 786641
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: