Improve vertical text support

RESOLVED FIXED in Firefox 40

Status

()

Core
SVG
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: longsonr, Assigned: jfkthame)

Tracking

unspecified
mozilla40
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox40 fixed)

Details

Attachments

(4 attachments, 2 obsolete attachments)

(Reporter)

Description

3 years ago
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).
(Reporter)

Comment 1

3 years ago
Created attachment 8591196 [details] [diff] [review]
swap.txt
Assignee: nobody → longsonr
Attachment #8591196 - Flags: review?(cam)
Attachment #8591196 - Flags: feedback?(jfkthame)
(Reporter)

Updated

3 years ago
Blocks: 319163
(Reporter)

Updated

3 years ago
Attachment #8591196 - Attachment is patch: true
(Assignee)

Comment 2

3 years ago
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.
Attachment #8591196 - Flags: feedback?(jfkthame) → feedback+
(Assignee)

Comment 3

3 years ago
Created attachment 8591206 [details]
vertical-text testcase mentioned above
(Assignee)

Comment 4

3 years ago
Created attachment 8591214 [details] [diff] [review]
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.
Attachment #8591214 - Flags: feedback?(longsonr)
(Reporter)

Comment 5

3 years ago
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.
Attachment #8591214 - Flags: review+
Attachment #8591214 - Flags: feedback?(longsonr)
Attachment #8591214 - Flags: feedback+
(Reporter)

Comment 6

3 years ago
Created attachment 8591399 [details]
mouse.svg

DOM seems a bit of a problem
(Assignee)

Comment 7

3 years ago
Created attachment 8592839 [details]
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.)
(Reporter)

Updated

3 years ago
Summary: Fix vertical text selection → Improve vertical text support
(Assignee)

Comment 8

3 years ago
Created attachment 8594773 [details] [diff] [review]
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.
Attachment #8594773 - Flags: review?(longsonr)
(Assignee)

Updated

3 years ago
Assignee: longsonr → jfkthame
Status: NEW → ASSIGNED
(Reporter)

Comment 9

3 years ago
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
Attachment #8594773 - Flags: review?(longsonr) → review+
(Assignee)

Updated

3 years ago
Depends on: 1155261
(Assignee)

Comment 11

3 years ago
Filed bug 1156366 as a followup about text-on-a-path, which still fails to render in vertical modes.
(Assignee)

Updated

3 years ago
Attachment #8591196 - Attachment is obsolete: true
Attachment #8591196 - Flags: review?(cam)
(Assignee)

Updated

3 years ago
Attachment #8591214 - Attachment is obsolete: true
(Assignee)

Comment 13

3 years ago
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).
https://hg.mozilla.org/mozilla-central/rev/d12b22963699
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox40: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
You need to log in before you can comment on or make changes to this bug.