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)
Core
Graphics
Tracking
()
RESOLVED
FIXED
mozilla1.9.1b3
People
(Reporter: dbaron, Assigned: jtd)
Details
(Keywords: fixed1.9.1)
Attachments
(3 files, 4 obsolete files)
2.67 KB,
patch
|
Details | Diff | Splinter Review | |
2.72 KB,
text/html
|
Details | |
11.58 KB,
patch
|
dbaron
:
review+
|
Details | Diff | Splinter Review |
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 | ||
Updated•16 years ago
|
Assignee: nobody → jdaggett
Assignee | ||
Comment 1•16 years ago
|
||
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
Assignee | ||
Updated•16 years ago
|
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?
Reporter | ||
Comment 3•16 years ago
|
||
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.
Assignee | ||
Comment 4•16 years ago
|
||
I think this is a simple fix, not much work at all.
Flags: blocking1.9.1? → blocking1.9.1+
Assignee | ||
Comment 5•16 years ago
|
||
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.
Assignee | ||
Updated•16 years ago
|
Target Milestone: --- → mozilla1.9.1b3
Assignee | ||
Comment 6•16 years ago
|
||
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)
Reporter | ||
Comment 7•16 years ago
|
||
(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...)
Assignee | ||
Comment 8•16 years ago
|
||
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
Assignee | ||
Comment 9•16 years ago
|
||
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.
Reporter | ||
Updated•16 years ago
|
Attachment #354127 -
Flags: superreview?(dbaron)
Attachment #354127 -
Flags: superreview-
Attachment #354127 -
Flags: review?(dbaron)
Attachment #354127 -
Flags: review-
Reporter | ||
Comment 10•16 years ago
|
||
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?
Reporter | ||
Comment 12•16 years ago
|
||
FLAG_FORMAT_EOT would only be present if the author used the optional format() function after the url() in the src: descriptor.
Assignee | ||
Comment 13•16 years ago
|
||
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.
Assignee | ||
Comment 15•16 years ago
|
||
(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•16 years ago
|
||
Probably better to use x-* MIME types if we go that route...
Assignee | ||
Comment 17•16 years ago
|
||
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
Assignee | ||
Comment 18•16 years ago
|
||
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)
Reporter | ||
Comment 19•16 years ago
|
||
Comment on attachment 356124 [details] [diff] [review]
patch, v.0.3a, fixup error in logging code
r=dbaron
Attachment #356124 -
Flags: review?(dbaron) → review+
Assignee | ||
Comment 20•16 years ago
|
||
Checked in on trunk:
http://hg.mozilla.org/mozilla-central/rev/2bed74c80db8
Status: NEW → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 21•16 years ago
|
||
Checked in on 1.9.1 branch:
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/68dccc314605
Keywords: fixed1.9.1
You need to log in
before you can comment on or make changes to this bug.
Description
•