475891 - (unicode-range) implement unicode-range support in user font set

Status: RESOLVED FIXED

(Core :: Graphics, defect, P2)

Description

[John Daggett (:jtd)]

To support the unicode-range feature of @font-face, the gfxUserFontSet needs to add support for segmented fonts.  With the unicode-range feature, multiple @font-face rules can essentially be unioned into a single font face such that a single font name in a font list maps to a set of fonts, each with a potentially restricted char map (i.e. the intersection of the font's cmap with the range specified in the @font-face definition).  The current font lookup maps <family name, style> ==> font-entry where font-entry is a specific *face* based on the style.  This will need to be modified to <family name, style> ==> [list of font-entry objs], where the list of font-entry objects reflects each of the possible fonts for a given family.  From that point on, the font-entry objects turn into gfxFont's and the code functions as is.

Simple example:

@font-face {
  font-family: JapaneseWithGentium;
  src: local(MSMincho);
  /* no range specified, defaults to entire range */
}

@font-face {
  font-family: JapaneseWithGentium;
  src: url(Gentium.ttf);
  unicode-range: U+0-2FF;
}

I also think with this we should be able to support font linking on Windows more easily.  See bug 362093 "Support FontLink" for details.

Latest spec:
http://dev.w3.org/csswg/css3-fonts/

Note: there are still ongoing discussions in www-style related to this feature, but hopefully most of those will be ironed out over the next month or two.

Comment 1

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Jonathan has some work-in-progress on refactoring the font setup, which may bear on this.

Comment 2

[John Daggett (:jtd)]

Attachment: patch, v.0.1, add unicode-range support to gfxUserFontSet

Passes in parsed unicode-range data when adding a user font.  Next step is to rework loading code to load at font matching time.

Comment 8

[Darxus]

All those duplicates have different test cases.  Please check them before closing this bug.  

Also blocks ietestcenter.

Comment 9

[John Daggett (:jtd)]

As part of a solution for a default font problem on Samsung Galaxy devices (bug 636042), Jonathan put together a patch that allows an array of fonts to be returned from FindFontsForStyle.

https://bugzilla.mozilla.org/attachment.cgi?id=548890

Comment 10

[Jonathan Kew [:jfkthame]]

(In reply to John Daggett (:jtd) from comment #9)
> As part of a solution for a default font problem on Samsung Galaxy devices
> (bug 636042), Jonathan put together a patch that allows an array of fonts to
> be returned from FindFontsForStyle.
> 
> https://bugzilla.mozilla.org/attachment.cgi?id=548890

I've made this into a new issue (bug 677900) that blocks this and several others.

Comment 11

[Bug Filler]

This bug prevents subset Chinese Web Font from working properly, therefore it keeps Chinese font from being used over the web.

Since other browsers had this feature supported for more than a year, this bug really make firefox look bad.

Comment 12

[Paul Kögel]

please fix this!

Comment 14

[Eric A. Meyer]

Giving this one a nudge.  Unless there’s some expectation that 'unicode-range' will change behavior in the near future, this seems like it should get implemented as soon as is reasonable.  WebKit has had this for quite some time now, as has Internet Explorer.

Comment 16

[Mathias Bynens]

Note that `text-transform` also needs to be considered for this. For example, if the HTML contains:

    <p>FOO BAR BAZ</p>

…but this is lowercased through CSS:

    p { text-transform: lowercase; }

…then the font should only be downloaded if the *lowercased* text matches any of the symbols in the given `unicode-range`.

Comment 19

[Karim Ratib]

For reference, a simple test of the feature is here: http://meyerweb.com/eric/css/tests/css3/show.php?p=unicode-range

Comment 20

[Karim Ratib]

FWIW, I've implemented a polyfill for unicode-range using jQuery: https://github.com/infojunkie/jquery-unicode-range. Proof-of-concept working currently, comments and pull requests welcome.

Comment 21

[yiorsi]

Nobody fix it?

Comment 22

[Ben Kelly [:bkelly, not reviewing]]

FYI, Jake Archibald wrote a blog post about the impact this issue has on web site optimization:

  http://jakearchibald.com/2014/minimising-font-downloads/

Comment 23

[Alfonso Martinez]

This would be an excellent feature to use, and it's a shame only Chrome and Opera really support it currently. I have no example to provide, only the sincere desire to have this work in FF when I do begin to experiment with it.

Comment 24

[John Daggett (:jtd)]

Attachment: patch, implement unicode-range support (non-linux) (wip)

First pass on full implementation for non-Linux platforms. Linux implementation will happen on bug 1056479, since only Linux doesn't use the font matching code in gfxFontGroup.

Comment 25

[John Daggett (:jtd)]

Note: depends on patches on bug 998869.

Comment 26

[John Daggett (:jtd)]

One thing that remains to do for this is to figure out some sort of "load only one font at a time logic". We explicitly want to avoid starting the load of a general fallback font while a language-specific subset font is loading.

Ex:

  @font-face {
    font-family: test;
    src: url(fallback.woff);
    /* nothing specified ==> defaults to u+0:10ffff */
  }

  @font-face {
    font-family: test;
    src: url(generaluse.woff);
    unicode-range: u+0-4ff;
  }

For characters within the u+000-4ff range we don't want to load the fallback font unless the general use font loads and doesn't contain a given character. The current patch would load both for any character in this range.

My thinking right now is the logic should be:

  if (character is in range and font is not loaded) 
    if (no other font in same family is loading)
      load font

This would assure that fallback.woff would only be loaded for characters outside the u+0-4ff range *or* for characters not available in generaluse.woff.

Comment 27

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to John Daggett (:jtd) from comment #26)
>   if (character is in range and font is not loaded) 
>     if (no other font in same family is loading)

Maybe this test should be (no other font in same family and with matching unicode-range is loading)?

>       load font

Comment 28

[John Daggett (:jtd)]

(In reply to David Baron [:dbaron] (UTC-7) (needinfo? for questions) from comment #27)
> (In reply to John Daggett (:jtd) from comment #26)
> >   if (character is in range and font is not loaded) 
> >     if (no other font in same family is loading)
> 
> Maybe this test should be (no other font in same family and with matching
> unicode-range is loading)?

Yeah, that makes more sense.

Comment 29

[John Daggett (:jtd)]

Attachment: patch v2, implement unicode-range support (non-linux)

Comment 30

[John Daggett (:jtd)]

Attachment: patch, unicode-range reftests

Comment 31

[John Daggett (:jtd)]

Attachment: patch v3, implement unicode-range support (non-linux)

Implement unicode-range support for non-Linux platforms.

- pass the unicode-range map into the cmap of the userfont container entry
- revise style matching to allow a number of fonts to be matched for a given weight/width/style combination
- when building fontlist, add these multiple fonts to the fontlist
- only load one font per family at a time when font matching

Comment 32

[Jonathan Kew [:jfkthame]]

Comment on attachment 8504477 [details] [diff] [review]
patch v3, implement unicode-range support (non-linux)

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

I haven't finished reading through all this yet, but posting some interim comments... in particular, I think you can apply 'const' to a bunch of pointers/references, to make it clearer where things are only being read, never modified.

Have you tested whether this plays well with the MathML code? I'm a bit concerned that trying to provide fonts for MathML, using unicode-range to split up the various math alphabets (for example) -- which seems a very reasonable use-case -- may not behave properly, but I haven't tried to construct a testcase yet.

Leaving r? flag, as I intend to look at this some more.

::: gfx/thebes/gfxFont.cpp
@@ +209,5 @@
>  
>  bool
>  gfxFontCache::HashEntry::KeyEquals(const KeyTypePointer aKey) const
>  {
> +    gfxCharacterMap *fontUnicodeRangeMap = mFont->GetUnicodeRangeMap();

const gfxCharacterMap*

@@ +2999,5 @@
>      style.size *= SMALL_CAPS_SCALE_FACTOR;
>      style.variantCaps = NS_FONT_VARIANT_CAPS_NORMAL;
>      gfxFontEntry* fe = GetFontEntry();
>      bool needsBold = style.weight >= 600 && !fe->IsBold();
> +    return fe->FindOrMakeFont(&style, needsBold, mUnicodeRangeMap.get());

I don't think we need to explicitly .get() here, do we?

@@ +3009,5 @@
>      gfxFontStyle style(*GetStyle());
>      style.AdjustForSubSuperscript(aAppUnitsPerDevPixel);
>      gfxFontEntry* fe = GetFontEntry();
>      bool needsBold = style.weight >= 600 && !fe->IsBold();
> +    return fe->FindOrMakeFont(&style, needsBold, mUnicodeRangeMap.get());

ditto

::: gfx/thebes/gfxFont.h
@@ +1614,4 @@
>          return mFontEntry->HasCharacter(ch); 
>      }
>  
> +    gfxCharacterMap* GetUnicodeRangeMap() const {

Can this use const gfxCharacterMap*?

@@ +1617,5 @@
> +    gfxCharacterMap* GetUnicodeRangeMap() const {
> +        return mUnicodeRangeMap.get();
> +    }
> +
> +    void SetUnicodeRangeMap(gfxCharacterMap* aUnicodeRangeMap) {

And this?

@@ +2017,5 @@
>      nsAutoPtr<gfxFontShaper>   mGraphiteShaper;
>  
> +    // if a userfont with unicode-range specified, contains map of *possible*
> +    // ranges supported by font
> +    nsRefPtr<gfxCharacterMap> mUnicodeRangeMap;

And this. It doesn't change once it's been set, does it?

::: gfx/thebes/gfxFontEntry.cpp
@@ +276,5 @@
>  
>  already_AddRefed<gfxFont>
> +gfxFontEntry::FindOrMakeFont(const gfxFontStyle *aStyle,
> +                             bool aNeedsBold,
> +                             gfxCharacterMap* aUnicodeRangeMap)

const gfxCharacterMap* again

@@ +1178,5 @@
> +//
> +// Example: with target 600 and font weight 800, distance will be 200. With
> +// target 300 and font weight 600, distance will be 1300, since heavier weights
> +// are farther away than lighter weights. If the target is 5 and the font weight
> +// 995, the distance would be 1990 for the same reason.

This comment seems odd, given the (current) CSS constraint that weights are multiples of 100.

@@ +1227,2 @@
>      if (!mHasStyles)
>          FindStyleVariations(); // collect faces for the family, if not already done

While you're here, let's add the missing braces.

@@ +1228,5 @@
>          FindStyleVariations(); // collect faces for the family, if not already done
>  
>      NS_ASSERTION(mAvailableFonts.Length() > 0, "font family with no faces!");
>  
> +    aFontEntryList.Clear();

This shouldn't be needed; we only call this method with a brand-new, empty array for the results. If anything, add an NS_ASSERTION here that checks it is empty.

@@ +1310,4 @@
>  
> +    uint32_t minDistance = 0xffffffff;
> +    gfxFontEntry* matched = nullptr;
> +    bool inserted = false;

No need for a separate bool, you can just test !aFontEntryList.IsEmpty().

@@ +1310,5 @@
>  
> +    uint32_t minDistance = 0xffffffff;
> +    gfxFontEntry* matched = nullptr;
> +    bool inserted = false;
> +    for (int32_t i = count - 1; i >= 0; i--) {

Is the backward iteration here a deliberate choice, to give priority to faces defined later in the case of @font-face rules with overlapping ranges? (I haven't followed all the code to see whether that's in fact how it works, but it seems plausible.) If that's so, please include a comment noting the significance of the direction here.

@@ +1318,5 @@
> +            (StyleStretchDistance(fe, wantItalic, aFontStyle.stretch) *
> +             MAX_WEIGHT_DISTANCE);
> +        if (distance > minDistance) {
> +            continue;
> +        } else if (distance < minDistance) {

No need for 'else' after 'continue'.

@@ +1324,5 @@
> +            if (inserted) {
> +                aFontEntryList.Clear();
> +                inserted = false;
> +            }
> +            minDistance = distance;

And if you similarly 'continue' here, the following 'else' can also go.

@@ +1335,4 @@
>          }
>      }
>  
> +    if (matched) {

There should *always* be a matched font, shouldn't there? So let's assert that.

@@ +1415,3 @@
>  bool
>  gfxFontFamily::FindWeightsForStyle(gfxFontEntry* aFontsForWeights[],
>                                     bool anItalic, int16_t aStretch)

Haven't you just removed the only user of this method (in FindFontForStyle)? If so, let's nuke it altogether, rather than fixing it up to still compile!

::: gfx/thebes/gfxFontEntry.h
@@ +62,5 @@
>      gfxCharacterMap() :
>          mHash(0), mBuildOnTheFly(false), mShared(false)
>      { }
>  
> +    gfxCharacterMap(gfxSparseBitSet& aOther) :

const gfxSparseBitSet&

@@ +313,5 @@
>  
> +    already_AddRefed<gfxFont>
> +    FindOrMakeFont(const gfxFontStyle *aStyle,
> +                   bool aNeedsBold,
> +                   gfxCharacterMap* aUnicodeRangeMap = nullptr);

I think this can be const gfxCharacterMap*.

::: gfx/thebes/gfxTextRun.cpp
@@ +1743,5 @@
>  
>      nsRefPtr<gfxFont> font = ff.Font();
>      if (!font) {
> +        gfxFontEntry* fe = mFonts[i].FontEntry();
> +        gfxCharacterMap* unicodeRangeMap = nullptr;

const gfxCharacterMap*

@@ +1793,5 @@
> +    }
> +}
> +
> +bool
> +gfxFontGroup::FontLoadingForFamily(gfxFontFamily* aFamily, uint32_t aCh)

This looks like it could be declared as a const method, I think...

@@ +1800,5 @@
> +    for (uint32_t i = 0; i < count; ++i) {
> +        FamilyFace& ff = mFonts[i];
> +        if (ff.IsLoading() && ff.Family() == aFamily) {
> +            gfxUserFontEntry* ufe =
> +                static_cast<gfxUserFontEntry*>(ff.FontEntry());

...as could the |ff| and |ufe| variables.

::: gfx/thebes/gfxUserFontSet.cpp
@@ +877,5 @@
>                                           bool& aNeedsBold,
>                                           bool& aWaitForUserFont)
>  {
>      aWaitForUserFont = false;
> +    gfxFontEntry* fe = aFamily->FindFontForStyle(aFontStyle, aNeedsBold);

For peace of mind, it might be nice to retain the assertion that used to be in FindUserFontEntry:

    NS_ASSERTION(!fe || fe->mIsUserFontContainer, ....)

Comment 33

[Jonathan Kew [:jfkthame]]

Attachment: Testcase where unicode-range behaves incorrectly

I was going to experiment with math character ranges, but before I got very far with testcases, I found a (non-math) example that AFAICT behaves incorrectly with the patch here; see attached.

What I see is both 'a' glyphs being rendered from STIXGeneral. However, if I remove the third @font-face rule (which shouldn't be relevant for these characters anyhow), they render in STIX and Optima respectively, as expected.

Comment 34

[John Daggett (:jtd)]

Attachment: patch v4, implement unicode-range support (non-linux)

Updated based on review comments.

> @@ +1617,5 @@
> > +    gfxCharacterMap* GetUnicodeRangeMap() const {
> > +        return mUnicodeRangeMap.get();
> > +    }
> > +
> > +    void SetUnicodeRangeMap(gfxCharacterMap* aUnicodeRangeMap) {
> 
> And this?
> 
> @@ +2017,5 @@
> >      nsAutoPtr<gfxFontShaper>   mGraphiteShaper;
> >  
> > +    // if a userfont with unicode-range specified, contains map of *possible*
> > +    // ranges supported by font
> > +    nsRefPtr<gfxCharacterMap> mUnicodeRangeMap;
> 
> And this. It doesn't change once it's been set, does it?

These are ref-counted objects, I don't think you can specify 'const' here.

> >  already_AddRefed<gfxFont>
> > +gfxFontEntry::FindOrMakeFont(const gfxFontStyle *aStyle,
> > +                             bool aNeedsBold,
> > +                             gfxCharacterMap* aUnicodeRangeMap)
> 
> const gfxCharacterMap* again

Here again, FindOrMakeFont passes this to another refptr so can't be const.

> @@ +1178,5 @@
> > +//
> > +// Example: with target 600 and font weight 800, distance will be 200. With
> > +// target 300 and font weight 600, distance will be 1300, since heavier weights
> > +// are farther away than lighter weights. If the target is 5 and the font weight
> > +// 995, the distance would be 1990 for the same reason.
> 
> This comment seems odd, given the (current) CSS constraint that weights are multiples of 100.

I've made this so that it doesn't assume the weights are multiples
of 100, since it does affect performance. I've added a comment to that effect.

> ::: gfx/thebes/gfxTextRun.cpp
> @@ +1743,5 @@
> >  
> >      nsRefPtr<gfxFont> font = ff.Font();
> >      if (!font) {
> > +        gfxFontEntry* fe = mFonts[i].FontEntry();
> > +        gfxCharacterMap* unicodeRangeMap = nullptr;
> 
> const gfxCharacterMap*

Calls FindOrMakeFont, can't be const.

Comment 35

[Jonathan Kew [:jfkthame]]

Comment on attachment 8508635 [details] [diff] [review]
patch v4, implement unicode-range support (non-linux)

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

This basically looks fine - just a couple of small nits below. But also one question - which may be more to do with my lack of understanding than anything about the code itself, but in that case, some additional notes to help readers follow what's going on would be much appreciated.

::: gfx/thebes/gfxTextRun.cpp
@@ +1690,1 @@
>              // or not. loading is initiated during font matching.

nit: please capitalize full-sentence comments, for better readability.

@@ +1880,5 @@
>              return font;
>          }
>  
> +        // in cases where unicode range might apply, use space char
> +        uint32_t space = 0x20;

This seems a bit arbitrary. Can you provide some comments to document what's going on here -- what will this be used for, and what (if anything) happens if some or all of the user fonts don't include <space> in their unicode-range? Might this cause the source whose unicode-range includes <space> to be loaded even if there's no <space> present in the text?

Might it be better to make this method GetFirstValidFontForChar(), and let callers pass a "character of interest"? They could still pass <space> if they don't know any better, but (for example) in MakeHyphenTextRun it'd presumably be better to pass '-'.

::: layout/style/FontFaceSet.cpp
@@ +881,5 @@
> +  aFontFace->GetDesc(eCSSFontDesc_UnicodeRange, val);
> +  unit = val.GetUnit();
> +  if (unit == eCSSUnit_Array) {
> +    unicodeRanges = new gfxCharacterMap();
> +    nsCSSValue::Array const & sources = *val.GetArrayValue();

Please rearrange this to the (more common, I believe) style of

  const nsCSSValue::Array& sources = ...

Comment 36

[John Daggett (:jtd)]

(In reply to Jonathan Kew (:jfkthame) from comment #35)
> Comment on attachment 8508635 [details] [diff] [review]
> patch v4, implement unicode-range support (non-linux)
> 
> Review of attachment 8508635 [details] [diff] [review]:
> -----------------------------------------------------------------
> @@ +1880,5 @@
> >              return font;
> >          }
> >  
> > +        // in cases where unicode range might apply, use space char
> > +        uint32_t space = 0x20;
> 
> This seems a bit arbitrary. Can you provide some comments to document what's
> going on here -- what will this be used for, and what (if anything) happens
> if some or all of the user fonts don't include <space> in their
> unicode-range? Might this cause the source whose unicode-range includes
> <space> to be loaded even if there's no <space> present in the text?
> 
> Might it be better to make this method GetFirstValidFontForChar(), and let
> callers pass a "character of interest"? They could still pass <space> if
> they don't know any better, but (for example) in MakeHyphenTextRun it'd
> presumably be better to pass '-'.

This is one area where I think spec additions need to be made. For situations where a downloadable font family is the first family in a font list, a font needs to be chosen for line metrics. So the question is, if the first family is one with a set of faces with different unicode-range values, which should be selected?  I simply chose to use the space character as an arbitrary way to pick one. I guess we could also simply pick the first font within the set of faces, that would be just as easy to define.

Comment 37

[Jonathan Kew [:jfkthame]]

(In reply to John Daggett (:jtd) from comment #36)

> This is one area where I think spec additions need to be made. For
> situations where a downloadable font family is the first family in a font
> list, a font needs to be chosen for line metrics. So the question is, if the
> first family is one with a set of faces with different unicode-range values,
> which should be selected?  I simply chose to use the space character as an
> arbitrary way to pick one. I guess we could also simply pick the first font
> within the set of faces, that would be just as easy to define.

More generally: if the font-family property for an element begins with a downloadable font family, but there is no actual text present, should the font be downloaded at all? Or should anything that might depend on font metrics (such as line-height) fall back to using the first *available* font, which may simply be the browser's default font, skipping any preceding (unloaded) user fonts in the list?

(See the XXX comment at gfxFontGroup::BuildFontList() in https://bugzilla.mozilla.org/page.cgi?id=splinter.html&bug=754215&attachment=8407464 for where I wondered about this behavior previously.)

Comment 38

[John Daggett (:jtd)]

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

> More generally: if the font-family property for an element begins with a
> downloadable font family, but there is no actual text present, should the
> font be downloaded at all? Or should anything that might depend on font
> metrics (such as line-height) fall back to using the first *available* font,
> which may simply be the browser's default font, skipping any preceding
> (unloaded) user fonts in the list?

Right, not pulling down a font at all is one option. I don't think it should be based on current load state because that would lead to non-deterministic display of pages which would suck for authors. So I think the options are:

  1. Pick an arbitrary character and use that (the patch here and Chrome use the space character).

  2. Use the first/last face defined for a family.

  3. Use the first platform font in the fontlist or the default font

At this point, I think it's simplest to align ourselves with what Chrome is doing. I'll post on www-style and we can adjust this based on the conclusions there.

Comment 39

[John Daggett (:jtd)]

Attachment: testcase, unicode-range pull test

This testcase demonstrates which fonts a browser uses for determine line metrics.

Steps:

1. Copy to a local http instance that logs access
2. Load page in browser
3. Note in the access logs which url's are pulled (they are all bogus so all the responses will be 404's)

Chrome/Gecko w/patch:

GET /tests/unicoderange-default.woff HTTP/1.1 404 234
GET /tests/unicoderange-u20.woff HTTP/1.1 404 230

Comment 40

[John Daggett (:jtd)]

Posted on www-style regarding what to do with font requests for metrics:
http://lists.w3.org/Archives/Public/www-style/2014Oct/0472.html

Comment 41

[John Daggett (:jtd)]

Attachment: patch v6, implement unicode-range support (non-linux)

This adds a character parameter to GetFirstValidFont and passes in a character in the two cases where that's something other than a space.

Unless there are other issues, I think we should try and get this landed. If there are changes due to the CSSWG deciding on some other behavior, we can adjust it in a follow-up bug.

Comment 42

[Jonathan Kew [:jfkthame]]

OK, I guess that's fine for now.

What's the status of this on Linux? I'm a bit concerned at the idea of landing unicode-range support for some platforms but not all, as this sounds like a real headache for web developers. Perhaps we should keep this behind a pref until we can turn it on for all platforms?

Comment 43

[Jonathan Kew [:jfkthame]]

(In reply to John Daggett (:jtd) from comment #41)
> Created attachment 8512383 [details] [diff] [review]
> patch v6, implement unicode-range support (non-linux)
> 
> This adds a character parameter to GetFirstValidFont 

Don't you need to add that parameter to the gfxPangoFontGroup override as well? Otherwise it will no longer be overriding the gfxFontGroup method at all, and I fear bad things will happen...

Comment 44

[John Daggett (:jtd)]

Attachment: patch v6a, implement unicode-range support (non-linux)

Sorry about that, I realized that after doing a try push. Fixed.

As for the Linux implementation, that's what I'm starting now. I'm going to try to bite the bullet and rework the gfxPangoFontGroup code to be less of a special snowflake (bug 1056479) but if that fails I'll try to at least restructure the load behavior so we can limit the use of the font to characters that match the unicode-range charset.

Comment 45

[Jonathan Kew [:jfkthame]]

Yeah, it'd be awesome if we could get Linux using the same font-matching code as everywhere else, though I'm concerned how tricky it'll be to maintain compatibility with people's varied fontconfig setups.

Meanwhile, what's your landing plan here? On the one hand, it'd be nice to get this landed and let people start testing, etc.; but on the other hand, I don't like the idea of shipping the feature without comparable support across all platforms. Thoughts?

Comment 46

[John Daggett (:jtd)]

(In reply to Jonathan Kew (:jfkthame) from comment #45)
> Yeah, it'd be awesome if we could get Linux using the same font-matching
> code as everywhere else, though I'm concerned how tricky it'll be to
> maintain compatibility with people's varied fontconfig setups.

Right, I'm trying to do the first thing but if that runs into problems, I think my plan B is to separate out the handling of downloadable fonts to deal with unicode-range correctly. But that's definitely sub-optimal.

> Meanwhile, what's your landing plan here? On the one hand, it'd be nice to
> get this landed and let people start testing, etc.; but on the other hand, I
> don't like the idea of shipping the feature without comparable support
> across all platforms. Thoughts?

I'd like to get this landed so that we get testing on this. Since I reworked weight matching, there's a potential for non-Linux-only regressions unrelated to unicode-range usage. I'd like to deal with those quickly.  If you're concerned about parity issues we could bracket things in #if RELEASE. If we simply do this around the point where a unicode-range cmap is passed around and instead pass a nullptr in, that would effectively make disable the feature.

Comment 47

[John Daggett (:jtd)]

Jonathan, please let me know what you'd like me to do here. I'd like to move this forward.

Comment 48

[Jonathan Kew [:jfkthame]]

Let's have this behind #ifndef RELEASE_BUILD for the time being, please. While it'd be good to get testing started, I am concerned about the complications it would introduce for web authors if we release this without support on all platforms.

Comment 49

[John Daggett (:jtd)]

(In reply to Jonathan Kew (:jfkthame) from comment #48)
> Let's have this behind #ifndef RELEASE_BUILD for the time being, please.
> While it'd be good to get testing started, I am concerned about the
> complications it would introduce for web authors if we release this without
> support on all platforms.

Argh, just realized we don't have a good way of doing this for features that are reftested. So either we (1) land this with the reftests commented out or (2) gate landing this until the Linux version is done. I think I'd prefer (1) here.

Comment 50

[Jonathan Kew [:jfkthame]]

(In reply to John Daggett (:jtd) from comment #49)
> (In reply to Jonathan Kew (:jfkthame) from comment #48)
> > Let's have this behind #ifndef RELEASE_BUILD for the time being, please.
> > While it'd be good to get testing started, I am concerned about the
> > complications it would introduce for web authors if we release this without
> > support on all platforms.
> 
> Argh, just realized we don't have a good way of doing this for features that
> are reftested. So either we (1) land this with the reftests commented out or
> (2) gate landing this until the Linux version is done. I think I'd prefer
> (1) here.

How about having a runtime pref instead of compile-time? It'd be simple enough to test Preferences::GetBool("layout.css.unicode-range.enabled") in the places where you're about to pass a range, and pass null if it's disabled.

Then we can have the pref explicitly forced on for the tests, but set the default to false on the release channel with an #ifdef in all.js.

Comment 51

[John Daggett (:jtd)]

Attachment: patch v7, implement unicode-range support (non-linux)

Ok, use a pref to hide unicode-range implementation from release builds. The code does this by simply not passing through the unicode-range map when the pref is not enabled.

Comment 52

[John Daggett (:jtd)]

Attachment: patch v2, unicode-range reftests

Updated to use unicode-range pref

Comment 53

[Jonathan Kew [:jfkthame]]

Comment on attachment 8517247 [details] [diff] [review]
patch v7, implement unicode-range support (non-linux)

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

OK, let's get this landed. One last question - does this allow us to enable some more currently-skipped tests? (And if not, why not? As the existing comment in the manifest will no longer make sense.)

::: gfx/thebes/gfxTextRun.cpp
@@ +2680,5 @@
> +        nsRefPtr<gfxFont> font = ff.Font();
> +        if (font) {
> +            if (font->HasCharacter(aCh)) {
> +                return font.forget();
> +            } else {

Don't need |else| after a |return|.

::: layout/reftests/font-face/reftest.list
@@ +51,3 @@
>  HTTP(..) == order-1.html order-1-ref.html
> +pref(layout.css.unicode-range.enabled,true) random-if(gtk2Widget) HTTP(..) == order-2.html order-2-ref.html # bug 1056479
> +pref(layout.css.unicode-range.enabled,true) random-if(gtk2Widget) HTTP(..) == order-3.html order-3-ref.html # bug 1056479

These should presumably be fails-if(gtk2Widget), rather than random-if, shouldn't they?

@@ +59,5 @@
>  HTTP(..) != prop-order-over-rule-order-1a.html prop-order-over-rule-order-1b.html
>  skip-if(B2G) HTTP(..) == cross-iframe-1.html cross-iframe-1-ref.html # bug 773482
>  
>  # Dynamic changes
> +# we need to skip these because of the bug that's causing order-2.html to fail

With unicode-range support enabled, order-2 is no longer marked as failing. So can we enable that here as well, and stop skipping these? (For non-gtk2Widget, at present.)

Comment 54

[Jonathan Kew [:jfkthame]]

Comment on attachment 8517250 [details] [diff] [review]
patch v2, unicode-range reftests

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

Please clean up spacing in the test files before landing. There are a bunch of lines that need de-<tab>bing.

These are OK as a beginning, but I think we should also have some slightly more extensive tests to check that unicode-range is really behaving as intended: not only that it's rendering correctly, but also that it's avoiding downloads that shouldn't happen. E.g. define a family with a bunch of resources for different ranges, and apply it to an element that contains text from only one of the ranges covered; we should check (e.g. with a server that logs the requests made) that the other resources are not requested. Then modify the text in the element to include a character from a different range, and check that the new resource (and only the correct one) gets requested, etc.

::: layout/reftests/font-face/reftest.list
@@ +62,5 @@
> +# unicode-range (Linux implementation - bug 1056479)
> +pref(layout.css.unicode-range.enabled,true) random-if(gtk2Widget) HTTP(..) == unicoderange-1.html unicoderange-1-ref.html
> +pref(layout.css.unicode-range.enabled,true) random-if(gtk2Widget) HTTP(..) == unicoderange-2.html unicoderange-2-ref.html
> +pref(layout.css.unicode-range.enabled,true) random-if(gtk2Widget) HTTP(..) == unicoderange-3.html unicoderange-3-ref.html
> +pref(layout.css.unicode-range.enabled,true) random-if(gtk2Widget) HTTP(..) == unicoderange-4.html unicoderange-4-ref.html

Are these really random on Linux at present, or can they be explicitly marked as failing until we have an implementation there?

Comment 55

[Jonathan Kew [:jfkthame]]

BTW, there's a test in bug 879963 that uses a "toy" .sjs server to log requests and then check that we've made the sequence of requests that were expected; you could do something along those lines to test that unicode-range is requesting the expected resources (and no others), including its behavior when new content is added to the text.

Comment 56

[John Daggett (:jtd)]

(In reply to Jonathan Kew (:jfkthame) from comment #54)
> These are OK as a beginning, but I think we should also have some slightly
> more extensive tests to check that unicode-range is really behaving as
> intended: not only that it's rendering correctly, but also that it's
> avoiding downloads that shouldn't happen.

I started working on tests using the Font Loading API and quickly found out that there are still bugs there that prevent it's use for that purpose. I'll file another bug to explicitly test load behavior when unicode-range is used.

Comment 57

[John Daggett (:jtd)]

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

> >  # Dynamic changes
> > +# we need to skip these because of the bug that's causing order-2.html to fail
> 
> With unicode-range support enabled, order-2 is no longer marked as failing.
> So can we enable that here as well, and stop skipping these? (For
> non-gtk2Widget, at present.)

No, these fail everywhere. I think these tests predate the use of timeouts, so the iframe trick fails to function correctly. The tests need to be reworked with the Font Loading API instead. I'll file a separate bug.

Comment 58

[John Daggett (:jtd)]

Pushed to inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/cc8520c640df
https://hg.mozilla.org/integration/mozilla-inbound/rev/5ae2eb9534c8

Comment 59

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/cc8520c640df
https://hg.mozilla.org/mozilla-central/rev/5ae2eb9534c8

Comment 60

[Karl Tomlinson (:karlt)]

(In reply to John Daggett (:jtd) from comment #44)
> As for the Linux implementation, that's what I'm starting now. I'm going to
> try to bite the bullet and rework the gfxPangoFontGroup code to be less of a
> special snowflake (bug 1056479) but if that fails I'll try to at least
> restructure the load behavior so we can limit the use of the font to
> characters that match the unicode-range charset.

(In reply to John Daggett (:jtd) from comment #46)
> (In reply to Jonathan Kew (:jfkthame) from comment #45)
> > Yeah, it'd be awesome if we could get Linux using the same font-matching
> > code as everywhere else, though I'm concerned how tricky it'll be to
> > maintain compatibility with people's varied fontconfig setups.
> 
> Right, I'm trying to do the first thing but if that runs into problems, I
> think my plan B is to separate out the handling of downloadable fonts to
> deal with unicode-range correctly. But that's definitely sub-optimal.

I expect the core change to produce the correct layout would be intersecting
the unicode range with the FC_CHARSET property on the pattern, in
gfxUserFcFontEntry::AdjustPatternToCSS().

But there is also managing the download, which would ideally be XP.

Comment 61

[John Daggett (:jtd)]

(In reply to Karl Tomlinson (:karlt) from comment #60)

> I expect the core change to produce the correct layout would be intersecting
> the unicode range with the FC_CHARSET property on the pattern, in
> gfxUserFcFontEntry::AdjustPatternToCSS().
> 
> But there is also managing the download, which would ideally be XP.

Preventing out-of-range use of a font is fairly easy, preventing the initial load (which is the key feature of this descriptor) is what the current code makes very difficult.

Comment 62

[yiorsi]

天朝人民发来贺电！〒▽〒

Comment 63

[yiorsi]

Demo: http://jsbin.com/jeqoguzeye/1/edit?html,output

Comment 64

[Jean-Yves Perrier [:teoli]]

Chris Mills documented unicode-range long ago: https://developer.mozilla.org/en-US/docs/Web/CSS/unicode-range
and a contributor, yisi, completed the support table.

I added a mention in: https://developer.mozilla.org/en-US/Firefox/Releases/36#CSS

Release Note Request
[Why is this notable]: a long-awaited feature by Web devs
[Suggested wording]: Added support for the unicode-range CSS descriptor.
[Links (documentation, blog post, etc)]: https://developer.mozilla.org/en-US/docs/Web/CSS/unicode-range

Comment 65

[Rod]

When 36 is released, will unicode-range still be behind a default-off preference layout.css.unicode-range.enabled? - we would love to start serving unicode-range to Firefox from Google Fonts when it becomes enabled by default.

FWIW, I tried unicode-range on developer edition (36.0a2), tl;dr works on Windows and Mac and rather broken on Ubuntu (as expected).

The tests I tried follow; I have distinguished PASS (worked exactly as expected), USABLE (downloaded latin even though no latin was used in text), and FAIL (did something worse than download a bonus latin).

1) http://jsbin.com/jocodedi/1/quiet, "<body></body>"
Expected: download nothing
OSX: PASS 
Win8.1: PASS
Ubuntu: PASS

2) http://jsbin.com/cibalono/1/quiet, Text “Hello”
Expected: download latin
OSX: PASS
Win8.1: PASS
Ubuntu: PASS

3) http://jsbin.com/xadaluwo/1/quiet, Euro sign with fallback
Expected: download fallback
OSX: USABLE, downloads fallback and latin
Win8.1: USABLE, downloads fallback and latin
Ubuntu: FAIL, downloads only latin.

4) http://jsbin.com/muwufeve/3/quiet, Text “лич”
Expected: download cyrillic
OSX: USABLE, downloads latin and cyrillic
Win8.1: USABLE, downloads latin and cyrillic
Ubuntu: FAIL, downloads only latin

5) http://jsbin.com/dopacilo/1/quiet, Euro sign with no defined fallback
Expected: download nothing
OSX: USABLE, downloads latin
Win8.1: USABLE, downloads latin
Ubuntu: USABLE, downloads latin

6) http://jsbin.com/jinaxibopi/quiet, mixed Latin and Cyrillic
Expected: download latin and cyrillic
OSX: PASS
Win8.1: PASS
Ubuntu: FAIL, downloads only latin, renders cyrillic in system font

The font files used by the tests have a query marker (?latin, ?cyrillic, etc) to make it easier to see what was downloaded. Here is a matrix of how some other browsers do on tests 1-5 (6 is new): https://docs.google.com/a/google.com/spreadsheets/d/18h-1gaosu4-KYxH8JUNL6ZDuOsOKmWfauoai3CS3hPY/edit?pli=1#gid=0.

Comment 66

[John Daggett (:jtd)]

(In reply to Rod from comment #65)
> When 36 is released, will unicode-range still be behind a default-off
> preference layout.css.unicode-range.enabled? - we would love to start
> serving unicode-range to Firefox from Google Fonts when it becomes enabled
> by default.

As your own testing indicates, unicode-range is not yet supported on Linux. The decision was made here to leave this feature disabled in release builds until the Linux implementation is complete (see comment 48). This will be resolved by bug 1056479. After that work lands we will enable this feature in release builds.

The backend issue is that the Linux font support code is distinct from other platforms in the way it handles fonts, mainly due to the desire to tightly integrate with fontconfig and have it handle font selection. Among other things, this makes carefully managing font loads difficult.

Comment 67

[Aleksey]

Still not fixed in Firefox 36.0.4 on Linux x86-64.
In this your example: 
https://mdn.mozillademos.org/en-US/docs/Web/CSS/unicode-range$samples/Examples?revision=728071
all text is in new "Ampersand" font, not only ampersand character.

Comment 68

[Jonathan Kew [:jfkthame]]

(In reply to alexlsh from comment #67)
> Still not fixed in Firefox 36.0.4 on Linux x86-64.

This is a known issue, see comments above. It's dependent on bug 1056479 being completed.

Comment 69

[Mark Brown]

This bug is marked as resolved but the "Live result" on this page doesn't show unicode-range working on this page
https://developer.mozilla.org/en-US/docs/Web/CSS/@font-face/unicode-range

Comment 70

[Mark Brown]

Tested in 40.0.3 on Mac OS

Comment 71

[Jonathan Kew [:jfkthame]]

See bug 1119062: it's not yet enabled by default in release builds. You can enable it in about:config (set layout.css.unicode-range.enabled to true, and reload the page) for testing in the meantime.

Comment 72

[chris]

As 1056479, 1119062 and 1304699 are all resolved fixed, can this bug be also closed?