1065002 - add an Orientation parameter to gfxFont::GetMetrics() and to the nsFontMetrics object

Status: RESOLVED FIXED

(Core :: Graphics: Text, defect)

Description

[Jonathan Kew [:jfkthame]]

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

Comment 1

[Jonathan Kew [:jfkthame]]

Attachment: pt 1 - move the declaration of gfxGlyphMetrics to be after gfxFont.

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

Comment 2

[Jonathan Kew [:jfkthame]]

Attachment: pt 2 - add an orientation parameter to gfxFont::GetMetrics, to allow requesting vertical metrics as well as horizontal.

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 3

[Jonathan Kew [:jfkthame]]

Attachment: pt 3 - add an orientation field to nsFontMetrics.

Comment 4

[John Daggett (:jtd)]

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.

Comment 5

[Jonathan Kew [:jfkthame]]

(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.

Comment 6

[Jonathan Kew [:jfkthame]]

Attachment: pt 2 - add an orientation parameter to gfxFont::GetMetrics, to allow requesting vertical metrics as well as horizontal.

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

Comment 7

[Jonathan Kew [:jfkthame]]

Attachment: pt 2 - add an orientation field to nsFontMetrics.

Comment 8

[Jonathan Kew [:jfkthame]]

Attachment: pt 1 - add an orientation parameter to gfxFont::GetMetrics, to allow requesting vertical metrics as well as horizontal.

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).

Comment 9

[Jonathan Kew [:jfkthame]]

Attachment: pt 1 - add an orientation parameter to gfxFont::GetMetrics, to allow requesting vertical metrics as well as horizontal.

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 10

[John Daggett (:jtd)]

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).

Comment 11

[Jonathan Kew [:jfkthame]]

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 12

[Jonathan Kew [:jfkthame]]

Attachment: pt 1.1 - Rename gfxFont::GetMetrics to GetHorizontalMetrics, and add a GetMetrics wrapper to access it.

Comment 13

[Jonathan Kew [:jfkthame]]

Attachment: pt 1.2 - Add CreateVerticalMetrics to gfxFont, to read/synthesize metrics for vertical layout.

Comment 14

[Jonathan Kew [:jfkthame]]

Attachment: pt 1.3 - Add an Orientation parameter to gfxFont::GetMetrics and dispatch to horizontal or vertical as needed.

Comment 15

[John Daggett (:jtd)]

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.

Comment 16

[Jonathan Kew [:jfkthame]]

Attachment: pt 1.2 - Add CreateVerticalMetrics to gfxFont, to read/synthesize metrics for vertical layout.

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 17

[John Daggett (:jtd)]

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.

Comment 18

[Jonathan Kew [:jfkthame]]

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

Comment 19

[Carsten Book [:Tomcat]]

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