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

RESOLVED FIXED in Firefox 48

Status

()

defect
RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: bugzilla, Assigned: xidorn)

Tracking

(Blocks 1 bug, {testcase})

Trunk
mozilla48
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox45 affected, firefox48 fixed)

Details

Attachments

(6 attachments, 1 obsolete attachment)

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.
Blocks: writing-mode
Keywords: testcase
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.
Flags: needinfo?(jfkthame)
See Also: → 1229743
See Also: → 1175846
Summary: 'text-decoration: underline' with various font sizes and mixed text → 'text-decoration: underline' does not keep constant position in vertical writing (with various font sizes and mixed text)
Assignee: nobody → slyu
Status: NEW → ASSIGNED
Posted file underline_test_case.xht (obsolete) —
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?
Flags: needinfo?(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).
Flags: needinfo?(dbaron)
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.
New updated test cases that includes text-orientation:sideways and lang=...
Attachment #8732079 - Attachment is obsolete: true
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.
Firefox 48.0a1 buildID=20160321030217 rendering is also quite wrong if/when sideways-rl and overline are checked in the DHTML version test.
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 is how "writing-mode: vertical-rl; text-orientation:sideways" looks like after removing the lines.
Posted file 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.
(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.
(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.
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.
Assignee: slyu → quanxunzhen
Flags: needinfo?(jfkthame)
Hmmm, it seems we don't have any existing test for testing the unbrokenness of text decoration lines... Just patch then.
Blocks: 1097499
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?
Flags: needinfo?(quanxunzhen)
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.
Flags: needinfo?(quanxunzhen)
(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.
Attachment #8737112 - Flags: review?(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 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/
Attachment #8737112 - Flags: review?(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).
Attachment #8737112 - Flags: review?(jfkthame)
(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 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?
Attachment #8737112 - Flags: review?(jfkthame)
Attachment #8737112 - Flags: review?(jfkthame) → review+
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 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/
Attachment #8737112 - Attachment description: MozReview Request: Bug 1220438 - Correct baseline offset computation of text decoration for vertical-rl. r?jfkthame → MozReview Request: Bug 1220438 - Correct baseline offset computation of text decoration for vertical-rl. r=jfkthame
https://hg.mozilla.org/mozilla-central/rev/5fbeb35adff1
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
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
(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?
(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
See Also: 1175846
Duplicate of this bug: 1175846
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
See Also: → 1264812
QA Whiteboard: [good first verify]
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]
You need to log in before you can comment on or make changes to this bug.