Closed
Bug 1157951
Opened 9 years ago
Closed 9 years ago
text-decoration doesn't work properly with text-overflow in vertical writing mode
Categories
(Core :: Layout: Text and Fonts, defect)
Core
Layout: Text and Fonts
Tracking
()
RESOLVED
FIXED
mozilla40
Tracking | Status | |
---|---|---|
firefox40 | --- | fixed |
People
(Reporter: xidorn, Assigned: jfkthame)
References
Details
Attachments
(3 files, 5 obsolete files)
9.52 KB,
patch
|
roc
:
review+
|
Details | Diff | Splinter Review |
31.71 KB,
patch
|
Details | Diff | Splinter Review | |
2.88 KB,
patch
|
Details | Diff | Splinter Review |
With bug 1117227 landed, text-overflow will work for vertical text. However, it seems that text-decoration won't work properly when overflow mark is present. More precisely, underline disappears, and overline is extended to the overflow marker.
Assignee | ||
Comment 1•9 years ago
|
||
Note that bug 1155261 may have some effect here, if decorations are being incorrectly clipped away in some cases (although I don't expect it to fix the problem of decorations that extend to the overflow marker when they shouldn't).
Assignee | ||
Comment 2•9 years ago
|
||
(Stealing this bug as I happened to run into this in the course of investigating other issues...) The problem here arises because nsCharClipDisplayItem assumes horizontal writing. As a preliminary step, this patch renames various fields/variables using 'start' and 'end' instead of 'left' and 'right', so that they'll be less confusing when used in conjunction with vertical writing modes. No functional change.
Attachment #8597387 -
Flags: review?(roc)
Assignee | ||
Updated•9 years ago
|
Assignee: quanxunzhen → jfkthame
Status: NEW → ASSIGNED
Assignee | ||
Comment 3•9 years ago
|
||
And here's the actual bug-fix that makes decorations work correctly in the presence of text-overflow.
Attachment #8597389 -
Flags: review?(roc)
Attachment #8597389 -
Flags: feedback?(quanxunzhen)
Reporter | ||
Comment 4•9 years ago
|
||
Comment on attachment 8597389 [details] [diff] [review] patch 2 - Make nsCharClipDisplayItem aware of vertical writing modes Review of attachment 8597389 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/base/nsDisplayList.h @@ +3629,5 @@ > + mStart = aStartEdge > 0 ? r.y + aStartEdge : nscoord_MIN; > + mEnd = aEndEdge > 0 ? std::max(r.YMost() - aEndEdge, mEnd) : nscoord_MAX; > + } else { > + mStart = aStartEdge > 0 ? r.x + aStartEdge : nscoord_MIN; > + mEnd = aEndEdge > 0 ? std::max(r.XMost() - aEndEdge, mEnd) : nscoord_MAX; What do the terms start/end refer here? If it refers to flow-relative directions, it doesn't look correct to me. Inline-start should map to XMost in horizontal rtl text, shouldn't it? This map seems to be closer to the concept line-left/right of line-relative direction. Using start/end could probably be confusing.
Attachment #8597389 -
Flags: feedback?(quanxunzhen) → feedback-
Assignee | ||
Comment 5•9 years ago
|
||
(In reply to Xidorn Quan [:xidorn] (UTC+12) from comment #4) > Comment on attachment 8597389 [details] [diff] [review] > patch 2 - Make nsCharClipDisplayItem aware of vertical writing modes > > Review of attachment 8597389 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: layout/base/nsDisplayList.h > @@ +3629,5 @@ > > + mStart = aStartEdge > 0 ? r.y + aStartEdge : nscoord_MIN; > > + mEnd = aEndEdge > 0 ? std::max(r.YMost() - aEndEdge, mEnd) : nscoord_MAX; > > + } else { > > + mStart = aStartEdge > 0 ? r.x + aStartEdge : nscoord_MIN; > > + mEnd = aEndEdge > 0 ? std::max(r.XMost() - aEndEdge, mEnd) : nscoord_MAX; > > What do the terms start/end refer here? If it refers to flow-relative > directions, it doesn't look correct to me. Inline-start should map to XMost > in horizontal rtl text, shouldn't it? > > This map seems to be closer to the concept line-left/right of line-relative > direction. Using start/end could probably be confusing. I'm not sure what are the best names to use here. They're physical coordinates rather than logical (flow-relative), but may be either horizontal or vertical, depending which is the inline axis. This doesn't correspond to IStart/IEnd, but also doesn't necessarily correspond to LineLeft/LineRight (in the case of sideways-left, line-left will be bottom, but physical start will still be top). Maybe we should just add comments to try and clarify this, but I'd also be happy to hear suggestions for naming that would help make things clearer.
Reporter | ||
Comment 6•9 years ago
|
||
I have no clear idea. TBH this is one of the reason I didn't convert them in bug 1117227. Probably some more tedious name like TopOrLeft and RightOrBottom? Or probably we should create new term pair for it like... little end and big end?
Assignee | ||
Comment 7•9 years ago
|
||
(In reply to Xidorn Quan [:xidorn] (UTC+12) from comment #6) > I have no clear idea. TBH this is one of the reason I didn't convert them in > bug 1117227. Probably some more tedious name like TopOrLeft and > RightOrBottom? Yes, I wondered about doing that -- indeed, there's already an example of topOrLeft/bottomOrRight in nsTextFrame that I landed in bug 1093553 -- but I do feel it makes the code seem a bit awkward to read. > Or probably we should create new term pair for it like... > little end and big end? Maybe "Visual Inline Start/End", which we could abbreviate (with explanatory comment) as VisIStart/VisIEnd? (And similarly VisBStart/VisBEnd if we need the block-axis version.) Would that be reasonable?
Sounds good to me.
Assignee | ||
Comment 9•9 years ago
|
||
Updated to use VisIStart/End naming. This is the preliminary patch that just renames things, with no "real" code changes. I stayed with plain 'start' and 'end' for some local variables, as this seemed easier to read within the local context, but made fields and parameters use the fuller VisIStart/End names to avoid ambiguity there.
Attachment #8597941 -
Flags: review?(roc)
Assignee | ||
Updated•9 years ago
|
Attachment #8597387 -
Attachment is obsolete: true
Attachment #8597387 -
Flags: review?(roc)
Assignee | ||
Comment 10•9 years ago
|
||
And here's the actual bug fix that makes decorations paint properly in the presence of text-overflow.
Attachment #8597942 -
Flags: review?(roc)
Assignee | ||
Updated•9 years ago
|
Attachment #8597389 -
Attachment is obsolete: true
Attachment #8597389 -
Flags: review?(roc)
Attachment #8597941 -
Flags: review?(roc) → review+
Comment on attachment 8597942 [details] [diff] [review] patch 2 - Make nsCharClipDisplayItem aware of vertical writing modes Review of attachment 8597942 [details] [diff] [review]: ----------------------------------------------------------------- Great! Got a test for this?
Attachment #8597942 -
Flags: review?(roc) → review+
Assignee | ||
Comment 12•9 years ago
|
||
Here are a handful of tests. (They pass for me locally, though I won't be surprised if tryserver suggests a little fuzz is needed.)
Attachment #8598579 -
Flags: review?(roc)
Assignee | ||
Comment 13•9 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=b3420f765710
Attachment #8598579 -
Flags: review?(roc) → review+
Assignee | ||
Comment 14•9 years ago
|
||
Tryserver helpfully pointed out that patch 1 had a couple of typos, resulting in some reftest failures. Now fixed; carrying over r=roc.
Assignee | ||
Updated•9 years ago
|
Attachment #8597941 -
Attachment is obsolete: true
Assignee | ||
Comment 15•9 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=3feae509c813
Assignee | ||
Comment 16•9 years ago
|
||
And one more start/end mixup to fix. :( Third time's a charm...
Assignee | ||
Updated•9 years ago
|
Attachment #8598693 -
Attachment is obsolete: true
Assignee | ||
Comment 17•9 years ago
|
||
Just rebasing after the fix to part 1. Carrying over r=roc.
Assignee | ||
Updated•9 years ago
|
Attachment #8597942 -
Attachment is obsolete: true
Assignee | ||
Comment 18•9 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=113291872262
Assignee | ||
Comment 19•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/43e25604c700 https://hg.mozilla.org/integration/mozilla-inbound/rev/70d6d34115a0 https://hg.mozilla.org/integration/mozilla-inbound/rev/ebd9da2efb59
Comment 20•9 years ago
|
||
sorry had to backout ebd9da2efb59 for test failures like: https://treeherder.mozilla.org/logviewer.html#?job_id=9396486&repo=mozilla-inbound
Comment 21•9 years ago
|
||
Backout: https://hg.mozilla.org/integration/mozilla-inbound/rev/5875eb931610
https://hg.mozilla.org/mozilla-central/rev/43e25604c700 https://hg.mozilla.org/mozilla-central/rev/70d6d34115a0
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
Assignee | ||
Comment 23•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/5b8444aa59f3
Comment 24•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/5b8444aa59f3
Flags: in-testsuite+
You need to log in
before you can comment on or make changes to this bug.
Description
•