Closed Bug 465452 Opened 16 years ago Closed 16 years ago

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

Categories

(Core :: Graphics, defect, P3)

defect

Tracking

()

RESOLVED FIXED
mozilla1.9.1b3

People

(Reporter: dbaron, Assigned: jtd)

Details

(Keywords: fixed1.9.1)

Attachments

(3 files, 4 obsolete files)

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 .
Flags: blocking1.9.1?
Assignee: nobody → jdaggett
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.
Priority: -- → P3
Summary: handling of format() function in @font-face 'src' descriptor is not very future-proof → [@font-face] handling of format() function in @font-face 'src' descriptor is not very future-proof
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?
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.
I think this is a simple fix, not much work at all.
Flags: blocking1.9.1? → blocking1.9.1+
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.
Target Milestone: --- → mozilla1.9.1b3
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.
Attachment #354121 - Attachment is obsolete: true
Attachment #354127 - Flags: superreview?(dbaron)
Attachment #354127 - Flags: review?(dbaron)
(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...)
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
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.
Attachment #354127 - Flags: superreview?(dbaron)
Attachment #354127 - Flags: superreview-
Attachment #354127 - Flags: review?(dbaron)
Attachment #354127 - Flags: review-
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.
(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?
FLAG_FORMAT_EOT would only be present if the author used the optional format() function after the url() in the src: descriptor.
Attached file eot load tests (obsolete) —
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.
Attached file eot load tests
Fix up URLs
Attachment #355686 - Attachment is obsolete: true
(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?
Probably better to use x-* MIME types if we go that route...
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.
Attachment #354127 - Attachment is obsolete: true
Fixed a problem when userfont logging enabled, otherwise same as v.0.3. Tested on all platforms.
Attachment #355938 - Attachment is obsolete: true
Attachment #356124 - Flags: review?(dbaron)
Comment on attachment 356124 [details] [diff] [review] patch, v.0.3a, fixup error in logging code r=dbaron
Attachment #356124 - Flags: review?(dbaron) → review+
Status: NEW → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: