Closed
Bug 1153510
Opened 10 years ago
Closed 10 years ago
Improve vertical text support
Categories
(Core :: SVG, defect)
Core
SVG
Tracking
()
RESOLVED
FIXED
mozilla40
Tracking | Status | |
---|---|---|
firefox40 | --- | fixed |
People
(Reporter: longsonr, Assigned: jfkthame)
References
Details
Attachments
(4 files, 2 obsolete files)
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•10 years ago
|
||
Assignee: nobody → longsonr
Attachment #8591196 -
Flags: review?(cam)
Attachment #8591196 -
Flags: feedback?(jfkthame)
Reporter | ||
Updated•10 years ago
|
Attachment #8591196 -
Attachment is patch: true
Assignee | ||
Comment 2•10 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•10 years ago
|
||
Assignee | ||
Comment 4•10 years ago
|
||
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•10 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•10 years ago
|
||
DOM seems a bit of a problem
Assignee | ||
Comment 7•10 years ago
|
||
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•10 years ago
|
Summary: Fix vertical text selection → Improve vertical text support
Assignee | ||
Comment 8•10 years ago
|
||
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•10 years ago
|
Assignee: longsonr → jfkthame
Status: NEW → ASSIGNED
Reporter | ||
Comment 9•10 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 | ||
Comment 10•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/0b070d503800
Assignee | ||
Comment 11•10 years ago
|
||
Filed bug 1156366 as a followup about text-on-a-path, which still fails to render in vertical modes.
Assignee | ||
Comment 12•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/a458faecce96
Assignee | ||
Updated•10 years ago
|
Attachment #8591196 -
Attachment is obsolete: true
Attachment #8591196 -
Flags: review?(cam)
Assignee | ||
Updated•10 years ago
|
Attachment #8591214 -
Attachment is obsolete: true
Assignee | ||
Comment 13•10 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).
Assignee | ||
Comment 14•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/d12b22963699
Comment 15•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/d12b22963699
Status: ASSIGNED → RESOLVED
Closed: 10 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.
Description
•