465452 - [@font-face] handling of format() function in @font-face 'src' descriptor is not very future-proof

Status: RESOLVED FIXED

(Core :: Graphics, defect, P3)

Description

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

I wrote some tests for the handling of the format() function in @font-face.  The first three are already in mozilla-central, and then the three that I wrote once I understood the behavior of the first three are in http://hg.mozilla.org/users/dbaron_mozilla.com/patches/file/a892feeea372/src-format-more-tests

I think there are basically two problems, with the second potentially becoming much more of an issue once the first is fixed.

First, GFX doesn't have a flag for unknown font format strings, even though the list may well be extended in the future.  This means we'll skip trying to load a font that's format("embedded-opentype") or format("svg"), but we'll still try to load a font if it's marked as format("unknown").  Fixing this would require adding a new gfxUserFontSet::FLAG_FORMAT_UNKNOWN or some such, and then making InsertFontFaceRule set that flag for format values that are unknown.

Second, the current handling doesn't seem to be in the spirit of this example:
# A font containing tables to support multiple advanced font formats:
#    src: url(DoulosSILR.ttf) format("opentype","truetype-aat");
from http://dev.w3.org/csswg/css3-fonts/#src .  It skips trying to download a font if it has any format that isn't supported.  (As a side note, the spec should probably define expected behavior here.)  In other words, our current implementation in gfxWindowsPlatform::IsFontFormatSupported would not try to download the font in the above example, even though it should.

Steps to reproduce:
   1. copy layout/reftests/fonts/ and layout/reftests/font-face/ to a Web
server (right now, I have them on http://dbaron.org/tmp/reftest/font-face/ if
you want to use that copy).  (This is required because of security restrictions
on cross-directory loads for file: URLs.  The reftest harness runs them as HTTP
reftests.)
   2. check whether the src-list-format-* tests match their references (see reftest.list for the correspondence; 4-6 use the same references as 1-3)

Fixing the first issue would be expected to fix src-list-format-1.html and src-list-format-2.html .

Fixing the second issue would be expected to fix src-list-format-6.html .

But fixing the first issue without fixing the second issue would be expected to break src-list-format-3.html .

Comment 1

[John Daggett (:jtd)]

In the format unknown case, having an "unknown format" makes sense as long as it's clear that implies we explicitly ignore it when loading.  Also, I think it makes sense to ignore an @font-face rule that only contains unknown formats in the src list.

In the second case, if one of the formats is acceptable the font should load.

Comment 2

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

john/dbaron: what's the impact of this if we don't fix it for 1.9.1?  And how difficult would the fix be?

Comment 3

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

The second problem above is more serious, since it could prevent Web authors from doing what's described in that spec example in the future because it prevents us from downloading the font.  (That said, I'm not sure how important it is that they be able to do that.)

I think the fix should be reasonably straightforward (a few hours work, plus compiling and testing across platforms?).

But John might have better answers.

Comment 4

[John Daggett (:jtd)]

I think this is a simple fix, not much work at all.

Comment 5

[John Daggett (:jtd)]

Attachment: patch, v.0.1, add unknown hint flag and accept known types before rejecting

First pass at a fix, needs more testing.

In the case where no hint is specified, I've added an explicit check for files ending in .eot so that we skip @font-face rules made for IE.

Comment 6

[John Daggett (:jtd)]

Attachment: patch, v.0.2, add test for rejecting .eot files

Add in an extra reftest for rejecting .eot files.  The markA.eot file is actually not a valid EOT file, it's just a copy of markA.ttf.  So it will load without a problem if files with .eot extension are not explicitly rejected.  Safari uses the same handling.

Comment 7

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

(In reply to comment #5)
> In the case where no hint is specified, I've added an explicit check for files
> ending in .eot so that we skip @font-face rules made for IE.

I'm not sure this is a good idea.  If authors want to, they could browser-sniff what they serve for a .eot file in order to add support for additional browsers; I'm not sure how many would do this, but I think that kind of thing works for a lot of other types because the Web is based on MIME types rather than extensions.

(That said, I wonder what HTTP "Accept" header we're sending when we request downloadable fonts...)

Comment 8

[John Daggett (:jtd)]

Actually, my main concern is how to work around the limitations of the existing IE EOT implementation, so that authors can set up pages that serve either .ttf for modern browsers or .eot for IE.  Checking for the .eot filetype would eliminate a lot of unneeded downloads for pages with rules for all browsers.  I'm sure user agent sniffing is possible but that seems like an added burden for authors.  I agree that ideally we wouldn't have to do this sort of thing.

I've checked in a valid EOT font for testing use with IE, reftests/fonts/markB.eot.  This is also available here:

http://people.mozilla.org/~jdaggett/fonts/markB.eot

Comment 9

[John Daggett (:jtd)]

Attachment: hacky patch for saving eot form of downloaded font

Quick and dirty patch for saving the EOT form of a downloaded .ttf font.  Saves it to the Windows temp directory in the form mozfontXXX.eot.  Null root string is set so the font is not domain restricted.

Comment 10

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

Comment on attachment 354127 [details] [diff] [review]
patch, v.0.2, add test for rejecting .eot files

>+    // reject unsupported formats
>     // Pango doesn't apply features from AAT TrueType extensions.
>     // Assume that if this is the only SFNT format specified,
>     // then AAT extensions are required for complex script support.
>-    if ((aFormatFlags & gfxUserFontSet::FLAG_FORMAT_TRUETYPE_AAT) 
>-         && !(aFormatFlags & (gfxUserFontSet::FLAG_FORMAT_OPENTYPE | gfxUserFontSet::FLAG_FORMAT_TRUETYPE))) {
>+    if (aFormatFlags & (gfxUserFontSet::FLAG_FORMAT_EOT | 
>+                        gfxUserFontSet::FLAG_FORMAT_SVG |
>+                        gfxUserFontSet::FLAG_FORMAT_TRUETYPE_AAT)) {
>+        return PR_FALSE;
>+    }
>+
>+    // at this point, the only bit that could be set is the unknown flag
>+    NS_ASSERTION(!(aFormatFlags & (~gfxUserFontSet::FLAG_FORMAT_UNKNOWN)),
>+                 "strange font format hint set");
>+                 
>+    // format hint set but unknown
>+    if (aFormatFlags & (gfxUserFontSet::FLAG_FORMAT_UNKNOWN)) {
>+        return PR_FALSE;
>+    }

It seems like this whole chunk could be reduced to:
 (1) the assertion that aFormatFlags doesn't have any unexpected bits (which could also come at the very beginning of the function)

 (2) if (aFormatFlags != 0) { return PR_FALSE; }

(This occurs three times.)

>+    // explicitly exclude .eot formats
>+    nsresult rv;
>+    nsCAutoString path;
>+
>+    rv = aFontURI->GetPath(path);
>+    if (NS_SUCCEEDED(rv) && StringEndsWith(path, NS_LITERAL_CSTRING("eot"))) {
>         return PR_FALSE;
>     }

I would prefer not doing this; Web authors should be able to use whatever file names they want.  The Web operates on MIME types rather than on extensions.  Removing this also implies removing the new test.

(If roc or vlad disagree with me here, I might be willing to reconsider...)


Other than that, this looks fine.

Comment 11

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

(In reply to comment #10)
> >+    // explicitly exclude .eot formats
> >+    nsresult rv;
> >+    nsCAutoString path;
> >+
> >+    rv = aFontURI->GetPath(path);
> >+    if (NS_SUCCEEDED(rv) && StringEndsWith(path, NS_LITERAL_CSTRING("eot"))) {
> >         return PR_FALSE;
> >     }
> 
> I would prefer not doing this; Web authors should be able to use whatever file
> names they want.  The Web operates on MIME types rather than on extensions. 
> Removing this also implies removing the new test.
> 
> (If roc or vlad disagree with me here, I might be willing to reconsider...)

I agree with this, fwiw -- do we need a different way of excluding, then?  But I don't fully understand the code -- when would we hit this case when FLAG_FORMAT_EOT wasn't set?

Comment 12

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

FLAG_FORMAT_EOT would only be present if the author used the optional format() function after the url() in the src: descriptor.

Comment 13

[John Daggett (:jtd)]

Attachment: eot load tests

Tests to see what types of @font-face rules could be used to support both raw TTF and EOT in the same stylesheet.

For both of the rules below, IE throws out the @font-face rule and default rendering occurs:

@font-face {
  font-family: test;
  src: url(test.ttf) format("truetype"), url(test.eot);
}

@font-face {
  font-family: test;
  src: url(test.eot);
  src: url(test.ttf) format("truetype"), url(test.eot);
}

IE will load test.eot when specified with two separate rules (it throws out the second one):

@font-face {
  font-family: test;
  src: url(test.eot);
}

@font-face {
  font-family: test;
  src: url(test.ttf) format("truetype");
}

Another alternative for authors is simple to define two separate families, 'testEOT' and 'testTTF' for example:

@font-face {
  font-family: testEOT;
  src: url(test.eot);
}

@font-face {
  font-family: testTTF;
  src: url(test.ttf) format("truetype");
}

But this is not as desirable, since authors now need to specify two family names rather than one:

body { font-family: testTTF, testEOT; }

IE also doesn't do glyph fallback with downloaded fonts, so missing codepoints render as small boxes.

All four cases in this example render correctly in FF 3.1 latest, Safari 3.2.1, and Opera 10 preview.

Comment 14

[John Daggett (:jtd)]

Attachment: eot load tests

Fix up URLs

Comment 15

[John Daggett (:jtd)]

(In reply to comment #10)

> I would prefer not doing this; Web authors should be able to use whatever file
> names they want.  The Web operates on MIME types rather than on extensions. 
> Removing this also implies removing the new test.
> 
> (If roc or vlad disagree with me here, I might be willing to reconsider...)

I'm definitely not happy about doing this but without this we will end up pulling down the eot file for sites that try to make pages that work both in IE and in FF/Safari/Opera.  And there are currently no MIME types defined for fonts.

Are there other options here?  Should we just use arbitrary MIME types (e.g font/eot, font/ttf, font/otf) to include/exclude files?

Comment 16

[Boris Zbarsky [:bzbarsky]]

Probably better to use x-* MIME types if we go that route...

Comment 17

[John Daggett (:jtd)]

Attachment: patch, v.0.3, review changes and remove .eot extension sniffing

I've moved the whole issue of .eot files into a separate bug, bug 472647.  The logic with this patch is essentially what dbaron proposed in comment 10.  Also added a reftest to test "truetype-aat" format hint.

Comment 18

[John Daggett (:jtd)]

Attachment: patch, v.0.3a, fixup error in logging code

Fixed a problem when userfont logging enabled, otherwise same as v.0.3.  Tested on all platforms.

Comment 19

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

Comment on attachment 356124 [details] [diff] [review]
patch, v.0.3a, fixup error in logging code

r=dbaron

Comment 20

[John Daggett (:jtd)]

Checked in on trunk:
http://hg.mozilla.org/mozilla-central/rev/2bed74c80db8

Comment 21

[John Daggett (:jtd)]

Checked in on 1.9.1 branch:
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/68dccc314605