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

Status: RESOLVED FIXED

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

Description

[Mikhail Sogrine]

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

Comment 1

[Mikhail Sogrine]

Attachment: Normal text

Comment 2

[Mikhail Sogrine]

Attachment: Same text when min font size is set

Comment 3

[Mikhail Sogrine]

Attachment: Same text when font size is decreased manually

Comment 4

[Boris Zbarsky [:bzbarsky]]

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.....

Comment 5

[Mikhail Sogrine]

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?

Comment 6

[Maarten van Hees]

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.

Comment 7

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

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.

Comment 8

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

What makes this difficult is that the font min size is per lang-group. 
(font.min-size.{fixed,variable}.<lang>)

Comment 9

[kill this account]

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.

Comment 10

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

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').

Comment 11

[rbs]

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.)

Comment 12

[kill this account]

I particularly attached to either one as long as we don't break things too much
for users.

Comment 13

[Madhur Bhatia]

Attachment: reduced testcase

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

Comment 14

[Sebastian von Knorring]

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.

Comment 15

[rbs]

Even text zoom has troubles with http://www.bn.com/. The comprehensive fix is
going to come from the full zoom bug 4821.

Comment 16

[Hal Canary]

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; }

Comment 17

[Boris Zbarsky [:bzbarsky]]

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

Comment 18

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

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

Comment 19

[Asa Dotzler [:asa]]

moving nomination from duped bug.

Comment 20

[Jason Clouse]

Would it be possible to add a "Minimum Line Height" preference to go along with
the "Minimum Font Size"?

Comment 21

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

There's no need for an additional preference -- we just need to scale the
line-height when the minimum font size applies.

Comment 22

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

Attachment: patch

Comment 23

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

Attachment: testcase for additional bug being fixed

This testcase shows the bug fixed by the typo the patch fixes (changing
aInherited to inherited).

Comment 24

[Boris Zbarsky [:bzbarsky]]

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.

Comment 25

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

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.)

Comment 26

[Boris Zbarsky [:bzbarsky]]

> 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?

Comment 27

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

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.

Comment 28

[rbs]

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
  ...
}

Comment 29

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

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 30

[Boris Zbarsky [:bzbarsky]]

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.  ;)

Comment 31

[Jason Clouse]

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.

Comment 32

[Boris Zbarsky [:bzbarsky]]

 * { line-height: normal ! important } 

in userContent.css will handle that, no?

Comment 33

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

Fix checked in to trunk, 2003-11-17 21:53 -0800.

Comment 34

[Jason Spiro]

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

Comment 35

[R.K.Aa.]

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