John Daggett (:jtd) Assignee
	Description • 10 years ago

The CSS3 Fonts spec details several cases where it's necessary to implement synthetic fallback for font-variant properties:

* font-variant-caps

If the particular feature is not supported in the font, the browser must synthesize alternates. This is existing behavior for font-variant where the browser creates a allcaps, size=80% version of the font.

* font-variant-position

If superscript/subscript alternates are not supported for the entire text run, the entire text run uses synthesized alternates.

There are probably a number of steps here, mainly involving the text frame creation code that synthesizes these alternates currently.

	Florian Bender
	Comment 1 • 10 years ago

As far as I can see, this is the last item to fix this bug:

(In reply to John Daggett (:jtd) from comment #0)
> * font-variant-position
> 
> If superscript/subscript alternates are not supported for the entire text
> run, the entire text run uses synthesized alternates.

Right? Are there any specific bugs on file already, or are you planning to fix this in this bug?

	John Daggett (:jtd) Assignee
	Comment 2 • 10 years ago

Here. What's needed how that 1015603 has landed is the fallback for petite-caps and the subscript/superscript synthesis.

	John Daggett (:jtd) Assignee
	Comment 3 • 10 years ago

Fallback logic for font-variant-caps values:

small-caps:
  if (smcp)
    use default textrun, feat = smcp
  else
    make fake smallcaps run, only lower case letters

all-small-caps:
  if (c2sc && smcp)
    use default textrun, feat = c2sc,smcp
  else
    make fake smallcaps run, both upper/lower case letters

petite-caps:
  if (pcap)
    use default textrun, feat = pcap
  else if (smcp)
    use default textrun, feat = smcp
  else
    make fake smallcaps run, only lower case letters

all-petite-caps:
  if (c2pc && pcap)
    use default textrun, feat = c2pc,pcap
  else if (c2sc && smcp)
    use default textrun, feat = c2sc,smcp
  else
    make fake smallcaps run, both upper/lower case letters

unicase:
  if (unic)
    use default textrun, feat = unic
  else
    make fake smallcaps run, only upper case letters

We could make the fallback for all-small-caps and all-petite-caps use a combination of toLower and smcp but that adds more complexity to something that's already relative complicated.

	John Daggett (:jtd) Assignee
	Comment 4 • 10 years ago

Implements fallback for font-variant-caps property values (as described in comment 3). This doesn't implement the petite-caps ==> real small-caps fallback just yet, that's up next. And I'm fairly confident I've broken what Greek special handling there was but that should be easy to rectify. Not sure what the heck happens in the Greek unicase case but... all good things in time...

	John Daggett (:jtd) Assignee
	Comment 5 • 10 years ago

This is the fully flushed-out implementation of font-variant-caps fallback. The petite-caps and all-petite-caps cases will now fallback to the small-caps and all-small-caps cases. Unicase falls back to using 'c2sc' (without 'smcp') when 'unic' is unavailable (which is most of the time). This is better than the previous patch which did synthetic fallback for this case.

The fallback case for petite-caps/all-petite-caps is not terribly efficient, there are multiple lookups of whether a font supports a set of features due to where features get inserted into the final list of features for the shaper. But we can address this at some later point, I don't think efficiency is critical in these cases.

	John Daggett (:jtd) Assignee
	Comment 6 • 10 years ago

Reftests using Latin, Greek, Cyrillic codepoints with the latest version of FiraSans. Since this font supports small caps but not petite caps, the fallback for most cases is equivalent to just enabling 'smcp'. I added an extra step to assure than none of these match the text displayed when font-variant-caps is set to normal, so we can catch when this somehow is completely broken.

	John Daggett (:jtd) Assignee
	Comment 7 • 10 years ago

tryserver run
https://tbpl.mozilla.org/?tree=Try&rev=bf6c3f88db94

	John Daggett (:jtd) Assignee
	Comment 8 • 10 years ago

Support for font-variant-position fallback moved to separate bug, bug 1024804.

	Jonathan Kew [:jfkthame]
	Comment 9 • 10 years ago

Comment on attachment 8439021 [details] [diff] [review]
patch, implement font-variant-caps fallback

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

This basically looks good, but I think it would benefit from a bit of rearrangement to make things easier to understand. Please see what you think about suggestions/comments below...

::: gfx/src/nsFont.cpp
@@ +282,5 @@
> +  if (variant == NS_FONT_VARIANT_SMALL_CAPS) {
> +    setting.mTag = TRUETYPE_TAG('s','m','c','p');
> +    aStyle->featureSettings.AppendElement(setting);
> +  } else {
> +    aStyle->variantCaps = variantCaps;

With the variantCaps value being part of gfxFontStyle, I think it'd be cleaner to *not* do the featureSettings stuff below (mapping variantCaps values to OpenType tags), but handle all that on the gfx side (as part of MergeFontFeatures, maybe).

::: gfx/thebes/gfxFont.cpp
@@ +917,5 @@
> +
> +    int32_t featIndex = FallbackFeatureIndex(aFeatureTag);
> +    if (featIndex < 0) {
> +        return false;
> +    }

Rather than looking up a feature index here, you could just replace the low byte of the feature tag with the script code, as the features involved are adequately distinguished by the remaining bytes. (Include a comment pointing this out, and assert that the feature tag passed in is one of the known tags.)

@@ +996,5 @@
> +
> +    int32_t featIndex = FallbackFeatureIndex(aFeatureTag);
> +    if (featIndex < 0) {
> +        return false;
> +    }

As above.

@@ +2775,5 @@
> +}
> +
> +#define CASE_UPPER 0x1
> +#define CASE_LOWER 0x2
> +#define CASE_BOTH (CASE_UPPER | CASE_LOWER)

How about just having two separate bools, instead of bit flags here?

@@ +2788,5 @@
> +    aTransformCase = 0;
> +    switch (aVariantCaps) {
> +        case NS_FONT_VARIANT_CAPS_SMALLCAPS:
> +            ok = SupportsFeature(aScript, HB_TAG('s','m','c','p'));
> +            aTransformCase = CASE_LOWER;

I find it confusing that SupportsVariantCaps will return CASE_LOWER in aTransformCase even if the font supports the smcp feature - as in that case we don't want to do anything synthetic here. IMO, aTransformCase (or the pair of bools that I'd like to have instead!) should only be set to indicate where we actually need to apply a synthetic transform instead of using a real font feature.

Which means, if I'm thinking straight, that aTransformCase will only be set if this function is going to return false (meaning the actual layout feature(s) needed were not available).

@@ +2821,5 @@
> +            if (!ok) {
> +                ok = SupportsFeature(aScript, HB_TAG('c','2','s','c'));
> +                aFallbackToSmallCaps = ok;
> +            }
> +            aTransformCase = CASE_UPPER;

Unicase falls back to c2sc? Hmm.... ok, I guess that matches the text currently in css3 fonts, but it's not much of a simulation of the "unicase" feature as described in the OT registry. I wonder if we should reconsider that.

@@ +5608,5 @@
>          // create the glyph run for this range
>          if (matchedFont) {
> +            bool needsFakeSmallCaps = false;
> +            bool petiteToSmallCaps = false;
> +            uint32_t transformCase = 0;

I think the code here would be clearer with a couple of booleans, something like applyFakeSmallCapsToLower and applyFakeSmallCapsToUpper - and SupportsVariantCaps would set them ONLY if a fake run is needed.

Or an alternative I think I could live with, if you want to keep this as a single variable with bit flags: rename transformCase to applyFakeSmallCapsToCases, and return it as the function result from SupportVariantCaps; if no bits are set, then the required feature(s) are available and we don't need a fake small caps run. We shouldn't need the separate needsFakeSmallCaps return value as well as the flags telling us what to apply it to.

@@ +5760,3 @@
>          kSpecialUpper     // specials: don't shrink, but apply uppercase mapping
>      };
> +    RunCaseState runCase = kPassThru;

This isn't really RunCaseState any more, as it doesn't reflect just the case of the run, but rather the action to be performed on it (which depends on the case of the characters combined with the case-transform options passed in).

So let's rename the enum to something like
    enum RunCaseAction {
        kNoChange,
        kUppercaseReduce,
        kUppercase
    };

and rename runCase and chCase to runAction and chAction.

::: gfx/thebes/gfxFont.h
@@ +1619,5 @@
>          return mFontEntry->HasGraphiteTables();
>      }
>  
> +    // whether a feature is supported by the font
> +    bool SupportsFeature(int32_t aScript, uint32_t aFeatureTag);

This only supports a specific list of caps-related features, right? It's not a general SupportsFeature method. So (a) name it something like SupportsCapsFeature; and (b) clarify this in the comment.

@@ +1626,5 @@
> +    // aFallbackToSmallCaps true when petite caps should fallback to small caps
> +    // aTransformCase indicates which case(s) should be synthesized
> +    bool SupportsVariantCaps(int32_t aScript, uint32_t aVariantCaps,
> +                             bool& aFallbackToSmallCaps,
> +                             uint32_t& aTransformCase);

This needs some more explanation; what does it mean that a case "should be synthesized"? (I think you mean "should be sent through the synthetic-smallcaps mechanism", right? I.e. have the uppercase transform applied, and be rendered with a smaller font.)

And maybe split aTransformCase into two simple bools, rather than messing with bit flags? aSyntheticSmallCapsFromLowercase and ...FromUppercase?

::: gfx/thebes/gfxGraphiteShaper.cpp
@@ +146,5 @@
> +                break;
> +            default:
> +                break;
> +        }
> +    }

For Graphite, the result of SupportsVariantCaps is not dependent on aScript, so it doesn't need to be called on every use of ShapeText; it can be done once during shaper initialization.

It occurs to me we could pass a flag (either a separate ShaperType enum, or a special script value to mean Graphite instead of OpenType shaping) to SupportsVariantCaps and thence to SupportsFeature, instead of it having to determine for itself every time which technology to check. We've made the decision which shaper to use already, before getting here, so we can propagate that decision instead of re-making it at a lower level.

	John Daggett (:jtd) Assignee
	Comment 10 • 10 years ago

(In reply to Jonathan Kew (:jfkthame) from comment #9)
> > +            if (!ok) {
> > +                ok = SupportsFeature(aScript, HB_TAG('c','2','s','c'));
> > +                aFallbackToSmallCaps = ok;
> > +            }
> > +            aTransformCase = CASE_UPPER;
> 
> Unicase falls back to c2sc? Hmm.... ok, I guess that matches the text
> currently in css3 fonts, but it's not much of a simulation of the
> "unicase" feature as described in the OT registry. I wonder if we should
> reconsider that.

Hmm, re-reading the OT registry description I think you're probably right.
Probably best to just treat 'unicase' as a feature with no fallback.

	John Daggett (:jtd) Assignee
	Comment 11 • 10 years ago

(In reply to Jonathan Kew (:jfkthame) from comment #9)

Revised patch based on review comments. Removed 'unicase' fallback case
(will follow-up on www-style once this lands to fix spec).

> ::: gfx/thebes/gfxFont.h
> @@ +1619,5 @@
> >          return mFontEntry->HasGraphiteTables();
> >      }
> >  
> > +    // whether a feature is supported by the font
> > +    bool SupportsFeature(int32_t aScript, uint32_t aFeatureTag);
> 
> This only supports a specific list of caps-related features, right? It's not
> a general SupportsFeature method. So (a) name it something like
> SupportsCapsFeature; and (b) clarify this in the comment.

Well, I'm using it in some other patches to check for subs/sups support, so
it's not really a SupportsCapsFeature method. Added a comment.

> It occurs to me we could pass a flag (either a separate ShaperType enum, or
> a special script value to mean Graphite instead of OpenType shaping) to
> SupportsVariantCaps and thence to SupportsFeature, instead of it having to
> determine for itself every time which technology to check. We've made the
> decision which shaper to use already, before getting here, so we can
> propagate that decision instead of re-making it at a lower level.

Hmmm, not sure that buys us much here.  In the non-Graphite case the first
condition will always fail on mGraphiteShaper. Not sure passing a bool
around helps much.

	John Daggett (:jtd) Assignee
	Comment 12 • 10 years ago

Updated to remove the unicase tests (since these now don't have fallback behavior).

	Jonathan Kew [:jfkthame]
	Comment 13 • 10 years ago

Comment on attachment 8441870 [details] [diff] [review]
patch, implement font-variant-caps fallback

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

Looks OK, modulo a few nits.

::: gfx/src/nsFont.cpp
@@ +278,5 @@
> +  // passed into gfxFontStyle to deal with appropriate fallback.
> +  // for now, font-variant setting overrides font-variant-caps
> +  // when font-variant becomes a shorthand, this will be removed
> +  if (variant == NS_FONT_VARIANT_SMALL_CAPS) {
> +    aStyle->variantCaps = NS_FONT_VARIANT_CAPS_SMALLCAPS;

This looks like it should make the separate smallCaps field in gfxFontStyle virtually obsolete, no?

There's a place in CanvasRenderingContext2D that creates a gfxFontStyle; if that were updated to set the variantCaps field instead, I think we could trim out the gfxFontStyle smallCaps field.

Maybe as a followup?

::: gfx/thebes/gfxFont.cpp
@@ +881,5 @@
>      mHasGraphiteTables = HasFontTable(TRUETYPE_TAG('S','i','l','f'));
>  }
>  
> +#define FEATURE_SHIFT 8 // 8 bits for script
> +#define FEATURE_SCRIPT_MASK ((1 << FEATURE_SHIFT) - 1)

As we're no longer actually using this to shift a feature index, how about simplifying this and just saying

#define FEATURE_SCRIPT_MASK 0x000000ff // script index replaces low byte of tag

And you could include

PR_STATIC_ASSERT(MOZ_NUM_SCRIPT_CODES <= FEATURE_SCRIPT_MASK,
                 "too many script codes!");

to check that there's still room when the list of script codes grows.

@@ +2152,5 @@
>          aMergedFeatures.Put(feature.mTag, feature.mValue);
>      }
>  
> +    // font-variant-caps - handled here due to the need for fallback handling
> +    // petite caps and unicase cases can fallback to appropriate smallcaps

not "and unicase"

@@ +2154,5 @@
>  
> +    // font-variant-caps - handled here due to the need for fallback handling
> +    // petite caps and unicase cases can fallback to appropriate smallcaps
> +    uint32_t variantCaps = aStyle->variantCaps;
> +    switch (variantCaps) {

The contents of this switch should be updated to the 4-space indent used in this file, I think. Until such time as we do a global indent fix...

@@ +2165,5 @@
> +
> +      case NS_FONT_VARIANT_CAPS_ALLPETITE:
> +        aMergedFeatures.Put(
> +            aAddSmallCaps ? HB_TAG('c','2','s','c') : HB_TAG('c','2','p','c'),
> +            1);

Ewww...the line wrapping here seems awkward. I think it'd probably read better if you break after the ":" and align the HB_TAGs, and then the trailing "1);" will fit on the same line.

@@ +2170,5 @@
> +        // fall through to the petite-caps case
> +      case NS_FONT_VARIANT_CAPS_PETITECAPS:
> +        aMergedFeatures.Put(
> +            aAddSmallCaps ? HB_TAG('s','m','c','p') : HB_TAG('p','c','a','p'),
> +            1);

Ditto. At least try it, and see what you think of how it looks.

@@ +2773,5 @@
> +}
> +
> +#define CASE_UPPER 0x1
> +#define CASE_LOWER 0x2
> +#define CASE_BOTH (CASE_UPPER | CASE_LOWER)

These are now obsolete.

@@ +2802,5 @@
> +                aSyntheticUpperToSmallCaps = true;
> +            }
> +            break;
> +        case NS_FONT_VARIANT_CAPS_PETITECAPS:
> +            aFallbackToSmallCaps = false;

This was already initialized above.

@@ +2813,5 @@
> +                aSyntheticLowerToSmallCaps = true;
> +            }
> +            break;
> +        case NS_FONT_VARIANT_CAPS_ALLPETITE:
> +            aFallbackToSmallCaps = false;

And again.

@@ +2828,5 @@
> +            }
> +            break;
> +        default:
> +            break;
> +    }

Just for my sanity, to confirm I'm following things correctly and document the expected results, let's assert some things:

(a) if we're returning true, then we shouldn't be using anything synthetic:

NS_ASSERTION(!(ok && (aSyntheticLowerToSmallCaps ||
                      aSyntheticUpperToSmallCaps)),
             "shouldn't use synthetic features if we found real ones");

(b) if we found a fallback feature, that counts as ok:

NS_ASSERTION(!(!ok && aFallbackToSmallCaps),
             "if we found a usable fallback, that counts as ok");

Right?

::: gfx/thebes/gfxFont.h
@@ +279,5 @@
>      bool IgnoreGDEF() const { return mIgnoreGDEF; }
>      bool IgnoreGSUB() const { return mIgnoreGSUB; }
>  
> +    bool SupportsOpenTypeFeature(int32_t aScript, uint32_t aFeatureTag);
> +    bool SupportsGraphiteFeature(uint32_t aFeatureTag);

As for the gfxFont::SupportsFeature method, I think you should include a comment here noting that these methods only support a specific list of features, not any generic feature tag.

(Having dispensed with the feature index aspect, they can in principle support many more features - but as they rely on the features being uniquely distinguished by three of their four tag characters, we need to keep the assertion that will force us to review this list whenever we want to check for anything new.)

@@ +1624,5 @@
> +    bool SupportsFeature(int32_t aScript, uint32_t aFeatureTag);
> +
> +    // whether the font supports "real" small caps, petite caps etc.
> +    // aFallbackToSmallCaps true when petite caps should fallback to small caps
> +    // aTransformCase indicates which case(s) should be synthesized

Comment needs updating.

	Jonathan Kew [:jfkthame]
	Comment 14 • 10 years ago

Comment on attachment 8441873 [details] [diff] [review]
patch, reftests for font-variant-caps fallback

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

::: layout/reftests/font-features/caps-fallback-allpetitecaps.html
@@ +22,5 @@
> +}
> +
> +p::before {
> +  content: "Aa Bb Gg Δδ Γγ Σσ Бб Фф";
> +}

Any particular reason to use p::before instead of just including the text directly in the <p> element? It seems like an unnecessary level of indirection; in general, I think we should keep testcases as straightforward as reasonably possible for the feature they're testing.

	John Daggett (:jtd) Assignee
	Comment 15 • 10 years ago

Revised based on review comments. Carrying forward r+ jfkthame

Will add a follow-on patch to trim out the smallCaps field.

	John Daggett (:jtd) Assignee
	Comment 16 • 10 years ago

Revised based on comments.

	John Daggett (:jtd) Assignee
	Comment 17 • 10 years ago

The variantCaps value now handles the functionality for which the smallCaps bool field in gfxFontStyle was used.  Trim out.

	Jonathan Kew [:jfkthame]
	Comment 18 • 10 years ago

Comment on attachment 8443269 [details] [diff] [review]
patch, reftests for font-variant-caps fallback

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

Personally, I'd probably trim the unused CSS rules out of each file, leaving only the one that's actually used in each case. (It's not easy to spot the difference between caps-fallback-smallcaps1 and caps-fallback-smallcaps2 here!) But if you really prefer to leave them this way, I suppose that's OK.

	Jonathan Kew [:jfkthame]
	Comment 19 • 10 years ago

Comment on attachment 8443271 [details] [diff] [review]
followup patch, trim smallCaps field from gfxFontStyle

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

::: gfx/thebes/gfxFont.h
@@ +153,2 @@
>      // caps variant (small-caps, petite-caps, etc.)
>      // (note: once font features are enabled this will subsume the bool above)

This comment can go, too. :)

	John Daggett (:jtd) Assignee
	Comment 20 • 10 years ago

Pushed to inbound, including changes based on review comments:
https://hg.mozilla.org/integration/mozilla-inbound/rev/97ff78dbfdc8
https://hg.mozilla.org/integration/mozilla-inbound/rev/e60a15293355
https://hg.mozilla.org/integration/mozilla-inbound/rev/59a9a1ecb4fe

	Ryan VanderMeulen [:RyanVM]
	Comment 21 • 10 years ago

https://hg.mozilla.org/mozilla-central/rev/97ff78dbfdc8
https://hg.mozilla.org/mozilla-central/rev/e60a15293355
https://hg.mozilla.org/mozilla-central/rev/59a9a1ecb4fe

	Jean-Yves Perrier [:teoli]
	Comment 22 • 10 years ago

Comment added on 
https://developer.mozilla.org/en-US/docs/Web/CSS/font-variant-caps
https://developer.mozilla.org/en-US/Firefox/Releases/33

	[Baboo]
	Comment 23 • 10 years ago

The ß-SS mapping applies always as well to fonts that have an uppercase ß character? I think newer IE versions might handle that differently.

	John Daggett (:jtd) Assignee
	Comment 24 • 10 years ago

(In reply to [Baboo] from comment #23)
> The ß-SS mapping applies always as well to fonts that have an uppercase ß
> character? I think newer IE versions might handle that differently.

Wrong bug?

	Jean-Yves Perrier [:teoli]
	Comment 25 • 10 years ago

(In reply to John Daggett (:jtd) from comment #24)
> (In reply to [Baboo] from comment #23)
> > The ß-SS mapping applies always as well to fonts that have an uppercase ß
> > character? I think newer IE versions might handle that differently.
> 
> Wrong bug?

In fact, no, but let's phrase it as the generic question :-)

With font-variant-caps: small-caps, we change characters from lowercase to uppercase. What happens for characters like the ß? If an uppercase version doesn't exist in the font, will it be changed to SS? And if it does exist will it use the small-caps ß glyph?

	Jonathan Kew [:jfkthame]
	Comment 26 • 10 years ago

(In reply to Jean-Yves Perrier [:teoli] from comment #25)
> (In reply to John Daggett (:jtd) from comment #24)
> > (In reply to [Baboo] from comment #23)
> > > The ß-SS mapping applies always as well to fonts that have an uppercase ß
> > > character? I think newer IE versions might handle that differently.
> > 
> > Wrong bug?
> 
> In fact, no, but let's phrase it as the generic question :-)
> 
> With font-variant-caps: small-caps, we change characters from lowercase to
> uppercase. What happens for characters like the ß? If an uppercase version
> doesn't exist in the font, will it be changed to SS? And if it does exist
> will it use the small-caps ß glyph?

The behavior should not depend on what's present in the font. The standard uppercase mapping for "ß" is "SS"; see http://www.unicode.org/Public/UCD/latest/ucd/SpecialCasing.txt; see also http://en.wikipedia.org/wiki/%C3%9F#Substitution_and_all_caps.

The (relatively newly-encoded) U+1E9E LATIN CAPITAL LETTER SHARP S "ẞ" should be used only if explicitly written as such by the author, not as a result of font-variant or text-transform styling.

	John Daggett (:jtd) Assignee
	Comment 27 • 10 years ago

(In reply to Jean-Yves Perrier [:teoli] from comment #25)
> With font-variant-caps: small-caps, we change characters from lowercase to
> uppercase. What happens for characters like the ß? If an uppercase version
> doesn't exist in the font, will it be changed to SS? And if it does exist
> will it use the small-caps ß glyph?

Jonathan has answered this I think but let me just add that bugs related to how specific characters are transformed is *not* this bug, however related it may seem. Your question deals with the specifics of synthetic small-caps while this bug is basically, "use font variant glyph otherwise fallback to synthetic small-caps".