832313 - anomalies in line-height calculation on OS X

Status: NEW

(Core :: Layout: Text and Fonts, defect)

Description

[Jonathan Kew [:jfkthame]]

Attachment: screenshot of the testcase on OS X

There's something odd about default line-height calculation in the Mac font backend. See the testcase in the URL, and attached screenshot of its result. Note how the line heights for the cases with *even* font sizes are unexpectedly large compared to the rest of the heights.

In addition, it's clear that the default line-height is not being rounded to an integer number of device pixels, which is normally desirable in order to provide a more consistent visual result.

(This is probably the gfxMacFont issue mentioned in bug 657864 comments 0 and 6, which was not the primary focus of that bug, and never got investigated.)

The failure to round default line-height to pixels means that layout/reftests/text/subpixel-lineheight-1 is very fragile; it just happens to pass because at font-size 14.5px with the default font, the line-height does come out as an integer, but at many other font sizes it would fail.

Comment 1

[Jonathan Kew [:jfkthame]]

It turns out the problem here is only partly related to the rounding discussed in bug 657864; there's also a further issue.

In GetNormalLineHeight, we find the following code fragment[1]:

  case eCompensateLeading:
    if (!internalLeading && !externalLeading)
      normalLineHeight = NSToCoordRound(emHeight * NORMAL_LINE_HEIGHT_FACTOR);
    else
      normalLineHeight = emHeight+ internalLeading + externalLeading;
    break;

This has two problems, I think:

(1) When the NORMAL_LINE_HEIGHT_FACTOR kicks in, to override the metrics that were returned by the font, we do not ensure that the resulting line-height is an integer number of device pixels. This will lead to irregular line spacing when the text is rendered at pixel-snapped vertical positions, despite the efforts in the font-metrics code (bug 657864) to return whole-pixel default line heights.

(2) Because of the rounding of line-height and ascent/descent in the font-metrics code, it's possible that the externalLeading and/or internalLeading values may fluctuate up *and down* by a pixel as the font size increases. E.g. at a particular font-size increment, the (rounded) line-height increases by one device pixel, and so do both ascent and descent; hence one of the leading values must *decrease* by one pixel.

This means that with some fonts, where the nominal (external+internal) leading values are sufficiently small, the pixel-rounded values from font metrics may fluctuate between zero and non-zero as the font size changes. And this in turn means that we'll get abrupt jumps between the two methods of computing normalLineHeight here. This results in the unexpected (out-of-order) line-height values seen in the testcase here.

[1] http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsHTMLReflowState.cpp#2408

Comment 2

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

(In reply to Jonathan Kew (:jfkthame) from comment #1)
> This has two problems, I think:
> 
> (1) When the NORMAL_LINE_HEIGHT_FACTOR kicks in, to override the metrics
> that were returned by the font, we do not ensure that the resulting
> line-height is an integer number of device pixels. This will lead to
> irregular line spacing when the text is rendered at pixel-snapped vertical
> positions, despite the efforts in the font-metrics code (bug 657864) to
> return whole-pixel default line heights.

Seems easy to fix.

> (2) Because of the rounding of line-height and ascent/descent in the
> font-metrics code, it's possible that the externalLeading and/or
> internalLeading values may fluctuate up *and down* by a pixel as the font
> size increases. E.g. at a particular font-size increment, the (rounded)
> line-height increases by one device pixel, and so do both ascent and
> descent; hence one of the leading values must *decrease* by one pixel.

One option seems to be adding a method to the font metrics API, returning a boolean that says whether the font provided any leading.

Another option would be getting rid of the compensation entirely.


Are you interested in fixing this?

Comment 3

[Jonathan Kew [:jfkthame]]

(In reply to David Baron [:dbaron] (needinfo? me) (UTC-8) from comment #2)

> > (2) Because of the rounding of line-height and ascent/descent in the
> > font-metrics code, it's possible that the externalLeading and/or
> > internalLeading values may fluctuate up *and down* by a pixel as the font
> > size increases. E.g. at a particular font-size increment, the (rounded)
> > line-height increases by one device pixel, and so do both ascent and
> > descent; hence one of the leading values must *decrease* by one pixel.
> 
> One option seems to be adding a method to the font metrics API, returning a
> boolean that says whether the font provided any leading.
> 
> Another option would be getting rid of the compensation entirely.

I was wondering about a third option:

  normalLineHeight = std::max(NSToCoordRound(emHeight * NORMAL_LINE_HEIGHT_FACTOR),
                              emHeight + internalLeading + externalLeading);

> Are you interested in fixing this?

I'm interested, but probably won't work on it right now - so if you or anyone else cares to take it, feel free. Any change here will want visual review across a range of platforms/fonts/sites, in addition to whatever unit tests we have.

Comment 4

[Brad Werth [:bradwerth]]

Attachment: Bug 832313 Part 1: Change GetNormalLineHeight() when under the eCompensateLeading preference to better compensate for fonts with erratic leading metrics.

Review commit: https://reviewboard.mozilla.org/r/87792/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/87792/

Comment 5

[Brad Werth [:bradwerth]]

Attachment: Bug 832313 Part 2: Add a mochitest to confirm the default font has increasing line heights for increasing font sizes.

Review commit: https://reviewboard.mozilla.org/r/87794/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/87794/

Comment 6

[Brad Werth [:bradwerth]]

(In reply to Jonathan Kew (:jfkthame) from comment #3)
> I was wondering about a third option:
> 
>   normalLineHeight = std::max(NSToCoordRound(emHeight *
> NORMAL_LINE_HEIGHT_FACTOR),
>                               emHeight + internalLeading + externalLeading);

I implemented this approach in the proposed patch.

Comment 7

[Jonathan Kew [:jfkthame]]

Comment on attachment 8803567 [details]
Bug 832313 Part 1: Change GetNormalLineHeight() when under the eCompensateLeading preference to better compensate for fonts with erratic leading metrics.

https://reviewboard.mozilla.org/r/87792/#review86952

I think this makes sense as a basis for the "compensate" calculation, but by itself it's not sufficient: it doesn't address the issue of rounding (see comment 1, point (1)) to device pixels.

And I suspect that might explain a bunch of the test failures seen on tryserver; with this change, the emHeight * NORMAL_LINE_HEIGHT_FACTOR may be kicking in for cases where the font metrics had a small but non-zero leading, so we previously used the (pixel-rounded) font metrics to compute line-height, but now the (non-rounded) emHeight * NORMAL_LINE_HEIGHT_FACTOR calculation may override that.

So I think we should first fix the rounding issue (to device pixels, not CSS px!) here, and then see what test failures are left; it's likely that at least some of the tests that failed on try are overly fragile (or simply bogus) and need to be adjusted, but let's deal with the rounding here first.

(Clearing r? for now, as although I think this moves in the right direction, it's not enough by itself.)

Comment 8

[Jonathan Kew [:jfkthame]]

Comment on attachment 8803568 [details]
Bug 832313 Part 2: Add a mochitest to confirm the default font has increasing line heights for increasing font sizes.

https://reviewboard.mozilla.org/r/87794/#review86958

::: layout/style/test/test_line_height_continuity.html:43
(Diff revision 1)
> +
> +  let lastLineHeight = 0;
> +  for (let i = minFontSize; i <= maxFontSize; i += stepFontSize) {
> +    let thisLineHeight = testFontOfSize(i);
> +
> +    ok(thisLineHeight >  lastLineHeight, "Font size " + i + " line height " + thisLineHeight + " should be bigger than font size " + (i - stepFontSize) + " line height of " + lastLineHeight + ".");

I think you'll want this to be >=, not >, particularly once pixel-rounding of the line height is properly implemented. With rounding, the computed height won't change for every half-pixel increase in font size; but we can assert that it should never _decrease_ (as it currently does in some cases).

Comment 9

[Jonathan Kew [:jfkthame]]

(In reply to Jonathan Kew (:jfkthame) from comment #8)
> I think you'll want this to be >=, not >, particularly once pixel-rounding
> of the line height is properly implemented. With rounding, the computed
> height won't change for every half-pixel increase in font size; but we can
> assert that it should never _decrease_ (as it currently does in some cases).

Aha -- I triggered some additional Win8 tests on your try run, and sure enough, this shows up there.

Comment 10

[Brad Werth [:bradwerth]]

Comment on attachment 8803567 [details]
Bug 832313 Part 1: Change GetNormalLineHeight() when under the eCompensateLeading preference to better compensate for fonts with erratic leading metrics.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/87792/diff/1-2/

Comment 11

[Brad Werth [:bradwerth]]

Comment on attachment 8803568 [details]
Bug 832313 Part 2: Add a mochitest to confirm the default font has increasing line heights for increasing font sizes.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/87794/diff/1-2/

Comment 12

[Brad Werth [:bradwerth]]

Updated both the test as noted in comment 8, and updated the code to implement device pixel rounding (flooring) as noted in comment 7. Here's a list of the tests that failed in the first try push, and what I think their current status is:

* layout/style/test/test_line_height_continuity.html: fixed.
* layout/forms/test/test_textarea_resize.html: working again.
* layout/reftests/bugs/289480.html: still fails. This is a complicated case that "draws" a picture using divs, and this patch adds a striped effect in part of the image, due to some misalignment of overlapping div elements.
* layout/reftests/text-overflow/aligned-baseline.html: working again.
* layout/reftests/flexbox/flexbox-align-self-baseline-horiz-3.xhtml: fails due to differing block size calculations for an input element in a flexbox (looks correct -- same as a sibling div element) versus an input element in a display:block container (looks too short). This may be a larger issue that was relying on fragile behavior of line height calculations to be correct.
* layout/reftests/text/subpixel-lineheight-1a.html: fails because it attempts to measure pixel-snapping of text with a 9.6px vs a 10.4px top margin. The code will obviously not support this since the device pixel clamping happens without considering margins. Not sure what to make of this -- also relying on fragile behavior or is there later device-pixel clamping code that needs attention?
* layout/reftests/css-ruby/lang-specific-style-1.html: unknown because it had been failing on Win8 which I can't test directly. We'll see what try server yields with the new code.
* layout/reftests/css-sizing/min-intrinsic-with-percents-across-elements.html: unknown status since it was failing on Android. Looks like it compares sizes of input elements, which is a noted failure above.
* layout/reftests/forms/select/out-of-bounds-selectedindex.html: unknown status Android test that measures select and option elements.
* layout/reftests/image-element/element-paint-native-widget.html: unknown status Android test that measures text input elements.
* layout/reftests/svg/foreignObject-form-theme.svg: unknown status Android test that measures select and option elements.

So it looks like the greatest problem is with a difference to the rendering of input elements. I'll keep poking at it.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=391046911139

Comment 13

[Jonathan Kew [:jfkthame]]

Comment on attachment 8803567 [details]
Bug 832313 Part 1: Change GetNormalLineHeight() when under the eCompensateLeading preference to better compensate for fonts with erratic leading metrics.

https://reviewboard.mozilla.org/r/87792/#review87282

r=me for trying this change to the computation, but obviously we still need to figure out what to do about the various tests before we can land it.

::: layout/generic/ReflowInput.cpp:2724
(Diff revision 2)
> +      NSIntPixelsToAppUnits(
> +        NSAppUnitsToIntPixels(
> +          NSToCoordRound(emHeight * NORMAL_LINE_HEIGHT_FACTOR),
> +          aFontMetrics->AppUnitsPerDevPixel()
> +        ),
> +        aFontMetrics->AppUnitsPerDevPixel()
> +      ));

This should work OK, I think, but it seems to involve more rounding steps than strictly necessary, which probably costs perf/code-size and may occasionally affect the outcome in edge cases. Specifically, there's no need to round the emHeight*NORMAL_LINE_HEIGHT_FACTOR calculation in appUnits, prior to converting it to device pixels.

How about rewriting this along the lines of

    normalLineHeight = std::max(
      emHeight + internalLeading + externalLeading,
      NSIntPixelsToAppUnits(
        NSToIntRound(emHeight * NORMAL_LINE_HEIGHT_FACTOR
                     / aFontMetrics->AppUnitsPerDevPixel()),
        aFontMetrics->AppUnitsPerDevPixel()));

to skip the intermediate rounding?

Comment 14

[Jonathan Kew [:jfkthame]]

Comment on attachment 8803568 [details]
Bug 832313 Part 2: Add a mochitest to confirm the default font has increasing line heights for increasing font sizes.

https://reviewboard.mozilla.org/r/87794/#review87284

Comment 15

[Jonathan Kew [:jfkthame]]

(In reply to Brad Werth [:bradwerth] from comment #12)
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=391046911139

I think you should also make sure to check the Linux64 reftests; they're currently "excluded" on Treeherder, but I believe we're about to make them visible by default, and it looks like this is breaking some 1302389-scrolled-rect-* tests.

Comment 16

[Brad Werth [:bradwerth]]

Comment on attachment 8803567 [details]
Bug 832313 Part 1: Change GetNormalLineHeight() when under the eCompensateLeading preference to better compensate for fonts with erratic leading metrics.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/87792/diff/2-3/

Comment 17

[Brad Werth [:bradwerth]]

Comment on attachment 8803568 [details]
Bug 832313 Part 2: Add a mochitest to confirm the default font has increasing line heights for increasing font sizes.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/87794/diff/2-3/

Comment 18

[Brad Werth [:bradwerth]]

(In reply to Jonathan Kew (:jfkthame) from comment #13)
> How about rewriting this along the lines of
> 
>     normalLineHeight = std::max(
>       emHeight + internalLeading + externalLeading,
>       NSIntPixelsToAppUnits(
>         NSToIntRound(emHeight * NORMAL_LINE_HEIGHT_FACTOR
>                      / aFontMetrics->AppUnitsPerDevPixel()),
>         aFontMetrics->AppUnitsPerDevPixel()));
> 
> to skip the intermediate rounding?

Did this in most recent patch, thank you. While attempting to diagnose and fix tests, I discovered that text rendering has a minimum line height that supersedes the value returned from GetNormalLineHeight(), which is the only reason why some of the current tests are passing. layout/reftests/flexbox/flexbox-align-self-baseline-horiz-3.xhtml passes even when the returned line height is 0! If you have insights into where this minimum line height is applied, I'd appreciate the tip.

Comment 19

[Jonathan Kew [:jfkthame]]

Hmm... well, ComputeLineHeight() doesn't always rely on GetNormalLineHeight(), that depends on the kind of value the line-height property has. But I guess that's not what you're dealing with here.

Another point is that for HTMLInputElements, at least, there's a minimum applied here:
https://dxr.mozilla.org/mozilla-central/rev/d26ac63f1b81c3fce35448a7c502e95e0b5c56c0/layout/generic/ReflowInput.cpp#2795-2804

The main thing you're seeing, though, is probably the result of nsLineLayout::VerticalAlignFrames, which stacks up the frames that make up a block. This takes account of the actual height of the frames within the line, which can result in more height than the computed line-height would suggest (think about what happens when there's a large <img> within a line of text, for example). If line-height is set to an explicit value, the actual size of text frames on the line will be ignored for this purpose, but with line-height:normal, the size of (non-empty) text frames will also be considered:
https://dxr.mozilla.org/mozilla-central/rev/d26ac63f1b81c3fce35448a7c502e95e0b5c56c0/layout/generic/nsLineLayout.cpp#2237-2280

Comment 20

[Brad Werth [:bradwerth]]

I'm not actively working on this anymore. Taking myself off the bug.