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-paint-api] Running author function without proper preparation. #743

Closed
yuki3 opened this Issue Apr 3, 2018 · 15 comments

Comments

Projects
None yet
5 participants
@yuki3
Copy link

yuki3 commented Apr 3, 2018

In short, invoke a paint callback does not follow Web IDL nor HTML specs. The CSS Painting API spec and its algorithms are defined using ECMAScript directly, and it violates Web IDL and HTML specs.

I cannot list up all the places that are conflicting with Web IDL and/or HTML, so let me show an example.

In invoke a paint callback, step 5.3. "Let paintInstance be the result of Construct(paintCtor)."; this step invokes an author function |paintCtor| without performing prepare to run script nor prepare to run a callback. This is NOT allowed in Web IDL nor HTML. |paintCtor| will run without the Incumbent realm having been set up, so we cannot tell what would happen if |paintCtor| invoked an Web API that uses the Incumbent realm.

I originally reported the same issue to Animation Worklet. It seems better to report this to CSS Paint API, too, so I'm now reporting here.

Ideally, each Web API spec should not be defined with ECMAScript. Web IDL provides all the necessary abstraction and it's easier with Web IDL to make each spec consistent.

@yuki3

This comment has been minimized.

Copy link
Author

yuki3 commented Apr 5, 2018

@annevk , you seem active in this spec, too. Could you triage this issue?

@annevk

This comment has been minimized.

Copy link
Member

annevk commented Apr 5, 2018

@yuki3 I've only been active as reviewer, but I didn't really review these aspects carefully.

@bfgeek should look at this and maybe @domenic can help.

What did Chrome end up shipping here?

@yuki3

This comment has been minimized.

Copy link
Author

yuki3 commented Apr 5, 2018

As Chromium has not yet supported the true Incumbent realm and Chromium is kind-of simulating the Incumbent realm with a hack, it's currently working fine. However, Blink bindings team is currently working to support the true Incumbent realm. Once the true Incumbent realm gets supported, Chromium will be in trouble with this issue.

Given that Firefox correctly implements the Incumbent realm, Firefox should have difficulty to implement CSS Paint API as is.

@yuki3

This comment has been minimized.

Copy link
Author

yuki3 commented Apr 5, 2018

Chromium is kind-of simulating the Incumbent realm with a hack, it's currently working fine

I meant that the current implementation/hack for the Incumbent realm in Chromium doesn't rely on "prepare to run script" nor "prepare to run a callback". But Blink bindings team is planning/working to implement it correctly as spec'ed (using "prepare to run script" and "prepare to run a callback").

@annevk

This comment has been minimized.

Copy link
Member

annevk commented Apr 5, 2018

Looking at the open bugs (e.g., https://bugzilla.mozilla.org/show_bug.cgi?id=1302328 and dependencies) it doesn't seem like we've started doing much yet. However, it seems this would also affect https://webaudio.github.io/web-audio-api/#audioworklet which we are working on somewhat as far as I know.

Having said that, given that a worklet only ever has one global, wouldn't the incumbent always be the same as the current global anyway?

cc @jetvillegas @padenot

@bfgeek

This comment has been minimized.

Copy link
Contributor

bfgeek commented Apr 5, 2018

I think we should be able to change:
Let paintInstance be the result of Construct(paintCtor).
to:
Let paintInstance be the result of constructing paintCtor, with no arguments.

I think I based this off the old web components spec initially, which also performed a direct construct (https://w3c.github.io/webcomponents/spec/custom/)

The only concrete difference I can see in our implementation is this will run microtasks at the end of the ctor, when it doesn't look like we currently are. (This will be minor enough for us to fix).

@yuki3

This comment has been minimized.

Copy link
Author

yuki3 commented Apr 6, 2018

I didn't know that Web IDL now supports Construct a callback function type value. That's great and matches this use case.

I have another request, somewhat orthogonal to this, but somewhat relevant to this.

In registerPaint algorithm:
The algorithm is preparing "paint function" as a callback function (but exactly speaking, the manner to create a callback function is not correct. This is another place to be fixed. But this is a side note).
The "paint function" is invoked as a callback function in invoke a paint callback with "paintInstance" as the callback this value.

This is pretty similar to "callback interface" (not callback function) in Web IDL. Why don't we define these algorithms using "callback interface"? I'd recommend a well-defined common manner rather than a unique way.

@yuki3 yuki3 closed this Apr 6, 2018

@yuki3 yuki3 reopened this Apr 6, 2018

@yuki3

This comment has been minimized.

Copy link
Author

yuki3 commented Apr 6, 2018

(I'm sorry for my misoperation of closing this issue.)

@bfgeek

This comment has been minimized.

Copy link
Contributor

bfgeek commented Apr 7, 2018

callback interface has different semantics to what we are doing here.

Using a callback interface would mean each time we need to access the paint or animate function off the instance, instead what we do here is look up the paint method upon registration, and then invoke that each time.

This was done to be consistent with custom elements which uses the exact same semantics.
@domenic has some of the history here, but this pattern is consistent with the other "register a class" methods on the platform.

@bfgeek

This comment has been minimized.

Copy link
Contributor

bfgeek commented Apr 7, 2018

Also according to https://heycam.github.io/webidl/#es-callback-function a callback function and a JS Function represent the same thing. I can change the registerPaint algorithm to use the converting WebIDL, as this should have no side-effects.

@domenic

This comment has been minimized.

Copy link

domenic commented Apr 7, 2018

callback function = JS function + callback context. Callback context is important for computing the incumbent realm, indeed. Probably you'll want to copy the changes from https://github.com/whatwg/html/pull/1471/files#diff-36cd38f49b9afa08222c0dc9ebfe35ebL66547

@annevk

This comment has been minimized.

Copy link
Member

annevk commented Apr 8, 2018

If we use this pattern in more places, isn't that indication we should start abstracting it?

@bfgeek

This comment has been minimized.

Copy link
Contributor

bfgeek commented Apr 8, 2018

From when I had a more limited understanding - heycam/webidl#101

@yuki3

This comment has been minimized.

Copy link
Author

yuki3 commented Apr 9, 2018

Using a callback interface would mean each time we need to access the paint or animate function off the instance, instead what we do here is look up the paint method upon registration, and then invoke that each time.

To me, these two things don't make much difference. I can tell the differences, but do we really want the differences? I.e. own properties on the object are ignored, the property look-up is performed only once when it gets registered, etc. I don't understand what's good if we ignore own properties. Reducing the property look-up would be performance-win, but is it important? For typical use cases, there shouldn't be a behavioral difference. Maybe I'm missing something.

This was done to be consistent with custom elements which uses the exact same semantics.

I read Element definition, and have the same question. Why do we need to make (minor IMHO) differences here? Maybe I should learn something here.

callback function = JS function + callback context. Callback context is important for computing the incumbent realm, indeed.

Yes. And it's important when it's converted, because the incumbent realm changes by time.

If we use this pattern in more places, isn't that indication we should start abstracting it?

I first thought that "custom elements" has unique requirements, that I don't know yet, due to its nature, and only custom elements needs a special way of registration. I'm not sure if CSS Painting API has the same requirements or not. If not, maybe CSS Painting API should be defined in a different way?

If many things have the same requirements and this pattern should be used widely, then I'm happy with this pattern abstracted/defined in Web IDL.

@css-meeting-bot

This comment has been minimized.

Copy link
Member

css-meeting-bot commented Apr 9, 2018

The Working Group just discussed callback interface vs. not, and agreed to the following resolutions:

  • RESOLVED: Keep the design as-is
  • RESOLVED: Conform to webIDL callbacks in the spec
The full IRC log of that discussion <dael> Topic: callback interface vs. not
<dael> github: https://github.com//issues/743
<dael> iank_: We built to be consistant with custom elements API when you register paint or custom element we'll callback to anything that exists and then not touch it again. We did it to be consistant with custom elements.
<dael> iank_: Other then the APIs which always look up function before you invoke it....our version is strict that's slightly more relaxed. There's small stuff that needs to go in, but it's a question of if we should use callback effects.
<dael> iank_: I prefer the system we've got for consistency and it's slightly faster.
<dael> majidvp: One of the problems with current spec you use the functions but it's the webIDL pattern.
<dael> iank_: We need to change them to webIDL format. We need to change the existing stuff to webIDL callback, but not a full callback interface.
<dael> majidvp: Can you make a callback interface?
<dael> iank_: Yes.
<dael> majidvp: I rememebr some warning in webIDL that you can't construct one.
<dael> iank_: There's now a section in webIDL spec [looks for it]
<dael> majidvp: A callback function can be a callback interface?
<dael> iank_: No, it can be literally the function passed into it. We're expecting a function to be passed into webIDL. Main thing is going through and makeing sure incumbent realm is correct.
<dael> iank_: It's fine if we go with that. Callback interface vs not is the question.
<dael> iank_: I'd prefer as-is to be consistent with custom elements.
<dael> iank_: Unless there's a really good reason why we shouldn't.
<dael> iank_: Here I think sticking with what we have may be a performance hit based o nhow many times we're invoking the function.
<dael> iank_: I'd prefer to keep as is.
<dael> TabAtkins: I'd prefer caching
<dael> iank_: We can also, assuming we code....you can wrap the behavior as well.
<dael> brian: Can you re-register?
<dael> iank_: You can't re-register but you can wrap it up and re-call it.
<dael> Rossen: Anyone think we should not keep the current design?
<dael> iank_: We'll need to make some small changes to align.
<dael> Rossen: Objections to keep the design as-is?
<dael> iank_: Do we need to resolve to make changes to use webIDL?
<dael> RESOLVED: Keep the design as-is
<dael> RESOLVED: Conform to webIDL callbacks in the spec

@css-meeting-bot css-meeting-bot removed the Agenda+ label Apr 9, 2018

@bfgeek bfgeek closed this in f9f174d Jun 21, 2018

majido added a commit to WICG/animation-worklet that referenced this issue Aug 7, 2018

Use webidl algorithms for construction and invocation (#104)
We resolved to continue using the "cache" props approach [here](w3c/css-houdini-drafts#743 (comment)) but  we still need to be consistent in using webidl algorithms for invoking and construction operation.

The following changes fix this:
 - Use VoidFunction type for constructor, and Function type for animate and destroy callbacks
 - Use  convert algorithm to convert incoming values to proper types upon registration
 - Use invoke/construct algorithms to call or construct. This ensure the proper  setup in place which addresses the original reported issue.

Fixes #94
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.