Closed
Bug 1220438
Opened 10 years ago
Closed 9 years ago
'text-decoration: underline' does not keep constant position in vertical writing (with various font sizes and mixed text)
Categories
(Core :: Layout: Block and Inline, defect)
Core
Layout: Block and Inline
Tracking
()
RESOLVED
FIXED
mozilla48
People
(Reporter: bugzilla, Assigned: xidorn)
References
(Blocks 1 open bug)
Details
(Keywords: testcase)
Attachments
(6 files, 1 obsolete file)
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.
Reporter | ||
Updated•10 years ago
|
Blocks: writing-mode
Keywords: testcase
Reporter | ||
Comment 1•10 years ago
|
||
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)
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)
Updated•9 years ago
|
Assignee: nobody → slyu
Status: NEW → ASSIGNED
Comment 2•9 years ago
|
||
Comment 3•9 years ago
|
||
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)
Comment 5•9 years ago
|
||
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•9 years ago
|
||
New updated test cases that includes text-orientation:sideways and lang=...
Attachment #8732079 -
Attachment is obsolete: true
Reporter | ||
Comment 7•9 years ago
|
||
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.
Reporter | ||
Comment 8•9 years ago
|
||
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•9 years ago
|
||
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•9 years ago
|
||
This is how "writing-mode: vertical-rl; text-orientation:sideways" looks like after removing the lines.
Comment 11•9 years ago
|
||
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.
Assignee | ||
Comment 13•9 years ago
|
||
(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.
Assignee | ||
Comment 14•9 years ago
|
||
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)
Assignee | ||
Comment 15•9 years ago
|
||
Hmmm, it seems we don't have any existing test for testing the unbrokenness of text decoration lines... Just patch then.
Assignee | ||
Comment 16•9 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/43771/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/43771/
Attachment #8737112 -
Flags: review?(jfkthame)
Comment 17•9 years ago
|
||
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)
Assignee | ||
Comment 18•9 years ago
|
||
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)
Reporter | ||
Comment 19•9 years ago
|
||
(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.
Updated•9 years ago
|
Attachment #8737112 -
Flags: review?(jfkthame)
Comment 20•9 years ago
|
||
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?
Assignee | ||
Comment 21•9 years ago
|
||
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 22•9 years ago
|
||
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)
Assignee | ||
Comment 23•9 years ago
|
||
(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.
Assignee | ||
Comment 24•9 years ago
|
||
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)
Updated•9 years ago
|
Attachment #8737112 -
Flags: review?(jfkthame) → review+
Comment 25•9 years ago
|
||
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.
Assignee | ||
Comment 26•9 years ago
|
||
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
Comment 27•9 years ago
|
||
Comment 28•9 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox48:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Reporter | ||
Comment 29•9 years ago
|
||
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
Assignee | ||
Comment 30•9 years ago
|
||
(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?
Reporter | ||
Comment 31•9 years ago
|
||
(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
Reporter | ||
Comment 33•9 years ago
|
||
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
Updated•9 years ago
|
QA Whiteboard: [good first verify]
Comment 34•9 years ago
|
||
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.
Description
•