Skip to content

[font-loading] FontFaceSetLoadEvent's fontfaces should use FrozenArray #810

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

Closed
foolip opened this issue Dec 15, 2016 · 10 comments
Closed

[font-loading] FontFaceSetLoadEvent's fontfaces should use FrozenArray #810

foolip opened this issue Dec 15, 2016 · 10 comments

Comments

@foolip
Copy link
Member

foolip commented Dec 15, 2016

https://drafts.csswg.org/css-font-loading/#fontfacesetloadevent

Current IDL:

[Constructor(DOMString type, optional FontFaceSetLoadEventInit eventInitDict),
 Exposed=Window,Worker]
interface FontFaceSetLoadEvent : Event {
  readonly attribute sequence<FontFace> fontfaces;
};

https://heycam.github.io/webidl/#idl-sequence says "Sequences must not be used as the type of an attribute or constant" so this needs to be:

[Constructor(DOMString type, optional FontFaceSetLoadEventInit eventInitDict),
 Exposed=Window,Worker]
interface FontFaceSetLoadEvent : Event {
  [SameObject] readonly attribute FrozenArray<FontFace> fontfaces;
};

[SameObject] thrown in because it's true, a drive-by.

A test for this would have to check that event.fontfaces===event.fontfaces, and that new FontFaceSetLoadEvent('type', { fontfaces: x }).fontfaces!==x but that the array length and members are equal.

Lists of things are fun. @LoonyBean

@bzbarsky
Copy link

For what it's worth, Gecko's IDL for this is:

[Cached, Constant] readonly attribute sequence<FontFace> fontfaces;

which is just missing [Frozen] to be functionally equivalent to [SameObject] readonly attribute FrozenArray<FontFace> fontfaces;.

The [Frozen] thing came up during code review for the Gecko code (the original patch did include it) and was supposed to lead to the filing of a spec issue, because the spec didn't define the behavior of this getter at all. It looks like the spec issue never got filed. :(

@foolip
Copy link
Member Author

foolip commented Dec 16, 2016

Great! @LoonyBean did write a test for this that would be easy to put in web-platform-tests, but I guess it's still csswg-test for this spec?

@bzbarsky
Copy link

Are there web platform tests added for this?

@foolip
Copy link
Member Author

foolip commented Dec 19, 2016

@LoonyBean hasn't landed the Chromium change for this yet, but it has a test that would be easy to contribute if putting it in web-platform-tests is acceptable. There isn't even a font-loading directory in csswg-test. @gsnedders, please tell us what to do :)

@heycam
Copy link
Contributor

heycam commented Dec 21, 2016

Yeah, looks like I forgot to file an issue at the time. :( Although a few months later @RByers reported the issue (and I replied to it!), but I guess it never got acted upon. Anyway, thanks for getting it fixed now, @foolip.

@gsnedders
Copy link
Member

@astearns do we have any policy on where new testsuites go?

@astearns
Copy link
Member

We're moving everything to wpt, so if it can go into wpt to begin with I don't see any reason not to do that.

@foolip
Copy link
Member Author

foolip commented Feb 14, 2017

@LoonyBean, what happened in the end with the tests for this? Looks like Blink's test in third_party/WebKit/LayoutTests/fast/css/fontfacesetloadevent-constructor.html could easily be upstreamed if we just know where to put it? @gsnedders, maybe you can just ping this issue after csswg-tests are merged instead of us trying to figure out an interim place for the test?

@gsnedders gsnedders self-assigned this Feb 14, 2017
@gsnedders
Copy link
Member

@foolip can you file an issue for the tests and assign it to me?

@foolip
Copy link
Member Author

foolip commented Feb 14, 2017

@gsnedders, web-platform-tests/wpt#4822 is all yours :)

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

No branches or pull requests

5 participants