Skip to content
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

[css-fonts] FontFaceSet uses referential equality for calculating .has() #273

Closed
craigkovatch opened this issue Jul 6, 2016 · 9 comments
Assignees

Comments

@craigkovatch
Copy link

FontFaceSet.has is not very useful right now, as it appears to use referential equality when comparing. So if I want to avoid adding duplicate FontFaces to my document, as currently speced I will have to do my own iteration and deep value comparison.

Could FontFace expose some sort of comparison method that could be used by FontFaceSet to provide easier deduping? i.e. so the following code could work:

var font1 = new FontFace("test", "url(test.woff)", {});
document.fonts.add(font1);
var font2 = new FontFace("test, "url(test.woff)", {});
if (document.fonts.has(font2)) {; // this should be useful
  continue;
}
@tabatkins
Copy link
Member

The current behavior is just how JS Sets work, so .has() isn't going to change.

However, we could possible add a looser comparison function. What is currently causing you to add duplicates to the page? Why can't you avoid doing that in the first place?

@craigkovatch
Copy link
Author

craigkovatch commented Jul 6, 2016

The content I'm Canvas-rendering (and thus the fonts needed to render) can change based on server responses. Currently I'm having to generate a hash of all the necessary font properties and store it locally to ensure I don't add a duplicate font. But the general question of "is this font already defined in the document" seems like a fairly common scenario, and one that may puzzle many others when .has() is relying on referential equality rather than (deep) value equality.

Is it possible to define a looser comparison function that FontFaceSet.has() would use? Or would that need to be a separate method in FontFaceSet?

Also, since FontFaceSet can't be initialized outside of the inbuilt document.fonts member (i.e. I can't maintain other FontFaceSets to compare FontFaces across), I think the question is begged: what's the value of making FontFaceSet "set-like" if the set methods aren't particularly useful?

@craigkovatch
Copy link
Author

It may also be reasonable for me to file a bug against the browser(s) to say "should no-op if an identical font definition is added".

@litherum
Copy link
Contributor

litherum commented Jul 7, 2016

The implementation of this comparison function is site-specific, and therefore cannot be implemented by the browser. For example, the browser doesn't know th example to questions like these:

  • Many browsers don't implement font-stretch. Should this be a part of the comparison function? What happens when these browsers start implementing it? Existing websites break?
  • Should two URLs with different fragment identifiers be considered distinct?
  • What about "url(foo.woff)" compared with "url(foo.woff), url(fallback.woff)". These appear to be the same only if all the characters in the elements styled with them are supported by foo.woff
  • foo.woff and foo.woff2 are often conceptually same font
  • Are url(foo.ttf) and local(foo) distinct?
  • How about unicode-range? If the ranges for the two fonts don't overlap? If they overlap by only one code point?

@craigkovatch
Copy link
Author

craigkovatch commented Jul 7, 2016

I see what you're saying about the complexity of implementing this in a smart way, but I'm not suggesting the comparison be that clever. Just do value equality for each member of FontFace. All the pertinent vales are DOMString. No need for parsing the semantics of each value.

@litherum
Copy link
Contributor

litherum commented Jul 7, 2016

Such an implementation would give incorrect results for many (most) applications and would be actively harmful.

@craigkovatch
Copy link
Author

craigkovatch commented Jul 7, 2016

How and why? The point is to be able to construct FontFace objects and compare them to the Document's set. If you're going to make such a strong claim, provide an example of how -- within such a context -- this could be incorrect or harmful.

I suggest that the current scheme is already incorrect and possibly harmful in its propensity for false negatives.

@tabatkins
Copy link
Member

I agree that for the suggested usage (a single app trying to dedupe its own stuff) a generic and simple string-comparison method would work fine.

However, because such a simple method works fine, it's less clear that we need to add anything to the spec - it's trivial to write a comparison method that iterates over the set and checks if a given font is already there. A native implementation won't be any faster than what you can write yourself, as it would have to do iterate-and-compare too. And I'm not sure that the use-case is widespread enough to justify shipping an implementation to everyone else. :/

@craigkovatch
Copy link
Author

I think those are reasonable points. You're right that it's not that hard to implement. It's kind of a bummer to make everyone implement such a low-level method. But my bigger concern is that people won't realize that .has() doesn't work in any useful way and the many debugging hours that could be lost to such a detail.

At the very least, what would you think about the spec offering a note of caution?

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

No branches or pull requests

3 participants