Closed Bug 1157951 Opened 4 years ago Closed 4 years ago

text-decoration doesn't work properly with text-overflow in vertical writing mode

Categories

(Core :: Layout: Text and Fonts, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla40
Tracking Status
firefox40 --- fixed

People

(Reporter: xidorn, Assigned: jfkthame)

References

Details

Attachments

(3 files, 5 obsolete files)

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.
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).
(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: quanxunzhen → jfkthame
Status: NEW → ASSIGNED
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)
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-
(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.
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?
(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?
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)
Attachment #8597387 - Attachment is obsolete: true
Attachment #8597387 - Flags: review?(roc)
And here's the actual bug fix that makes decorations paint properly in the presence of text-overflow.
Attachment #8597942 - Flags: review?(roc)
Attachment #8597389 - Attachment is obsolete: true
Attachment #8597389 - Flags: review?(roc)
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+
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)
Tryserver helpfully pointed out that patch 1 had a couple of typos, resulting in some reftest failures. Now fixed; carrying over r=roc.
Attachment #8597941 - Attachment is obsolete: true
And one more start/end mixup to fix. :( Third time's a charm...
Attachment #8598693 - Attachment is obsolete: true
Just rebasing after the fix to part 1. Carrying over r=roc.
Attachment #8597942 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/43e25604c700
https://hg.mozilla.org/mozilla-central/rev/70d6d34115a0
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
Depends on: 1159950
You need to log in before you can comment on or make changes to this bug.