Bug 156035 - [Font loading] document.fonts.check() always return true
Summary: [Font loading] document.fonts.check() always return true
Status: REOPENED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: Safari Technology Preview
Hardware: Mac OS X 10.11
: P2 Normal
Assignee: Myles C. Maxfield
URL:
Keywords: InRadar
: 184532 (view as bug list)
Depends on:
Blocks:
 
Reported: 2016-03-30 12:55 PDT by Arthur
Modified: 2023-12-13 19:51 PST (History)
10 users (show)

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Arthur 2016-03-30 12:55:50 PDT
Some times document.fonts.check() doesn't work. More specifically, it works, but always returns -- true. I do not know what is the reason, because it works fine in our staging env and fails on my local and production envs.

Repro:

1. Go to https://m.oumy.com/#/
2. Browser should load 3 Roboto fonts. None is loading.
3. Run in console: document.fonts.check('16px Roboto') and document.fonts.check('16px Zzz') -- both returns -- true.

4. Go to https://html5.oumy.com:8080/#/
5. See that 3 Roboto fonts are loaded.
6. Run in console: document.fonts.check('16px Roboto') and document.fonts.check('16px Zzz') -- both returns -- true.
Comment 1 Arthur 2016-03-30 23:16:33 PDT
Just for more context:

"Zzz" is unknowns font, i.e. random test to test
"Roboto" is font which I actually wont to use.

By some reason, when page is loading `document.fonts.check()` returns different values for those 2 different sites.

On page load, on both sites, `document.fonts.check()` returns true for both fonts ("Zzz", "Roboto"). On a second site where Roboto is loaded it's understandable why `fonts.check()` returns true (because font was really loaded), in all other cases everything is weird :-)

My code of loading fonts is approximately like this:

if (!document.fonts.check(...)) { // e.g. "16px Roboto"
  document.fonts.load(...).then((face) => document.fonts.add(face));
}
Comment 2 Arthur 2016-03-30 23:40:50 PDT
Oh, https://html5.oumy.com:8080/ actually has an old version of site running and it's not using Fonts Loading API to load fonts, so fonts loading works. Now it looks like `document.fonts.check()` always just returns true.

Sorry for misleading.
Comment 3 Jon Lee 2016-03-31 00:07:21 PDT
(In reply to comment #2)
> Oh, https://html5.oumy.com:8080/ actually has an old version of site running
> and it's not using Fonts Loading API to load fonts, so fonts loading works.

Did you mean to say m.oumy.com is not using the Fonts Loading API?
Comment 4 Arthur 2016-03-31 00:58:07 PDT
No. m.oumy.com is production and it uses Fonts Loading API and it works everywhere (where supported)except on Safari Tech Preview.

https://html5.oumy.com:8080/ is a staging env and it seems to have an older version of site, because I was testing everything locally. Staging uses fallback (fonts loading library) to load fonts, this is why it works in Tech Preview.
Comment 5 Myles C. Maxfield 2016-03-31 16:05:26 PDT
I've investigated https://m.oumy.com/#/.

The page creates the following @font-face blocks:
@font-face {
    font-family: AppFont;
    font-style: normal;
    font-weight: 400;
    src: local('Roboto'), local('Roboto-Regular'), local('sans-serif')
}

@font-face {
    font-family: AppFont;
    font-style: normal;
    font-weight: 500;
    src: local('Roboto Medium'), local('Roboto-Medium'), local('sans-serif-medium')
}

@font-face {
    font-family: AppFont;
    font-style: normal;
    font-weight: 700;
    src: local('Roboto Bold'), local('Roboto-Bold'), local('sans-serif-bold')
}

@font-face {
    font-family: AppFont;
    font-style: normal;
    font-weight: 300;
    src: local('Roboto Light'), local('Roboto-Light'), local('sans-serif-light')
}

It then calls document.fonts.check() three times, with the following arguments:
"normal 400 48px Roboto"
"normal 500 48px Roboto"
"normal 700 48px Roboto"


Because the above @font-face blocks are not using the font-family "Roboto", the check() call is not interested in them.

Therefore, no fonts are found that match the query (because OS X doesn't have any preinstalled fonts named "Roboto". In this situation, the spec [1] says: "If font face list is empty, or all fonts in the font face list either have a status attribute of "loaded" or are system fonts, return true."

We are correctly returning "true" here.

[1] https://drafts.csswg.org/css-font-loading/#font-face-set-check
Comment 6 Myles C. Maxfield 2016-03-31 16:14:25 PDT
https://html5.oumy.com:8080/#/ also includes the additional @font-faces:

@font-face {
  font-family: 'Roboto';
  font-style: normal;
  font-weight: 400;
  src: local('Roboto'), local('Roboto-Regular'), url(https://fonts.gstatic.com/s/roboto/v15/CrYjSnGjrRCn0pd9VQsnFOvvDin1pK8aKteLpeZ5c0A.woff) format('woff');
}
@font-face {
  font-family: 'Roboto';
  font-style: normal;
  font-weight: 500;
  src: local('Roboto Medium'), local('Roboto-Medium'), url(https://fonts.gstatic.com/s/roboto/v15/RxZJdnzeo3R5zSexge8UUbO3LdcAZYWl9Si6vvxL-qU.woff) format('woff');
}
@font-face {
  font-family: 'Roboto';
  font-style: normal;
  font-weight: 700;
  src: local('Roboto Bold'), local('Roboto-Bold'), url(https://fonts.gstatic.com/s/roboto/v15/d-6IYplOFocCacKzxwXSOLO3LdcAZYWl9Si6vvxL-qU.woff) format('woff');
}


This explains why the fonts are loaded successfully on that page.
Comment 7 Myles C. Maxfield 2016-03-31 16:16:40 PDT
I'm going to mark this bug as already working correctly. If you think there's more to do, please feel free to re-open.
Comment 8 Arthur 2016-03-31 17:01:52 PDT
First all, it's working in every other browser and isn't working in Safari Tech Preview, so obviously, something is wrong with it.

Second, you are wrong.



> "If font face list is empty, or all fonts in the font face list either have a status attribute of "loaded" or are system fonts, return true."

Returned font face list can be empty only if such font was found and then was removed from matched list because it didn't match unicode-range test. Otherwise "found flag" is set to false and check() must return false

So if Roboto isn't system font and it's not already loaded in FontFaceSet (document.fonts), implementation here must return false.

I really hate that I have to explain spec for you.

Next, this is font faces used by both m.oumy.com and html5.oumy.com: https://gist.github.com/NekR/fb3db9bd88f605f1e106a9069ae95cfc

This is default font defined for the app, it's fallback and as you see, it's not loading any fonts.

Roboto fonts are loaded from script with either Font Loading API (if available) or with fallback library. As I said, html5.oumy.com has older version of app and therefore has different code. I just reviewed that old code again, and thing is, it's actually using Fonts Loading API, but not using document.fonts.check(). This is why fonts are loaded on html5.oumy.com (not using document.fonts.check()) and not loading on m.oumy.com (using document.fonts.check()).

Furthermore, AppFont isn't related here at all. Once fonts are loaded (or script determines that browser already has those fonts), 'fonts' class is added to document body, which overrides AppFont usage and tells to page to use only Roboto.


One more thing -- go to any page in Safari Tech Preview and run document.fonts.check('16px sdfjsdkfjskdj') and it will return true. I doubt that it's correct behavior.



And the last, please treat this comment as the last comment on this tracker because I do not like those "WontFixes" just because you were not able to figure out what is going on. Why I should care about your browser if you do not care?

I really do not need it as a developer, I always can say to my client "this is browser bug and I cannot do anything here", but instead, I am trying to help.
Comment 9 Ryosuke Niwa 2016-03-31 17:51:22 PDT
(In reply to comment #8)
>
> One more thing -- go to any page in Safari Tech Preview and run
> document.fonts.check('16px sdfjsdkfjskdj') and it will return true. I doubt
> that it's correct behavior.

This behavior matches that of Firefox (44.0.2 and 48.0a1) as well as the spec text as far as I can tell.

I agree this is somewhat counterintuitive. Perhaps we might want to raise this concern in the CSS WG. This could be an interoperability issue already given Chrome returns false in this case if content authors are assuming that's the right behavior.
Comment 10 Timothy Hatcher 2016-03-31 18:12:25 PDT
Marking as "WORKSFORME" instead of "WONTFIX" or "FIXED" since that is the right resolution here (unless we determine otherwise after talking to the spec authors.)
Comment 11 Myles C. Maxfield 2016-03-31 18:48:55 PDT
Reopening due to spec issues.
Comment 12 Tab Atkins 2016-03-31 18:52:49 PDT
(In reply to comment #5)
> Therefore, no fonts are found that match the query (because OS X doesn't
> have any preinstalled fonts named "Roboto". In this situation, the spec [1]
> says: "If font face list is empty, or all fonts in the font face list either
> have a status attribute of "loaded" or are system fonts, return true."
> 
> We are correctly returning "true" here.
> 
> [1] https://drafts.csswg.org/css-font-loading/#font-face-set-check

Nope, read step *3* of the algorithm, which checks the "found faces" flag.  It lets check() distinguish between "found some faces, but threw them out, no valid matches" and "you clearly typo'd, there's no font by that name anywhere on this page".

The check() method in the example in your comment should *throw*, not return true.  (Sorry I haven't specified the error that should be thrown; feel free to suggest what it should be.)
Comment 13 Myles C. Maxfield 2016-03-31 20:11:47 PDT
I'd like to provide some background here.

1. Before May 19, 2004 the spec stated that this situation should return true.
2. On May 19, 2004, the spec switched to stating that this situation should return false. This Git revision [1] switched it.
3. On May 27, 2005, the spec switch to stating that this situation should thrown an exception, but it didn't state which kind of exception should be thrown. This Git revision [2] switched it. This decision was made based on the W3C thread starting at [3], where Cameron and John from Mozilla state their issues with this situation returning false. Tab from Google argues that this situation should be treated as an error condition, and he modified the spec to state that an exception should be thrown.

The latest development is item #3, so the current state of the spec [4] states that this situation should throw an exception, but doesn't state which kind of exception should be thrown. The TR version of the spec [5] is stale, and was created between items #2 and #3, so it states that this situation should return false.

WebKit generally implements the latest version of any spec, so the most relevant document is the current draft [4]. That being said, currently no browsers implement the behavior stated in the spec. WebKit generally favors interoperability over exactly following specs. IE / Edge don't implement this API, Firefox implements the spec as of item #1, and Chrome implements the spec as of item #2. The Safari Technical Preview currently agrees with Firefox.

This was not an accident. I agree with the arguments that Cameron and John made, and I am participating in the discussion about this issue in the W3C. You can see my response to the thread at [6]. I argue that the behavior of item #1 should be restored.

Because this issue is still in flux (and is clearly contentious, given the amount of changes this issue in the spec has undergone), I will participate in discussions at the W3C on this topic until they conclude. At that point, I'll update WebKit (and this bug).

[1] https://github.com/w3c/csswg-drafts/commit/35f2b975e8c914ffa901fbfba7f8ca93d18bfdba
[2] https://github.com/w3c/csswg-drafts/commit/e73686097b5699745e58c42788be17b1821aa38f
[3] https://lists.w3.org/Archives/Public/www-style/2015May/0002.html
[4] https://drafts.csswg.org/css-font-loading/
[5] https://www.w3.org/TR/css-font-loading/
[6] http://lists.w3.org/Archives/Public/www-style/2016Apr/0000.html
Comment 14 Myles C. Maxfield 2016-03-31 21:39:02 PDT
(In reply to comment #13)
> I'd like to provide some background here.
> 
> 1. Before May 19, 2004 the spec stated that this situation should return
> true.
> 2. On May 19, 2004, the spec switched to stating that this situation should
> return false. This Git revision [1] switched it.
> 3. On May 27, 2005, the spec switch to stating that this situation should
> thrown an exception, but it didn't state which kind of exception should be
> thrown. This Git revision [2] switched it. This decision was made based on
> the W3C thread starting at [3], where Cameron and John from Mozilla state
> their issues with this situation returning false. Tab from Google argues
> that this situation should be treated as an error condition, and he modified
> the spec to state that an exception should be thrown.
> 
> The latest development is item #3, so the current state of the spec [4]
> states that this situation should throw an exception, but doesn't state
> which kind of exception should be thrown. The TR version of the spec [5] is
> stale, and was created between items #2 and #3, so it states that this
> situation should return false.
> 
> WebKit generally implements the latest version of any spec, so the most
> relevant document is the current draft [4]. That being said, currently no
> browsers implement the behavior stated in the spec. WebKit generally favors
> interoperability over exactly following specs. IE / Edge don't implement
> this API, Firefox implements the spec as of item #1, and Chrome implements
> the spec as of item #2. The Safari Technical Preview currently agrees with
> Firefox.
> 
> This was not an accident. I agree with the arguments that Cameron and John
> made, and I am participating in the discussion about this issue in the W3C.
> You can see my response to the thread at [6]. I argue that the behavior of
> item #1 should be restored.
> 
> Because this issue is still in flux (and is clearly contentious, given the
> amount of changes this issue in the spec has undergone), I will participate
> in discussions at the W3C on this topic until they conclude. At that point,
> I'll update WebKit (and this bug).
> 
> [1]
> https://github.com/w3c/csswg-drafts/commit/
> 35f2b975e8c914ffa901fbfba7f8ca93d18bfdba
> [2]
> https://github.com/w3c/csswg-drafts/commit/
> e73686097b5699745e58c42788be17b1821aa38f
> [3] https://lists.w3.org/Archives/Public/www-style/2015May/0002.html
> [4] https://drafts.csswg.org/css-font-loading/
> [5] https://www.w3.org/TR/css-font-loading/
> [6] http://lists.w3.org/Archives/Public/www-style/2016Apr/0000.html

Whoops, s/2004/2014/g & s/2005/2015/g
Comment 15 Adrian Gawor 2020-11-09 07:54:10 PST
Today I've been struck by this issue, so I did a a bit of digging and to make long story short I've stumbled upon this line of code https://github.com/WebKit/webkit/blob/950143da027e80924b4bb86defa8a3f21fd3fb1e/Source/WebCore/css/CSSFontFaceSet.cpp#L406
Correct me if I'm wrong but in my personal and not so humble opinion it's plain wrong as check method should produce false as well in case of:
   face.get().status() == CSSFontFace::Status::Loading
Comment 16 Adrian Gawor 2020-11-09 08:29:15 PST
On top of above Firefox impl (https://hg.mozilla.org/mozilla-central/file/default/layout/style/FontFaceSet.cpp#l358) and as well Chromium (https://chromium.googlesource.com/chromium/src.git/+/refs/heads/master/third_party/blink/renderer/core/css/font_face_set.cc#221 and https://chromium.googlesource.com/chromium/src.git/+/refs/heads/master/third_party/blink/renderer/core/css/css_segmented_font_face.cc#180) pretty much boils down to status != Loaded 
Would be great to have unified approach to that (read as same as other browsers). Currently in WebKit FontFaceSet::check returns true even if font face is still loading.
Comment 17 Sam Sneddon [:gsnedders] 2022-12-01 03:15:41 PST
*** Bug 184532 has been marked as a duplicate of this bug. ***
Comment 18 Sam Sneddon [:gsnedders] 2022-12-01 03:16:31 PST
rdar://39380289
Comment 19 Vitor Roriz 2023-01-09 04:23:53 PST
Shouldn't this be closed since https://github.com/w3c/csswg-drafts/issues/5744 decided for not throwing anymore for this case?
Comment 20 Ahmad Saleem 2023-12-13 19:50:05 PST
(In reply to Vitor Roriz from comment #19)
> Shouldn't this be closed since
> https://github.com/w3c/csswg-drafts/issues/5744 decided for not throwing
> anymore for this case?

This links to following Chromium / Blink commit: https://chromium-review.googlesource.com/c/chromium/src/+/4913179 ; which refers following WPT: https://wpt.fyi/results/css/css-fonts/fallback-url-to-local.html?label=experimental&label=master&aligned=

Which STP184 fails.