1607308 - wrong text-underline-offset zero position with text-underline-position:under

Status: RESOLVED FIXED

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

Description

[Bill Goldstein [:aja] (UTC-6)]

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:73.0) Gecko/20100101 Firefox/73.0

Expected results:

Seems that with text-underline-position:under the text-underline-offset:auto Underline Offset Origin (Zero Position) is incorrectly relative to alphabetic baseline rather than the text-under edge.

Possible blocker for bug 1606997 "Let CSS text-underline-position property ride the train to release".

Comment 1

[Jonathan Kew [:jfkthame]]

This is part of a broader set of changes that were just made to the CSS Text Decoration spec last week; see

https://drafts.csswg.org/css-text-decor-4/#text-underline-position-property
https://drafts.csswg.org/css-text-decor-4/#underline-offset
https://drafts.csswg.org/css-text-decor-4/#line-offset-zero

Briefly, the from-font value of text-underline-offset (which we already support) is removed and instead a from-font value is added to text-underline-position; and then text-underline-offset is measured from a newly-defined "zero position" that depends on the value of position.

Also, both text-underline-offset and text-decoration-thickness are extended to support percentage values. We should do that in a separate bug, I guess.

Comment 2

[Jonathan Kew [:jfkthame]]

Attachment: Bug 1607308 - Move the from-font value from text-underline-offset to text-underline-position, as per recent spec changes, and fix interaction between position and offset. r=emilio

Depends on D59777

Comment 3

[Bill Goldstein [:aja] (UTC-6)]

After reviews, if you push a try build with this and patch from 1607534 including Win64, I'd be happy to test before it hits autoland.

Comment 4

[Jonathan Kew [:jfkthame]]

Thanks Bill! There's actually a try build already at https://treeherder.mozilla.org/#/jobs?repo=try&revision=9c79c4e2ffeba83e1c562ff9b270a4a71be9ea7b you could test - I don't remember if the patches there are exactly what's under review now but it's certainly close, and behavior ought to be the same I think.

I'll do another try build after any last adjustments, but if you have a chance to test that build already and let me know if it behaves as expected, that'd be great.

Comment 5

[Bill Goldstein [:aja] (UTC-6)]

Don't know if it's offsetting incorrectly from text-under edge, or according to spec,
but text-underline-position:under and text-underline-offset:auto has descenders touching the underline.
Setting offset to 1px, the descenders and underline don't touch.
16px Times New Roman.

Comment 6

[Bill Goldstein [:aja] (UTC-6)]

Same with setting offset to .0625em or 6.25% (1/16px).

Comment 7

[Bill Goldstein [:aja] (UTC-6)]

FWIW, viewed at 200% Zoom, it looks like that with an extra 1px or .0625em or 6.25% offset, it'd match Chromium's text-underline-position:under (they don't currently implement text-underline-offset property).

Comment 8

[Jonathan Kew [:jfkthame]]

Hmm... it's not entirely clear to me what text-underline-offset:auto ought to do when text-underline-position:under is in effect; I was assuming it should result in a zero offset, but AFAICS the spec does not explicitly require that (or any other particular behavior), so we may want to adjust it.

But in any case, close examination indicates that we're not getting the "zero position" quite right, so that position:under and offset:0 places the underline slightly higher than it should, regardless of whether auto should behave like zero in this case or not. So I'll look into that; it may indicate an issue with the font metrics we're retrieving to determine where the descent should be.

Comment 10

[Jonathan Kew [:jfkthame]]

(In reply to Jonathan Kew (:jfkthame) from comment #8)

But in any case, close examination indicates that we're not getting the "zero position" quite right, so that position:under and offset:0 places the underline slightly higher than it should, regardless of whether auto should behave like zero in this case or not. So I'll look into that; it may indicate an issue with the font metrics we're retrieving to determine where the descent should be.

OK, I see what's going on here -- we're using the wrong "descent" metric for position: under. We should base the under position on the font's maxDescent value, which comes directly from the descent field in the hhea or OS/2 table of the font, not on emDescent which is a synthetic value based on partitioning the em-height into ascent and descent portions, in proportion to the font's declared ascent and descent. This is used to determine where to put the baseline within the overall em height, but if the font's ascent+descent ≠ em-size then it may not match the actual font metrics.

Switching use of emDescent to maxDescent when handling position:under fixes the "zero position" of the underline.

The remaining question is what to do for offset:auto in this case: the spec leaves this up to implementations ("the UA chooses an appropriate offset for underlines") in all cases except when position:from-font was used, so it's not "wrong" for us to use an offset of zero here (as the patch currently does), but on consideration, I think it'd be better to apply the font's underlinePosition so that the resulting underline is slightly below the font's descent, just as the default underline is slightly below the alphabetic baseline. An author who wants the underline exactly at the descent can use offset:0 to achieve this.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e55052ed4064
Move the from-font value from text-underline-offset to text-underline-position, as per recent spec changes, and fix interaction between position and offset. r=emilio

Comment 12

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Failed to create upstream wpt PR due to merge conflicts. This requires fixup from a wpt sync admin.

Comment 13

[Andrei Ciure[:aciure]]

Backed out 2 changesets (bug 1607308, bug 1607534) for causing text-decoration-underline-position-vertical-ja.html to permafail
https://hg.mozilla.org/integration/autoland/rev/7b3bfdc4fd78aa3ad4bae727b0319d7378c33dbf

push that caused the backout: https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=testfailed%2Cbusted%2Cexception&classifiedState=unclassified&revision=64dab97e6878b19ebcd97477bd84a7d2c90ef663

failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&searchStr=android%2C7.0%2Cx86-64%2Copt%2Cweb%2Cplatform%2Ctests%2Ctest-android-em-7.0-x86_64%2Fopt-geckoview-web-platform-tests-reftests-e10s-3%2Cw%28wr3%29&fromchange=261538034212154bb1d49200ef46d7075d21b589&tochange=8a4c5eff7a71989d66ec4b58e7f423de2fba247e

Comment 14

[Jonathan Kew [:jfkthame]]

Ah, this actually causes a currently-failing test to start passing, so we need to adjust the metadata. Sorry I missed that. (The test concerned is still dubious, as noted in the issue pointed to by the annotation, but it now happens to pass on android at least.)

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ae6fc7e3e68f
Move the from-font value from text-underline-offset to text-underline-position, as per recent spec changes, and fix interaction between position and offset. r=emilio

Comment 16

[Bill Goldstein [:aja] (UTC-6)]

Nice work.
text-underline-offset:auto text-underline-position:under with above patch now looks to be a proper accounting-style underline...
it clears the descenders (with all text-decoration styles, i.e. dashed, dotted, double, solid, and wavy ).
Will test again when the autoland build with the refactoring in Bug 1612822 is complete.

Comment 17

[Bill Goldstein [:aja] (UTC-6)]

Same as above with Bug 1612822 refactor, so text-underline-offset:auto with text-underline-position:under seems okay now.
I also tried text-underline-offset:0 with text-underline-position:under and that does NOT clear the descenders, however.

Comment 18

[Jonathan Kew [:jfkthame]]

(In reply to Bill Goldstein [:aja] (UTC-6) from comment #17)

Same as above with Bug 1612822 refactor, so text-underline-offset:auto with text-underline-position:under seems okay now.

Great, that's what I would expect - thanks for testing.

I also tried text-underline-offset:0 with text-underline-position:under and that does NOT clear the descenders, however.

Right, this will place the underline at (not slightly below) the font's descender metric, which (depending how the font designer has set the ascent/descent metrics) is likely to just touch the bottom of typical descenders. I believe that's the expected behavior.

Comment 19

[Bill Goldstein [:aja] (UTC-6)]

I'd expect zero offset for position under not to touch descenders at all, and therefore be unaffected by ink skip, as is now the case for auto offset.

FWIW, looks like https://bugs.chromium.org/p/chromium/issues/detail?id=785230 is now "Assigned" (though I don't see an assignee).
Zero position interop should be the main concern.

Comment 20

[Jonathan Kew [:jfkthame]]

I'd expect zero offset for position under not to touch descenders at all

The spec says that the zero position should be the "text-under edge", but I'm not sure where (or whether) that term is defined... I took it to mean the font's descent metric, but maybe this needs to be clarified. (It's complicated by the fact that OpenType font files contain up to three different "descent" metrics, which may all differ.)

There is also no guarantee that the "descent" metric provided by the font, and used by Firefox as the "text-under edge" here, actually corresponds to the bottom of the descenders of the glyphs.

If you're concerned that the interpretation of the zero-offset position for under here is wrong, I think it'd be helpful to bring this up with the CSS working group and seek clarification. It's not clear to me what other basis besides "the descent metric from the font" we could usefully rely on.

Comment 21

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/ae6fc7e3e68f

Comment 22

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/21754 for changes under testing/web-platform/tests

Comment 23

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Can't merge web-platform-tests PR due to failing upstream checks:
Github PR https://github.com/web-platform-tests/wpt/pull/21754
* Community-TC (pull_request) (https://community-tc.services.mozilla.com/tasks/groups/LI5cxGaXRRux4S3A2GOsNQ)

Comment 24

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream PR merged by jgraham