543715 - distinguish 'oblique' and 'italic' values for font-style property

Status: RESOLVED FIXED

(Core :: Layout: Text and Fonts, defect)

Description

[Jonathan Kew [:jfkthame]]

CSS (and good typography) makes a distinction between italic and oblique fonts; we don't currently handle this, either for platform fonts or for @font-face.

For a test using a font family that has both Oblique and Italic faces, see

    http://people.mozilla.com/~jkew/lmr-test/lmrtest.html

(The first set of lines require the LMRTest fonts to be installed locally; see page source for the URLs to download them. The remaining lines use @font-face.)

Note that the results with @font-face depend on the order of the @font-face rules; whichever of 'italic' or 'oblique' is specified last will override the other.

On Mac OS X, the Fonts panel (e.g., in TextEdit.app) shows and can access all three styles of the LMRTest family. No browser that I have tried (Safari4/Mac, Opera10/Win, IE8/Win, or Firefox3.6) currently handles the styles correctly, however.

Comment 1

[Jim Henley]

My style font-style:italic; suddenly stopped working with latest upgrade. MAC Firefox Snow Leopard. My apologies if this doesn't belong here, but it needs to be fixed pretty quickly.

Comment 2

[philippe (part-time)]

(In reply to comment #1)
> My style font-style:italic; suddenly stopped working with latest upgrade. MAC
> Firefox Snow Leopard. My apologies if this doesn't belong here, but it needs to
> be fixed pretty quickly.

No this does not belong here. Please file a new bug, preferably with a minimal testcase.

Comment 3

[MrX1980]

example and "definition":
http://www.w3.org/TR/css3-fonts/#font-style0
http://dev.w3.org/csswg/css3-fonts/#propdef-font-style

Comment 4

[Gingerbread Man]

(In reply to Jonathan Kew (:jfkthame) from comment #0)
> we don't currently handle this, either for platform fonts

Then how come the testcase in bug 1170953 worked up until Firefox 41?

Comment 5

[Jonathan Kew [:jfkthame]]

Oh, so I guess this did work under the fontconfig/gfxPangoFonts backend. I don't think we had support for it on any other platforms.

Comment 6

[Dominik Röttsches (drott)]

I came across the same issue when trying to test the font style matching clauses of the CSS fonts module font matching algorithm. An additional option for testing can be found in my csswg-test PR https://github.com/w3c/csswg-test/pull/810

The test fonts I generated for this PR produce a ligature for the attributes that they have in their TTF filename, e.g:
CSSMatchingTest_ultraexpanded_oblique_sixhundred.ttf, produces a ligature "ultraexpanded" for the word stretch, "oblique" for style, "600" for weight.

Comment 7

[John Daggett (:jtd)]

Attachment: patch p1 - distinguish between italic/oblique for platform fonts

Distinguish between italic and oblique faces within a font family for platform fonts. Only DirectWrite and Linux will be affected by this, since other platforms don't have a mechanism to distinguish between italic and oblique faces.

Comment 8

[John Daggett (:jtd)]

Attachment: patch p2 - distinguish between italic/oblique for userfonts

Somewhat large patch but most of this is simply a bool ==> int parameter conversion.

Comment 9

[John Daggett (:jtd)]

Attachment: patch p2 - italic/oblique reftests

Comment 10

[John Daggett (:jtd)]

One thing to note, the 'Kinnari' font exists with some Ubuntu installations and contains both an italic and an oblique face. In that environment, the last of the reftests will test that font family, otherwise it will test userfont handling.

Comment 11

[Jonathan Kew [:jfkthame]]

Comment on attachment 8673479 [details] [diff] [review]
patch p1 - distinguish between italic/oblique for platform fonts

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

::: gfx/thebes/gfxFontEntry.h
@@ +389,5 @@
>      nsString         mName;
>      nsString         mFamilyName;
>  
>      bool             mItalic      : 1;
> +    bool             mOblique     : 1;

ISTM it'd make more sense to have a single (two-bit) mStyle field for the three values Normal, Italic and Oblique, given that they're mutually exclusive. We've already got the relevant NS_FONT_STYLE_* constants in gfxFontConstants.h; can't we just propagate those through to the fontEntry level? Implementing Italic and Oblique as independent bool flags here just means we end up having to check both of them in lots of places.

This change will obviously have a bunch of knock-on effects through these patches, so I'll hold off on trying to review further for now.

Comment 12

[John Daggett (:jtd)]

Attachment: patch p1 - distinguish between italic and oblique

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

> ISTM it'd make more sense to have a single (two-bit) mStyle field for the
> three values Normal, Italic and Oblique, given that they're mutually
> exclusive. We've already got the relevant NS_FONT_STYLE_* constants in
> gfxFontConstants.h; can't we just propagate those through to the fontEntry
> level? Implementing Italic and Oblique as independent bool flags here just
> means we end up having to check both of them in lots of places.

Makes sense. I've updated the patch and merged in the changes to a single patch.

Comment 13

[Jonathan Kew [:jfkthame]]

Comment on attachment 8674015 [details] [diff] [review]
patch p1 - distinguish between italic and oblique

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

This is fine as far as it goes, modulo a global rename of 'mItalicStyle' and friends (see below), but I think we should extend it to at least FT2 and Mac platforms. For platforms other than DWrite and fontconfig, we should check the OBLIQUE bit in the 'OS/2' fsSelection field (if the table is v.4 or later) to identify oblique faces. See https://www.microsoft.com/typography/otspec/os2.htm#fss.

(Feel free to do this in a separate additional patch, if that's simpler.)

::: gfx/thebes/gfxDWriteFonts.cpp
@@ +85,5 @@
>          static_cast<gfxDWriteFontEntry*>(aFontEntry);
>      nsresult rv;
>      DWRITE_FONT_SIMULATIONS sims = DWRITE_FONT_SIMULATIONS_NONE;
>      if ((GetStyle()->style & (NS_FONT_STYLE_ITALIC | NS_FONT_STYLE_OBLIQUE)) &&
> +        !(fe->IsItalic() || fe->IsOblique()) &&

This should use a single fe->IsUpright() accessor.

::: gfx/thebes/gfxFT2FontList.cpp
@@ +323,5 @@
>      fe->mFTFontIndex = aFLE.index();
>      fe->mWeight = aFLE.weight();
>      fe->mStretch = aFLE.stretch();
> +    fe->mItalicStyle = (aFLE.italic() ?
> +                        NS_FONT_STYLE_ITALIC : NS_FONT_STYLE_NORMAL);

The FontListEntry needs to be extended to store the full value of the mItalicStyle (or mFontShape, see comment on gfxFontEntry.h) field, so that we can maintain the distinction that we should be able to get from the OS/2 table.

@@ +381,5 @@
>                                const uint8_t* aFontData)
>  {
>      FT2FontEntry *fe = new FT2FontEntry(aName);
> +    fe->mItalicStyle = (FTFaceIsItalic(aFace) ?
> +                        NS_FONT_STYLE_ITALIC : NS_FONT_STYLE_NORMAL);

Better to check fsSelection so that we can identify oblique fonts.

::: gfx/thebes/gfxFont.cpp
@@ +2290,5 @@
>      // If the font may be rendered with a fake-italic effect, we need to allow
>      // for the top-right of the glyphs being skewed to the right, and the
>      // bottom-left being skewed further left.
> +    if (mStyle.style != NS_FONT_STYLE_NORMAL &&
> +        !(mFontEntry->IsItalic() || mFontEntry->IsOblique())) {

Easier to read as:

    if (mStyle.style != NS_FONT_STYLE_NORMAL && mFontEntry->IsUpright()) {
        ...
    }

::: gfx/thebes/gfxFontEntry.cpp
@@ +1178,5 @@
> +    }
> +    return distance;
> +}
> +
> +// stretch distance ==> [0,18]

This will be in the range 0..9 with the cleanup below.

@@ +1199,5 @@
>          }
>          // if the computed "distance" here is negative, it means that
>          // aFontEntry lies in the "non-preferred" direction from aTargetStretch,
>          // so we treat that as larger than any preferred-direction distance
>          // (max possible is 8) by adding an extra 10 to the absolute value

s/8/4/ and s/10/5/ here, as you're no longer doubling the distance values above.

@@ +1201,5 @@
>          // aFontEntry lies in the "non-preferred" direction from aTargetStretch,
>          // so we treat that as larger than any preferred-direction distance
>          // (max possible is 8) by adding an extra 10 to the absolute value
>          if (distance < 0) {
>              distance = -distance + 10;

So this can now use +5.

::: gfx/thebes/gfxFontEntry.h
@@ +122,5 @@
>      bool IsUserFont() const { return mIsDataUserFont || mIsLocalUserFont; }
>      bool IsLocalUserFont() const { return mIsLocalUserFont; }
>      bool IsFixedPitch() const { return mFixedPitch; }
> +    bool IsItalic() const { return mItalicStyle & NS_FONT_STYLE_ITALIC; }
> +    bool IsOblique() const { return mItalicStyle & NS_FONT_STYLE_OBLIQUE; }

I don't think we should treat these as independent bit flags. mItalicStyle is a field with three distinct enumerated values (OK, the values are #define'd constants rather than an enum, but that's beside the point.)

So it should be

  bool IsItalic() const { return mItalicStyle == NS_FONT_STYLE_ITALIC; }

etc.

(And in many/most places, I think we want to test for either italicness or obliqueness. If we add an IsUpright() accessor that tests mItalicStyle == NS_FONT_STYLE_NORMAL, we can do that more concisely.)

@@ +389,5 @@
>  
>      nsString         mName;
>      nsString         mFamilyName;
>  
> +    uint8_t          mItalicStyle : 2; // italic/oblique

I'd propose naming this mShape (or mFontShape), rather than mItalicStyle; it's choosing among three (potential) shapes normal/italic/oblique that a family may include.

Similarly, s/aItalicStyle/aFontShape/ and similar names throughout the patch.

(Oh, I just noticed that there are a lot of 'uint32_t aItalicStyle' parameters in various files; why aren't they uint8_t?)

(I'd have liked to call this mStyle, to reflect the name of the 'font-style' property and NS_FONT_STYLE_* constants, but that's too confusing given the existing gfxFontStyle struct, and various 'style' variables that use it. But I think 'shape' works, too, and is used in this way in existing typographic literature.)

::: gfx/thebes/gfxGDIFontList.cpp
@@ +306,5 @@
>          if (aCh > 0xFFFF)
>              return false;
>  
>          // previous code was using the group style
>          gfxFontStyle fakeStyle;  

While you're here, please clean up the trailing space.

@@ +312,1 @@
>              fakeStyle.style = NS_FONT_STYLE_ITALIC;

Shouldn't we maintain the distinction here, by doing

  fakeStyle.style = mFontShape;

Though in practice I suppose it won't matter because GDI doesn't support oblique as distinct from italic; any such font would have to be in a separate family anyway.

::: gfx/thebes/gfxMacFont.cpp
@@ +62,5 @@
>  
>      // synthetic oblique by skewing via the font matrix
>      bool needsOblique =
>          (mFontEntry != nullptr) &&
> +        (!(mFontEntry->IsItalic() || mFontEntry->IsOblique()) &&

mFontEntry->IsUpright()

@@ +67,1 @@
>           (mStyle.style & (NS_FONT_STYLE_ITALIC | NS_FONT_STYLE_OBLIQUE))) &&

Hmm, this line treats these as bit flags, which I think is wrong; mStyle.style isn't a bit field, it's a single enumerated value. Fix it while you're here?

::: gfx/thebes/gfxMacPlatformFontList.h
@@ +90,1 @@
>      

Maybe clean up trailing whitespace while you're in the neighborhood.

::: gfx/thebes/gfxMacPlatformFontList.mm
@@ +520,5 @@
>          // Cocoa fails to set the Italic traits bit for HelveticaLightItalic,
>          // at least (see bug 611855), so check for style name endings as well
>          if ((macTraits & NSItalicFontMask) ||
>              [facename hasSuffix:@"Italic"] ||
>              [facename hasSuffix:@"Oblique"])

"Oblique" here should be mapped to NS_FONT_STYLE_OBLIQUE, surely. (Unless checking OS/2 fsSelection makes this entirely redundant.)

Comment 14

[John Daggett (:jtd)]

Attachment: patch p1 - distinguish between italic and oblique

> >      fe->mFTFontIndex = aFLE.index();
> >      fe->mWeight = aFLE.weight();
> >      fe->mStretch = aFLE.stretch();
> > +    fe->mItalicStyle = (aFLE.italic() ?
> > +                        NS_FONT_STYLE_ITALIC : NS_FONT_STYLE_NORMAL);
> 
> The FontListEntry needs to be extended to store the full value of the
> mItalicStyle (or mFontShape, see comment on gfxFontEntry.h) field, so
> that we can maintain the distinction that we should be able to get
> from the OS/2 table.

I think you're right but I think that's a lot more work than is needed at this point since having families with both italic and oblique faces isn't going to happen on mobile. We need to explicitly support Linux and userfonts. We can figure out what to do about supporting other platforms later. I'll file a separate bug for this.

> ::: gfx/thebes/gfxFontEntry.cpp
> @@ +1178,5 @@
> > +    }
> > +    return distance;
> > +}
> > +
> > +// stretch distance ==> [0,18]
> 
> This will be in the range 0..9 with the cleanup below.
> 
> @@ +1199,5 @@
> >          }
> >          // if the computed "distance" here is negative, it means that
> >          // aFontEntry lies in the "non-preferred" direction from aTargetStretch,
> >          // so we treat that as larger than any preferred-direction distance
> >          // (max possible is 8) by adding an extra 10 to the absolute value
> 
> s/8/4/ and s/10/5/ here, as you're no longer doubling the distance values above.
> 
> @@ +1201,5 @@
> >          // aFontEntry lies in the "non-preferred" direction from aTargetStretch,
> >          // so we treat that as larger than any preferred-direction distance
> >          // (max possible is 8) by adding an extra 10 to the absolute value
> >          if (distance < 0) {
> >              distance = -distance + 10;
> 
> So this can now use +5.

The comments were actually wrong before! :) I reworked these routines a bit more and revised the comments.

> >      nsString         mName;
> >      nsString         mFamilyName;
> >  
> > +    uint8_t          mItalicStyle : 2; // italic/oblique
> 
> I'd propose naming this mShape (or mFontShape), rather than
> mItalicStyle; it's choosing among three (potential) shapes
> normal/italic/oblique that a family may include.
> 
> Similarly, s/aItalicStyle/aFontShape/ and similar names throughout the patch.
> 
> (Oh, I just noticed that there are a lot of 'uint32_t aItalicStyle'
> parameters in various files; why aren't they uint8_t?)
> 
> (I'd have liked to call this mStyle, to reflect the name of the
> 'font-style' property and NS_FONT_STYLE_* constants, but that's too
> confusing given the existing gfxFontStyle struct, and various 'style'
> variables that use it. But I think 'shape' works, too, and is used in
> this way in existing typographic literature.)

Sorry, I don't agree with mShape here, that's way too ambiguous. Maybe mSlant? Or I could live with mStyle and aStyle.

> >              fakeStyle.style = NS_FONT_STYLE_ITALIC;
> 
> Shouldn't we maintain the distinction here, by doing
> 
>   fakeStyle.style = mFontShape;
> 
> Though in practice I suppose it won't matter because GDI doesn't
> support oblique as distinct from italic; any such font would have to
> be in a separate family anyway.

Right, GDI doesn't support separate oblique faces.

> >          // Cocoa fails to set the Italic traits bit for HelveticaLightItalic,
> >          // at least (see bug 611855), so check for style name endings as well
> >          if ((macTraits & NSItalicFontMask) ||
> >              [facename hasSuffix:@"Italic"] ||
> >              [facename hasSuffix:@"Oblique"])
> 
> "Oblique" here should be mapped to NS_FONT_STYLE_OBLIQUE, surely.
> (Unless checking OS/2 fsSelection makes this entirely redundant.)

This code is a workaround, I really don't think it's necessary to tweak it. If there's a reasonable way to support oblique faces under OSX, I think we can do that on the follow on bug.

Comment 15

[John Daggett (:jtd)]

Filed bug 1215489 to improve platform support for sniffing oblique faces.

Comment 16

[Jonathan Kew [:jfkthame]]

Comment on attachment 8674835 [details] [diff] [review]
patch p1 - distinguish between italic and oblique

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

Getting close, though I think I see a couple of actual bugs that have crept in, and we still need a global change of the "italicStyle" field/variable names.

::: gfx/thebes/gfxAndroidPlatform.cpp
@@ +327,5 @@
>  gfxFontEntry*
>  gfxAndroidPlatform::LookupLocalFont(const nsAString& aFontName,
>                                      uint16_t aWeight,
>                                      int16_t aStretch,
> +                                    uint32_t aItalicStyle)

Why not uint8_t for this (here and throughout)?

::: gfx/thebes/gfxDWriteFontList.h
@@ +79,5 @@
>                                IDWriteFont *aFont) 
>        : gfxFontEntry(aFaceName), mFont(aFont), mFontFile(nullptr),
>          mForceGDIClassic(false)
>      {
> +        DWRITE_FONT_STYLE italic = aFont->GetStyle();

s/italic/style/ here, I think. Or maybe dwriteStyle, because its values are not necessarily the same as our NS_FONT_STYLE_*.

::: gfx/thebes/gfxFontEntry.cpp
@@ +1154,5 @@
>      }
>      return nullptr;
>  }
>  
> +#define SLOPE_SHIFT 2 // number of bits to contain slope distance

Let's not use the term "slope" here, please -- that sounds like it would refer to a value (integer? float?) directly representing an angle, to implement something like a (hypothetical) "font-slope: 20deg;" property allowing authors to explicitly set a degree of obliqueness.

I suppose I can live with "style" if you won't accept "shape" (see [1] for an example; it's by no means a novel usage). We already use "style" as a field name for the same thing within gfxFontStyle, for example.

Whichever, please make it consistent throughout this patch, replacing all the [am]ItalicStyle occurrences as appropriate.

[1] https://books.google.co.uk/books?id=iX9MAQAAQBAJ&pg=PA333&lpg=PA333&dq=font+shape+upright+italic+slanted

@@ +1158,5 @@
> +#define SLOPE_SHIFT 2 // number of bits to contain slope distance
> +
> +// assume italic/oblique constants can be used as bit flags
> +static_assert((NS_FONT_STYLE_ITALIC | NS_FONT_STYLE_OBLIQUE) == 0x3,
> +              "font style constants have changed, SlopeDistance needs revision");

This isn't needed with the suggested rewrite of SlopeDistance below.

@@ +1176,5 @@
> +
> +    // not the same and no alternate italic/oblique
> +    uint32_t noOther = (((either >> 1) & either) ^ 0x1) & notSame;
> +
> +    return notSame + noOther;

How very cryptic! Can't we just do a simple:

  if (aFontStyle == aTargetStyle) {
    return 0; // styles match exactly ==> 0
  }
  if (aFontStyle == NS_FONT_STYLE_NORMAL ||
      aTargetStyle == NS_FONT_STYLE_NORMAL) {
    return 2; // one is normal (but not the other) ==> 2
  }
  return 1; // neither is normal; must be italic vs oblique ==> 1

which seems much easier to understand, IMO.

@@ +1188,4 @@
>  {
>      // Compute a measure of the "distance" between the requested style
>      // and the given fontEntry,
>      // considering italicness and font-stretch but not weight.

This comment is stale -- this helper considers only stretch.

@@ +1202,5 @@
>          }
>          // if the computed "distance" here is negative, it means that
>          // aFontEntry lies in the "non-preferred" direction from aTargetStretch,
>          // so we treat that as larger than any preferred-direction distance
> +        // (max possible is 8) by adding an extra 5 to the absolute value

The max possible preferred-direction distance here is 4, not 8; anything larger than that (in absolute value) must have been in the non-preferred direction. (Which is why adding 5 -- or 4 would do, actually -- is enough to make even the smallest non-preferred distance greater than any preferred value.)

@@ +1327,5 @@
>      if (mIsSimpleFamily) {
>          // Family has no more than the "standard" 4 faces, at fixed indexes;
>          // calculate which one we want.
>          // Note that we cannot simply return it as not all 4 faces are necessarily present.
> +        bool wantItalic = aFontStyle.style & NS_FONT_STYLE_ITALIC;

I think this should be

  bool wantItalic = aFontStyle.style != NS_FONT_STYLE_NORMAL;

so that we'll correctly choose the italic face from a "simple" family if the requested font style was oblique.

@@ +1491,5 @@
>      int32_t rank = 0;
>      if (aStyle) {
>           // italics
> +         bool wantItalic = (aStyle->style != NS_FONT_STYLE_NORMAL);
> +         if (aFontEntry->IsUpright() != wantItalic) {

I think the sense of this is now backwards. But it'd be easier to follow if you replace the wantItalic bool with a wantUpright one, to compare against IsUpright().

Comment 17

[Jonathan Kew [:jfkthame]]

BTW, both the last two notes above are issues that should result in reftest failures, I think -- they have the potential to cause incorrect font selection/styling, and we should have tests that cover them. Could you please check whether that is in fact the case, and if not, add some appropriate tests for them? Thanks.

Comment 18

[John Daggett (:jtd)]

Attachment: patch p1 - distinguish between italic and oblique

Revised based on review comments, switched to uint8_t, mStyle/aStyle.

> >      if (mIsSimpleFamily) {
> >          // Family has no more than the "standard" 4 faces, at fixed indexes;
> >          // calculate which one we want.
> >          // Note that we cannot simply return it as not all 4 faces are necessarily present.
> > +        bool wantItalic = aFontStyle.style & NS_FONT_STYLE_ITALIC;
> 
> I think this should be
> 
>   bool wantItalic = aFontStyle.style != NS_FONT_STYLE_NORMAL;
> 
> so that we'll correctly choose the italic face from a "simple" family
> if the requested font style was oblique.

I've made it so that a "simple family" is limited to faces with *only* an italic face, no oblique faces. Both the setup and lookup logic follow this. I did switch the code here to use '==' instead of '&'.

Comment 19

[Jonathan Kew [:jfkthame]]

(In reply to John Daggett (:jtd) from comment #18)
> Created attachment 8675249 [details] [diff] [review]
> patch p1 - distinguish between italic and oblique
> 
> 
> Revised based on review comments, switched to uint8_t, mStyle/aStyle.
> 
> > >      if (mIsSimpleFamily) {
> > >          // Family has no more than the "standard" 4 faces, at fixed indexes;
> > >          // calculate which one we want.
> > >          // Note that we cannot simply return it as not all 4 faces are necessarily present.
> > > +        bool wantItalic = aFontStyle.style & NS_FONT_STYLE_ITALIC;
> > 
> > I think this should be
> > 
> >   bool wantItalic = aFontStyle.style != NS_FONT_STYLE_NORMAL;
> > 
> > so that we'll correctly choose the italic face from a "simple" family
> > if the requested font style was oblique.
> 
> I've made it so that a "simple family" is limited to faces with *only* an
> italic face, no oblique faces. Both the setup and lookup logic follow this.
> I did switch the code here to use '==' instead of '&'.

My point is that if it's determined (at setup time) to be a simple family (i.e. it has only upright and italic faces), but the request is for font-style:oblique, the correct result is the italic face, NOT the upright one. So you do need to make this "!= NORMAL" rather than "== ITALIC".

Comment 20

[Jonathan Kew [:jfkthame]]

I tried this locally, BTW, and confirmed that (as I expected), a testcase like

  data:text/html,<div style="font-family:times">Hello <span style="font-style:oblique">world

ends up using the upright face for "world" (with our synthetic-italic slant applied to it), because of the failure to use the italic face for 'oblique' in a simple family.

Now, I think there's an argument to be made that this is in fact a good result -- that it's better to use synthetically-slanted upright face to satisfy 'oblique' than to use a true italic face. But if so, (a) we'd need to adjust the full font-matching algorithm to behave similarly (basically, we'd need to make StyleDistance behave asymmetrically), and (b) this is not what the CSS Fonts spec currently calls for, so we should raise it as a spec issue in the WG.

So for the purpose of this bug, at least, I believe we need to fix this as above. If you think the changed is a good idea, we should address that separately in a followup.

Comment 21

[John Daggett (:jtd)]

Attachment: patch p1 - distinguish between italic and oblique

Revised based on review comments.

Comment 22

[John Daggett (:jtd)]

Attachment: patch p2 - italic/oblique reftests

Add an additional test for simple handling of italic/oblique with generics

Comment 23

[Jonathan Kew [:jfkthame]]

Comment on attachment 8675378 [details] [diff] [review]
patch p2 - italic/oblique reftests

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

For thoroughness, please add a couple more tests for the kinnari family: in addition to oblique!=italic, let's confirm that oblique!=normal and italic!=normal.

Comment 24

[Jonathan Kew [:jfkthame]]

Comment on attachment 8675378 [details] [diff] [review]
patch p2 - italic/oblique reftests

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

::: layout/reftests/font-matching/simple-oblique-ref.html
@@ +2,5 @@
> +<html>
> +<head>
> +<title>oblique italic equivalence</title>
> +<meta http-equiv="Content-Type" content="text/html; charset=utf-8" />
> +  

stray whitespace (in both test and ref files)

Comment 25

[Jonathan Kew [:jfkthame]]

Comment on attachment 8675377 [details] [diff] [review]
patch p1 - distinguish between italic and oblique

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

Thanks, looks like this should work correctly now. Though I've realized there's a followup we should do to improve the font-matching that happens during fallback; see below.

::: gfx/thebes/gfxFont.cpp
@@ +2289,5 @@
>  
>      // If the font may be rendered with a fake-italic effect, we need to allow
>      // for the top-right of the glyphs being skewed to the right, and the
>      // bottom-left being skewed further left.
> +    if (mStyle.style != NS_FONT_STYLE_NORMAL && mFontEntry->IsUpright()) {

Actually, it's strictly speaking a separate (trivial) bug, but while you're here I think it'd be correct to add

  && mStyle.allowSyntheticStyle

to this test, wouldn't it?

::: gfx/thebes/gfxFontEntry.cpp
@@ +1478,5 @@
>      int32_t rank = 0;
>      if (aStyle) {
>           // italics
> +         bool wantUpright = (aStyle->style == NS_FONT_STYLE_NORMAL);
> +         if (aFontEntry->IsUpright() == wantUpright) {

It occurs to me that we should really replace this with something based on StyleDistance, so that it properly respects italic vs oblique.

@@ +1483,5 @@
>               rank += 10;
>           }
>  
>          // measure of closeness of weight to the desired value
>          rank += 9 - DeprecatedAbs(aFontEntry->Weight() / 100 - aStyle->ComputeWeight());

And here, we should use a computation based on WeightDistance.

I think the best thing to do here, actually, would be to scrap CalcStyleMatch and its "rank" altogether, and leverage WeightStyleStretchDistance instead. We can easily reverse the tests in FindFontForChar and SearchAllFontsForChar so that they look for the smallest "distance" instead of the highest "rank", replacing the GlobalFontMatch::mMatchRank field with mMatchDistance.

Could you do that as a followup patch, please?

Comment 26

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/bc90276ec090
https://hg.mozilla.org/integration/mozilla-inbound/rev/2d1d8e9b095c

Comment 27

[Phil Ringnalda (:philor)]

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/b57b1467ee6e because Mulet is why we can't have nice things, https://treeherder.mozilla.org/logviewer.html#?job_id=15822283&repo=mozilla-inbound

Comment 28

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/10c908b9b224
https://hg.mozilla.org/integration/mozilla-inbound/rev/cb70cbb7fe6f

Comment 29

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/10c908b9b224
https://hg.mozilla.org/mozilla-central/rev/cb70cbb7fe6f

Comment 30

[Armen [:armenzg]]

It seems that the failure that Mulet was facing is the same one that I'm hitting for the new linux64 platform we're bringing up.

Is there a specific font needed for this? Any graphical changes?

This blocks us from moving to the new TaskCluster infrastructure. Any pointers in here will be greatly appreciated. bug 1224641.

Comment 31

[Jean-Yves Perrier [:teoli]]

Doc updated:
https://developer.mozilla.org/en-US/Firefox/Releases/44#CSS
and
https://developer.mozilla.org/en-US/docs/Web/CSS/font-style

Comment 32

[John Daggett (:jtd)]

(In reply to Jean-Yves Perrier [:teoli] from comment #31)
> Doc updated:
> https://developer.mozilla.org/en-US/Firefox/Releases/44#CSS
> and
> https://developer.mozilla.org/en-US/docs/Web/CSS/font-style

Thanks!

Couple comments on this. I think it would be good to highlight the fact that this is dependent on the underlying platform support for oblique faces. Under OSX and older versions of Windows, there's no explicit support for this but we could probably improve that (see bug 1215489).

The other thing is that this *will* make a difference for @font-face families, since those are completely independent of platform font data for which regards to font selection. The descriptor definitions (e.g. font-style: italic or font-style: oblique) are the only information used when selecting a given face.

In general, this is a fairly subtle choice. It's relatively rare to have a family with *both* italic and oblique faces, so 'font-style: oblique' is generally synonymous with 'font-style: italic'.

Comment 33

[Jean-Yves Perrier [:teoli]]

Ni/ myself so I don't forget to address this after Mozlando.