1403521 - text-decoration with wavy line in vertical writing modes is rendered offset to the right

Status: RESOLVED FIXED

(Core :: Layout: Text and Fonts, enhancement, P3)

Description

[Jonathan Kew [:jfkthame]]

When rendering a wavy text-decoration line in vertical mode, we draw it too far to the right. This appears to be specific to the 'wavy' line rendering; other decorations such as solid or dotted lines appear at the expected position.

Compare:

data:text/html,<div style="writing-mode:sideways-rl;font-size:60px;text-decoration:red wavy underline">hello world

data:text/html,<div style="writing-mode:sideways-rl;font-size:60px;text-decoration:red dotted underline">hello world

Or with vertical Chinese:

data:text/html;charset=utf-8,<div style="writing-mode:vertical-rl;font-size:60px;text-decoration:red wavy underline line-through overline;">%E4%BD%A0%E5%A5%BD

data:text/html;charset=utf-8,<div style="writing-mode:vertical-rl;font-size:60px;text-decoration:red dashed underline line-through overline;">%E4%BD%A0%E5%A5%BD

Comment 1

[Jonathan Kew [:jfkthame]]

The 'double' line style also suffers from this problem.

Comment 2

[Jonathan Kew [:jfkthame]]

Attachment: testcase for text-decoration rendering issues in vertical mode

Here's a sample file that demonstrates the issues with positioning of the text-decoration lines in vertical writing modes. Even the single line is slightly misplaced (compare sideways-lr and sideways-rl, which should look exactly like the horizontal example, with rotation), but the double and wavy lines make the issues much more obvious.

There is also a similar discrepancy in the SVG text, although currently we only support single lines there so the issue is less glaring than it is for the complex HTML decorations.

Comment 3

[Jonathan Kew [:jfkthame]]

Attachment: screenshot of the testcase with current Nightly on macOS

Comment 4

[Jonathan Kew [:jfkthame]]

Attachment: Correct positioning of text-decoration lines in vertical writing modes

Comment 5

[Jonathan Kew [:jfkthame]]

Attachment: screenshot after patch is applied

Comment 6

[Jonathan Kew [:jfkthame]]

Comment on attachment 8914098 [details] [diff] [review]
Correct positioning of text-decoration lines in vertical writing modes

This gives us the expected result for all the various writing modes and decoration line styles, as per testcase above. I'll see if I can put together a reftest to help verify it (and to make sure we don't re-break any of these cases next time we touch the code).

It's possible that reworking the nsCSSRendering::GetDecorationRect code to do all its workings in a line-relative coordinate system rather than physical coordinates would lead to some simplifications, and avoid the need to adjust for different modes in as many places. But that would be a more extensive rewrite that I'd prefer to consider separately; here, I'm aiming to just fix the currently-broken cases without affecting anything else.

Comment 7

[Jonathan Kew [:jfkthame]]

Attachment: Reftests for positioning of text-decoration in vertical mode

Here are a couple of simple reftests, comparing decorations in vertical (sideways-*) mode with the equivalent horizontal writing mode, rotated using a CSS transform. I've made the actual text invisible, as we don't get a pixel-perfect match between rotated rendering and natively-vertical rendering, but the decoration line positions should match pretty well. I've pushed this to try, to see if it'll need some fuzz annotations (which wouldn't surprise me, though it passes cleanly for me locally): https://treeherder.mozilla.org/#/jobs?repo=try&revision=6a618649896af1a0735bc6ace57092ab63736155

Comment 8

[Jonathan Kew [:jfkthame]]

Comment on attachment 8914098 [details] [diff] [review]
Correct positioning of text-decoration lines in vertical writing modes

Hmm, tryserver seems to be running into an unexpected editor-reftest crash on OS X; clearing r? on the patch while I look into that.

Comment 9

[Jonathan Kew [:jfkthame]]

Attachment: Correct positioning of text-decoration lines in vertical writing modes

Turns out it's possible we might call nsTextFrame::CombineSelectionUnderlineRect before the textframe has constructed its textrun, so we need to do EnsureTextRun here before trying to dereference mTextRun. This fixes the macOS crash in editor tests.

Comment 10

[Daniel Holbert [:dholbert] (PTO until 2/24)]

Comment on attachment 8916708 [details] [diff] [review]
Correct positioning of text-decoration lines in vertical writing modes

Review of attachment 8916708 [details] [diff] [review]:
-----------------------------------------------------------------

I'm willing to believe this is correct, but it needs a bit clearer documentation, I think.

::: layout/painting/nsCSSRendering.cpp
@@ +4246,5 @@
> +      if (aParams.vertical) {
> +        if (aParams.sidewaysLeft) {
> +          offset = aParams.offset + lineThickness - r.Height();
> +        } else {
> +          offset = aParams.offset - lineThickness;

It's unclear to me what this math is doing.  (In particular -- it's not clear how |offset| differs semantically from |aParams.offset| -- but maybe if that were clearer, these writing-mode-specific additions/subtractions would make more intuitive sense?)

Could you add a comment (perhaps alongside the declaration of the |offset| local variable) to clarify what it represents (and how that differs from |aParams.offset|, and why we add/subtract these various things to convert between them [to the extent that that doesn't become obvious])?

@@ +4271,5 @@
>        NS_ERROR("Invalid decoration value!");
>    }
>  
>    if (aParams.vertical) {
>      r.y = baseline + floor(offset + 0.5);

While we're here -- could you add some documentation for this "plus or minus floor(offset + 0.5)" logic, too?  It's not directly touched by this patch, but it's related, because it's the spot where this "offset" value (the thing being modified in this bug) actually gets used.

(It looks like we add the floor() expression in vertical writing modes, but subtract it in horizontal writing modes.)

Comment 11

[Daniel Holbert [:dholbert] (PTO until 2/24)]

Comment on attachment 8916612 [details] [diff] [review]
Reftests for positioning of text-decoration in vertical mode

Review of attachment 8916612 [details] [diff] [review]:
-----------------------------------------------------------------

Could you add some reftests for "vertical-rl" and "vertical-lr", too?  It looks like those were broken & being fixed here, too (based on your screenshots) -- and the math is clearly different for that case vs. for "sideways", so the existing reftests don't seem to cover that new codepath yet.

Comment 12

[Daniel Holbert [:dholbert] (PTO until 2/24)]

(In reply to Daniel Holbert [:dholbert] from comment #11)
> [...] the math is clearly different for that case vs. for "sideways"
> so the existing reftests don't seem to cover that new codepath yet.

(Er, I may have overstaded things there -- this ^^ was based on my recollection of the main patch, but upon looking at it again, I guess the math is only special for "sidewaysLeft".  So maybe your "sideways-rl" reftest is sufficient to provide code-coverage for the other vertical writing modes...?  Nonetheless -- it still seems like it'd be worth testing "vertical-rl" and/or "vertical-rl" for thoroughness.)

Comment 13

[Jonathan Kew [:jfkthame]]

I'm not sure we can readily reftest the decoration positioning in vertical-* modes, at least not using the same approach of comparing to a rotated horizontal-mode reference. The trouble is that in vertical-* modes, we'll be using a center baseline rather than the alphabetic baseline we use in horizontal or sideways-* modes, and therefore the positioning will differ in a significant (but font-metrics-dependent and not readily predictable) way.

I suppose by using a known font (perhaps even Ahem) for vertical-* tests, we may be able to allow for this in a reliable way and come up with a reference rendering we can use; I'll look into it.

Comment 14

[Jonathan Kew [:jfkthame]]

Attachment: Correct positioning of text-decoration lines in vertical writing modes

Thanks for calling me on this, and asking for documentation; in the process of trying to clarify things, I ended up with what I think is a much better patch. :) Instead of adding so many alternate codepaths to GetTextDecorationRectInternal, we can actually -eliminate- a coordinate-inversion that the caller was doing for sideways-lr mode, and keep GetTextDecorationRectInternal simpler as well. No point in flipping coordinates at one level, only to have to reverse calculations again further down.

Comment 15

[Daniel Holbert [:dholbert] (PTO until 2/24)]

Comment on attachment 8917115 [details] [diff] [review]
Correct positioning of text-decoration lines in vertical writing modes

Review of attachment 8917115 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for the documentation! This is much clearer.  r=me, though there's one thing that might merit further-clarification, per below:

::: layout/painting/nsCSSRendering.cpp
@@ +4225,5 @@
> +
> +  // Calculate adjusted offset based on writing-mode/orientation and thickness
> +  // of decoration line. The input value aParams.offset is the nominal position
> +  // (offset from baseline) where we would draw a single, thin decoration line;
> +  // but for a wavy or double line, we'll need to move the bounding rect of the

s/thin/infinitely-thin/, I think?  (or "zero-thickness")  I think that's what you're trying to say here, but I'm not sure.

(Right now, it's unclear whether "thin" means infinitely-thin, vs. a basic single-line.  This is an important distinction, because the former interpretation implies that no line-thickness has been factored into the positioning yet, vs. the latter implies that some has.)

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/91bdf32e14f5
Correct positioning of text-decoration lines in vertical writing modes. r=dholbert
https://hg.mozilla.org/integration/mozilla-inbound/rev/2ec64f31c306
Reftests for positioning of text-decoration in vertical mode. r=dholbert

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/91bdf32e14f5
https://hg.mozilla.org/mozilla-central/rev/2ec64f31c306