1153510 - Improve vertical text support

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Robert Longson [:longsonr]]

Although the text renders vertically with writing-mode:vertical-rl, we treat it as though its bounding rect were still horizontal, and therefore it's impossible to select more than the first character or two of the line (i.e. within the bounds of the imaginary horizontal line).

Comment 1

[Robert Longson [:longsonr]]

Attachment: swap.txt

Comment 2

[Jonathan Kew [:jfkthame]]

Comment on attachment 8591196 [details] [diff] [review]
swap.txt

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

Yes, I expected we'd need to do things like this, as vertical textruns return "line-relative" metrics. This swap makes things a lot better; however, there are still some similar issues that will need to be fixed (perhaps even in the same bug).

I'll attach a simple testcase that shows a couple of problems (with this patch applied):

(1) Incorrect position of vertical text in relation to the (x,y) origin where it is supposed to be drawn. This looks like another case where x and y coordinates need to be swapped; perhaps it can be added to this bug. (Selecting the text, and observing the red dot in relation to the selection rect, makes it more apparent that the problem is reversed x/y offsets in the positioning.)

(2) In the text-orientation:upright case, we don't seem to have quite the right bounds; the last character or so of the text can't be selected, and when resizing the window, rendering turds are left at the end of the text because we're apparently not invalidating a large enough area. This seems like it may be a separate issue.

Comment 3

[Jonathan Kew [:jfkthame]]

Attachment: vertical-text testcase mentioned above

Comment 4

[Jonathan Kew [:jfkthame]]

Attachment: Swap x/y coordinates for vertical textrun positioning in GetTransform... methods

Here are a couple of additional Swap()s that further improve things: the vertical text is now positioned better. The problem with the last glyph(s) of the text-orientation:upright text remains, however.

Comment 5

[Robert Longson [:longsonr]]

Comment on attachment 8591214 [details] [diff] [review]
Swap x/y coordinates for vertical textrun positioning in GetTransform... methods

Can you create an IsVertical() member function and use it c.f. the IsRightToLeft() of http://mxr.mozilla.org/mozilla-central/source/layout/svg/SVGTextFrame.cpp#524

Otherwise this is super.

Comment 6

[Robert Longson [:longsonr]]

Attachment: mouse.svg

DOM seems a bit of a problem

Comment 7

[Jonathan Kew [:jfkthame]]

Attachment: testcase with more varieties of vertical text

Looks like there are a bunch more places in SVGTextFrame.cpp where we need to make coordinate handling aware of vertical mode. This testcase fails with current trunk code, and still fails (pretty spectacularly) with the two patches above.

I've got some WIP locally that makes things considerably better, but it's not yet fully correct so I will hold off on posting a further patch for a while.

(I wonder if it really makes sense to treat this bug as just being about vertical text selection, or if we should try to handle all the vertical text positioning/bounds/etc issues together, as they tend to be all rather inter-dependent.)

Comment 8

[Jonathan Kew [:jfkthame]]

Attachment: Improve handling of vertical writing mode in SVG text frames

Here's a more complete patch that makes the testcase from comment 7 display much better. There are still problems due to incorrect overflow areas on some of the text frames (turn on paint flashing and selecting various bits of the text makes this much more obvious); bug 1155261 will fix this. Also, text-on-a-path (e.g. see example from comment 6) still fails with vertical writing mode. Still, I think this is a useful start.

Comment 9

[Robert Longson [:longsonr]]

Comment on attachment 8594773 [details] [diff] [review]
Improve handling of vertical writing mode in SVG text frames

>-  // use a dummy WritingMode, because nsTextFrame::GetLogicalBaseLine
>-  // doesn't use it anyway
>-  WritingMode writingMode;
>+  WritingMode writingMode = aFrame->GetWritingMode();

The point of having two switch statements was to have one for stuff that didn't require calling MeasureText and one for stuff that did. If we're always going to call MeasureText then we want one switch, alternatively we could keep the two switches in which case we need to be cleverer about when we can return the baseline in the cases we don't need to call MeasureText for.

>+  gfxTextRun::Metrics metrics =
>+    aTextRun->MeasureText(0, aTextRun->GetLength(), gfxFont::LOOSE_INK_EXTENTS,
>+                          nullptr, nullptr);
>+
>   switch (aDominantBaseline) {
>     case NS_STYLE_DOMINANT_BASELINE_HANGING:
>     case NS_STYLE_DOMINANT_BASELINE_TEXT_BEFORE_EDGE:
>-      return 0;
>+      return writingMode.IsVerticalRL()
>+             ? metrics.mAscent + metrics.mDescent : 0;
>     case NS_STYLE_DOMINANT_BASELINE_USE_SCRIPT:
>     case NS_STYLE_DOMINANT_BASELINE_NO_CHANGE:
>     case NS_STYLE_DOMINANT_BASELINE_RESET_SIZE:
>       // These three should not simply map to 'baseline', but we don't
>       // support the complex baseline model that SVG 1.1 has and which
>       // css3-linebox now defines.
>       // (fall through)
>     case NS_STYLE_DOMINANT_BASELINE_AUTO:
>     case NS_STYLE_DOMINANT_BASELINE_ALPHABETIC:
>-      return aFrame->GetLogicalBaseline(writingMode);
>+      return writingMode.IsVerticalRL()
>+             ? metrics.mAscent + metrics.mDescent -
>+               aFrame->GetLogicalBaseline(writingMode)
>+             : aFrame->GetLogicalBaseline(writingMode);
>     case NS_STYLE_DOMINANT_BASELINE_MIDDLE:
>       return aFrame->GetLogicalBaseline(writingMode) -
>         SVGContentUtils::GetFontXHeight(aFrame) / 2.0 *
>         aFrame->PresContext()->AppUnitsPerCSSPixel() * aFontSizeScaleFactor;
>   }
> 
>-  gfxTextRun::Metrics metrics =
>-    aTextRun->MeasureText(0, aTextRun->GetLength(), gfxFont::LOOSE_INK_EXTENTS,
>-                          nullptr, nullptr);
>-
>   switch (aDominantBaseline) {
>     case NS_STYLE_DOMINANT_BASELINE_TEXT_AFTER_EDGE:
>     case NS_STYLE_DOMINANT_BASELINE_IDEOGRAPHIC:
>-      return metrics.mAscent + metrics.mDescent;
>+      return writingMode.IsVerticalLR()
>+             ? 0 : metrics.mAscent + metrics.mDescent;
>     case NS_STYLE_DOMINANT_BASELINE_CENTRAL:
>     case NS_STYLE_DOMINANT_BASELINE_MATHEMATICAL:
>       return (metrics.mAscent + metrics.mDescent) / 2.0;
>   }


> 
>+  WritingMode wm = mFrame->GetWritingMode();

would prefer writingMode as the variable name

>-    nscoord frameWidth = GetFirstPrincipalChild()->GetRect().width;
>+    nscoord frameLen = vertical ? GetFirstPrincipalChild()->GetRect().height
>+                                : GetFirstPrincipalChild()->GetRect().width;

and frameLength here

r=longsonr with review comments addressed

Comment 10

[Jonathan Kew [:jfkthame]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0b070d503800

Comment 11

[Jonathan Kew [:jfkthame]]

Filed bug 1156366 as a followup about text-on-a-path, which still fails to render in vertical modes.

Comment 12

[Jonathan Kew [:jfkthame]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a458faecce96

Comment 13

[Jonathan Kew [:jfkthame]]

Comment on attachment 8594773 [details] [diff] [review]
Improve handling of vertical writing mode in SVG text frames

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

::: layout/svg/SVGTextFrame.cpp
@@ +1162,5 @@
>    gfxFloat runAdvance =
>      aContext->AppUnitsToGfxUnits(textRun->GetAdvanceWidth(offset, length,
>                                                            nullptr));
>  
> +  nscoord pos = wm.IsVertical() ? p.y : p.x;

Oops: this needs to be a gfxFloat, not nscoord, as |p| is a gfxPoint not nsPoint. The loss of precision here was the cause of mochitest failures on the initial landing (backed out above).

Comment 14

[Jonathan Kew [:jfkthame]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d12b22963699

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/d12b22963699