See bug 902762 comment 26. This will allow us to ask fonts for either Horizontal or Vertical metrics, as needed for layout.

This just moves the gfxGlyphExtents class later in the file, because the following patch will make it depend on gfxFont.

The metrics returned for orientation=vertical here are a bit hackish for now, but enough to get us started. I'm sure we'll want to refine this in due course.

Comment on attachment 8486617 [details] [diff] [review] pt 1 - move the declaration of gfxGlyphMetrics to be after gfxFont. How about breaking this out into a separate file? gfxFont.cpp is really way, way too big.

(In reply to John Daggett (:jtd) from comment #4) > Comment on attachment 8486617 [details] [diff] [review] > pt 1 - move the declaration of gfxGlyphMetrics to be after gfxFont. > > How about breaking this out into a separate file? gfxFont.cpp is really way, > way too big. It certainly is. Well, if we're going to do that, how about we bite the bullet and split gfxFont.cpp into several major parts, to make it a bit more manageable? I've filed bug 1066043 for this; patch coming shortly. Then I'll rebase this and the rest of the vertical-text stuff on top of that.

Rebased on top of bug 1066043; dropped the former pt 1 here, no longer needed.

Minor update to improve the behavior of 'fake' vertical metrics we use when font lacks an actual vmtx table (typical for horizontal-script fonts used in sideways vertical text).

One more update to vertical-metrics, enabling us to pass unit tests when vertical support is enabled: https://tbpl.mozilla.org/?tree=Try&rev=3fd0d32339e3. (This try push includes bug 1068027, bug 1065002 parts 1 and 2, bug 902762 parts 1-7, and bug 902799, as well as a local patch to enable vertical writing mode).

Comment on attachment 8491322 [details] [diff] [review] pt 1 - add an orientation parameter to gfxFont::GetMetrics, to allow requesting vertical metrics as well as horizontal. Review of attachment 8491322 [details] [diff] [review]: ----------------------------------------------------------------- ::: gfx/thebes/gfxDWriteFonts.cpp @@ +138,1 @@ > } You're basically repeating this code across all platform font types. Better to rename the current 'GetMetrics' to 'GetHorizontalMetrics' (pure virtual) and create a new GetMetrics containing the logic above, with 'return *mMetrics;' replaced with 'return GetHorizontalMetrics();'. ::: gfx/thebes/gfxFont.cpp @@ +3225,5 @@ > + // May be modified if actual vertical metrics are available > + // (typically CJK fonts). > + const Metrics& horizMetrics = GetMetrics(eHorizontal); > + ::memcpy(metrics, &horizMetrics, sizeof(Metrics)); > + Sorry but this is not such a great assumption. Looking at the definition of 'vhea' defines ascent/descent/linegap differently. Better to simply use 1/2 em-height as the OpenType spec page suggests is typical. I really think the logic should just be to synthesize a default for all these values and let the 'vhea' data override that. @@ +3279,5 @@ > + metrics->maxDescent = > + std::max(metrics->maxDescent, int16_t(os2->xAvgCharWidth) * > + gfxFloat(mFUnitsConvFactor)); > + } > + } omit. @@ +3304,5 @@ > + // XXX probably not appropriate for vertical! > + SET_SIGNED(underlineOffset, post->underlinePosition); > + SET_UNSIGNED(underlineSize, post->underlineThickness); > + } > + } Reading the underline metrics doesn't make sense, they aren't appropriate for upright runs at all. @@ +3311,5 @@ > +#undef SET_SIGNED > + > + metrics->spaceWidth = metrics->aveCharWidth; > + > + CalculateDerivedMetrics(*metrics); I don't think this will be right for the strikeout metrics, since this method calculates the strikeout to be 1/2 x-height. For vertical, strikeout should be centered I think. ::: gfx/thebes/gfxFont.h @@ +1445,5 @@ > + enum Orientation { > + eHorizontal, > + eVertical > + }; > + virtual const gfxFont::Metrics& GetMetrics(Orientation aOrientation) = 0; As noted above, change s/GetMetrics/GetHorizontalMetrics/ and create a new method for GetMetrics(orientation).

I've moved the old GetMetrics to GetHorizontalMetrics, and added a GetMetrics wrapper to handle the orientation, as suggested; also tidied up stuff in CreateVerticalMetrics. I've tried to do something a bit more sensible for underline and strikeout position, but until we have code that can use these to draw decoration lines on a vertical run, it didn't seem worth worrying too much... I'm sure we will find things to refine once we see this in actual use, and can review the effects of various heuristics.

Comment on attachment 8496515 [details] [diff] [review] pt 1.2 - Add CreateVerticalMetrics to gfxFont, to read/synthesize metrics for vertical layout. (In reply to Jonathan Kew (:jfkthame) from comment #11) > I've moved the old GetMetrics to GetHorizontalMetrics, and added a > GetMetrics wrapper to handle the orientation, as suggested; also tidied up > stuff in CreateVerticalMetrics. I've tried to do something a bit more > sensible for underline and strikeout position, but until we have code that > can use these to draw decoration lines on a vertical run, it didn't seem > worth worrying too much... I'm sure we will find things to refine once we > see this in actual use, and can review the effects of various heuristics. Sorry but I don't think that's really the right approach. If we're going to default to anything it should *not* be the horizontal defaults. This approach wastes time pulling in these values when I doubt they would be of much use in the vertical case. Please use a fixed set of defaults for now with 'vhea' values overriding those. Then we can determine what makes sense based on implementation experience.

OK, here's a version that minimizes copying from the horizontal metrics (though in the case of a non-sfnt font, we'll still use the horizontal metrics that the platform back-end figured out as a basis for synthesizing stuff here).

Comment on attachment 8496875 [details] [diff] [review] pt 1.2 - Add CreateVerticalMetrics to gfxFont, to read/synthesize metrics for vertical layout. Review of attachment 8496875 [details] [diff] [review]: ----------------------------------------------------------------- Ok, this is fine for now. We can refine this later based on real usage.

https://hg.mozilla.org/integration/mozilla-inbound/rev/acfc96bde64d https://hg.mozilla.org/integration/mozilla-inbound/rev/adbf88894825 https://hg.mozilla.org/integration/mozilla-inbound/rev/c9c0762bbe78 https://hg.mozilla.org/integration/mozilla-inbound/rev/cb0565ea1091

https://hg.mozilla.org/mozilla-central/rev/acfc96bde64d https://hg.mozilla.org/mozilla-central/rev/adbf88894825 https://hg.mozilla.org/mozilla-central/rev/c9c0762bbe78 https://hg.mozilla.org/mozilla-central/rev/cb0565ea1091