If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

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

RESOLVED FIXED in mozilla35

Status

()

Core
Graphics: Text
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: jfkthame, Assigned: jfkthame)

Tracking

unspecified
mozilla35
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments, 7 obsolete attachments)

16.62 KB, patch
jtd
: review+
Details | Diff | Splinter Review
10.55 KB, patch
jtd
: review+
Details | Diff | Splinter Review
25.27 KB, patch
jtd
: review+
Details | Diff | Splinter Review
10.24 KB, patch
jtd
: review+
Details | Diff | Splinter Review
(Assignee)

Description

3 years ago
See bug 902762 comment 26. This will allow us to ask fonts for either Horizontal or Vertical metrics, as needed for layout.
(Assignee)

Comment 1

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

Updated

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

Comment 2

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

Comment 3

3 years ago
Created attachment 8486619 [details] [diff] [review]
pt 3 - add an orientation field to nsFontMetrics.
Attachment #8486619 - Flags: review?(jdaggett)

Comment 4

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

Comment 5

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

Updated

3 years ago
Depends on: 1066043
(Assignee)

Comment 6

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

Updated

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

Updated

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

Comment 7

3 years ago
Created attachment 8489239 [details] [diff] [review]
pt 2 - add an orientation field to nsFontMetrics.
Attachment #8489239 - Flags: review?(jdaggett)
(Assignee)

Updated

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

Comment 8

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

Updated

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

Updated

3 years ago
Attachment #8489239 - Attachment description: pt 3 - add an orientation field to nsFontMetrics. → pt 2 - add an orientation field to nsFontMetrics.
(Assignee)

Comment 9

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

Updated

3 years ago
Attachment #8490703 - Attachment is obsolete: true
Attachment #8490703 - Flags: review?(jdaggett)

Updated

3 years ago
Attachment #8489239 - Flags: review?(jdaggett) → review+

Comment 10

3 years ago
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).
Attachment #8491322 - Flags: review?(jdaggett) → review-
(Assignee)

Comment 11

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

Comment 12

3 years ago
Created attachment 8496514 [details] [diff] [review]
pt 1.1 - Rename gfxFont::GetMetrics to GetHorizontalMetrics, and add a GetMetrics wrapper to access it.
Attachment #8496514 - Flags: review?(jdaggett)
(Assignee)

Comment 13

3 years ago
Created attachment 8496515 [details] [diff] [review]
pt 1.2 - Add CreateVerticalMetrics to gfxFont, to read/synthesize metrics for vertical layout.
Attachment #8496515 - Flags: review?(jdaggett)
(Assignee)

Comment 14

3 years ago
Created attachment 8496516 [details] [diff] [review]
pt 1.3 - Add an Orientation parameter to gfxFont::GetMetrics and dispatch to horizontal or vertical as needed.
Attachment #8496516 - Flags: review?(jdaggett)
(Assignee)

Updated

3 years ago
Attachment #8491322 - Attachment is obsolete: true

Updated

3 years ago
Attachment #8496514 - Flags: review?(jdaggett) → review+

Updated

3 years ago
Attachment #8496516 - Flags: review?(jdaggett) → review+

Comment 15

3 years ago
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.
Attachment #8496515 - Flags: review?(jdaggett) → review-
(Assignee)

Comment 16

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

Updated

3 years ago
Attachment #8496515 - Attachment is obsolete: true

Comment 17

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

Comment 18

3 years ago
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
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
You need to log in before you can comment on or make changes to this bug.