To support vertical text rendering, gfx text run construction code needs to support layout using vertical metrics/features. The harfbuzz code already has some support for this, although the list of default features needs fixing ('vrt2' shouldn't be on by default). Given that this happens in the shaper code I think it would make sense to initially use the harfbuzz shaper exclusively for vertical text runs to avoid having to futz with the platform shaper code for this. One open question is what form the glyph position data should take, whether it's purely physical coordinates or logicalized in some way.

John, can you email harfbuzz list re turning 'vrt2' off? My gut feeling is that I agree with you, but the spec suggests that it should be on by default.

Breakdown of work needed: * orientation segmentation (UTR50? script codes for now?) * shape with harfbuzz set to HB_DIRECTION_IS_VERTICAL for upright runs * pull in vertical metrics (e.g. HBGetGlyphVAdvance, kerning?) * figure out how to save vertical positioning data within text runs, extents etc. * add textrun params for vertical textrun construction * back end changes to support drawing upright runs (needed?) and rotated runs Ignore for now: baseline data in font The vertical defaults issue with harfbuzz has already been fixed.

A couple notes about the approach I'm planning to take here: * Textruns will have a 'vertical' flag. When this is true, the x and y coordinates for glyph positioning data will be swapped, so that a "simple" glyph record has vertical advance, etc. This allows vertical textruns to be stored just as efficiently as horizontal, using simple CompressedGlyph records in most cases. * Rather than giving gfxFont a parallel set of methods to get vertical metrics (glyph advance, ascent/descent, etc), and making callers decide which to use at every call site, we'll instantiate separate "vertical fonts" where the existing methods such as GetGlyphWidth will return vertical advance instead of horizontal. * Vertical textruns don't always use vertical fonts! When text-orientation is sideways, we'll construct a textrun with the vertical flag set, so glyph advances are vertical, but we'll use a normal (horizontal) font instance so as to get the horizontal-layout metrics. In this case, the glyphs will need to be rotated 90° when they're drawn. I suppose we'll need a flag to indicate whether the rotation is CW or CCW. We'll need some additional gfxFont API to get the vertical origin of glyphs in order to position them properly when drawing vertical fonts. This should use the VORG table in CFF fonts that have it, and otherwise, the glyph extents plus the top sidebearing from vmtx; if there's no vmtx table, we'll synthesize arbitrary metrics that give a fixed advance and constant origin. I've got some work-in-progress patches that I'll attach here, although not requesting review yet as they're still in a state of flux, with various unfinished pieces.

Note that the only reason we left liga on for vertical text in harfbuzz is that Kazuraki uses that. I'm fine disabling it and leaving to user to enable. I think we should talk to Adobe to release a version of it that doesn't rely on 'liga'. Using requiredFeature might be one option for lack of a better option.

> We'll need some additional gfxFont API to get the vertical origin of glyphs in order to position them properly when drawing vertical fonts. Note that if you pass that information to HarfBuzz (in get_v_origin), HarfBuzz takes care of doing the shift. In fact, if you don't set that function, it synthesizes already. But hooking up VORG is even better.

(In reply to Behdad Esfahbod from comment #14) > Note that the only reason we left liga on for vertical text in harfbuzz is > that Kazuraki uses that. I'm fine disabling it and leaving to user to > enable. I think we should talk to Adobe to release a version of it that > doesn't rely on 'liga'. Using requiredFeature might be one option for lack > of a better option. As I recall, we discussed this a while back on the OpenType list. Rather than create vertical-only features for things like 'liga' I think the preferred suggestion was to add flags to the lookups(?) that classified them as vertical-only or horizontal-only. Or something like that, I'd need to lookup the details. I think the explicit disabling of ligatures in textrun code is an anachronism that we should probably trim out. The right place for default features is in harfbuzz code.

Updated part 5 to include vorg support.

Comment on attachment 8465353 [details] [diff] [review] pt 9 - disable common ligatures by default in vertical fonts No longer needed, as harfbuzz has disabled 'liga' by default in vertical text.

Basic support for drawing vertical textruns in canvas.

I suggest you don't set h_origin callback. Just add a comment that it's intentional. In the future I like to optimize the case that it's not set.

As I've been working with these patches, I've concluded that a change in strategy is needed; the model where we have separate horizontal and vertical instances of gfxFont does not work well with the rest of our text architecture. So I've modified the approach outlined in comment 3, and will post a new set of patches shortly. The new model maintains a single gfxFont that supports both horizontal and vertical layout; an Orientation parameter is added to GetMetrics(), so that we can request H or V variants of the font metrics, and textruns have orientation flags that determine which way their content is to be shaped. For vertical textruns, we still use the convention that X and Y coordinates are swapped, so that the textrun's "advance width" is actually vertical, and the glyph advances and offsets are interpreted in the swapped coordinate system. Orientation flags are added not just to gfxTextRun but also to the GlyphRuns within the textrun, so that we can support text-orientation:mixed without needing to create separate textruns (or even frames) wherever there's a change in the resolved (upright/rotated) orientation of the individual characters. A gfxTextRun's orientation can be any of Horizontal, Vertical-Upright, Vertical-Sideways-Right, Vertical-Sideways-Left, or Vertical-Mixed. A GlyphRun is never Vertical-Mixed, however; by the time we're generating glyphs, Mixed will have been resolved to either Upright or Sideways-Right. This is done based on UTR50 properties, at the same time as font matching. (Actually, initial implementation may omit support for the Sideways-Left orientation, which is not needed for typical CJK use cases.) The nsFontMetrics object that layout uses to interface with a gfx font-group will be initialized with a specific orientation, vertical or horizontal, which it will store and then pass to gfxFont::GetMetrics when needed. So an nsFontMetrics is orientation-specific (unlike a gfxFont). This seems to provide the cleanest code for clients, which can create a single nsFontMetrics to suit their writing-mode, and then call its various metrics-accessors without needing to pass orientation every time.

Unbreak the Windows build by adding missing exports to symbols.def.in.

Comment on attachment 8486627 [details] [diff] [review] pt 5 - handle vertical textruns when painting text in nsTextFrame Review of attachment 8486627 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/generic/nsTextFrame.cpp @@ +5950,5 @@ > const nscoord frameWidth = GetSize().width; > gfxPoint framePt(aPt.x, aPt.y); > + gfxPoint textBaselinePt; > + if (vertical) { > + textBaselinePt = gfxPoint(aPt.x + mAscent, aPt.y); Doesn't this need need to use |aPt.y + frameHeight|, parallel to |aPt.x + frameWidth| in the non-vertical case (or frameISize for both)? Or does some other factor that I haven't noticed mean you don't need to do that?

Comment on attachment 8486628 [details] [diff] [review] pt 6 - handle vertical textruns for mouse click in nsTextFrame Review of attachment 8486628 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/generic/nsTextFrame.cpp @@ +6301,5 @@ > PropertyProvider provider(this, iter, nsTextFrame::eInflated); > // Trim leading but not trailing whitespace if possible > provider.InitializeForDisplay(false); > + gfxFloat width = mTextRun->IsVertical() ? aPoint.y : > + (mTextRun->IsRightToLeft() ? mRect.width - aPoint.x : aPoint.x); Again, what about the combination of vertical and right-to-left?

Comment on attachment 8486630 [details] [diff] [review] pt 7 - draw caret appropriately for vertical textruns Review of attachment 8486630 [details] [diff] [review]: ----------------------------------------------------------------- Similar problem here (it needs more complicated handling here because of all the bidi indicator stuff). If you've decided that RTL text in vertical context is out of scope at this stage, we at least want to put in some XXX comments about it, but I think it's worth getting right from the start.

(In reply to Simon Montagu :smontagu from comment #35) > Comment on attachment 8486627 [details] [diff] [review] > pt 5 - handle vertical textruns when painting text in nsTextFrame > > Review of attachment 8486627 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: layout/generic/nsTextFrame.cpp > @@ +5950,5 @@ > > const nscoord frameWidth = GetSize().width; > > gfxPoint framePt(aPt.x, aPt.y); > > + gfxPoint textBaselinePt; > > + if (vertical) { > > + textBaselinePt = gfxPoint(aPt.x + mAscent, aPt.y); > > Doesn't this need need to use |aPt.y + frameHeight|, parallel to |aPt.x + > frameWidth| in the non-vertical case (or frameISize for both)? Or does some > other factor that I haven't noticed mean you don't need to do that? The addition of frameWidth in horizontal is only for the RTL case; so far, I simply haven't attempted to deal with RTL text in vertical mode. Yes, for that combination we probably need a similar condition here. (In reply to Simon Montagu :smontagu from comment #36) > Comment on attachment 8486628 [details] [diff] [review] > pt 6 - handle vertical textruns for mouse click in nsTextFrame > > Review of attachment 8486628 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: layout/generic/nsTextFrame.cpp > @@ +6301,5 @@ > > PropertyProvider provider(this, iter, nsTextFrame::eInflated); > > // Trim leading but not trailing whitespace if possible > > provider.InitializeForDisplay(false); > > + gfxFloat width = mTextRun->IsVertical() ? aPoint.y : > > + (mTextRun->IsRightToLeft() ? mRect.width - aPoint.x : aPoint.x); > > Again, what about the combination of vertical and right-to-left? Again, not yet supported at all. I suppose I can try adding in those conditions in a number of places, and see how close to "working" it comes....

(In reply to Simon Montagu :smontagu from comment #37) > Comment on attachment 8486630 [details] [diff] [review] > pt 7 - draw caret appropriately for vertical textruns > > Review of attachment 8486630 [details] [diff] [review]: > ----------------------------------------------------------------- > > Similar problem here (it needs more complicated handling here because of all > the bidi indicator stuff). If you've decided that RTL text in vertical > context is out of scope at this stage, we at least want to put in some XXX > comments about it, but I think it's worth getting right from the start. As noted just above, I was willfully ignoring it for now. But I'll have a look and see what we can do...

nsfontmetrics-orientation

Comment on attachment 8489237 [details] [diff] [review] pt 7 - draw caret appropriately for vertical textruns Sorry, total bzexport fail; ignore this!

Rebased on top of bug 1066043.

Rebased following bug 1037340.

In addition to rebasing as needed, added support for RTL text within vertical (sideways) text to these patches.

Comment on attachment 8490350 [details] [diff] [review] pt 6 - handle vertical textruns for mouse click in nsTextFrame Review of attachment 8490350 [details] [diff] [review]: ----------------------------------------------------------------- I think you attached the same patch as before here

Comment on attachment 8490351 [details] [diff] [review] pt 7 - draw caret appropriately for vertical textruns Review of attachment 8490351 [details] [diff] [review]: ----------------------------------------------------------------- I think this is good, though it makes my head spin a bit -- let's get it in and file bugs on the behaviour if necessary

Oops, so I did; the hunk to handle this ended up in patch 7 by mistake. I've moved it to this one, where it more logically belongs.

jdaggett: review ping?

Comment on attachment 8490336 [details] [diff] [review] pt 1 - pass a 'vertical' flag to font shapers, and support vertical shaping through harfbuzz. Review of attachment 8490336 [details] [diff] [review]: ----------------------------------------------------------------- ::: gfx/thebes/gfxHarfBuzzShaper.cpp @@ +360,5 @@ > + int16_t(hhea->ascender)); > + return; > + } > + } > + Don't you mean vhea here?!? The use of hhea values doesn't make sense to me.

(In reply to John Daggett (:jtd) from comment #55) > Comment on attachment 8490336 [details] [diff] [review] > pt 1 - pass a 'vertical' flag to font shapers, and support vertical shaping > through harfbuzz. > > Review of attachment 8490336 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: gfx/thebes/gfxHarfBuzzShaper.cpp > @@ +360,5 @@ > > + int16_t(hhea->ascender)); > > + return; > > + } > > + } > > + > > Don't you mean vhea here?!? The use of hhea values doesn't make sense to me. vhea values will be wrong. The value needed here is the y distance between horizontal origin (baseline-left) and vertical origin (top center), and a good heuristic for that is the font ascent. The ascent value in vhea is always set to 0.5EM, and that is used for aligning the ideographic baseline to the alphabetic baseline. That said, I agree with the comment in there that we need OS/2 typoAscender, NOT the hhea value (which is used for clipping?).

(In reply to Behdad Esfahbod from comment #56) > That said, I agree with the comment in there that we need OS/2 typoAscender, > NOT the hhea value (which is used for clipping?). No, it's not used for clipping; that's the winAscent/Descent values from OS/2, which can mean they vary widely from the intended line spacing. The hhea ascent/descent values should be OK; they've often been used by Mac software since the early days of TrueType. As suggested by the comment, OS/2 'typo' values might be preferable, but remember that they may not be present: the OS/2 table is optional under OS X, and some commonly-used fonts don't have it. So I took the easy way for the time being.

(In reply to Jonathan Kew (:jfkthame) from comment #57) > (In reply to Behdad Esfahbod from comment #56) > > That said, I agree with the comment in there that we need OS/2 typoAscender, > > NOT the hhea value (which is used for clipping?). > > No, it's not used for clipping; that's the winAscent/Descent values from > OS/2, which can mean they vary widely from the intended line spacing. The > hhea ascent/descent values should be OK; they've often been used by Mac > software since the early days of TrueType. As suggested by the comment, OS/2 > 'typo' values might be preferable, but remember that they may not be > present: the OS/2 table is optional under OS X, and some commonly-used fonts > don't have it. So I took the easy way for the time being. Sounds good. I'm probably going to do the same in hb-ot-font.

Comment on attachment 8490348 [details] [diff] [review] pt 4 - support for orientation:sideways-right when drawing vertical textruns Review of attachment 8490348 [details] [diff] [review]: ----------------------------------------------------------------- r+ with additional comment ::: gfx/thebes/gfxFont.cpp @@ +1893,5 @@ > + aPt->y * aRunParams.devPerApp); > + const Metrics& metrics = GetMetrics(eHorizontal); > + aRunParams.context->SetMatrix(aRunParams.context->CurrentMatrix(). > + Translate(p).Rotate(M_PI / 2.0).Translate(-p). > + Translate(gfxPoint(0, metrics.emAscent - metrics.emDescent) / 2)); Hmmm, this is a bit mysterious. Could we add a comment here explaining this calculation?

Comment on attachment 8490336 [details] [diff] [review] pt 1 - pass a 'vertical' flag to font shapers, and support vertical shaping through harfbuzz. Review of attachment 8490336 [details] [diff] [review]: ----------------------------------------------------------------- r+ with name change, comments ::: gfx/thebes/gfxFont.cpp @@ +2344,5 @@ > gfxShapedText *aShapedText) > { > bool ok = false; > > + if (FontCanSupportGraphite() && !aVertical) { Maybe an 'xxx' comment here? I do think Graphite could be useful for vertical. ::: gfx/thebes/gfxHarfBuzzShaper.cpp @@ +356,5 @@ > + const HMetricsHeader* hhea = > + reinterpret_cast<const HMetricsHeader*>(hb_blob_get_data(hheaTable, &len)); > + if (len >= sizeof(HMetricsHeader)) { > + *aY = -FloatToFixed(GetFont()->FUnitsToDevUnitsFactor() * > + int16_t(hhea->ascender)); Ok, second time through I see how ascender kinda makes sense here. I would suggest verifying this with fonts like Arial Unicode MS, which contains CJK characters but lacks vertical metrics. @@ +1117,5 @@ > + > + // We rely on the fact that horizontal and vertical metrics tables > + // share the same structure, just with renamed fields, so we can > + // read the vhea/vmtx tables with the same code as hhea/hmtx, > + // and just interpret the results in the vertical coordinate system. I think it might be better to rename instances of HMetrics*/HLongMetric to GlyphMetrics*/GlyphLongMetric (or whatever you prefer) and make it clear that the horiz/vert structure is the same. @@ +1441,5 @@ > continue; > } > > + hb_position_t i_offset, b_offset; > + hb_position_t i_advance, b_advance; Comment here to indicate these are inline/block offsets/advances.

Comment on attachment 8490346 [details] [diff] [review] pt 2 - support for vertical textruns and fonts through gfxTextRun::Draw Review of attachment 8490346 [details] [diff] [review]: ----------------------------------------------------------------- ::: gfx/thebes/gfxFont.cpp @@ +1689,5 @@ > const TextRunDrawParams& runParams(aBuffer.mRunParams); > const FontDrawParams& fontParams(aBuffer.mFontParams); > > + double glyphX, glyphY; > + if (fontParams.isVerticalFont) { Ew. Since fontParams.isVerticalFont and runParams.isRTL are constant across the run, it sucks that we need to have this within the inner loop here. Might there be a way of templating this out?

I think we need some reftests here or on the vertical canvas bug, even if they are just simple sanity test bugs.

(In reply to John Daggett (:jtd) from comment #60) > Comment on attachment 8490336 [details] [diff] [review] > pt 1 - pass a 'vertical' flag to font shapers, and support vertical shaping > through harfbuzz. > > Review of attachment 8490336 [details] [diff] [review]: > ----------------------------------------------------------------- > > r+ with name change, comments > > ::: gfx/thebes/gfxFont.cpp > @@ +2344,5 @@ > > gfxShapedText *aShapedText) > > { > > bool ok = false; > > > > + if (FontCanSupportGraphite() && !aVertical) { > > Maybe an 'xxx' comment here? I do think Graphite could be useful for > vertical. Please don't. Graphite might be a useful piece of technology as is, but I don't think we should promote it any further. There's no reason to believe that any vertical script would need it. If there's anything needed by any script and OpenType can't do it, we should fix that in OpenType. We have good working relationship with all major OpenType implementers and can fix things properly.

(In reply to Behdad Esfahbod from comment #63) > Please don't. Graphite might be a useful piece of technology as is, but I > don't think we should promote it any further. There's no reason to believe > that any vertical script would need it. If there's anything needed by any > script and OpenType can't do it, we should fix that in OpenType. We have > good working relationship with all major OpenType implementers and can fix > things properly. As of now, there's no vertical-shaping support in Graphite anyway, so the question doesn't arise. AFAIK, the Graphite team is not aware of any compelling need for it. However, if that were to change -- if a concrete use-case comes up, and someone creates an implementation -- then I don't think we should necessarily dismiss the idea of supporting it in Gecko.

(In reply to John Daggett (:jtd) from comment #62) > I think we need some reftests here or on the vertical canvas bug, even if > they are just simple sanity test bugs. Until we start building m-c with WRITING_MODE_VERTICAL_ENABLED (in WritingModes.h), there's no way to run such tests as part of our standard testsuites. So for now, I'm building locally with vertical enabled, and testing the individual pieces manually as they get implemented. But yes, we certainly want to start creating reftests for when we start enabling this code (and for local testing during development, of course).

Pushed with adjustments as per review comments above. https://hg.mozilla.org/integration/mozilla-inbound/rev/b5cf9eedcfbd https://hg.mozilla.org/integration/mozilla-inbound/rev/a43834fc6674 https://hg.mozilla.org/integration/mozilla-inbound/rev/1eb84f1edaf2 https://hg.mozilla.org/integration/mozilla-inbound/rev/645fe1d0eb82 https://hg.mozilla.org/integration/mozilla-inbound/rev/5d9f0299598d https://hg.mozilla.org/integration/mozilla-inbound/rev/6232edca52e2 https://hg.mozilla.org/integration/mozilla-inbound/rev/e7422ba80faa (In reply to John Daggett (:jtd) from comment #61) > Comment on attachment 8490346 [details] [diff] [review] > pt 2 - support for vertical textruns and fonts through gfxTextRun::Draw > Ew. Since fontParams.isVerticalFont and runParams.isRTL are constant across > the run, it sucks that we need to have this within the inner loop here. > Might there be a way of templating this out? AFAICT, this would basically involve replicating the whole of the DrawGlyphs loop, as well as DrawOneGlyph; and we'd need multiple copies of it, as we have horizontal, vertical-upright, and vertical-sideways renderings to consider, and for each of these there's LTR vs RTL. For the sake of pulling a few tests out of the inner loop, I don't think it'd be worth the added code bloat. The if-tests on these flags should be pretty insignificant compared to the total amount of work we're doing to render a glyph.

https://hg.mozilla.org/mozilla-central/rev/b5cf9eedcfbd https://hg.mozilla.org/mozilla-central/rev/a43834fc6674 https://hg.mozilla.org/mozilla-central/rev/1eb84f1edaf2 https://hg.mozilla.org/mozilla-central/rev/645fe1d0eb82 https://hg.mozilla.org/mozilla-central/rev/5d9f0299598d https://hg.mozilla.org/mozilla-central/rev/6232edca52e2 https://hg.mozilla.org/mozilla-central/rev/e7422ba80faa

(In reply to Behdad Esfahbod from comment #58) > (In reply to Jonathan Kew (:jfkthame) from comment #57) > > (In reply to Behdad Esfahbod from comment #56) > > > That said, I agree with the comment in there that we need OS/2 typoAscender, > > > NOT the hhea value (which is used for clipping?). > > > > No, it's not used for clipping; that's the winAscent/Descent values from > > OS/2, which can mean they vary widely from the intended line spacing. The > > hhea ascent/descent values should be OK; they've often been used by Mac > > software since the early days of TrueType. As suggested by the comment, OS/2 > > 'typo' values might be preferable, but remember that they may not be > > present: the OS/2 table is optional under OS X, and some commonly-used fonts > > don't have it. So I took the easy way for the time being. > > Sounds good. I'm probably going to do the same in hb-ot-font. FWIW, I came across this block in FreeType: https://github.com/behdad/freetype/blob/7024155328925a93b7b9e4e2a085d9ac0bab0ddc/src/truetype/ttgload.c#L1282 Quoting: * However, the specification lacks the precise definition of vertical * phantom points. Greg Hitchcock provided the following explanation. * * - a `vmtx' table is present * * For any glyph, the minimum and maximum y values (`ymin' and `ymax') * are given in the `glyf' table, the top side bearing (tsb) and advance * height (ah) are given in the `vmtx' table. The bottom side bearing * (bsb) is then calculated as * * bsb = ah - (tsb + ymax - ymin) , * * and the initial position of vertical phantom points is * * pp3 = (x, round(ymax + tsb)) , * pp4 = (x, round(pp3 - ah)) . * * See below for value `x'. * * - no `vmtx' table in the font * * If there is an `OS/2' table, we set * * DefaultAscender = sTypoAscender , * DefaultDescender = sTypoDescender , * * otherwise we use data from the `hhea' table: * * DefaultAscender = Ascender , * DefaultDescender = Descender . * * With these two variables we can now set * * ah = DefaultAscender - sDefaultDescender , * tsb = DefaultAscender - yMax , * * and proceed as if a `vmtx' table was present. * * Usually we have * * x = aw / 2 , (1) * * but there is one compatibility case where it can be set to * * x = -DefaultDescender - * ((DefaultAscender - DefaultDescender - aw) / 2) . (2) * * and another one with * * x = 0 . (3) * * In Windows, the history of those values is quite complicated, * depending on the hinting engine (that is, the graphics framework). * * framework from to formula * ---------------------------------------------------------- * GDI Windows 98 current (1) * (Windows 2000 for NT) * GDI+ Windows XP Windows 7 (2) * GDI+ Windows 8 current (3) * DWrite Windows 7 current (3) * * For simplicity, FreeType uses (1) for grayscale subpixel hinting and * (3) for everything else. I don't know how reliable that is. I don't understand the x positioning for cases 2 and 3.

FWIW, I've also seen CoreText sometimes use x=0, other times x=aw/2. I'm guessing that it does those for TrueType vs CFF respectively. We should test Windows and CoreText and adjust Firefox / HarfBuzz if needed.