Closed Bug 1024804 Opened 10 years ago Closed 10 years ago

implement support for font-variant-position fallback behavior

Categories

(Core :: Graphics: Text, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla34

People

(Reporter: jtd, Assigned: jtd)

References

(Blocks 1 open bug)

Details

(Keywords: dev-doc-complete)

Attachments

(7 files, 7 obsolete files)

687.52 KB, image/png
Details
344.99 KB, image/png
Details
154.49 KB, image/png
Details
17.26 KB, patch
jfkthame
: review+
Details | Diff | Splinter Review
16.41 KB, patch
jfkthame
: review+
Details | Diff | Splinter Review
44.09 KB, patch
jfkthame
: review+
Details | Diff | Splinter Review
7.58 KB, patch
jtd
: review+
Details | Diff | Splinter Review
Split off from bug 961558.

The font-variant-position property allows access to subscript/superscript glyphs for a font:

  font-variant-position: sub | super | normal

The CSS3 Fonts spec dictates fallback when there are characters in a textrun for which no subscript/superscript glyphs are available. In this situation, synthesized glyphs need to be created based on the subscript/superscript metrics supplied by the font.
Keywords: dev-doc-needed
Implementation steps for this:

1. Check for sups/subs when font-variant-position != normal

2. If sups/subs not available, shape using reduced size font with a rendering offset,
   calculated from the OS/2 sub/superscript metrics or using reasonable defaults

   subSize = size * ( OS2.ySubscriptYSize / emSize )
   subOffset = size * ( OS2.ySubscriptYOffset / emSize )

   superSize = size * ( OS2.ySuperscriptYSize / emSize )
   superOffset = size * ( OS2.ySuperscriptYOffset / emSize )

   reasonable defaults = Adobe font defaults?

3. Implement drawing with an offset within gfxFont::Draw.
   Hmmm, how does this affect glyph extents...

4. If sups/subs available, check to assure all characters in the run
   have sups/subs substitutions

OS/2 sub/superscript metrics:
http://www.microsoft.com/typography/otspec/os2.htm#subxs
argh, spelling...

s/assure/ensure/
With this patch, if a font lacks support for subscript/superscript variants (subs/sups), synthetic glyphs are generated.  This patch uses default metrics for subscript/superscript glyphs, based on those used in Adobe fonts.

Next steps:

- synthesize using OS/2 table subscript/superscript metrics
- for fonts supporting the subs/sups features, check to make sure all characters in a string have variants and fallback to synthetic if they don't

The second part is tricky for performance reasons but required to support things like C<sub>n</sub>H<sub>2n</sub>O<sub>n</sub> since most fonts with variants have variants for numbers 0-9 but not for a wide range of alphabetic characters and common brackets.
Depends on: 1029307
Updated to use the fixed offset constants used in bug 1029307.
Attachment #8442573 - Attachment is obsolete: true
Left is Safari using HTML subscripts/superscripts. Right is patched code showing fallback glyph synthesis for font-variant-position. Note how the lines cleanly flow in the patched code version, since there is no variation in the linebox height across lines.
Shows the same example with Whitney, a font that contains subscript/superscript variants. Note how CnH2nOn is incorrectly displayed in the first column. This is because the font lacks variant glyphs for alphabetic characters and the current code does not fallback in this situation yet (as the spec requires).
There's a subtle complication here for implementing this fallback. Because the fallback needs to occur for *any* character with a given textrun, it needs to occur across scripts and fonts selected. Since the textrun code segments runs into contiguous script runs and then into contiguous font runs, the fallback logic to deal with the case where a textrun contains n script segments and the ith segment lacks variant support in one of its fonts. I think the easiest way to deal with this is simply to flush the glyph runs built so far and start over with synthetics enabled.

Simple example:

sup { font-size: inherit; vertical-align: baseline; font-variant-position: super }

e<sup>2πik/n</sup>

Given a font that supports variant glyphs for the superscript '2' but not the other characters, synthetic superscripts for the entire contents of the sup element should be synthesized and not just for the portion after the 2.
Attachment #8445048 - Attachment is obsolete: true
Fonts used: Minion Pro, Whitney, Calibri, Linux Biolinum
In the screenshot example, e<sup>2
One thing to unfortunately point out is that font designers clearly expect this feature to be used in a very limited set of cases. The e^2i case for Minion Pro looks very ugly because the superscript variants for 2 and i don't use the same baseline (!?!). This is the design of these glyphs, there's nothing Firefox can (or should) do to "fix" this.
The last part of this is to sniff the entire textrun for variant glyphs. My strategy for this is to cache the set of glyphs that are feature inputs for the subs/sups features in the font entry, and then check the default mapping of characters in the textrun to verify that every character is covered.

This *isn't* completely correct, since OpenType shaping involves a pipeline such that other features could alter the glyph ids used as input to the feature substitution for subscript/superscipts. But, meh, this is probably good enough for 99% of authoring uses of this feature.

Given a <font,script> pair:

1. Create a hbset containing input glyphs for subs/sups lookups
2. Cache by feature/script in the font entry

Given a string for a given <font,script>:

1. Create a hbset containing all default glyphs from the cmap
2. Fetch the subs/sups lookups hbset
3. Intersect the two sets into a new hbset
4. The result should be equal to the hbset created in (1)
5. Otherwise, fallback is required

This may not be the most efficient way of doing this but the sizes of the objects involved are fairly small. Typical fonts support that subscript/superscript glyphs have glyphs for digits plus a small smattering of other glyphs, so in the range of 15-20 glyphs. And the size of textruns for subscript/superscripts are typically in the 1-5 character range.
First part, implementing fallback across script runs but without variant glyph detection.
Attachment #8449223 - Attachment is obsolete: true
Attachment #8450815 - Flags: review?(jfkthame)
Detect whether all characters in a script run have variant glyphs or not.
Attachment #8450817 - Flags: review?(jfkthame)
Comment on attachment 8450815 [details] [diff] [review]
patch part1 v5 - implement subscript/superscript variant fallback

Review of attachment 8450815 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/thebes/gfxFont.cpp
@@ +903,5 @@
>                   aFeatureTag == HB_TAG('c','2','s','c') ||
>                   aFeatureTag == HB_TAG('p','c','a','p') ||
> +                 aFeatureTag == HB_TAG('c','2','p','c') ||
> +                 aFeatureTag == HB_TAG('s','u','p','s') ||
> +                 aFeatureTag == HB_TAG('s','u','b','s'),

You need this change in SupportsGraphiteFeature as well, I think.

@@ +6017,5 @@
>      return fe->FindOrMakeFont(&style, needsBold);
>  }
>  
> +gfxFloat
> +gfxFont::CalculateSupSuperSize(gfxFloat aBaseSize, float& aBaselineOffset)

There's no need to pass in aBaseSize here; that's just mStyle.size. And it makes the call-site confusing: it's called as

  CalculateSupSuperSize(style.size, style.baselineOffset);

but one of those is an in-param, the other an out-param.

So I'd suggest removing aBaseSize, and pass aBaselineOffset as a pointer instead of a reference.

And return mStyle.size * ratio, so that you're returning an actual size, as that's what the method name implies. (Or else rename it as ...Ratio. But given that mStyle.size is being factored in to the baseline offset here, it might as well be factored into the size as well.)

Oh, and in the method name: s/SupSuper/SubSuper/.

@@ +6027,5 @@
> +    gfxFloat t = (aBaseSize - NS_FONT_SUB_SUPER_SMALL_SIZE) /
> +                 (NS_FONT_SUB_SUPER_LARGE_SIZE - NS_FONT_SUB_SUPER_SMALL_SIZE);
> +    t = std::max(std::min(t, 1.0), 0.0);
> +    subSuperSizeRatio = (1.0 - t) * NS_FONT_SUB_SUPER_SIZE_RATIO_SMALL +
> +                   t * NS_FONT_SUB_SUPER_SIZE_RATIO_LARGE;

This seems rather cryptic.

And given that the default text size (16px) is below the SMALL threshold, it'd be nice to avoid the computations altogether in that case.

How about a formulation along the lines of

    if (mStyle.size <= NS_FONT_SUB_SUPER_SMALL_SIZE) {
        ratio = NS_FONT_SUB_SUPER_SIZE_RATIO_SMALL;
    } else if (mStyle.size >= NS_FONT_SUB_SUPER_LARGE_SIZE) {
        ratio = NS_FONT_SUB_SUPER_SIZE_RATIO_LARGE;
    } else {
        ...calculate it (without the max/min stuff)
    }

Oh, it just occurred to me that this is being done in terms of device-pixel size. (So on a Retina screen, the default size of 32 dev px is between SMALL and LARGE after all.) I think that's probably wrong, though; IMO, if we're going to vary the scaling of super/subscripts based on size, it should be the CSS px size that we use, so that the behavior isn't dependent on device resolution or page zoom.

@@ +7933,5 @@
> +    ResetGlyphRuns();
> +    memset(reinterpret_cast<char*>(mCharacterGlyphs), 0,
> +           mLength * sizeof(CompressedGlyph));
> +
> +    // xxx -- need to do something about mDetailedGlyphs?!?

Yes. The DetailedGlyphStore is optimized for the (usual) case where we set up glyph records sequentially. And I'm not sure offhand how it would handle creating duplicate entries for the same character offset.

I think all it takes is mDetailedGlyphs = nullptr, isn't it?

::: gfx/thebes/gfxFont.h
@@ +2188,5 @@
>  
>      bool                       mKerningSet;     // kerning explicitly set?
>      bool                       mKerningEnabled; // if set, on or off?
>  
> +    gfxFloat                   mSubSuperOffset;

I think I'd prefer this to be called mBaselineShift or mBaselineOffset.

In principle, we might find other uses for it besides fake sub/superscripts. ('BASE' table support? I haven't thought through what that would take...)

@@ +3468,5 @@
> +        eShapingState_Aborted,                // abort initial iteration
> +        eShapingState_ForceFallbackFeature    // redo with fallback forced on
> +    };
> +
> +    ShapingState GetShapingState() { return mShapingState; }

Should be a const method.
Comment on attachment 8450817 [details] [diff] [review]
patch part2 v1 - scan for variant glyphs

Review of attachment 8450817 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/thebes/gfxFont.cpp
@@ +995,5 @@
> +    }
> +
> +    NS_ASSERTION(aFeatureTag == HB_TAG('s','u','p','s') ||
> +                 aFeatureTag == HB_TAG('s','u','b','s'),
> +                 "use of unknown feature tag");

Please include a comment to remind us that the code depends (because of SCRIPT_FEATURE) on the fact that the features we support here are unique even after discarding the LSB of each tag.

@@ +1016,5 @@
> +            // for now. (Compare gfxHarfBuzzShaper.)
> +            hbScript = HB_SCRIPT_LATIN;
> +        } else {
> +            hbScript = hb_script_t(GetScriptTagForCode(aScript));
> +        }

I've seen something like this before. :) ISTM perhaps it's time to factor this out into a static helper, so that we can do something like

    gfxHarfBuzzShaper::GetHBScriptUsedForShaping(aScript);

@@ +1065,5 @@
>                   aFeatureTag == HB_TAG('c','2','s','c') ||
>                   aFeatureTag == HB_TAG('p','c','a','p') ||
> +                 aFeatureTag == HB_TAG('c','2','p','c') ||
> +                 aFeatureTag == HB_TAG('s','u','p','s') ||
> +                 aFeatureTag == HB_TAG('s','u','b','s'),

This change belongs in patch 1.

@@ +2942,3 @@
>  bool
> +gfxFont::SupportsSubSuperscript(uint32_t aSubSuperscript, const T *aString,
> +                                uint32_t aLength, int32_t aRunScript)

Given that this will (a) be rarely used, and (b) normally apply to short runs, I don't think it's worth doing the template thing and ending up with two copies of the entire method. Just write a char* wrapper that expands 8-bit text into an nsAutoString and calls the real char16_t* version.

@@ +2970,5 @@
> +    }
> +    gfxHarfBuzzShaper* shaper =
> +        static_cast<gfxHarfBuzzShaper*>(mHarfBuzzShaper.get());
> +    if (!shaper->Initialize()) {
> +        return false;

If we ever hit this early-return, we'll leak the defaultGlyphsInRun set. Just move that declaration later.

::: gfx/thebes/gfxFont.h
@@ +590,5 @@
>      nsTArray<gfxFont*> mFontsUsingSVGGlyphs;
>      nsAutoPtr<gfxMathTable> mMathTable;
>      nsTArray<gfxFontFeature> mFeatureSettings;
>      nsAutoPtr<nsDataHashtable<nsUint32HashKey,bool>> mSupportedFeatures;
> +    nsDataHashtable<nsUint32HashKey,hb_set_t*> *mFeatureInputs;

Why not an nsAutoPtr here? As it is, I think you're leaking this hashtable (if it gets created).

@@ +1651,5 @@
>                               bool& aFallbackToSmallCaps,
>                               bool& aSyntheticLowerToSmallCaps,
>                               bool& aSyntheticUpperToSmallCaps);
>  
> +

No extra blank line.
Updated based on review comments.
Attachment #8450815 - Attachment is obsolete: true
Attachment #8450815 - Flags: review?(jfkthame)
Attachment #8453851 - Flags: review?(jfkthame)
Updated based on review comments.
Attachment #8450817 - Attachment is obsolete: true
Attachment #8450817 - Flags: review?(jfkthame)
Attachment #8453852 - Flags: review?(jfkthame)
Updated with fixes to the general font-variant-position tests. With fallback rendering implemented it's no longer possible to have a simplistic reference rendering. The other reftests handle this so just avoid testing the subs=0 and sups=0 cases.
Attachment #8450820 - Attachment is obsolete: true
Attachment #8453855 - Flags: review?(jfkthame)
(In reply to Jonathan Kew (:jfkthame) from comment #16)

> Oh, it just occurred to me that this is being done in terms of device-pixel
> size. (So on a Retina screen, the default size of 32 dev px is between SMALL
> and LARGE after all.) I think that's probably wrong, though; IMO, if we're
> going to vary the scaling of super/subscripts based on size, it should be
> the CSS px size that we use, so that the behavior isn't dependent on device
> resolution or page zoom.

I think this is probably reasonable. But I think it would make sense to do this in a follow-on patch or a separate bug since I think the code in the patch is a good first pass on this.  Not quite sure how to determine CSS pixel size within gfx code.
(In reply to John Daggett (:jtd) from comment #21)
> (In reply to Jonathan Kew (:jfkthame) from comment #16)
> 
> > Oh, it just occurred to me that this is being done in terms of device-pixel
> > size. (So on a Retina screen, the default size of 32 dev px is between SMALL
> > and LARGE after all.) I think that's probably wrong, though; IMO, if we're
> > going to vary the scaling of super/subscripts based on size, it should be
> > the CSS px size that we use, so that the behavior isn't dependent on device
> > resolution or page zoom.
> 
> I think this is probably reasonable. But I think it would make sense to do
> this in a follow-on patch or a separate bug since I think the code in the
> patch is a good first pass on this.  Not quite sure how to determine CSS
> pixel size within gfx code.

The gfxFont itself doesn't know, but at the textrun-building level it shouldn't be a problem. You can pass the textrun's GetAppUnitsPerDevUnit() down through GetSubSuperscriptFont(), and use this to convert device pixels to appUnits and thence to CSS pixels.
Revised to determine scale factor based on CSS pixel size rather than device pixel size.
Attachment #8453851 - Attachment is obsolete: true
Attachment #8453851 - Flags: review?(jfkthame)
Attachment #8454441 - Flags: review?(jfkthame)
Comment on attachment 8454441 [details] [diff] [review]
patch part1 v7 - implement subscript/superscript variant fallback

Review of attachment 8454441 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/thebes/gfxFont.cpp
@@ +43,5 @@
>  #include "mozilla/Telemetry.h"
>  #include "gfxSVGGlyphs.h"
>  #include "gfxMathTable.h"
>  #include "gfx2DGlue.h"
> +#include "nsDeviceContext.h"

No need for this - there's a lighter-weight header, "mozilla/AppUnits.h", that will give you a definition for AppUnitsPerCSSPixel().

@@ +2201,5 @@
>      }
>  
> +    // font-variant-position - handled here due to the need for fallback
> +    uint32_t variantSubSuper = aStyle->variantSubSuper;
> +    switch (variantSubSuper) {

The local variable here looks redundant - just do switch (aStyle->variantSubSuper).

@@ +5521,5 @@
> +        redo = false;
> +
> +        if (sizeof(T) == sizeof(uint8_t) && !transformedString) {
> +
> +    #ifdef PR_LOGGING

Let's unindent the preprocessor directives.

@@ +6027,5 @@
>      bool needsBold = style.weight >= 600 && !fe->IsBold();
>      return fe->FindOrMakeFont(&style, needsBold);
>  }
>  
> +void 

stray trailing space

@@ +6030,5 @@
>  
> +void 
> +gfxFont::CalculateSubSuperSizeAndOffset(int32_t aAppUnitsPerDevPixel,
> +                                        gfxFloat& aSubSuperSizeRatio,
> +                                        float& aBaselineOffset)

On looking at this again, I think it'd be neater to do it entirely as a method that modifies a gfxFontStyle. I'll post a small followup to show what I mean.
Attachment #8454441 - Flags: review?(jfkthame) → review+
Comment on attachment 8453852 [details] [diff] [review]
patch part2 v2 - scan for variant glyphs

Review of attachment 8453852 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/thebes/gfxFont.cpp
@@ +130,5 @@
>      mGrFaceInitialized(false),
>      mCheckedForColorGlyph(false),
>      mWeight(500), mStretch(NS_FONT_STRETCH_NORMAL),
>      mUVSOffset(0), mUVSData(nullptr),
> +    mFeatureInputs(nullptr),

This is redundant for an nsAutoPtr, isn't it?

@@ +164,5 @@
>      mGrFaceInitialized(false),
>      mCheckedForColorGlyph(false),
>      mWeight(500), mStretch(NS_FONT_STRETCH_NORMAL),
>      mUVSOffset(0), mUVSData(nullptr),
> +    mFeatureInputs(nullptr),

ditto

@@ +2926,5 @@
>  
>      return ok;
>  }
>  
> +bool 

stray trailing space

@@ +2937,5 @@
> +    return SupportsSubSuperscript(aSubSuperscript, unicodeString.get(),
> +                                  aLength, aRunScript);
> +}
> +
> +bool 

and another

::: gfx/thebes/gfxHarfBuzzShaper.cpp
@@ +1006,2 @@
>      } else {
> +        scriptTag = gfxHarfBuzzShaper::GetHBScriptUsedForShaping(aScript);

the class prefix is redundant here
Attachment #8453852 - Flags: review?(jfkthame) → review+
Suggested followup to revise how synthetic super/subscript font variants are set up. This seems simpler/cleaner to me as a single self-contained method.
Attachment #8454707 - Flags: review?(jdaggett)
Comment on attachment 8454707 [details] [diff] [review]
followup - replace gfxFont::CalculateSubSuperSizeAndOffset with gfxFontStyle::AdjustForSubSuperscript.

Review of attachment 8454707 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/thebes/gfxFont.cpp
@@ +6704,5 @@
> +
> +    // calculate reduced size, roughly mimicing behavior of font-size: smaller
> +    float cssSize = size * aAppUnitsPerDevPixel / AppUnitsPerCSSPixel();
> +    if (cssSize < NS_FONT_SUB_SUPER_SMALL_SIZE) {
> +        cssSize *= NS_FONT_SUB_SUPER_SIZE_RATIO_SMALL;

oops, obviously I mean just size *= ....

@@ +6706,5 @@
> +    float cssSize = size * aAppUnitsPerDevPixel / AppUnitsPerCSSPixel();
> +    if (cssSize < NS_FONT_SUB_SUPER_SMALL_SIZE) {
> +        cssSize *= NS_FONT_SUB_SUPER_SIZE_RATIO_SMALL;
> +    } else if (cssSize >= NS_FONT_SUB_SUPER_SMALL_SIZE) {
> +        cssSize *= NS_FONT_SUB_SUPER_SIZE_RATIO_LARGE;

and here
Comment on attachment 8453855 [details] [diff] [review]
patch part3 v2 - reftests for font-variant-position fallback

Review of attachment 8453855 [details] [diff] [review]:
-----------------------------------------------------------------

I think I'm missing something - why a special hacked Fira with "blank omega" here?
Attachment #8454707 - Flags: review?(jdaggett) → review+
(In reply to Jonathan Kew (:jfkthame) from comment #28)

> I think I'm missing something - why a special hacked Fira with "blank omega"
> here?

Two reasons. One to have a font where feature support/character support for given characters won't change. If we used a general purpose Fira Sans it could change over time which might affect what's tested here. The "blank omega" is to allow an explicit, simple test of cross-script fallback. The e^2ω should never render as e^2, even though the omega glyph is blank, because the ω doesn't have a subscript/superscript variant.
(In reply to John Daggett (:jtd) from comment #29)
> (In reply to Jonathan Kew (:jfkthame) from comment #28)
> 
> > I think I'm missing something - why a special hacked Fira with "blank omega"
> > here?
> 
> Two reasons. One to have a font where feature support/character support for
> given characters won't change. If we used a general purpose Fira Sans it
> could change over time which might affect what's tested here.

As long as we're testing with fonts in layout/reftest/fonts, rather than relying on installed system fonts, we are in control of whether/when any updates happen. So yes, we want to put testing fonts there.

> The "blank
> omega" is to allow an explicit, simple test of cross-script fallback. The
> e^2ω should never render as e^2, even though the omega glyph is blank,
> because the ω doesn't have a subscript/superscript variant.

I don't think that requires hacking a font to have a blank omega. How about just comparing

  e<span class=sup>2ω</span>

and

  e&#xb2;<span class=sup>&#x3c9;</span>

(which should *not* render the same).

Or compare

  e<span class=sup>2</span>

with

  e<span class=sup>2<span class=white>&#x3c9;</span></span>

(where .white obviously sets .color:white so that the omega will not be visible).

So ISTM a standard Fira Sans would be fine for the tests here; let's not hack fonts more than necessary for testing.
(In reply to Jonathan Kew (:jfkthame) from comment #30)

> I don't think that requires hacking a font to have a blank omega.

Hmmm, it doesn't require it but having that test is a very direct, simple way to assuring that a span that (1) crosses scripts and (2) includes characters that do and don't have subscript/superscript variants is not rendered with a mixture of variants and fallback forms.

> How about just comparing
> 
>   e<span class=sup>2ω</span>
> 
> and
> 
>   e&#xb2;<span class=sup>&#x3c9;</span>
> 
> (which should *not* render the same).
> 
> Or compare
> 
>   e<span class=sup>2</span>
> 
> with
> 
>   e<span class=sup>2<span class=white>&#x3c9;</span></span>
> 
> (where .white obviously sets .color:white so that the omega will not be
> visible).
> 
> So ISTM a standard Fira Sans would be fine for the tests here; let's not
> hack fonts more than necessary for testing.

Several of the tests here do the testing along the lines you've outlined. They use the Unicode subscript/superscript codepoints as a way of testing whether variants are selected or not. But I think in the context of a test that is run against other browsers that we not rely exclusively on that form of testing.

There's also the need to compare with a font that doesn't support subscript/superscript features at all, which is why one of the two test fonts used here has been modified to not include support for the subs/sups features.

Having test fonts allows us to test this feature more completely than simply using just Fira Sans.
Comment on attachment 8453855 [details] [diff] [review]
patch part3 v2 - reftests for font-variant-position fallback

Review of attachment 8453855 [details] [diff] [review]:
-----------------------------------------------------------------

I'm not debating that it's good to have specific, known fonts for testing. I just didn't see that it was worth hacking the omega glyph; a standard Fira, plus a copy with features stripped, would have been adequte IMO. But whatever..... <shrug>.
Attachment #8453855 - Flags: review?(jfkthame) → review+
Depends on: 1141676
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: