1808788 - Firefox places Ahem text 1px too far to the right (line-top side), with vertical writing-mode and font-size:25px (and 100% pixel scaling, i.e. no HiDPI)

Status: NEW

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

Description

[Daniel Holbert [:dholbert]]

STR:

1. Load attached testcase, in Firefox on a device with 100% pixel scaling

(this is important; the bug doesn't repro at some HiDPI settings)

EXPECTED RESULTS:
A 25px orange square, followed by a purple square.

ACTUAL RESULTS:
There's a 1px strip of purple at the right of the first square.
There's a 1px strip of orange at the left of the second square.

This is an indication that we're laying out the Ahem characters at the wrong baseline position, placing them 1px too "high up" in line-relative terms (too far to the right).

We fail at least one WPT tests as a result of this. This is the root cause of our failure at
https://wpt.fyi/results/css/css-position/sticky/position-sticky-writing-modes.html

(The screenshots of the test failure there show a 1px strip of mismatch on either edge of the rectangles, are effectively due to this issue here.)

Comment 1

[Daniel Holbert [:dholbert]]

Attachment: (resource for use in testcase: Ahem.ttf)

Comment 2

[Daniel Holbert [:dholbert]]

Attachment: testcase 1

Comment 3

[Daniel Holbert [:dholbert]]

Attachment: testcase 2 (just added a scale, to make the mismatching strip larger)

Here's the same testcase but with a CSS transform scale-up, to make it easier to see the mismatching strip.

Comment 4

[Daniel Holbert [:dholbert]]

Attachment: screenshot of testcase 1 and 2 in Firefox Nightly (left) vs Chrome (right)

Comment 5

[Daniel Holbert [:dholbert]]

When we fix this, we should be able to remove this .ini file (and possibly adjust/remove some other WPT failure annotations):
https://searchfox.org/mozilla-central/rev/4ebfb48f7e82251145afa4a822f970931dd06c68/testing/web-platform/meta/css/css-position/sticky/position-sticky-writing-modes.html.ini

Comment 6

[Daniel Holbert [:dholbert]]

Looking in rr, it looks like we end up with x=-30 (i.e. a half pixel) on the text frame here. Here's a snippet of the frame tree:

            line@7f689413a390 count=1 state=block,clean,prevmarginclean,not-impacted,not-wrapped,no-break,clear-before:none,clear-after:none(x=0, y=0, w=1500, h=1500) ink-overflow=(x=-30, y=0, w=1560, h=1500) scr-overflow=(x=-30, y=0, w=1560, h=1500) <
              Block(div)(3)@7f6894139e10 parent=7f6894139bb0 next=7f6894139fc8 (x=0, y=0, w=1500, h=1500) wm=v-lr-ltr logical-size=((1500 x 1500)) ink-overflow=(x=-30, y=0, w=1560, h=1500) scr-overflow=(x=-30, y=0, w=1560, h=1500) [content=7f6888b045e0] [cs=7f68891d05c8] modified <
                line@7f6894139f78 count=1 state=inline,clean,prevmarginclean,not-impacted,not-wrapped,no-break,clear-before:none,clear-after:none(x=0, y=0, w=1500, h=1500) wm=v-lr-ltr cs=((0 x 1500)) logical-rect=(0,0,1500,1500) ink-overflow=(x=-30, y=0, w=1560, h=1500) scr-overflow=(x=-30, y=0, w=1560, h=1500) <
                  Text(0)"X"@7f6894139ed8 parent=7f6894139e10 (x=-30, y=0, w=1560, h=1500) wm=v-lr-ltr logical-size=((1500 x 1560)) parent-wm=v-lr-ltr cs=((1500 x 1500)) logical-rect=(0,-30,1500,1560) [content=7f6888b07280] [cs=7f68891d0b68:-moz-text] modified [run=7f68891fd200][0,1,T] 
                >
              >
            >

This 30-app-unit (half-pixel) offset gets calculated in nsLineLayout::VerticalAlignLine, at this line:

pfd->mBounds.BStart(lineWM) += baselineBCoord;

...where at this point pfd->mBounds.BStart(lineWM) is -780 and baselineBCoord is 750 (note the 30 app unit difference in magnitude).

I think 750 is the "correct/desired" value there, for my test to produce expected-results; and 780 is the "problematic" value. 750 makes sense as a value here since it works out to 12.5px, which is precisely half of the font-size. (And we use a central baseline for vertical-writing-mode text, which is why half-the-font-size makes sense in baselineBCoord). Whereas 780 is 13px, which is 12.5px rounded-up-to-the-nearest-pixel. And I think that difference is what's causing our positioning to be off by a half-pixel here (and then visually a whole pixel).

This might really all be ~correct/allowable, though -- arguably part of the issue here is just that we've got text with a central baseline, with that central baseline happening to fall on a fractional pixel value. Normally that wouldn't be noticeable, but it's undesirable for Ahem which aims to have whole-pixel alignment for reliable test rendering. 25px happens to be the recommended font-size for WPT tests, per https://web-platform-tests.org/writing-tests/ahem.html?highlight=ahem#usage , but since the central baseline of 25px is a fractional value, perhaps we should change that recommendation for tests that use a vertical writing mode?

Comment 7

[Daniel Holbert [:dholbert]]

The 13px (aka 780 app unit) values seem to come from our usage of nsFontMetrics::MaxDescent() when we evaluate this expression:

	    nscoord fontAscent =
	        wm.IsLineInverted() ? fm->MaxDescent() : fm->MaxAscent();

That ends up getting us here:

	static gfxFloat ComputeMaxDescent(const gfxFont::Metrics& aMetrics,
	                                  gfxFontGroup* aFontGroup) {
[...]
	  return floor(std::max(minDescent, aMetrics.maxDescent) + 0.5);
	}

Here, aMetrics.maxDescent is 12.5 (the value we expect as our central baseline for 25px-font-size text), and it's what std::max chooses. We then add 0.5 and then floor the result, ending up at 13. (i.e. we round to the nearest whole pixel value). And then up the stack, we convert that into app units and get 780 (the app unit equivalent of 13px)

So, tl;dr: we're are explicitly deciding against using a fractional-pixel-value for our central baseline (in the rounding "add-0.5-and-floor" logic in ComputeMaxDescent here). So we use 13px as our central baseline for the line layout here, which is why we end up off-by-1px.

Comment 8

[Daniel Holbert [:dholbert]]

The maxDescent rounding logic comes from bug 1136557, for what it's worth:
https://hg.mozilla.org/mozilla-central/rev/f02bd650e228#l1.15

I don't know to what extent it's possible to reconsider that rounding or add further nuance there. But I think as long as 13px is considered a valid central baseline for 25px-sized Ahem text (as I think it probably is), we're liable to get off-by-1px test failures like this; and we should probably seek to get tests and the recommendation updated accordingly.

jfkthame, does this all make sense to you? (Or do you think it's worth trying to achieve the half-pixel central baseline here to match the test expectations?)

Comment 9

[Daniel Holbert [:dholbert]]

(In reply to Daniel Holbert [:dholbert] from comment #0)

We fail at least one WPT tests as a result of this. This is the root cause of our failure at
https://wpt.fyi/results/css/css-position/sticky/position-sticky-writing-modes.html

I've spun off bug 1808997 to adjust this WPT test so that it's no longer reliant on this fiddly central-baseline-rounding edge case, BTW; and I've transferred the dependencies to that bug.

Comment 10

[Daniel Holbert [:dholbert]]

Attachment: Bug 1808788: Use an even font-size in WPT position-sticky-writing-modes.html, so that it's not laying out text with a fractional central baseline. r?jfkthame

Before this patch, this test was using Ahem text with a 25px font-size and a
vertical writing mode. The vertical writing-mode causes the text to be
baseline-aligned using a central baseline, which ends up being at a
half-pixel position, since our font-size is odd. Specifically: the central
baseline is 25px/2 = 12.5px inwards from the edge of the glyph.

The UA may choose to round this fractional central baseline to a whole pixel
value when sizing/placing content, which in turn casues this test to fail with
content being 1px away from where the test expects it to be.

This patch addresses this issue by making the test use an even number as the
font-size (30px), which produces a nice whole-number pixel value for the
central basleine. This avoids creating a depending on the UA's choice of
rounding behavior.

This makes the test pass in Firefox, and I believe it continues to pass in
other browsers where it was already passing.

Comment 11

[Phabricator Automation]

Comment on attachment 9311161 [details]
Bug 1808788: Use an even font-size in WPT position-sticky-writing-modes.html, so that it's not laying out text with a fractional central baseline. r?jfkthame

Revision D166232 was moved to bug 1808997. Setting attachment 9311161 [details] to obsolete.

Comment 12

[Daniel Holbert [:dholbert]]

(In reply to Daniel Holbert [:dholbert] from comment #9)

I've spun off bug 1808997 to adjust this WPT test

(And of course I copypasted the wrong bug number into my editor when writing a patch for that bug. :) Sorry for the bug spam; I've moved the patch/phab-revision to bug 1808997 now.)

Comment 13

[Jonathan Kew [:jfkthame]]

This reproduces for me on Windows and Linux (even on a high-dpi display, though it comes and goes at various zoom levels), but interestingly, not on macOS.

Actually, I'm not sure this is exclusive to vertical writing mode (though it may be more evident there). Removing the writing-mode property from the testcase, I see similar mismatches at the top/bottom of the glyph (depending on zoom level and display resolution).

I suspect the root of the issue is that we try to "snap" metrics to whole (device) pixels in gfx/thebes when setting up the font instances, but there's a tension between rounding individual ascent/descent/line-gap metrics vs rounding the overall extent of the font, and exactly where the baseline ends up may depend on those decisions.

So this would require a careful study of how we set up and use the various font metrics, which is kinda gnarly and fragile (and platform-specific).