770780 - Implement CSS3 text module text-underline-position

Status: RESOLVED FIXED

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

Description

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

I think that if we would implement text-underline-position and add our own values for selection underlines, we could test all selection underlines on all platforms.

Comment 1

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: part.1 Add CSS3 -moz-text-underline-position property

Comment 2

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: part.2 Implement only simple behavior of text-underline-position

Comment 3

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: testcase

Comment 4

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: part.1 Add CSS3 -moz-text-underline-position property

Comment 5

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: part.2 Implement only simple behavior of text-underline-position

Comment 6

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: part.3 Shift down block box's underline positioned under to the lowest text frame's edge in each line

Comment 7

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: part.4 Shift down inline box's underline positioned under to the lowest text frame's edge

Comment 8

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: part.5 Support text-underline-position on floating first-letter

Comment 9

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

I'll post reftests and then, I'll request reviews.

Comment 10

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: part.1 Add CSS3 -moz-text-underline-position property

Comment 11

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

test builds here:
https://tbpl.mozilla.org/?tree=Try&usebuildbot=1&rev=b23c197b2c20

Comment 12

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: part.6 Add reftests for text-underline-position

Comment 13

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: part.3 Shift down block box's underline positioned under to the lowest text frame's edge in each line

Comment 14

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: part.6 Add reftests for text-underline-position

Comment 15

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

The newest test builds:
https://tbpl.mozilla.org/?tree=Try&usebuildbot=1&rev=e8eaf32916e8

Comment 16

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 681938 [details] [diff] [review]
part.2 Implement only simple behavior of text-underline-position

This patch implements "alphabetic" perfectly (except floating first-letter path).

For "underline", this isn't correct if the decorated box has some other text frames whose bottom content edge is lower than the text frame. It's being fixed by following patches.

Comment 17

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 683002 [details] [diff] [review]
part.3 Shift down block box's underline positioned under to the lowest text frame's edge in each line

This patch fixes the "under" underline position when the underline is owned by its parent block or propagated by ancestor block.

nsLineLayout stores the lowest edge position of each line into LowestEdgeForUnderUnderlineOfParent property of each nsIFrame. The value will be used as top of the underline.

Comment 18

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 681940 [details] [diff] [review]
part.4 Shift down inline box's underline positioned under to the lowest text frame's edge

This patch fixes the "under" underline position when an inline elements owns it.

The underline offset is stored to the inline frame's LowestEdgeForUnderUnderline property.

Comment 19

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 681941 [details] [diff] [review]
part.5 Support text-underline-position on floating first-letter

This patch fixes the overflow rect of floating first-letter frame child when its text-underline-position isn't auto.

Comment 20

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 683003 [details] [diff] [review]
part.6 Add reftests for text-underline-position

The reftests for underline position.

I have no idea how to test floating first letter case since floating first letter's rect is computed from its glyph rect unlike normal text frame.

Comment 21

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

Comment on attachment 681959 [details] [diff] [review]
part.1 Add CSS3 -moz-text-underline-position property

Normally we'd be implementing new properties behind a preference rather
than with a prefix, but given that we already implement a bunch of·
the other properties from http://dev.w3.org/csswg/css-text-decor-3/
behind a prefix, this is the right thing to do for now (though we will
hopefully be able to unprefix soon).

>-CSS_KEY(alphabetic, alphabetic)
>+//CSS_KEY(alphabetic, alphabetic)

remove, don't comment

(And maybe remove the existing commented ones from the SVG section
too... not that there's really even any reason for a separate SVG
section anymore.)

nsCSSPropList.h:

Please change VARIANT_AHK to VARIANT_HK; use auto in the keyword
list for keyword properties and not in the variant mask.  (You're
doing both, which is definitely wrong.)

nsRuleNode.cpp:

Remove the SETDSC_AUTO and replace the "// auto" parameter with 0.

r=dbaron with that

Comment 22

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

Could you explain in a bit more detail what behavior you've implemented for the different values of the property (auto, alphabetic, under)?  Having an explanation of what you're intending would help in reviewing the patch.

Comment 23

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to David Baron [:dbaron] from comment #22)
> Could you explain in a bit more detail what behavior you've implemented for
> the different values of the property (auto, alphabetic, under)?  Having an
> explanation of what you're intending would help in reviewing the patch.

auto: This is current behavior. Respecting the offset information of the font metrics, checking font.blacklist.underline_offset and adjusting in descent space.

alphabetic: always positioned at least 1px below the baseline. The gap guarantees the underline doesn't touch to the glyph which is on the baseline.

under: In the part.2 patch, the position is the em box's bottom edge. This is the basic behavior in most cases. In the part.3, the position is the most-bottom bottom edge of the textframe in the line in the element.

Comment 24

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: testcase #1 specified to block element

Comment 25

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: testcase #2 including different font size text and aligned middle

Comment 26

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: testcase #3 for under underline and vertical-align

Comment 27

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: testcase #4 specified to inline element

Comment 28

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

This isn't quite what I would have expected, although maybe it is the logical conclusion of the way the spec iscurently worded.

I'd have expected that 'alphabetic' and 'auto' would both roughly be the current behavior (where we use the underline position metrics from the font metrics), except that 'auto' has allowances for using different behavior for non-Latin scripts, whereas 'alphabetic' does not.

I think failing to use the underline position from the font metrics for either 'alphabetic' or 'auto' sounds like a bug to me, but I'm curious what fantasai thinks.

Comment 29

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Font metrics' underline offset does NOT indicate the best alphabetic underline position believed by the designer.

Some Latin fonts may indicate accounting underline position for the best position for its glyph and usage. Others may indicate alphabetic underline position. The others might have odd underline position which we ignore/adjust the underline offset of.

Some non-Latin fonts such as CJK fonts may indicate accounting underline position for the non-Latin characters. Others may indicate alphabetic underline position (probably such fonts are majority). The others might have odd underline position, for example, we met a Chinese font whose underline position is alphabetic position but Chinese characters are not aligned to baseline, aligned to the bottom of em-box. So, the underline crossed the Chinese character glyphs.

Anyway, I have no idea to know each font is whether Latin font or not since we know some fonts lie us.

So, I believe that 'auto' should keep current behavior. It also keeps compatibility with older Gecko. When web designers want to specify the underline position and need the compatibility with other browsers, they can use 'alphabetic' or 'under' since they can be laid out without font information.

Comment 30

[fantasai]

> I think failing to use the underline position from the font metrics for either 'alphabetic'
> or 'auto' sounds like a bug to me, but I'm curious what fantasai thinks.

As Masayuki says, there are fonts that place the underline metrics at the accounting-underline position. I agree it's best to use the font metrics in general, but you'd want to detect that case and ignore the underline metrics from such fonts.

For 'auto', probably the best behavior would be to do whatever the font says unless that position crosses subscripts or characters from an Asian script, and in that case switch to 'under'. If our current behavior is to just do whatever the font says, then that's probably alright for a first pass at 'auto', though.

[I suspect a halfway-decent heuristic for whether alphabetic underlining is appropriate for a character is to check, if it's a letter or number, that it belongs to a bicameral script.]

Comment 31

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

That sounds like a unnecessarily large amount of heuristics.  Heuristics like that are bad for implementation complexity and performance, interoperability (unless they're specified), and predictability of behavior to authors.

Comment 32

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to fantasai from comment #30)
> > I think failing to use the underline position from the font metrics for either 'alphabetic'
> > or 'auto' sounds like a bug to me, but I'm curious what fantasai thinks.
> 
> As Masayuki says, there are fonts that place the underline metrics at the
> accounting-underline position. I agree it's best to use the font metrics in
> general, but you'd want to detect that case and ignore the underline metrics
> from such fonts.

Hmm, if it were easy to detect whether the underline offset of the font is for accounting or alphabetic, we'd do it. However, I don't think that it's easy. In most cases, the font's descent is a couple of dev pixels. So, it's hard to understand if the underline offset is for accounting underline or alphabetic underline with proper gap for easy to read.

> For 'auto', probably the best behavior would be to do whatever the font says
> unless that position crosses subscripts or characters from an Asian script,
> and in that case switch to 'under'. If our current behavior is to just do
> whatever the font says, then that's probably alright for a first pass at
> 'auto', though.

We respect the font's underline offset, basically. If the underline offset it too far or over the baseline, we ignore that. I don't think that we should switch the underline position as 'under' when the text includes CJK characters because it causes changing the underline position in editable element's underline.

> [I suspect a halfway-decent heuristic for whether alphabetic underlining is
> appropriate for a character is to check, if it's a letter or number, that it
> belongs to a bicameral script.]

(In reply to David Baron [:dbaron] from comment #31)
> That sounds like a unnecessarily large amount of heuristics.  Heuristics
> like that are bad for implementation complexity and performance,
> interoperability (unless they're specified), and predictability of behavior
> to authors.

I agree with dbaron.

Comment 33

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

Comment on attachment 681938 [details] [diff] [review]
part.2 Implement only simple behavior of text-underline-position

I'm going to cancel these review requests until the spec is stable.

Once the spec is stable (e.g., at least once the issues in http://dev.w3.org/csswg/css-text-decor-3/issues-lc-2013 are all resolved by the CSS working group, and probably with a little time afterwards to stabilize), it might make sense to update the patches to the current spec.

(I'd also be interested to know why you think this property is important to implement.)

Comment 34

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

I posted some questions of the newest spec:
https://lists.w3.org/Archives/Public/www-style/2015May/0153.html

Comment 35

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

fantasai: Could you check the email (https://lists.w3.org/Archives/Public/www-style/2015May/0153.html) posted into www-style?

I think that text-underline-position should be [auto|under] || [left|right] for backward compatibility.

Comment 36

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

https://lists.w3.org/Archives/Public/www-style/2015Jun/0144.html
Thank you, fantasai!

I'll rewrite the patches with the new spec.

Comment 37

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Another idea has been posted to www-style from Apple.

So, I think that we shouldn't fix this bug at enabling vertical layout. I think that it's enough to set underline position in vertical layout. IE and Chrome must do so.

Comment 38

[Xidorn Quan [:xidorn] UTC+11]

This property has been moved to CSS Text Decoration Module.

Comment 39

[yisibl]

https://drafts.csswg.org/css-text-decor-3/#text-underline-position-property

Comment 40

[yisibl]

We must be test vertical writing mode.

Comment 41

[Gérard Talbot]

DHTML test of text-underline-position: [ auto | alphabetic | under ] in all 5 writing modes:

http://www.gtalbot.org/BugzillaSection/Bug770780Implement-text-underline-position-dhtml.html

When 'text-underline-position' is set to 'alphabetic', then the descenders of glyphs "p", "j", "y" must overlap the orange underline line in all 5 writing modes. The painting order has been defined in section E.2 of CSS2.x:
http://www.w3.org/TR/CSS21/zindex.html#painting-order
http://www.w3.org/TR/CSS22/zindex.html#painting-order

- - - - - - 

Firefox 56.0a1 buildID=20170723144049 does not support 'text-underline-position' when set to 'alphabetic' for 'vertical-rl' and 'vertical-lr' writing modes: in such code scenario, the descenders of glyphs "p", "j", "y" should overlap the orange underline line.

- - - - - - 

Firefox 56.0a1 buildID=20170723144049 does not support 'text-underline-position' when set to 'under' for 'sideways-rl' and 'sideways-lr' writing modes: in such code scenario, the descenders of glyphs "p", "j", "y"  should NOT overlap the orange underline line. In such code scenario, the descenders of glyphs "p", "j", "y" must NOT touch the orange underline line.

Comment 42

[Gérard Talbot]

Oops! I got confused... 'alphabetic' value of text-underline-position has been removed from the spec! I will immediately modify the test accordingly...

Comment 43

[Gérard Talbot]

DHTML test of text-underline-position: auto | [ under || [ left | right ] in all 5 writing modes in vertical typographic mode:

http://www.gtalbot.org/BugzillaSection/Bug770780Implement-text-underline-position-verti-typog-mode-dhtml.html

In horizontal typographic mode, 'under left', 'under right', 'left' and 'right' values are treated as 'under'.

Firefox 56.0a1 buildID=20170723144049 does NOT support 'text-underline-position' at all and does not support '-moz-text-underline-position' at all either. So I got confused there too... 

Now (sorry for the previous errors), those 2 tests should be correct and useful when Firefox supports 'text-underline-position' (or even '-moz-text-underline-position').

Comment 44

[Xidorn Quan [:xidorn] UTC+11]

Koji told me something interesting today.

Some Japanese epub readers use this property to put the underline to the left of Japanese text, and then use overline for underline, this is for working around that WebKit doesn't put underline on the right by default for Japanese.

This basically mean, any implementation which does the right thing for Japanese but doesn't have text-underline-position would have problem with those epub readers.

Comment 45

[Cameron McCormack (:heycam)]

We currently have this prioritised to work on some time in 2019.

Comment 46

[Jonathan Kew [:jfkthame]]

The original patches here are drastically bit-rotted by now (they predate stylo, for one thing, as well as the recent text-decoration enhancements), so it probably makes most sense to start afresh.

I have a new set of patches for the basic text-underline-position property.

These do not address making adjustments to the underline position to account for descendants of the decorated box (e.g. with larger font-size), as suggested in the long "Note" at https://www.w3.org/TR/css-text-decor-3/#text-underline-position-property and discussed further in https://www.w3.org/TR/css-text-decor-4/#line-position. Currently we do not implement the full behavior described there, but anyhow the spec notes that it is still "under review" (issue 2). I think that should be a followup once the spec is more mature.

Comment 47

[Jonathan Kew [:jfkthame]]

Attachment: Bug 770780 - Add support for parsing of the CSS text-underline-position property. r=emilio

Comment 48

[Jonathan Kew [:jfkthame]]

Attachment: Bug 770780 - Implement rendering support for text-underline-position in nsTextFrame. r=emilio

Depends on D54722

Comment 49

[Jonathan Kew [:jfkthame]]

Attachment: Bug 770780 - Enable CSS text-underline-position property on Nightly builds. r=emilio

Depends on D54723

Comment 50

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/309af05a80a5
Add support for parsing of the CSS text-underline-position property. r=emilio
https://hg.mozilla.org/integration/autoland/rev/dbb961ddfd68
Implement rendering support for text-underline-position in nsTextFrame. r=emilio
https://hg.mozilla.org/integration/autoland/rev/ad9f6cf23bb3
Enable CSS text-underline-position property on Nightly builds. r=emilio

Comment 51

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/309af05a80a5
https://hg.mozilla.org/mozilla-central/rev/dbb961ddfd68
https://hg.mozilla.org/mozilla-central/rev/ad9f6cf23bb3

Comment 52

[Sebastian Zartner [:sebo]]

Mentioned the addition of this property at https://developer.mozilla.org/en-US/docs/Mozilla/Firefox/Experimental_features#text-underline-position.

Sebastian

Comment 53

[Ryan VanderMeulen [:RyanVM]]

This seems like a good thing to call out in the Beta73 relnotes. Can you please set relnote-firefox? and fill out the form, Jonathan?

Comment 54

[Jonathan Kew [:jfkthame]]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #53)

This seems like a good thing to call out in the Beta73 relnotes. Can you please set relnote-firefox? and fill out the form, Jonathan?

Hmm.... currently, this is only preffed-on by default for Nightly builds, so a relnote seems premature. Though maybe we should consider allowing it to go to beta/release as well, given that it's been on nightly for a month with no issues.