Open Bug 1358377 Opened 7 years ago Updated 1 year ago

line box height calculation doesn't match CSS2 in how it considers multiple fonts

Categories

(Core :: Layout: Block and Inline, enhancement)

enhancement

Tracking

()

REOPENED
Tracking Status
firefox55 --- affected

People

(Reporter: dbaron, Unassigned)

References

Details

(Keywords: css2)

CSS 2.1 describes how 'line-heights' are calculated.  The two key parts are:

10.6.1: https://drafts.csswg.org/css2/visudet.html#inline-non-replaced
10.8 and 10.8.1: https://drafts.csswg.org/css2/visudet.html#line-height

The way CSS 2.1 describes things is that inlines contribute to the line box height, but text doesn't.  Each inline should consider the bounds of all of the text in it for its content height (10.6.1), and then the difference between that and the line-height is added as a half-leading on each side, considering each glyph separately.  (Note that this produces unbalanced lines when font fallback is involved in only some lines.)  Note that in reality line-height:normal is also determined based on fonts, but how it is is undefined; see https://github.com/w3c/csswg-drafts/issues/1254.

What Gecko does has a few problems:

 * we consider the bounds of all of the fonts used when we compute the content box size of an nsTextFrame (nsTextFrame::ReflowText calls gfxTextRun::BreakAndMeasureText which calls gfxTextRun::MeasureText which iterates over each font used and calls gfxTextRun::AccumulateMetricsForRun or gfxTextRun::AccumulatePartialLigatureMetrics, which in turn union the ascents and descents together, but we don't consider it for any of the calculations we do for the content-box height of an inline, and we never add external leading to this size.

* we specially consider the content-box size of a text frame for line-height calculations (contrary to the spec, but it doesn't make a huge difference) only when 'line-height' is 'normal'.  This means that 'line-height: normal' also unions in the ascent/descent of text, but *unioned* with the other incorrect calculations we do on inlines.  This is done in the setting of the canUpdate variable in nsLineLayout::VerticalAlignFrames which was added in bug 99010 in https://github.com/mozilla/gecko-dev/commit/5b02d7ca8a98ed0c80339f49f926fbf4862046b9

* when we compute the content-box height of an inline, we only use the first choice font (via nsLayoutUtils::SetBSizeFromFontMetrics called from nsInlineFrame::ReflowFrames) rather than using all used fonts as the spec describes.  This means we generally have a size that's too small if font fallback occurred (although it could be too large on one or both sides if no glyphs from the first choice font are used).

* On inlines and blocks, when 'line-height' is 'normal', we consider the external leading from the font metrics.  But I believe we're only getting this from the first-choice font, and we're adding this to the content-box size of the inline that doesn't match the spec as described above.  We never add external leading from any font to the size we got from all fonts.  What we should actually do depends on the resolution of https://github.com/w3c/csswg-drafts/issues/1254.


In order to fix this, we should probably:
 * remove the canUpdate hack in VerticalAlignFrames and stop considering text on its own
 * make the content box height calculations for inlines that we do in nsInlineFrame::ReflowFrames build up their sizes from their child text frames (when there are ) to match what the spec says in 10.6.1
 * figure out whether we want to consider the external leading from the first choice font or all fonts.  Probably all fonts is best, which means we probably want to compute it during text measurement the same time we compute ascent and descent.  We then need to propagate this information from text measurement to text frames and then to inline frames.


I'm under the impression that WebKit is substantially closer to the spec here, and probably Chromium and Edge as well.
The discussion in https://github.com/w3c/csswg-drafts/issues/1254#issuecomment-296129092 did suggest that we shouldn't rush here.  In particular:
 * the spec might change
 * there are reasons to think Gecko's behavior may be superior to the present spec, both in terms of leading to preserving baseline rhythm right now, and in terms of being a good approach leading into line-height-step.
Status: RESOLVED → REOPENED
Resolution: INACTIVE → ---
Severity: normal → S3
You need to log in before you can comment on or make changes to this bug.