1220438 - 'text-decoration: underline' does not keep constant position in vertical writing (with various font sizes and mixed text)

Status: RESOLVED FIXED

(Core :: Layout: Block and Inline, defect)

Description

[Gérard Talbot]

Unprefixed tests
----------------

with various font-sizes
- - - - - - - - - - - -

http://test.csswg.org/source/css-writing-modes-3/underline-font-size-vrl-004.xht

http://test.csswg.org/source/css-writing-modes-3/underline-font-size-vlr-005.xht


with identical font-sizes
- - - - - - - - - - - - -

http://test.csswg.org/source/css-writing-modes-3/underline-mixed-vrl-002.xht

http://test.csswg.org/source/css-writing-modes-3/underline-mixed-vlr-003.xht


Expected results
----------------

There should be one and only one vertical straight, unbroken line.


Explanations
------------

When latin and east-asian glyphs of varying font sizes (and with the same font sizes) are used in mixed text-orientation (therefore with text centrally baseline-aligned), then the text-decoration of the parent box should be used across the parent box.


Notes
-----

- The 2 pairs of tests are identical except for glyph, lang attribute (language of glyphs) and writing-modes: mongolian glyph vs japanese glyph
- Chrome 48.0.2547.0 passes the underline-mixed-vrl-002.xht and underline-mixed-vlr-003.xht tests but it fails the underline-font-size-vrl-004.xht and the underline-font-size-vlr-005.xht tests
- Edge 12 passes all 4 tests
- I am using Firefox 45.0a1 buildID=20151031030207 and it fails the 2 vrl tests
- there are 2 other tests available also: 
http://test.csswg.org/source/css-writing-modes-3/underline-font-size-vrl-002.xht
http://test.csswg.org/source/css-writing-modes-3/underline-font-size-vlr-003.xht
- I use Linux 3.16.0-66-generic x86_64, Qt: 4.8.6, KDE 4.14.3; Kubuntu (trusty) 14.04.03 LTS
- I've searched for duplicates and did not find any.

Comment 1

[Gérard Talbot]

Worth mentioning and related to this bug report is

http://mxr.mozilla.org/mozilla-central/source/layout/reftests/writing-mode/1091058-1.html

Firefox 45 will *not* render the underline of <div class="v-rl"> straight and unbroken while the underline of <div class="v-lr"> is straight and unbroken. In that test, glyphs and lang are identical.

Chrome 48 render the underline of both <div class="v-rl"> and <div class="v-lr"> straight and unbroken.

Comment 2

[Shing Lyu [:shinglyu]]

Attachment: underline_test_case.xht

Comment 3

[Shing Lyu [:shinglyu]]

Attachment: underline_test_case_screenshot.png

I suspect that the problem is caused by the English characters are rotated 90 degree but CJK characters are not. So I created this test case to test it. Seems like in case 2, if all three characters are CJK, the underline looks nice. But if any of the character is English, the line breaks.

Since we have a lot of conditional code that looks like

if (isVerticalRL) {
  // Swap the x and y axis
}

I guess there is bug when we handle character rotation. Does that make sense?

Comment 4

[David Baron :dbaron:]

The bug could be that we're doing the wrong thing in particular rotation cases, but it should still be found in the code that determines underline position (most likely the computation of baselineOffset in nsTextFrame::GetTextDecorations, likely via frameBStartOffset being wrong).

Comment 5

[Shing Lyu [:shinglyu]]

I did some more tests and found out that
1. In vertical-lr, the underline works correctly, It breaks only in vertical-rl
2. The text position is wrong if I don't specify "lang=..."
3. The CJK characters' position are wrong if I use "text-orientation: sideways"

I file separate bugs for 2 and 3.

Comment 6

[Shing Lyu [:shinglyu]]

Attachment: underline_test_case.xht

New updated test cases that includes text-orientation:sideways and lang=...

Comment 7

[Gérard Talbot]

DHTML version
http://www.gtalbot.org/BugzillaSection/Bug1220438-text-decoration-writing-modes-DHTML.html

> 1. In vertical-lr, the underline works correctly, It breaks only in vertical-rl

Agreed.

> 2. The text position is wrong if I don't specify "lang=..."
> 3. The CJK characters' position are wrong if I use "text-orientation: sideways"

Agreed.

Note that Firefox 48.0a1 buildID=20160321030217 rendering is quite wrong if/when vertical-rl, overline and sideways are checked in the DHTML version test.

Comment 8

[Gérard Talbot]

Firefox 48.0a1 buildID=20160321030217 rendering is also quite wrong if/when sideways-rl and overline are checked in the DHTML version test.

Comment 9

[Shing Lyu [:shinglyu]]

Attachment: Patch that fixed vertical-rl but breaks in text-orientation:sideways

Since vertical-lr works but vertical-rl doesn't. I tried to delete this if block that handles vertical-rl only. Now almost all test cases passes except "writing-mode: vertical-rl; text-orientation:sideways". 

I don't quite understand the math behind all these text positioning, but at least I pinned down the code that causes this issue so I'll dig deeper from here.

Comment 10

[Shing Lyu [:shinglyu]]

Attachment: Broken test case "writing-mode: vertical-rl; text-orientation:sideways" after my patch

This is how "writing-mode: vertical-rl; text-orientation:sideways" looks like after removing the lines.

Comment 11

[Mats Palmgren (inactive)]

Attachment: testcase

I was about to report a bug on this testcase but I suspect it might be
the same as this bug.  I'd expect the underlines to have the same
relative position vs the text in all these boxes.  Currently, there
is a 1px offset between the underlines in the vertical cases.

Comment 12

[David Baron :dbaron:]

(In reply to Shing Lyu [:slyu] from comment #9)
> Created attachment 8733674 [details] [diff] [review]
> Patch that fixed vertical-rl but breaks in text-orientation:sideways
> 
> Since vertical-lr works but vertical-rl doesn't. I tried to delete this if
> block that handles vertical-rl only. Now almost all test cases passes except
> "writing-mode: vertical-rl; text-orientation:sideways". 
> 
> I don't quite understand the math behind all these text positioning, but at
> least I pinned down the code that causes this issue so I'll dig deeper from
> here.

This sounds like good progress.

However, I think the code you tried removing probably makes sense, given that for vertical-rl, we are measuring baselines from the right edge of the frame.

One way to make progress is probably to carefully check what numbers are relative to.  That is, you can look at (a) what position in the frame the value is relative to and (b) which way positive numbers point, for each writing-mode.

For example:  nsIFrame::GetLogicalBaseline measures offsets from the block-start border edge of the frame, in whichever of X or Y is appropriate (so generally gives negative results for vertical-rl).  At least, that's what the nsFrame implementation does; it may be worth double-checking that that's also what the nsInlineFrame and nsTextFrame implementations do.

From there, you can figure out if the way baselineOffset is computed in nsTextFrame::GetTextDecorations makes sense, and whether the way it (as mBaselineOffset) is used in nsTextFrame::DrawTextRunAndDecorations makes sense.  You can do this through some combination of reading code and looking at values in simple examples (in gdb, or even just via printf debugging).

You can also look at other related numbers (those whose computations affect frameBStart, ascent, or mBaselineOffset/baselineOffset) in similar ways, and hopefully find the problem.

Comment 13

[Xidorn Quan [:xidorn] UTC+11]

(In reply to Mats Palmgren (:mats) from comment #11)
> Created attachment 8735602 [details]
> testcase
> 
> I was about to report a bug on this testcase but I suspect it might be
> the same as this bug.  I'd expect the underlines to have the same
> relative position vs the text in all these boxes.  Currently, there
> is a 1px offset between the underlines in the vertical cases.

I would say this is actually a different issue than what the test shows.

I've had a patch passes the tests as well as my own tests. But I need to figure out how to make the code look more sensible.

Comment 14

[Xidorn Quan [:xidorn] UTC+11]

Sorry, slyu, but since I've had a working patch, I'd steal this bug from you :)

Also, remove the ni? jfkthame.

I still need to figure out how can I write the tests given the tests provided by Gérard Talbot are not reftests.

Comment 15

[Xidorn Quan [:xidorn] UTC+11]

Hmmm, it seems we don't have any existing test for testing the unbrokenness of text decoration lines... Just patch then.

Comment 16

[Xidorn Quan [:xidorn] UTC+11]

Attachment: MozReview Request: Bug 1220438 - Correct baseline offset computation of text decoration for vertical-rl. r=jfkthame

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

Comment 17

[Shing Lyu [:shinglyu]]

I'm trying to understand how you solved it. So looks like the frameBStartOffset if measured from the block-end for vertical-rl, so it should be descent instead of ascent, but the original code used ascent.

I'm confused about line 5023, what are we substracting the "offset" from "frameBStartOffset" if nearestBlockFound? And what's the difference between offset and frameBStartOffset?

Comment 18

[Xidorn Quan [:xidorn] UTC+11]

Looking at the code of LazyGetLineBaselineOffset, it seems to me that would not work properly for vertical text at all, including vertical-lr.

That branch is triggered only when you specify a different value to vertical-align, which is not a very common use case for vertical text, and that might not affect my bug 1097499, so I'm not going to look into that deeper.

That's worth another bug, though. And if you are interested, you can try digging deeper there. Gérard Talbot may want to create some tests for those cases as well.

Comment 19

[Gérard Talbot]

(In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #14)
> I still need to figure out how can I write the tests given the tests
> provided by Gérard Talbot are not reftests.

Tests on 'text-decoration: underline' can not be reftests because the exact position of the underline is up to the UA. Firefox could (?) be using a font metric info (ySubscriptYOffset or some other font metric info) to position the underline but again the spec does not require this.

"
The exact position and thickness of line decorations is UA-defined in this level.
"
https://drafts.csswg.org/css-text-decor-3/#text-underline-position-property

- - - - - - - 

There is a bunch of 'text-decoration: underline' tests already available at

http://test.csswg.org/suites/css-text-decor-3_dev/nightly-unstable/html/chapter-2.htm#s2.6

and in

http://test.csswg.org/suites/css-writing-modes-3_dev/nightly-unstable/html/chapter-7.htm#s7.5

- - - - - - - 

A few tests (with each writing-modes values) based on the "ABCD" example (figure 6) given in 
https://drafts.csswg.org/css-text-decor-3/#text-underline-position-property
could be created and submitted.

I have already created:

http://www.gtalbot.org/BrowserBugsSection/CSS3TextDecoration/text-decoration-position-abcd.html

and

http://www.gtalbot.org/BrowserBugsSection/CSS3TextDecoration/text-decoration-position-sample-dhtml.html

- - - - - - - 

(In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #18)
> That branch is triggered only when you specify a different value to
> vertical-align, which is not a very common use case for vertical text, and
> that might not affect my bug 1097499, so I'm not going to look into that
> deeper.
> 
> That's worth another bug, though. And if you are interested, you can try
> digging deeper there. Gérard Talbot may want to create some tests for those
> cases as well.

As I understand your description (different value of vertical-align), I could create 5 static tests from text-decoration-position-sample-dhtml.html ... but again, those would not be reftests.

Comment 20

[Jonathan Kew (:jfkthame)]

Comment on attachment 8737112 [details]
MozReview Request: Bug 1220438 - Correct baseline offset computation of text decoration for vertical-rl. r=jfkthame

https://reviewboard.mozilla.org/r/43771/#review40607

::: layout/generic/nsTextFrame.cpp:4969
(Diff revision 1)
> +  // frameBStartOffset represents the offset from our baseline to f's
> +  // top (in horizontal wm) or left (in vertical wm) boundary in our
> +  // coordinate space. Note that, although the name includes "BStart",
> +  // when the writing mode is vertical-rl, it is actually block-end,
> +  // and consequently it starts from the descent instead of ascent.
> +  nscoord frameBStartOffset =
> +    wm.IsVerticalRL() ? GetSize().width - mAscent : mAscent;

Let's use a different name here, as this isn't really a logical-coordinate value such as we normally have for bStart etc. I think we could describe it as a "physical block-axis value" ... it's in the block axis, as opposed to the inline axis, but measured in a standard physical-coordinate direction (i.e. top-to-bottom or left-to-right) rather than the block-progression direction.

So I propose

    nscoord physicalBlockStartOffset = ...

or something like that - wdyt?

Comment 21

[Xidorn Quan [:xidorn] UTC+11]

Comment on attachment 8737112 [details]
MozReview Request: Bug 1220438 - Correct baseline offset computation of text decoration for vertical-rl. r=jfkthame

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

Comment 22

[Jonathan Kew (:jfkthame)]

Comment on attachment 8737112 [details]
MozReview Request: Bug 1220438 - Correct baseline offset computation of text decoration for vertical-rl. r=jfkthame

https://reviewboard.mozilla.org/r/43771/#review40945

Thanks for looking into this; it's always been broken, and this clearly makes it much more correct.

In testing this on OS X, at least, I'm still seeing one remaining issue: in sideways-rl mode, it looks like the decoration lines are appearing fractionally too far to the right -- i.e. underline is too tight to the baseline of the text, and overline is a little bit far away, when compared to the same text in horizontal mode. My guess is that we might have the sign reversed when accounting for the thickness of the decoration line itself.

E.g. compare the rendering of

    data:text/html,<div style="font:24px arial;writing-mode:sideways-rl"><u>hello <span style="text-decoration:overline">world

with the same example in horizontal mode.

(This is not a new problem resulting from your patch, it's already the case with trunk code; but as you're fixing things here, I wondered if you could check on that at the same time, and include a fix if it's straightforward, or else file a followup?)

::: layout/generic/nsTextFrame.cpp:4972
(Diff revision 2)
> -  // coordinate space
> +  WritingMode wm = GetWritingMode();
> +
> +  // physicalBlockStartOffset represents the offset from our baseline
> +  // to f's physical block start, which is top in horizontal writing
> +  // mode, and left in vertical writing modes, in our coordinate space.
> +  // This is physical block start is logical block start in most cases,

drop the first "is" here

::: layout/generic/nsTextFrame.cpp
(Diff revision 2)
>    // during the particular iteration
> -  nscoord frameBStartOffset = mAscent,
> +  nscoord baselineOffset = 0;
> -          baselineOffset = 0;
> -
> -  bool nearestBlockFound = false;
> -  bool vertical = GetWritingMode().IsVertical();

There are a number of wm.IsVertical() tests below; I'd be inclined to keep a local `bool vertical = wm.IsVertical();` rather than repeating the IsVertical() test each time (and relying on the compiler to optimize it).

Comment 23

[Xidorn Quan [:xidorn] UTC+11]

(In reply to Jonathan Kew (:jfkthame) from comment #22)
> Comment on attachment 8737112 [details]
> MozReview Request: Bug 1220438 - Correct baseline offset computation of text
> decoration for vertical-rl. r?jfkthame
> 
> https://reviewboard.mozilla.org/r/43771/#review40945
> 
> Thanks for looking into this; it's always been broken, and this clearly
> makes it much more correct.
> 
> In testing this on OS X, at least, I'm still seeing one remaining issue: in
> sideways-rl mode, it looks like the decoration lines are appearing
> fractionally too far to the right -- i.e. underline is too tight to the
> baseline of the text, and overline is a little bit far away, when compared
> to the same text in horizontal mode. My guess is that we might have the sign
> reversed when accounting for the thickness of the decoration line itself.
> 
> E.g. compare the rendering of
> 
>     data:text/html,<div style="font:24px
> arial;writing-mode:sideways-rl"><u>hello <span
> style="text-decoration:overline">world
> 
> with the same example in horizontal mode.
> 
> (This is not a new problem resulting from your patch, it's already the case
> with trunk code; but as you're fixing things here, I wondered if you could
> check on that at the same time, and include a fix if it's straightforward,
> or else file a followup?)

I believe it's worth a followup. The related code of the issue you mentioned is not the same as this bug.

This bug is about fixing the baseline offset calculation for inline elements (so that the line does not break), while the decoration line painting is the work of nsCSSRendering::PaintDecorationLine and its friends. It seems to me that various places in those functions which check params.vertical should probably check the exact writing mode instead.

Fixing this specific issue may not be very hard, since it is just some "solid" lines. However, other line styles, especially "double" and "wavy" could be more tricky. The math behind them does need some more investigation, which IMO doesn't fit in this bug. In addition, investigating those line styles would uncover some other issues on overflow area calculation, which further would need some more work.

Shing Lyu might be interested on these issues.

Comment 24

[Xidorn Quan [:xidorn] UTC+11]

Comment on attachment 8737112 [details]
MozReview Request: Bug 1220438 - Correct baseline offset computation of text decoration for vertical-rl. r=jfkthame

Given comment 23, do you think this patch is fine to go with the comments addressed?

Comment 25

[Jonathan Kew (:jfkthame)]

Comment on attachment 8737112 [details]
MozReview Request: Bug 1220438 - Correct baseline offset computation of text decoration for vertical-rl. r=jfkthame

https://reviewboard.mozilla.org/r/43771/#review40981

OK, sounds fair enough.

Comment 26

[Xidorn Quan [:xidorn] UTC+11]

Comment on attachment 8737112 [details]
MozReview Request: Bug 1220438 - Correct baseline offset computation of text decoration for vertical-rl. r=jfkthame

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

Comment 27

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5fbeb35adff1

Comment 28

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/5fbeb35adff1

Comment 29

[Gérard Talbot]

While using Firefox 48.0a1 buildID=20160406030221, I get expected results

- for tests in comment 0

- for all the text-decoration-* tests and underline-* tests in 
http://test.csswg.org/suites/css-writing-modes-3_dev/nightly-unstable/html/chapter-7.htm#s7.5

- for 
http://mxr.mozilla.org/mozilla-central/source/layout/reftests/writing-mode/1091058-1.html

- for attachment 8732079 [details]


For your information:

I do get constant (underline and overline) position when I use 
http://www.gtalbot.org/BugzillaSection/Bug1220438-text-decoration-writing-modes-DHTML.html
but a) the overline thickness (vertical-rl and overline) is not always constant and b) there is a repaint issue (that's bug 1163455 ; still happening) too. 

c) Some tests in bug 1229743 still fail.

d)
http://www.gtalbot.org/BrowserBugsSection/CSS3TextDecoration/text-decoration-position-sample-dhtml.html
also still fail

Gérard

Comment 30

[Xidorn Quan [:xidorn] UTC+11]

(In reply to Gérard Talbot from comment #29)
> While using Firefox 48.0a1 buildID=20160406030221, I get expected results
> 
> - for tests in comment 0
> 
> - for all the text-decoration-* tests and underline-* tests in 
> http://test.csswg.org/suites/css-writing-modes-3_dev/nightly-unstable/html/
> chapter-7.htm#s7.5
> 
> - for 
> http://mxr.mozilla.org/mozilla-central/source/layout/reftests/writing-mode/
> 1091058-1.html
> 
> - for attachment 8732079 [details]

These are things this bug intends to fix.

> For your information:
> 
> I do get constant (underline and overline) position when I use 
> http://www.gtalbot.org/BugzillaSection/Bug1220438-text-decoration-writing-
> modes-DHTML.html
> but a) the overline thickness (vertical-rl and overline) is not always
> constant and b) there is a repaint issue (that's bug 1163455 ; still
> happening) too. 
> 
> c) Some tests in bug 1229743 still fail.

I'd suggest these two are actually the same issue. Both of them seems to be issue on calculation of overflow rect.

> d)
> http://www.gtalbot.org/BrowserBugsSection/CSS3TextDecoration/text-decoration-
> position-sample-dhtml.html
> also still fail

This is a mix of the overflow rect issue above, and the issue of vertical writing modes with vertical-align I mentioned in comment 18. Could you file a separate bug for the latter issue?

Comment 31

[Gérard Talbot]

(In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #30)

> > d)
> > http://www.gtalbot.org/BrowserBugsSection/CSS3TextDecoration/text-decoration-
> > position-sample-dhtml.html
> > also still fail
> 
> This is a mix of the overflow rect issue above, and the issue of vertical
> writing modes with vertical-align I mentioned in comment 18. Could you file
> a separate bug for the latter issue?

bug 1262974 has been created

Comment 33

[Gérard Talbot]

Xidorn,

In this DHTML test

http://www.gtalbot.org/BugzillaSection/Bug1229743text-decoration-underline-different-font-size.html

1- set writing-mode to vertical-rl (or vertical-lr)
2- set text-orientation to mixed

and the text-decoration lines for the "uxz" strings will not keep constant position across the box. 

We can also see the same layout (in the Arial part) in

https://bug1229743.bmoattachments.org/attachment.cgi?id=8694691

Comment 34

[Tanvir Rahman]

I have reproduced this bug with Nightly 45.0a1(2015-10-31) on Windows 10, 64 bit!

The Bug's fix is now verified on Beta 48.0b9.

Build ID 	20160718142219
User Agent 	Mozilla/5.0 (Windows NT 10.0; WOW64; rv:48.0) Gecko/20100101 Firefox/48.0

[bugday-20160727]