Bug 158509 – [WebIDL] Add support for sequence<Dictionary>

• Status: RESOLVED
• Resolution: DUPLICATE
• Component: Bindings

[Rawinder Singh]

We need to generate bindings for Bug #158508, where the constructor takes a parameter "keyframes" - this parameter will be implemented as type "sequence<Dictionary>". Currently, there is no support for sequence<Dictionary>.

[Rawinder Singh]

Created attachment 287611 [details]
Patch

[Darin Adler]

Comment on attachment 287611 [details]
Patch

Dictionary is a deprecated type. We should not be using Dictionary for any new code. Where were you planning on using a sequence<Dictionary> and what is the specification that calls for that?

[Darin Adler]

Oh, I see. Let me look at bug 158508.

[Darin Adler]

I’m really concerned that the specification says:

"The type of this argument cannot be expressed in WebIDL since it relies on a partially-open dictionary type."

We need to fix WebIDL to be able to express these kinds of things, not write code that exposes JavaScript objets directly to our DOM using the WebCore::Dictionary hack.

I also don’t understand what "partially open" means here.

[Nikos Andronikos]

Partially open means that the set of properties allowed on the dictionary is 
not defined ahead of time (because the set of animatable properties isn't fixed).
My understanding, from chatting to Brian Birtles (editor of Web Animations) is
that support for this could be added to WebIDL, the WebIDL editors have given
their approval, but it's not going to happen in a reasonable time frame and we
don't want to block Web Animations waiting for it. 
The only advantage to adding support for partially open dictionaries in WebIDL
is that we can generate the list of allowable properties for the dictionary in the
bindings layer. Without it, we'll have to do so in the animation code.

[Sam Weinig]

(In reply to comment #4)
> I’m really concerned that the specification says:
> 
> "The type of this argument cannot be expressed in WebIDL since it relies on
> a partially-open dictionary type."
> 
> We need to fix WebIDL to be able to express these kinds of things, not write
> code that exposes JavaScript objets directly to our DOM using the
> WebCore::Dictionary hack.
> 
> I also don’t understand what "partially open" means here.

Dictionary is too powerful to use in most places. What are the actual constraints on the keyframe dictionary-like objects.  From a cursory look at the Web Animations spec, it looks like you can do things like:

[{ color: 'red' }]

bug also

[{ color: ['red', 'green'] }

So, far it sounds like we would want an internal representation that is something like:

struct {
    String easing;
    CompositeOperation composite;
    Vector<std::pair<String, std::variant<String, Vector<String>>>;
}

or maybe:

struct {
    String easing;
    CompositeOperation composite;
    HashMap<String, std::variant<String, Vector<String>>>;
}

(I believe the variant could be replaced with a straight String in cases where the "allow lists" bit is false). 

So, what we need is new dictionary-like syntax in WebIDL to express this conversion. The most straightforward syntax I can think of looks something like this:

[HalfOpenType=(long or sequence<double>)]
dictionary BasePropertyIndexedKeyframe {
    DOMString          easing = "linear";
    CompositeOperation composite;
};

Though the name is horrible and obviously needs to change, it would expect a struct like:

struct BasePropertyIndexedKeyframe {
    String easing;
    CompositeOperation composite;
    HashMap<String, std::variant<int, Vector<double>>>; // Could be a Vector too.
}

[Rawinder Singh]

> Dictionary is a deprecated type.

Could you please clarify - do you mean Dictionary as a class in WebKit?

> We need to fix WebIDL to be able to express these kinds of things, not write code that exposes JavaScript objets directly to our DOM using the WebCore::Dictionary hack.

Further, to the comment that Nikos made. We would only allow animatable properties on the Element to be changed through the keyframes parameter - I don't think that this would expose the JavaScript objects directly - is this right?

[Brian Birtles]

(In reply to comment #6)
> From a cursory look at
> the Web Animations spec, it looks like you can do things like:
> 
> [{ color: 'red' }]
> 
> bug also
> 
> [{ color: ['red', 'green'] }

Actually, the spec allows the following three forms:

  [ { color: 'red' }, { color: 'green' } ]

or:

  { color: [ 'red', 'green' ] }

or:

  { color: 'red' }


(Note that { opacity: [ 0, 1 ] } also works since values are read as strings)

Note that the getKeyframes() API returns values using the first of the above three forms. In Gecko internally we normalize the above three inputs to the above first form.

Also, bear in mind that since custom properties can be animated, the set of properties that can be animated is effectively open-ended (however the spec defines that we only read properties off the input object whose key is a recognized animatable property including where the key matches the syntax for a custom property name).

[Darin Adler]

(In reply to comment #7)
> > Dictionary is a deprecated type.
> 
> Could you please clarify - do you mean Dictionary as a class in WebKit?

Yes, Dictionary as a class in WebKit that bridges between JavaScript and DOM code is obsolete and should not be used in new code. And that includes the Dictionary syntax in IDL.

To give you a rough idea of why: It is not safe to run arbitrary JavaScript in the middle of the DOM code. Each time you get a JavaScript property it could be doing anything, including navigating to another web page, closing the browser window, etc. This needs to be done by the bindings, not in the middle of DOM code. The precise order that things are read out of a JavaScript object is observable behavior.

[Sam Weinig]

(In reply to comment #8)
> (In reply to comment #6)
> > From a cursory look at
> > the Web Animations spec, it looks like you can do things like:
> > 
> > [{ color: 'red' }]
> > 
> > bug also
> > 
> > [{ color: ['red', 'green'] }
> 
> Actually, the spec allows the following three forms:
> 
>   [ { color: 'red' }, { color: 'green' } ]
> 
> or:
> 
>   { color: [ 'red', 'green' ] }
> 
> or:
> 
>   { color: 'red' }
> 
> 

Can you combine the two? e.g.:

[ { color: ['red', 'blue'] }, { color: 'green' } ]


If not, that means the c++ type is something like:

std::variant<
    Vector<HashMap<String, String>>, 
    HashMap<String, std::variant<String, Vector<String>>
>

If so, something like:

std::variant<
    Vector<HashMap<String, std::variant<String, Vector<String>>>, 
    HashMap<String, std::variant<String, Vector<String>>
>


> (Note that { opacity: [ 0, 1 ] } also works since values are read as strings)
> 
> Note that the getKeyframes() API returns values using the first of the above
> three forms. In Gecko internally we normalize the above three inputs to the
> above first form.
> 
> Also, bear in mind that since custom properties can be animated, the set of
> properties that can be animated is effectively open-ended (however the spec
> defines that we only read properties off the input object whose key is a
> recognized animatable property including where the key matches the syntax
> for a custom property name).

Thanks Brian.

[Brian Birtles]

(In reply to comment #10)
> Can you combine the two? e.g.:
> 
> [ { color: ['red', 'blue'] }, { color: 'green' } ]

No.

(Specifically, the procedure to process a keyframes argument[1] first checks if the object is iterable or not, and if it *is*, it passes "allow lists = false" when it calls the procedure to read each of the keyframe objects in the iterable.)

[1] https://w3c.github.io/web-animations/#process-a-keyframes-argument

[Rawinder Singh]

(In reply to comment #6)
> From a cursory look at
> the Web Animations spec, it looks like you can do things like:
> 
> [{ color: 'red' }]
> 
> bug also
> 
> [{ color: ['red', 'green'] }

As there are multiple methods to pass in keyframes, I think we should try and implement just the canonical form first (and the others can come later), i.e.:

[{ offset: 0, margin: "10px" }, { offset: 1, margin: "20px 30px 40px 50px" }]

[Rawinder Singh]

(In reply to comment #9)
> (In reply to comment #7)
> > > Dictionary is a deprecated type.
> > 
> > Could you please clarify - do you mean Dictionary as a class in WebKit?
> 
> Yes, Dictionary as a class in WebKit that bridges between JavaScript and DOM
> code is obsolete and should not be used in new code. And that includes the
> Dictionary syntax in IDL.

If we cannot specify Dictionary on the IDL interface, e.g.: 

Constructor([Default=Undefined] Element? target, sequence<Dictionary> keyframes)

Do you have any thoughts on how I should proceed?

I don’t think defining a dictionary (e.g. dictionary KeyframeInit {…};) is an option as there are too many CSS and SVG properties that could be animated.

[Sam Weinig]

(In reply to comment #13)
> (In reply to comment #9)
> > (In reply to comment #7)
> > > > Dictionary is a deprecated type.
> > > 
> > > Could you please clarify - do you mean Dictionary as a class in WebKit?
> > 
> > Yes, Dictionary as a class in WebKit that bridges between JavaScript and DOM
> > code is obsolete and should not be used in new code. And that includes the
> > Dictionary syntax in IDL.
> 
> If we cannot specify Dictionary on the IDL interface, e.g.: 
> 
> Constructor([Default=Undefined] Element? target, sequence<Dictionary>
> keyframes)
> 
> Do you have any thoughts on how I should proceed?
> 
> I don’t think defining a dictionary (e.g. dictionary KeyframeInit {…};) is
> an option as there are too many CSS and SVG properties that could be
> animated.

I think we should add support to our generator for half-open dictionaries using something along the lines of the fallowing syntax:

[HalfOpenType=(DOMString or sequence<DOMString>)]
dictionary BasePropertyIndexedKeyframe {
    DOMString          easing = "linear";
    CompositeOperation composite;
};

Where you treat it like a dictionary for all the specified types, and the rest of the key/value pairs get processed into a HashMap contained in the struct.

[Rawinder Singh]

Ok. Thanks for the reply - I'll have a look at it(In reply to comment #14)
> (In reply to comment #13)
> > (In reply to comment #9)
> > > (In reply to comment #7)
> > > > > Dictionary is a deprecated type.
> > > > 
> > > > Could you please clarify - do you mean Dictionary as a class in WebKit?
> > > 
> > > Yes, Dictionary as a class in WebKit that bridges between JavaScript and DOM
> > > code is obsolete and should not be used in new code. And that includes the
> > > Dictionary syntax in IDL.
> > 
> > If we cannot specify Dictionary on the IDL interface, e.g.: 
> > 
> > Constructor([Default=Undefined] Element? target, sequence<Dictionary>
> > keyframes)
> > 
> > Do you have any thoughts on how I should proceed?
> > 
> > I don’t think defining a dictionary (e.g. dictionary KeyframeInit {…};) is
> > an option as there are too many CSS and SVG properties that could be
> > animated.
> 
> I think we should add support to our generator for half-open dictionaries
> using something along the lines of the fallowing syntax:
> 
> [HalfOpenType=(DOMString or sequence<DOMString>)]
> dictionary BasePropertyIndexedKeyframe {
>     DOMString          easing = "linear";
>     CompositeOperation composite;
> };
> 
> Where you treat it like a dictionary for all the specified types, and the
> rest of the key/value pairs get processed into a HashMap contained in the
> struct.

Okay. Thanks for your reply - I'll have a look at this.

[Anne van Kesteren]

FWIW, Tess pointed me here and I thought I'd point out https://www.w3.org/Bugs/Public/show_bug.cgi?id=20158 which now has discussion about standardizing "half-open dictionaries" in IDL (including "fully open" dictionaries).

[Darin Adler]

Given that the timing and frequency property fetching is visible in JavaScript because of the "getters" feature, half-open and open dictionaries are challenging from an interoperability point of view. If there is any difference in order of fetching between user agents it can be a source of incompatibility.

[Rawinder Singh]

This bug has been superseded by Bug #163381

[Rawinder Singh]

*** This bug has been marked as a duplicate of bug 163381 ***