New issue

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

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adds UA blacklist to @font-face test as a stopgap for false positives #1147

Merged
merged 1 commit into from Dec 6, 2013

Conversation

Projects
None yet
9 participants
@Wilto
Contributor

Wilto commented Dec 6, 2013

… at least until we can come up with a mo’ better feature test. Ref. issue #1120.

@davatron5000

This comment has been minimized.

davatron5000 commented Dec 6, 2013

+1 on this. I know Modernizr tries to avoid UA sniffs, but this should be a pretty select group of known false positives that most @font-face detection people aren't making accommodations for.

@paulirish

This comment has been minimized.

Member

paulirish commented Dec 6, 2013

@davatron5000 it's all good. We value accuracy more than theoretical purity. We have a few other UA blacklists in the suite. ;)

@patrickkettner

This comment has been minimized.

Member

patrickkettner commented Dec 6, 2013

only one code nit, then im good to merge.

@davatron5000 we do it from time to time, as a last resort.

see fileinput, history, and data-uri's

@paulirish

This comment has been minimized.

Member

paulirish commented Dec 6, 2013

Actually one larger thing.

Let's skip the testStyles() call completely if we hit the blacklist. Don't need to do all the work on these shitty device if we know we're going to ignore their answer anyway.

@patrickkettner

This comment has been minimized.

Member

patrickkettner commented Dec 6, 2013

ah, great call @paulirish

@Wilto

This comment has been minimized.

Contributor

Wilto commented Dec 6, 2013

Cheah cheah; on it.

@stucox

This comment has been minimized.

Member

stucox commented Dec 6, 2013

… at least until we can come up with a mo’ better feature test. Ref. issue #1120.

e.g. inject a custom font with some defining feature we can use to distinguish it from a native font, like a excessively wide character we can measure? Although that would have to be asynchronous 😦

@patrickkettner

This comment has been minimized.

Member

patrickkettner commented Dec 6, 2013

@stucox that was my thought.
Might be able to do a font-face that just renames a local monospace one, and compare widths. That wouldn't catch the IE bug, though. I wonder what the computed property of IE's font-face'd stuff is

@Wilto

This comment has been minimized.

Contributor

Wilto commented Dec 6, 2013

Refactored based on the above, tested on the following:

Android 1.6: Passes naturally
Android 2.0: Unconfirmed, but should be blacklisted
Android 2.1: Blacklisted
Android 2.2: Passes naturally
WP 7: Blacklisted
WP 7.5: Blacklisted
WP 7.8: Blacklisted
WP 8: Passes naturally
WP 7.5: Blacklisted
WebOS 1.4: Blacklisted
WebOS 2.0: Blacklisted
WebOS 2.0.1: Blacklisted

All the grown-up browsers pass as expected—current Safari, Chrome, Firefox, iOS, Android native/Chrome. Additionally, here are a few posts that mention @font-face support breaking as of 2.0, but working as expected as of the 2.2 update:
http://stackoverflow.com/questions/3200069/css-fonts-on-android
http://archivist.incutio.com/viewlist/css-discuss/115960

@patrickkettner

This comment has been minimized.

Member

patrickkettner commented Dec 6, 2013

could you rebase to a single commit?

Adds UA blacklist to @font-face test as a stopgap.
This addresses the issue of false positives on WebOS, Android 2.0/2.1,
and Windows Phone 7-7.8 while we consider a more robust feature test.
Ref. #1120.
@Wilto

This comment has been minimized.

Contributor

Wilto commented Dec 6, 2013

Done and done.

@paulirish

This comment has been minimized.

Member

paulirish commented Dec 6, 2013

👍

patrickkettner added a commit that referenced this pull request Dec 6, 2013

Merge pull request #1147 from Wilto/master
Adds UA blacklist to @font-face test as a stopgap for false positives

@patrickkettner patrickkettner merged commit 61e72d2 into Modernizr:master Dec 6, 2013

1 check passed

default The Travis CI build passed
Details
@patrickkettner

This comment has been minimized.

Member

patrickkettner commented Dec 6, 2013

Thanks, @Wilto!

@Integralist

This comment has been minimized.

Integralist commented Dec 9, 2013

Good to see it's not only us at BBC News who had to blacklist certain devices because of false positives. See this post by @kaelig http://blog.kaelig.fr/post/33373448491/testing-font-face-support-on-mobile-and-tablet back in Oct 2012.

@patrickkettner

This comment has been minimized.

Member

patrickkettner commented Dec 9, 2013

Ah, thanks for adding this. Headsup, @kaelig, the github link is broken. I believe you are looking for https://github.com/Modernizr/Modernizr/search?q=font-face&type=Issues&state=open :]

@patrickkettner

This comment has been minimized.

Member

patrickkettner commented Dec 9, 2013

For what its worth, it seems that android 2.1 has the same behavior as windows (ie it can @font-face local fonts only).

heres a demo

@kaelig

This comment has been minimized.

kaelig commented Dec 9, 2013

Thanks @patrickkettner, I updated the link.

@paulirish

This comment has been minimized.

Member

paulirish commented Dec 10, 2013

cc @viljamis for good measure

@RoelN

This comment has been minimized.

RoelN commented Dec 11, 2013

@patrickkettner Sorry to wiggly my way into this discussion, what's the IE bug? If you're talking about the solution @stucox mentioned (which is what mine does), I'm not aware of a bug. Or are you talking about the fact that IE8 and older need the external EOT font file?

I find this stuff very interesting but I'm still quite new to writing these kinds of tests, so pardon my n00bness. Is UA sniffing preferable because it's synchronous?

@patrickkettner

This comment has been minimized.

Member

patrickkettner commented Dec 11, 2013

Not at all, @RoelN. I was referring to mobile IE's local-only font-face support. You can use @font-face, and use a local: "fontname" to rename fonts, which is pretty much useless.

UA sniffing is preferred at the moment just because it is known quantity. synchronous is always preferred. We can't do any external downloads in modernizr, so we would need to base64 the font file, which causes a lot of bloat in the test.

@RoelN

This comment has been minimized.

RoelN commented Dec 11, 2013

I see, thanks! Yeah, the base64'ed TTF added about 1150 bytes to the Javascript. Base64'ing the EOT isn't possible as that's not supported by IE8 and below. (Just to be clear, all other browser don't need an external file.)

@timohanninen

This comment has been minimized.

timohanninen commented on 949d770 Jan 16, 2014

When will this be included to the production version? Sorry that I'm bothering you guys and asking, but I am absolute worthless in reading Github.

This comment has been minimized.

Member

patrickkettner replied Jan 16, 2014

Hey @timohanninen, once we ship v 3, this will be included. You could always clone the repo and build it yourself though.

@stucox stucox referenced this pull request Mar 2, 2014

Closed

v3.0 release notes #805

patrickkettner added a commit to patrickkettner/Modernizr that referenced this pull request Feb 22, 2015

Merge pull request Modernizr#1147 from Wilto/master
Adds UA blacklist to @font-face test as a stopgap for false positives
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment