Closed Bug 1074735 Opened 10 years ago Closed 10 years ago

make CSS text decorations (underline, overline, strikethrough) respect vertical writing modes

Categories

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

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla35

People

(Reporter: jfkthame, Assigned: jfkthame)

References

(Blocks 1 open bug)

Details

Attachments

(2 files)

The patches in bug 902762 will allow us to create and render vertical textruns, but we also need to make text decorations such as under- and overlines draw with the correct orientation.
This fixes up a couple of inappropriate values in CreateVerticalMetrics. They were unused/untested until now, but on reflection they really didn't make sense.
Attachment #8499631 - Flags: review?(smontagu)
Assignee: nobody → jfkthame
Status: NEW → ASSIGNED
With this patch, we get reasonable rendering of underline, overline and strikethrough on vertical textframes, and of the IME selection underlines. (I suspect XUL textbox will still need some more vertical-writing love; I haven't been testing that, only touching as needed to ensure it still compiles and works horizontally.)
Attachment #8499633 - Flags: review?(smontagu)
Blocks: 1076657
Attachment #8499631 - Flags: review?(smontagu) → review+
Comment on attachment 8499631 [details] [diff] [review]
pt 1 - Use more sensible values for underline and strikeout position in vertical font metrics.

Review of attachment 8499631 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/thebes/gfxFont.cpp
@@ +3435,1 @@
>  

On second thoughts, is this correct for vertical text? We don't yet support CSS3 text-underline-position, but until we do I think it makes sense to put underlines on the right of the text by default. WDYT?
*sigh* I really don't know how to use the review tool and get a sensible looking comment returned. Comment 3 applies to this line of the patch:

+    metrics->underlineOffset = - metrics->maxDescent - metrics->underlineSize;
(In reply to Simon Montagu :smontagu from comment #3)
> Comment on attachment 8499631 [details] [diff] [review]
> pt 1 - Use more sensible values for underline and strikeout position in
> vertical font metrics.
> 
> Review of attachment 8499631 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/thebes/gfxFont.cpp
> @@ +3435,1 @@
> >  
> 
> On second thoughts, is this correct for vertical text? We don't yet support
> CSS3 text-underline-position, but until we do I think it makes sense to put
> underlines on the right of the text by default. WDYT?

I wondered about that, but http://www.w3.org/TR/2014/CR-css-writing-modes-3-20140320/#line-directions says that in vertical writing modes, "under" maps to physical left (figure 16), except in the case of text-orientation:sideways-left (figure 17), which we're not yet implementing. See also the chart in the next section, http://www.w3.org/TR/2014/CR-css-writing-modes-3-20140320/#logical-to-physical.
I do suspect that for purely vertical upright Chinese/Japanese, an "underline" on the right may be preferred, but it's not clear to me how to reconcile that with the left-underline that would be wanted for any rotated Latin inclusions in the text; should the parts be "underlined" separately on different sides of the vertical text line?

ISTM that there may not be any completely satisfactory answer until we have more of CSS text decorations implemented, so that authors can decide what they want, but for the time being an "underline" on the left seemed like the most reasonable default. This also works better in conjunction with IME behavior in vertical text (at least in my experiments so far, on OS X), as the IME candidates are by default displayed in columns to the left of the "underlined" active input area.
OK, I'll buy that
Comment on attachment 8499633 [details] [diff] [review]
pt 2 - Support drawing CSS text decorations (underline, overline, strikeout) on vertical text frames.

Review of attachment 8499633 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/generic/nsTextFrame.cpp
@@ +4913,5 @@
>      fontMetrics->GetUnderline(underlineOffset, underlineSize);
>      nscoord maxAscent = fontMetrics->MaxAscent();
>  
>      gfxFloat appUnitsPerDevUnit = aPresContext->AppUnitsPerDevPixel();
> +    gfxFloat gfxWidth = 

Nit: trailing space
Attachment #8499633 - Flags: review?(smontagu) → review+
Comment on attachment 8499633 [details] [diff] [review]
pt 2 - Support drawing CSS text decorations (underline, overline, strikeout) on vertical text frames.

Review of attachment 8499633 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/generic/nsTextFrame.cpp
@@ +5142,1 @@
>        aDescentLimit);

Oops, this needed aVertical inserted as an extra param before the (optional) aDescentLimit. (Sigh.... optional parameters and implicit type conversions...)
Depends on: 1159729
You need to log in before you can comment on or make changes to this bug.