Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Improve font watching #66

Merged
merged 71 commits into from Jan 30, 2013

Conversation

Projects
None yet
3 participants
Owner

bramstein commented Nov 13, 2012

This pull requests aims to improve the accuracy of the active and inactive events on Webkit versions that are affected by the following bug:

https://bugs.webkit.org/show_bug.cgi?id=76684

The following changes are made:

  • Font metrics are now compared on both width and height. This is to reduce the chance of almost metrics compatible fonts being considered identical and thus failing detection.
  • The Webkit bug is detected using a null-webfont, and if it is present the detection code ignores the first change in font metrics and only fires the active event when another metrics change is detected.
  • If a timeout occurs and the Webkit bug is present we now assume that the font loaded but has identical metrics to the browser's last resort font. This might result in false active events in the rare case where the last resort font is metrics compatible with the webfont being loaded.
  • Test cases are added to emulate the broken Webkit behaviour.

All tests are passing and the code compiles without errors or warnings.

bramstein added some commits Nov 7, 2012

@bramstein bramstein Removed the test for the webkit font fallback bug in favour of a bett…
…er approximation of the actual behaviour.
d059cd4
@bramstein bramstein Added an accurate model of the broken way webkit used to handle fallb…
…ack fonts while loading webfonts. Also added two new tests that test for the case where a font is parsed and applied quickly and slowly (where the slow one currently fails.)
c282155
@bramstein bramstein Changed FontWatchRunner to take an extra argument indicating the pres…
…ence of the Webkit fallback bug. Changed the check_ method to do the correct thing if the bug is present. Updated the tests accordingly.
6e8bbc3
@bramstein bramstein Fixed formatting. 9fca6e7
@bramstein bramstein Added test for when the fallback font and the loaded font have the sa…
…me metrics.
b337386
@bramstein bramstein Fixed type annotations on core/fontwatcher. 9b294c3
@bramstein bramstein Changed the fontwatchrunner to test both width and height of the test…
… elements.
bb209f9
@bramstein bramstein Patched up the Google lastresortwebkitfontwatchrunner to match the ch…
…anges to core. This patch does not change its functionality.
2e27db4
@bramstein bramstein Added two tests for checking both the width and height when monitorin…
…g metric changes.
58e03c9
@bramstein bramstein Changed the behaviour of the timeout when the Webkit bug is present. …
…If the timeout happens, and we are running in a webkit browser with the bug and we observed at least one size change, we assume the font has loaded and has identical metrics to the last resort font. In this case we fire the active event instead of the inactive event.
2dcd257
@bramstein bramstein Added a test for detecting the Webkit fallback bug where it does not …
…respect the font stack while loading webfonts.
3351efc
Owner

bramstein commented Nov 13, 2012

I forgot to mention that this pull request also reverts the previous attempts in core at working around the Webkit bug.

@bramstein bramstein referenced this pull request Nov 13, 2012

Closed

Active event and Webkit #54

@seanami seanami commented on an outdated diff Nov 13, 2012

src/core/fontwatchrunner.js
- if ((this.originalSizeA_ != sizeA || this.originalSizeB_ != sizeB) &&
- this.lastObservedSizeA_ == sizeA && this.lastObservedSizeB_ == sizeB) {
- this.finish_(this.activeCallback_);
+ if (this.lastObservedSizeA_.width != sizeA.width || this.lastObservedSizeB_.width != sizeB.width ||
+ this.lastObservedSizeA_.height != sizeB.height || this.lastObservedSizeB_.height != sizeB.height) {
+ if ((this.hasWebkitFallbackBug_ && this.sizeChangeCount_ === 1) ||
+ (!this.hasWebkitFallbackBug_ && this.sizeChangeCount_ === 0)) {
+ this.finish_(this.activeCallback_);
+ } else {
+ this.lastObservedSizeA_ = sizeA;
+ this.lastObservedSizeB_ = sizeB;
+ this.sizeChangeCount_ += 1;
+ this.asyncCheck_();
+ }
@seanami

seanami Nov 13, 2012

Contributor

There's a problem with this approach where the font attempts to load in a browser with the WebKit fallback bug and then fails to render. The width will change from the initial fallback size to the size of the last resort fallback, and then back to the size of the initial fallback. This is two changes in size, but it doesn't mean that the font has successfully rendered. (In fact, it's not rendering.)

Instead of just waiting for two changes in size, we actually need to remember what the initial fallback size was. If the WebKit fallback bug is present, we also need to remember the last resort fallback size. Then, as we're watching the font, we need to ignore both of these sizes. Only a change to a third, new size would be considered active.

In the timeout case, we should only return active if the last observed size matches the last resort fallback size, and not in any other case.

@seanami seanami commented on an outdated diff Nov 13, 2012

src/core/fontwatcher.js
@@ -25,6 +26,47 @@ webfont.FontWatcher = function(domHelper, eventDispatcher, fontSizer,
webfont.FontWatcher.DEFAULT_VARIATION = 'n4';
/**
+ * @param {string} fontFamily
+ * @return {string}
+ * @private
+ */
+webfont.FontWatcher.prototype.createTestStyle_ = function(fontFamily) {
+ return "position:absolute;top:-999px;left:-999px;" +
+ "font-size:300px;width:auto;height:auto;" +
+ "line-height:normal;margin:0;padding:0;" +
+ "font-variant:normal;font-family:" + fontFamily + ";";
+};
@seanami

seanami Nov 13, 2012

Contributor

Can we combine this with webfont.FontWatchRunner.prototype.computeStyleString_ somehow so that we don't have so much duplicated style information? They're both doing essentially the same thing, I think.

@seanami seanami commented on an outdated diff Nov 13, 2012

src/core/fontwatcher.js
+ "font-size:300px;width:auto;height:auto;" +
+ "line-height:normal;margin:0;padding:0;" +
+ "font-variant:normal;font-family:" + fontFamily + ";";
+};
+
+/**
+ * Returns true if this browser has a bug that causes the font stack
+ * to not be respected while loading webfonts.
+ *
+ * @return {boolean}
+ * @private
+ */
+webfont.FontWatcher.prototype.hasFallbackBug_ = function() {
+ var font = this.domHelper_.createElement('style', null,
+ "@font-face{" +
+ "font-family:'__test__';" +
@seanami

seanami Nov 13, 2012

Contributor

I think we should use a font-family name that's more specific to the library, so it's less likely to collide with other things on the page. How about __webfontloader_test__ instead?

Contributor

seanami commented Nov 13, 2012

What do you think about adding code so that the bug detection code only runs in browsers that identify themselves as having a WebKit engine? We could pass a UserAgent instance in to FontWatcher and use the engine to determine whether to run the test or not. This would limit the scope of the changes for non-WebKit browsers and avoid potential undiscovered problems that might be caused by trying to load a null font in other browsers.

@seanami seanami commented on an outdated diff Nov 14, 2012

src/core/fontwatchrunner.js
this.originalSizeA_ = this.getDefaultFontSize_(
webfont.FontWatchRunner.DEFAULT_FONTS_A);
this.originalSizeB_ = this.getDefaultFontSize_(
webfont.FontWatchRunner.DEFAULT_FONTS_B);
- this.lastObservedSizeA_ = this.originalSizeA_;
- this.lastObservedSizeB_ = this.originalSizeB_;
- this.requestedFontA_ = this.createHiddenElementWithFont_(
- webfont.FontWatchRunner.DEFAULT_FONTS_A);
- this.requestedFontB_ = this.createHiddenElementWithFont_(
- webfont.FontWatchRunner.DEFAULT_FONTS_B);
+ this.lastObservedSizeA_ = null;
+ this.lastObservedSizeB_ = null;
+ this.requestedFontA_ = new webfont.FontRuler(this.domHelper_, this.fontSizer_,
+ this.fontFamily_ + ',' + webfont.FontWatchRunner.DEFAULT_FONTS_A,
+ this.fontDescription_, this.fontTestString_);
+ this.requestedFontB_ = new webfont.FontRuler(this.domHelper_, this.fontSizer_,
+ this.fontFamily_ + ',' + webfont.FontWatchRunner.DEFAULT_FONTS_B,
+ this.fontDescription_, this.fontTestString_);
@seanami

seanami Nov 14, 2012

Contributor

These FontRuler instances (which I love) are now actually reusable, right? I think we could name these properties something better, like fontRulerA_ and fontRulerB_ perhaps? Also, you could instantiate the rulers with just the default fonts, record the values for originalSizeA_ and originalSizeB_, and then set the font stack to include the main font family. This would reduce the number of FontRuler instances you instantiate and the setup/teardown of DOM elements. What do you think?

@seanami seanami commented on an outdated diff Nov 14, 2012

src/core/fontwatchrunner.js
this.originalSizeA_ = this.getDefaultFontSize_(
webfont.FontWatchRunner.DEFAULT_FONTS_A);
this.originalSizeB_ = this.getDefaultFontSize_(
webfont.FontWatchRunner.DEFAULT_FONTS_B);
- this.lastObservedSizeA_ = this.originalSizeA_;
- this.lastObservedSizeB_ = this.originalSizeB_;
- this.requestedFontA_ = this.createHiddenElementWithFont_(
- webfont.FontWatchRunner.DEFAULT_FONTS_A);
- this.requestedFontB_ = this.createHiddenElementWithFont_(
- webfont.FontWatchRunner.DEFAULT_FONTS_B);
+ this.lastObservedSizeA_ = null;
+ this.lastObservedSizeB_ = null;
@seanami

seanami Nov 14, 2012

Contributor

I think we should name these two properties something better. They're no longer set to the last observed size after each loop iteration. They're only used in the case that the webkit bug is present. Maybe we should call them webkitFallbackSizeA_ and webkitFallbackSizeB_ instead?

@seanami seanami commented on an outdated diff Nov 14, 2012

src/core/fontwatcher.js
@@ -14,6 +15,7 @@ webfont.FontWatcher = function(domHelper, eventDispatcher, fontSizer,
this.asyncCall_ = asyncCall;
this.getTime_ = getTime;
this.currentlyWatched_ = 0;
+ this.hasBug_ = userAgent.getEngine() === 'AppleWebKit' ? this.hasFallbackBug_() : false;
@seanami

seanami Nov 14, 2012

Contributor

I feel like the name hasBug_ is a little too generic. There are many bugs that browsers might have relating to web fonts. Can we call this property hasWebkitFallbackBug_ and the associated method checkWebkitFallbackBug so that we keep the way we refer to the bug in code consistent? (All of these long names get renamed by the compiler anyway.)

@seanami seanami commented on an outdated diff Nov 14, 2012

src/core/fontwatcher.js
+ * @return {boolean}
+ * @private
+ */
+webfont.FontWatcher.prototype.hasFallbackBug_ = function() {
+ var font = this.domHelper_.createElement('style', null,
+ "@font-face{" +
+ "font-family:'__webfontloader_test__';" +
+ "src:url(data:application/x-font-woff;base64,) format('woff')," +
+ "url(data:font/truetype;base64,) format('truetype');" +
+ "}"),
+ ruler = new webfont.FontRuler(this.domHelper_, this.fontSizer_, 'monospace', '', 'iii');
+
+ this.domHelper_.insertInto('head', font);
+ var beforeWidth = ruler.getSize().width;
+ ruler.setFont("'__webfontloader_test__', monospace, sans-serif", '');
+ var hasBug = beforeWidth !== ruler.getSize().width;
@seanami

seanami Nov 14, 2012

Contributor

I realized that this test now depends on the fact that monospace will have a different width than the last resort fallback font for the string "iii". Based on the data from your test page, are we pretty confident that that will be the case on all platforms?

I do like your choice of test string though, since "iii" is likely to be very narrow in non-monospace fonts and much wider in a monospace font. Clever.

Can we maybe add a note in the comment about the assumptions that make this test work and why the test string was chosen, just so that our assumptions are documented?

@seanami seanami commented on an outdated diff Nov 14, 2012

src-test/core/fontwatchertest.js
@@ -35,9 +35,21 @@ FontWatcherTest.prototype.setUp = function() {
}
};
+ this.fakeDomHelper_ = {
+ createElement: function(name, attrs, innerHtml) {
+ var element = document.createElement(name);
+ return element;
+ },
+ insertInto: function() {},
+ removeElement: function() {},
+ setStyle: function() {}
+ };
+
+ this.fakeUserAgent_ = new webfont.UserAgent('Firefox', '3.6', 'Gecko', '1.9.2', 'Macintosh', '10.6', undefined, true);
@seanami

seanami Nov 14, 2012

Contributor

We should probably just call this this.userAgent_ since it's actually a real instance of webfont.UserAgent and not just a stub object.

@seanami

seanami Nov 14, 2012

Contributor

Also, I think we should add a few more tests to this file:

  1. A test that verifies that the code to test for the webkit bug only runs on user agents that claim to have a WebKit engine.
  2. A test that exercises the code which tests for the webkit bug (probably with a fake DOM helper) to test that it performs the expected actions.
Contributor

seanami commented Nov 14, 2012

This is looking great! Just a few more comments on naming, testing, and documentation mostly. I love how you've extracted the font measurement tool into a reusable FontRuler class. Nicely done.

Contributor

rcarver commented Nov 14, 2012

Such good stuff. 👍 @bramstein !

@seanami seanami and 1 other commented on an outdated diff Nov 16, 2012

src/core/fontwatcher.js
+ "@font-face{" +
+ "font-family:'__webfontloader_test__';" +
+ "src:url(data:application/x-font-woff;base64,) format('woff')," +
+ "url(data:font/truetype;base64,) format('truetype');" +
+ "}"),
+ ruler = new webfont.FontRuler(this.domHelper_, this.fontSizer_, 'iii');
+
+ ruler.insert();
+
+ // First we set the font to monospace and the test string to `iii`. Based
+ // on our research, all platforms have at least a monospace, sans-serif,
+ // and serif font installed. By using a test string that has a very
+ // narrow width in non-monospace fonts it becomes easy to detect changes
+ // in width.
+ ruler.setFont('monospace');
+ this.domHelper_.insertInto('head', font);
@seanami

seanami Nov 16, 2012

Contributor

I feel like we shouldn't insert this until right before we're going to make the relevant measure (after we set the font for the second time). I'm not sure how all of the timing works out, but it seems like if the browser paused JS execution at the first width measurement, it could already be done trying to load the null font before the second measurement is made, leading to a false negative for the bug detection.

@bramstein

bramstein Nov 16, 2012

Owner

Good catch, fixed in c4924b9

Contributor

seanami commented Nov 16, 2012

This is looking great! Nice work Bram.

@seanami seanami commented on an outdated diff Dec 4, 2012

src/core/fontwatcher.js
+ * @return {boolean}
+ * @private
+ */
+webfont.FontWatcher.prototype.checkWebkitFallbackBug_ = function() {
+ // We build an empty webfont and try to set it as the font for our
+ // ruler. Even though this will fail (since our webfont is invalid)
+ // it will actually trigger the Webkit fallback bug.
+ var font = this.domHelper_.createElement('style', null,
+ "@font-face{" +
+ "font-family:'__webfontloader_test__';" +
+ "src:url(data:application/x-font-woff;base64,) format('woff')," +
+ "url(data:font/truetype;base64,) format('truetype');" +
+ "}"),
+ ruler = new webfont.FontRuler(this.domHelper_, this.fontSizer_, 'iii');
+
+ this.domHelper_.insertInto('head', font);
@seanami

seanami Dec 4, 2012

Contributor

I think I gave this comment once before, but the code has changed a few times since then. Don't we want to wait to insert the font-face rule until right before we make the second measurement, so that we're sure (as we can be) that the browser is still treating it as a web font and hasn't "figured out" that it's invalid yet? If we actually want to do that, then I think we should move line 50 here to just after line 75.

Or maybe this isn't important enough to change and then do all the testing over again... What do you think @bramstein?

@seanami

seanami Dec 5, 2012

Contributor

I ended up making this change in 8b4c6c6 so that we could test with it in place.

@seanami seanami commented on an outdated diff Dec 4, 2012

src/core/fontwatchrunner.js
- if ((this.originalSizeA_ != sizeA || this.originalSizeB_ != sizeB) &&
- this.lastObservedSizeA_ == sizeA && this.lastObservedSizeB_ == sizeB) {
- this.finish_(this.activeCallback_);
- } else if (this.getTime_() - this.started_ >= 5000) {
- this.finish_(this.inactiveCallback_);
+ if ((this.sizeEquals_(sizeA, this.fallbackSizeA_) && this.sizeEquals_(sizeB, this.fallbackSizeB_)) ||
+ (this.sizeEquals_(sizeA, this.lastResortSizeA_) && this.sizeEquals_(sizeB, this.lastResortSizeB_))) {
@seanami

seanami Dec 4, 2012

Contributor

@bramstein We should only compare to the last resort size here if we know the WebKit bug is present. Otherwise, we're going to give a false inactive for a font with the same size as the last resort font on platforms/browsers that don't have the bug.

I think this line should be:

(this.hasWebkitFallbackBug_ && this.sizeEquals_(sizeA, this.lastResortSizeA_) && this.sizeEquals_(sizeB, this.lastResortSizeB_))) {

Does that make sense?

@seanami

seanami Dec 5, 2012

Contributor

I ended up making a change to not detect or ignore any last resort fallback sizes unless the webkit bug is present in 4e829cc.

Sean McBride added some commits Dec 5, 2012

Sean McBride Insert the null web font just before the measurement is taken
This helps to ensure that we're measuring the font when the browser still
thinks it's a web font, instead of waiting until the browser knows that the
font isn't a valid font.
8b4c6c6
Sean McBride Don't detect or pay attention to last resort sizes unless webkit bug …
…exists

We don't want to ignore any sizes other than the fallback size unless the
webkit bug is present.
4e829cc

@seanami seanami and 1 other commented on an outdated diff Dec 5, 2012

src/core/fontwatchrunner.js
- this.requestedFontB_ = this.createHiddenElementWithFont_(
- webfont.FontWatchRunner.DEFAULT_FONTS_B);
+ this.hasWebkitFallbackBug_ = hasWebkitFallbackBug;
+
+ this.fontRulerA_ = new webfont.FontRuler(this.domHelper_, this.fontSizer_, this.fontTestString_);
+ this.fontRulerA_.insert();
+ this.fontRulerB_ = new webfont.FontRuler(this.domHelper_, this.fontSizer_, this.fontTestString_);
+ this.fontRulerB_.insert();
+
+ // If the webkit bug is present, get the last resort sizes that we need to
+ // ignore as well.
+ if (this.hasWebkitFallbackBug_) {
+ this.fontRulerA_.setFont(webfont.FontWatchRunner.LAST_RESORT_FONT, this.fontDescription_);
+ this.lastResortSizeA_ = this.fontRulerA_.getSize();
+ this.fontRulerB_.setFont(webfont.FontWatchRunner.LAST_RESORT_FONT, this.fontDescription_);
+ this.lastResortSizeB_ = this.fontRulerB_.getSize();
@seanami

seanami Dec 5, 2012

Contributor

Unfortunately, this isn't a foolproof way to get the correct last resort size for these two font rulers. It breaks down on Android, where the last resort fallback is determined by the last thing in the font stack (falling back to sans-serif if there's nothing recognized in the font stack). In addition, after some more research, I determined that on Android, the last-resort fallback for a given web font name is determined by the last thing in the first font stack where that web font name is used. Thus if you have

STACK_A: '__webfontloader_test__',serif,sans-serif
STACK_B: '__webfontloader_test__',sans-serif,monospace

Then both stacks will measure the width of sans-serif for the buggy period. However, if you use two null fonts and make each name different (like __webfontloader_test_1__ and __webfontloader_test_2__), then they will each measure according to their own last item in the stack. What crazy logic!

Suffice it to say that I think the only totally accurate way to determine the last resort fallback size that we should ignore for each ruler is to actually measure that last resort fallback size using a fresh null font for each pair of rulers. So much about the measurement of the last resort fallback is determined by the context that that seems like the only really safe way to measure it.

I think that means that we can move detection of the webkit bug within the FontWatchRunner, since we basically have to re-detect it in order to accurately measure it.

Now, I think that what we have here actually works today just by coincidence, because Android has sans-serif as the generic last-resort fallback that you get when you use the "" font stack, and we also have sans-serif as the last thing in FALLBACK_FONTS_A, which is the first CSS rule with the webfont name that the browser encounters. So it happens to line up today, but it's not necessarily guaranteed to on another platform.

Are we OK with that happenstance, or do we want to try to make the approach more bulletproof? I'm not sure what to do about that.

@seanami

seanami Dec 5, 2012

Contributor

Alright, I created a new branch off this one which sketches out the code for the approach that I'm talking about. It still needs unit tests fixed and added for the new code (especially in DomHelper), but it should give you an idea of what I'm talking about.

bs-improve-font-watching...bs-improve-font-watching-2

Maybe we can work with that and merge it into this branch if we decide we want to go with that approach?

@bramstein

bramstein Dec 5, 2012

Owner

Good catch! I prefer to not have any coincidences like that so I think we should go with your approach (which you proposed in the first place.)

I was planning to not have Android Chrome be detected as having the bug since it sort of respects the font stack, but the fact that it uses the last font in the first stack that uses the @font-face breaks that.

I'll start adding and fixing unit tests. I'll also investigate why this didn't show up in my webfontloader-proto tests.

Sean McBride added some commits Dec 5, 2012

Sean McBride Fix whitespace (tab to spaces) in DomHelper ff13db6
Sean McBride Initial code to combine bug and last resort size detection (needs tes…
…t fixes)

Because we need to know the last resort sizes to ignore, and the last resort
sizes can depend on the font stack and text content, we need to actually detect
them in the same context to be 100% sure that they'll be the same. This change
creates more null fonts on the page (one for each variation that we're
detecting), but it seems like these don't stay behind in the console, and they
hopefully shouldn't provide too much of a drain performance wise. If they do,
we can do additional work to use a single global null font rule for every
round of watchers (but since they're time-limited before the browser detects
that they don't contain a valid font, we'll need to make a new one for each
round of detection).
74c05dc
Sean McBride Chrome console complains about font/truetype MIME type
Use font/opentype instead, which doesn't cause complaints.
7bfb60d
Owner

bramstein commented on 8b4c6c6 Dec 5, 2012

Not that it really matters, but don't all WebKit browsers wait until downloading the font until the font is used? Still, better safe than sorry I guess :)

Contributor

seanami replied Dec 5, 2012

Yeah, that's true, but I'm not sure if that means they don't evaluate the data uri until that moment. I agree with the "better safe than sorry" sentiment. :)

Owner

bramstein commented on 4e829cc Dec 5, 2012

That was actually part of my plan to get rid of the null-font detection code. No other browser (based on my browserstack.com tests) falls back on the last resort font. There are two cases that could happen:

  • The last resort font matches one of the fallback stacks. This shouldn't be a problem since we have the two font rulers with serif and sans-serif where at least one is guaranteed to not be the last resort font.
  • The last resort font is metrics compatible with the font being loaded when a timeout happens. This should be fine because we have a this.hasWebkitFallbackBug check in the timeout.

Of course, it does get rid of two calls to getSize when the WebKit bug isn't present.

bramstein and others added some commits Dec 5, 2012

@seanami seanami and 1 other commented on an outdated diff Dec 6, 2012

src/core/fontwatcher.js
@@ -14,6 +15,7 @@ webfont.FontWatcher = function(domHelper, eventDispatcher, fontSizer,
this.asyncCall_ = asyncCall;
this.getTime_ = getTime;
this.currentlyWatched_ = 0;
+ this.checkWebkitFallbackBug_ = userAgent.getEngine() === 'AppleWebKit';
@seanami

seanami Dec 6, 2012

Contributor

What do you think about adding a version constraint to this boolean expression, so that we don't check for the bug on versions of WebKit that are new enough that we know they won't have the bug? (We'd probably use getEngineVersion for the purpose.) I remember you saying that the version wasn't always 100% accurate, but is there a version we could pick where we were certain (from testing) that everything newer doesn't have the bug?

It'd be nice if the more complex detection code didn't have to run for any latest-version browsers that are out there, because it would further limit the scope of the changes...

Just curious what you think. I'd be fine to leave it like this for now as well.

@bramstein

bramstein Dec 6, 2012

Owner

We can say for sure that engine versions after and including 536.11 do not have the bug. Earlier versions may or may not have the bug, which is caused by Chrome for iOS reporting an incorrect Webkit engine version. So yea, we can limit the check to versions below 536.11. I don't see any harm in running it on all Webkit versions (or even all browsers), but I don't have any strong arguments for or against (other than that I didn't notice any problems with running the check.)

@seanami

seanami Dec 6, 2012

Contributor

I like the idea of minimizing the impact of functional changes and extra stuff that's required in newer browser, so I think I'd like to add this. This has been added in e2dde82.

For future reorganization, it might be better if this sort of thing was detected during the parsing of a user agent within UserAgentParsing and then set in some sort of UserAgentOptions object that hangs off the user agent and can accept arbitrary aspects of the browser. On the other hand, maybe we won't have enough of these to make that worth it.

Sean McBride and others added some commits Dec 6, 2012

Sean McBride Only detect webkit fallback bug for older versions that may have it e2dde82
@bramstein bramstein First pass at moving browser information into a separate object. f822fd7
@bramstein bramstein Merge branch 'master' into bs-improve-font-watching bb49f9a
@bramstein bramstein Merge branch 'master' into bs-extend-user-agent-browser-info 1e66382
@bramstein bramstein Changed BrowserInfo enum to a real class. cd93994
@bramstein bramstein UserAgent parameters are not optional. a0063b5
@bramstein bramstein Removed UserAgent.isSupportingWebFont method. 8770cb9
@bramstein bramstein Merge branch 'bs-extend-user-agent-browser-info' into bs-improve-font…
…-watching
dc89a06
@bramstein bramstein Cleaned up version number parsing. d86e4f8
@bramstein bramstein Hardcoded detection of the webkit fallback bug and the android font s…
…tack bug.
ecd7c41
@bramstein bramstein Added tests for browserinfo bugs and did some minor renaming. 2bd7b2a
@bramstein bramstein Rewrote font watching strategy and updated tests. 311945b
@bramstein bramstein Removed NullFont methods. cc988ec
@bramstein bramstein Removed Android web font loading bug detection code. edc12d1
@bramstein bramstein Rewrote detection code to use generic font family sizes to detect the…
… last resort font.
98a84b0
@bramstein bramstein Fixed incorrect indexing of the generic font family lookup. 624cef8
@bramstein bramstein Added Apple Color Emoji to the generic (last resort) font families. A…
…ccording to http://www.opensource.apple.com/source/WebCore/WebCore-955.66/platform/graphics/mac/FontCacheMac.mm?txt, Apple Color Emoji is used as a fall back font when it is available. Since it is available on all iOS versions we'll always get it before any other fallback font. Since it doesn't match any of the other generic fonts either we'll have to add it to our list.
40c7ead
@bramstein bramstein Fixed FontWatchRunner tests. 05a4eb6
@bramstein bramstein Fixed lastresortwebkitfontwatchrunnertest.js. 2acec0b
@bramstein bramstein Renamed genericFontFamilySizes to lastResortSizes. 8a52b5a
@bramstein bramstein Removed old nullFontStyle shim. 42dc224
@bramstein bramstein Added clarifying comment about Apple Color Emoji. ddcef48
Owner

bramstein commented Jan 9, 2013

The pull request has been updated:

  • Font metrics are now compared on both width and height. This is to reduce the chance of almost metrics compatible fonts being considered identical and thus failing detection.
  • The WebKit bug is detected based on the WebKit version number. We found out this is the only reliable way of detecting the bug. The previously mentioned null-font strategy works in ideal circumstances but can be circumvented by the browser's resource queue (if downloading of the null-font is queued it will use the normal font stack fallback font, thus breaking detection.) The version number has been verified against the WebKit repository and revision number.
  • The detection of the last resort font is now based on a list of known last resort sizes that is only used when the bug is present.
  • Version number parsing has been cleaned up and an additional object to keep browser details in has been introduced. At the moment this only contains whether or not the browser supports web fonts, and if it has the bug. The idea is to put any information in this object that is not directly in the useragent string but derived from it.
  • If a timeout occurs and the Webkit bug is present we now assume that the font loaded but has identical metrics to the browser's last resort font. This might result in false active events in the rare case where the last resort font is metrics compatible with the webfont being loaded.
  • Test cases are added to emulate the broken Webkit behaviour.

All tests are passing and the code compiles without errors or warnings.

@seanami seanami commented on the diff Jan 11, 2013

src/core/initialize.js
@@ -23,4 +23,5 @@ webfont.UserAgent.prototype['getEngineVersion'] = webfont.UserAgent.prototype.ge
webfont.UserAgent.prototype['getPlatform'] = webfont.UserAgent.prototype.getPlatform;
webfont.UserAgent.prototype['getPlatformVersion'] = webfont.UserAgent.prototype.getPlatformVersion;
webfont.UserAgent.prototype['getDocumentMode'] = webfont.UserAgent.prototype.getDocumentMode;
-webfont.UserAgent.prototype['isSupportingWebFont'] = webfont.UserAgent.prototype.isSupportingWebFont;
+webfont.UserAgent.prototype['getBrowserInfo'] = webfont.UserAgent.prototype.getBrowserInfo;
+webfont.BrowserInfo.prototype['hasWebFontSupport'] = webfont.BrowserInfo.prototype.hasWebFontSupport;
@seanami

seanami Jan 11, 2013

Contributor

Do we need to export hasWebKitFallbackBug as well? If not, why not? (This exports stuff gets really confusing.)

@bramstein

bramstein Jan 14, 2013

Owner

Can't see anyone using it, but you are right that consistency is nice here. I've exported it as well.

Contributor

seanami commented Jan 11, 2013

Nice! This looks great.

Contributor

seanami commented Jan 24, 2013

This is looking great! I'm thinking we should get this deployed next week, barring further discussion with the Google guys.

@bramstein bramstein added a commit that referenced this pull request Jan 30, 2013

@bramstein bramstein Merge pull request #66 from typekit/bs-improve-font-watching
Improve font watching
647d9ca

@bramstein bramstein merged commit 647d9ca into master Jan 30, 2013

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment