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-typed-om]: There's no nice way to represent CSSUnparsedValue as a "list-plus" style object. #239

Closed
annevk opened this issue Jun 22, 2016 · 48 comments

Comments

@annevk
Copy link
Member

annevk commented Jun 22, 2016

Single-type iterables are reserved to certain legacy APIs with an indexed getter. I don't think we want that here.

@bzbarsky

@tabatkins
Copy link
Member

WebIDL doesn't say it's restricted to legacy APIs? It does indeed say that you need an indexed getter and a .length property, tho - we can fix that.

@annevk
Copy link
Member Author

annevk commented Jun 22, 2016

Indexed getters are legacy for all intents and purposes. As they require a proxy to implement and are therefore not really natural.

@annevk
Copy link
Member Author

annevk commented Jun 22, 2016

(There's a open issue against IDL to get them renamed.)

@bzbarsky
Copy link

Yeah, I think we do want the indexed getter. People keep talking about it as if it were legacy, but we still have no decent solution for a lot of use cases.

That said, in this case we could perhaps use an Array-valued property. At least as long as it does not need to be live in any way. If liveness of some sort is needed, then we're back to indexed getters.

@tabatkins
Copy link
Member

Nothing is live in the Typed OM. This is a serialization (in object form), and to change anything you have to explicitly set it again.

@annevk
Copy link
Member Author

annevk commented Jun 22, 2016

Yeah, I raised an issue earlier about this maybe just being an Array, but that got closed...

@bzbarsky what use cases though? We don't really want new live lists. And if you really need it, just have a method.

@bzbarsky
Copy link

Nothing is live in the Typed OM.

OK. Looks like you can't modify a CSSTokenStreamValue, so you could make it a frozen array, but...

@bzbarsky what use cases though?

Consider the case here, where you want to represent "iterable stream of tokens" in a thing that you could pass to StylePropertyMap.set. If you just make it an Array, you now have to make StylePropertyMap more complicated.

@shans shans changed the title CSS Typed: CSSTokenStreamValue is not valid IDL [css-typed-om]: CSSTokenStreamValue is not valid IDL Jun 24, 2016
@shans
Copy link
Contributor

shans commented Jun 24, 2016

I reopened #221 - it really shouldn't have been closed.

I'm OK with an Array, any objections? This definitely should not be live, and a simple representation is best.

Does the Array need to be frozen?

@annevk
Copy link
Member Author

annevk commented Jun 24, 2016

I think people decided they wanted to return frozen arrays where possible. I'm still not entirely convinced it adds value, but it seems to be the pattern we're using for now...

@bzbarsky because StylePropertyMap would have to accept an iterable and return an array rather than set/get of an object? Somewhat convincing, though I think the benefits of using a standard data structure outweigh that. (Ideally we'd have immutable standard data structures of course...)

@bzbarsky
Copy link

@bzbarsky because StylePropertyMap would have to accept an iterable

That depends. Is the idea to accept the new thing by reference or by value? Right now it accepts and stores it by reference, yes?

@shans
Copy link
Contributor

shans commented Aug 11, 2016

The main issue is that CSSTokenStreamValue needs to be a subclass of CSSValue.

Making it an iterable and adding the indexed getters/setters seems like the best solution?

@tabatkins tabatkins self-assigned this Aug 11, 2016
@annevk
Copy link
Member Author

annevk commented Aug 12, 2016

Please don't add more indexed getters/setters to the platform. Just use get() methods and such. Forcing new objects to be proxies is bad.

@annevk
Copy link
Member Author

annevk commented Aug 12, 2016

^^ @domenic

@tabatkins
Copy link
Member

And that goes directly against the common advice of "ugh, fake-list interfaces with getItem()/etc are so shitty". They're widely considered a legacy mistake every place they appear.

This is not a happy space to deal with, but it also shouldn't require a fully Proxy just to implement numeric setters. This is JS's fault for not yet exposing a hook for foo[bar] behavior. AWB has a proposal for that (a pair (for read/write) of Symbol-valued properties on Object that get called by the [0] syntax if set) , but it hasn't moved in a few years.

@annevk
Copy link
Member Author

annevk commented Aug 16, 2016

They're widely considered a legacy mistake every place they appear.

No, they're not actually. Talk to your JavaScript team.

@domenic
Copy link
Contributor

domenic commented Aug 16, 2016

Yeah, there's a reason we didn't add "indexed getters" to Map or Set or IteratorPrototype.

@tabatkins
Copy link
Member

I mean, this goes against years of advice I'd heard (for example, how the SVG DOM was so horrible in having tons of get() methods on all their array-like classes), and directly contradicts what Boris is saying in #239 (comment).

I'd be more for just making it a pure iterator with no direct access and require casting it to an array if you want that, than adding .get() methods. :/

(Or just getting JS to do the necessary hops to make indexed access not require a full Proxy, because that's dumb.)

@shans
Copy link
Contributor

shans commented Aug 17, 2016

So I'm leaning towards something that looks like

[constructor(sequence<DOMString or CSSVariableReferenceValue>]
interface CSSTokenStreamValue : CSSStyleValue {
  FrozenArray<(DOMString or CSSVariableReferenceValue)> components;
};

Purely because it seems to thread all the Opinions on display here.

If I could have magical ponies then I think CSSTokenStreamValue would just be an array that subclasses CSSStyleValue, but it seems those are somewhat in demand at the moment.

As a side note, y'all need some explainers about what current 'best practise' is, preferably one that has been reviewed by the TAG as well as all relevant JS vendor teams... I don't think the current drive-by conflicting assertions approach is working.

@tabatkins
Copy link
Member

Yeah, this is pretty terrible, but I'm happy to lay a lot of the blame on WebIDL being unmaintained for a while. This stuff has only gotten really important the last few years, exactly when Cam started backing off from editting. Whatever the "right" decision is, it needs to be documented in WebIDL for posterity.

@annevk
Copy link
Member Author

annevk commented Aug 17, 2016

Right, there are open issues against IDL to mark proxy-creating members as legacy.

@tabatkins
Copy link
Member

Except that @bzbarsky said:

People keep talking about it as if it were legacy, but we still have no decent solution for a lot of use cases.

There's clearly a lot of disagreement here, and we need to hash it out soon.

@FremyCompany
Copy link
Contributor

I would like to point out I (still) think using a property instead of making the instances be iterable is a good thing because whenever we are going to support new values that currenty are CSSTokenStreamValue/CSSUnparsedValue (for instance a CSS Shapes Typed OM), we would have to make CSSShapeValue itself be an iterable of String for that not to be a breaking change (and that wouldn't make sense to me).

If we go the property route, we could move the property to CSSValue (and return a Frozen array containing one item, the cssText, for any value type except CSSUnparseValue which might return a bigger array since it could contain custom property references). In that case, CSSUnparsedValue would be an empty interface whose only purpose would be to clarify that the value has no other type.

Maybe (just thinking out loud here) we can actually drop CSSUnparsedValue entirely, and decide to just leave unparsed things be a bare instance of CSSValue directly. I didn't think this through, though, but that looks reasonable at first glance.

@shans
Copy link
Contributor

shans commented Apr 3, 2017

@FremyCompany I don't think your concern applies any longer - CSSUnparsedValue now exists only to extract custom property references. If you have a CSS Shapes value that isn't currently supported, then it'll be a CSSValue with a .toString(). Which is more or less what you suggested :)

@shans
Copy link
Contributor

shans commented Apr 3, 2017

On discussion with Tab we decided to do:

[Constructor((DOMString or CSSVariableReferenceValue)... members)]
 interface CSSUnparsedValue : CSSStyleValue {
        iterable<(DOMString or CSSVariableReferenceValue)>;
       readonly attribute unsigned long length;
       getter (DOMString or CSSVariableReferenceValue) (unsigned long index);
 };

This is a readonly iterable that you need to reconstruct if you want to change. It sucks a bit but preserved typedness.

I'm going to commit that but leave this issue open against it.

@shans shans changed the title [css-typed-om]: CSSTokenStreamValue is not valid IDL [css-typed-om]: There's no nice way to represent CSSUnparsedValue as a "list-plus" style object. Apr 3, 2017
shans added a commit that referenced this issue Apr 3, 2017
@shans
Copy link
Contributor

shans commented Apr 3, 2017

@domenic @bzbarsky @tabatkins to get the full range of opinions...

@tabatkins
Copy link
Member

As we discussed in the past, @annevk, there's currently no way to get anything remotely array-like in WebIDL, for no particularly good reason. Even the plain iterator<> declaration is broken, and requires us to specify indexed getters and thus invoke proxies.

Fix that in WebIDL, and we can fix this spec. The only other possibility is making our interfaces super janky, so that you have to access a separate attribute/method to actually get the data as an Array.

@annevk
Copy link
Member Author

annevk commented Apr 4, 2017

How would that introduce jank?

@annevk
Copy link
Member Author

annevk commented Apr 4, 2017

(Also, if you actually need Array subclasses, we should just get those added instead of continuing with this hack.)

@tabatkins
Copy link
Member

"Jank" in the "looks terrible" meaning, not the "slow/jittery" meaning.

I don't necessarily need Array subclasses. At bare minimum, I need a value iterator; currently, those are broken in WebIDL and force you into being a Proxy. (This is why I said, in one of the IDL issues, that I could work around it by just using a kv iterator and manually setting up the keys as meaningless indexes - that gets me in the neighborhood of the correct behavior that value iterators should have.)

Having an Array subclass would be ideal, tho - without it, we can't reasonably make some of these classes mutate-able. Currently we're planning on just accepting that, so that you have to create fresh objects instead; alternately, we could go the terribly ugly route of having .get/.set arrayish methods.

@annevk
Copy link
Member Author

annevk commented Apr 5, 2017

With an Array subclass you can't have mutability either, since you can't observe changes to the Array. @domenic @tobie thoughts?

@tabatkins
Copy link
Member

We neither need nor want "live" mutability. Updates only happen if you call StyleMap.set().

But it's useful (and more performant) to be able to .get() a value out of a stylemap, fiddle with it, then .set() it back. (Rather than getting an immutable value, then having to construct a fresh value with the changes you want, then setting it back.) You can do this for all the other objects, but not the two Array-likes.

(The "get a value, mutate it, set it back" pattern is especially useful for animations, where you can just reuse a single object that's continually fiddled with, rather than creating garbage at rAF speed.)

@tabatkins
Copy link
Member

(In other words, I understand you need a Proxy (or ES to get off its ass and finally let us hook the obj[foo] notation like AWB has talked about for years) to implement "live" mutability. That's just not what we need here.)

@annevk
Copy link
Member Author

annevk commented Apr 5, 2017

Thanks for the clarification, that helps a fair bit.

@css-meeting-bot
Copy link
Member

The CSS Working Group just discussed Arraylike, and agreed to the following resolutions:

RESOLVED: Option 2- Use iterable
The full IRC log of that discussion
<fantasai> Topic: Arraylike
<TabAtkins> GitHub Topic: https://github.com/w3c/css-houdini-drafts/issues/239
<TabAtkins> https://docs.google.com/presentation/d/1pXoJ4vqRfjww7xJ8DYTYk8LBu0kGyGyg0yZm88w8xBw/edit?usp=sharing
<fantasai> TabAtkins: Couple interfaces in typed OM, e.g. CSSUnparsedValue, CSSTransformValue, that are just sequences of other values. They have other extra data, e.g. transform has an equivalent matrix
<fantasai> TabAtkins: Bat at their core,they are sequences of values. SO they really want to b arrays
<fantasai> TabAtkins: Really want to use array notations
<fantasai> TabAtkins: But making something arraylike, only way is to use Proxies, which are slow and not desired by impelmenters.
<fantasai> TabAtkins: annoying cus maps adn sets are easily faked
<fantasai> TabAtkins: WebIDL is also broken, can't even do iterbles without invoking Proxies
<fantasai> TabAtkins: WebIDL is doing something vey stupid for bad reasons. (I'm gonna be really judgemental here.)
<fantasai> TabAtkins: Wnated to go over these issues
<fantasai> SimonSapin: Can we fix WebIDl?
<fantasai> TabAtkins: First option is suck it up, use LegacyArrayClass. Invokes Proxy. Not great.
<fantasai> TabAtkins: It also means tha tany Array mentods that return an array, return a real array, not an instance of our arraylike.
<fantasai> till: That shouldn't be ture anymore with species
<fantasai> TabAtkins: That's option 3 :)
<fantasai> TabAtkins: I don't want ot do Option 1, but it is a possiblity
<fantasai> TabAtkins: Option 2 is Use iterable
<shane> Not an array, but can be turned into one: let arr = [...cssThing]
<shane> As written today, would still invoke Proxy; it’s written to require indexed properties. Totally fixable, just hasn’t been fixed yet in WebIDL.
<shane> (kv-iterator doesn’t act badly like this. It can be fixed!)
<shane> Probably good enough, at least as a first step.
<fantasai> TabAtkins: This lets you iterate in a for loop, or to cast things itno an array
<fantasai> TabAtkins: It's okay as a first step
<fantasai> TabAtkins: Fairly easy to eget to
<fantasai> TabAtkins: Problem is WebIDL value iterators, which is what we'd use here, still requires Proxy
<fantasai> TabAtkins: ....
<fantasai> TabAtkins: So WebIDL needs to be fixed
<fantasai> TabAtkins: This would be okay
<fantasai> TabAtkins: Coudl also hack around key-value iterator. Doesn't have the problems. But it's hack.
<fantasai> TabAtkins: This is a possible solution, particularly in ES6 casting is easy
<fantasai> TabAtkins: Option 3 is desirable one which is ES spec now has proper sublcassable arrays
<fantasai> TabAtkins: Similar to previous step, but it's a real Array
<fantasai> TabAtkins: Will also return the correct subclass from Array methods
<fantasai> ChrisL: Which version of ES spec did that come from, and can you compile this down to ES6?
<fantasai> till: It's ES6 and it is implemented in all engines
<fantasai> TabAtkins: Not fixed in WebIDL yet
<fantasai> TabAtkins: Only problem with this is tha tyou still can't intercept a set operation, like you can with maps
<fantasai> TabAtkins: So people can put random crap in your interface
<fantasai> TabAtkins: e.g. CSSUnparsedArray is suppoed to only have strings and ??
<fantasai> TabAtkins: Not sure what to do with that, do we throw when you add something that's invalid?
<fantasai> TabAtkins: Most of the time you can sanitize at  ...
<fantasai> leaverou: If the issue is that people can mess with your types, that's just how JS works
<fantasai> TabAtkins: In maplike, in calc(), because it has an impl of set method, can do type checking and tell you if you're doing somehting wrong right away
<fantasai> TabAtkins: If you're setting an aributary object to px key, will thorw, becaues not a number
<fantasai> TabAtkins: Doesn't look corect, and then later when you're setting into a property map, break down
<fantasai> leaverou: That's their own fault
<fantasai> TabAtkins: But we still have to handle corretness
<fantasai> TabAtkins: And we'd have to handle it via checking every set, iterating over it, even in cases where you haven't done anything wrong, which is more computation than is required.
<fantasai> TabAtkins: If you're strict at the start, then we don't have to do such extra checks
<fantasai> till: I think you fundamentally want Proxies ot implement that
<fantasai> ...
<TabAtkins> AWB had a proposal to let you hook the [] syntax, but that's never gone anywhere. :(
<fantasai> shane: At least with proposal 2 the only time we need to check correctness is on construction, and that is very ...
<fantasai> shane: We wouldn't be in stuation where every time we need to use contents we typecheck them
<fantasai> TabAtkins: So I think we should go with option 2 for now, and then in future, go into Array subclassing
<fantasai> Rossen: What would prevent us from going to Array subclassing in the future?
<fantasai> TabAtkins: Only if you're doing exotic checkes on whethe rit's an array or not
<fantasai> TabAtkins: If just iterating over it, won't notice the difference
<fantasai> till: The performance on Proxies might not be tha tmuch of a problem going forward
<fantasai> till: Engines are doing pretty good optmizations nowadays
<fantasai> till: If all you hve is a set hook, then should be optimizable
<fantasai> TabAtkins: I was told that's not the case
<fantasai> iank_: I was told we're not goint to b able to make Proxies fast anytime soon
<fantasai> till: in the general case, that's true, but htis isn't the general case
<fantasai> TabAtkins: If good news comes out, ok to change, but for now would go with option 2
<fantasai> Rossen: So you're proposing to go with option 2
<fantasai> Rossen: Any objections?
<fantasai> Rossen: Anyone for option 3 at this point?
<fantasai> RESOLVED: Option 2- Use iterable
<shane> https://docs.google.com/document/d/1NA020gku-tgEHMGK_8xkB6Ksfpd8ieL4z9jFoGchzbA/edit

@domenic
Copy link
Contributor

domenic commented Apr 18, 2017

One point that might have been missed in the above discussion is that anywhere you accept a sequence<sometype>, the type-checking is automatically done for you. So

we'd have to handle it via checking every set, iterating over it, even in cases where you haven't done anything wrong, which is more computation than is required.

Doesn't seem very accurate, because you do that anyway, I think.

@tabatkins
Copy link
Member

Right now we don't have sequence<> types tho - the context is things accepting a CSSStyleValue, which currently for all other types can just assume that the values are correct (because you can't write invalid values to them; they throw on setting).

sequence<> is just WebIDL-ese for convert-to-list<>, so yeah, a conversion/check happens.

@tobie
Copy link
Member

tobie commented Apr 18, 2017

Happy to bump this higher up in my priority list for WebIDL. Seems like fixing option 2 is a no brainer. Haven't thought about option 3 at all in terms of what it would imply for WebIDL. New syntax? Etc. Tracking this in whatwg/webidl#345.

Grouped these related issue in a dedicated project to see if that helps.

@domenic
Copy link
Contributor

domenic commented Apr 18, 2017

the context is things accepting a CSSStyleValue, which currently for all other types can just assume that the values are correct

That seems very developer-unfriendly. In general anything that accepts an iterable should accept a sequence<>. It's frustrating to developers if foo([x, y, z]) does not work, and they're forced to instead do foo(new CSSStyleValue([x, y, z])).

@tabatkins
Copy link
Member

The constructors take sequences, yes. I'm talking about the styleMap.set("css-property", someValue) method. Right now, that can depend on the fact that the information in the (private slots of) someValue is correct and doesn't need to be further checked. If someValue is an Array-like, and I can't intercept the sets, I can't depend on that. (I can intercept all attribute sets, via a setter method, and all Map-like sets, via the set method, so I can make such a guarantee for every single other object.)

@domenic
Copy link
Contributor

domenic commented Apr 19, 2017

I think it'd be very frustrating for authors if they couldn't do styleMap.set("css-property", [x, y, z]), but instead had to do styleMap.set("css-property", new CSSStyleValue([x, y, z])), and is a design smell.

@tabatkins
Copy link
Member

That is absolutely how we are designing these APIs, yes. It's rarely going to be a generic CSSStyleValue (those are opaque objects, for properties we don't yet expose a proper type for). It'll be something like styleMap.set("transform", new CSSTransformValue([CSS.translate(CSS.px(5)), CSS.rotate(CSS.deg(10))])).

@domenic
Copy link
Contributor

domenic commented Apr 19, 2017

Ok. Then we are so far outside the realm of good API design I am not really planning to spend more time on this.

@bzbarsky
Copy link

The fundamental problem people are attempting to solve here is to do something like:

styleMap.set("transform", "translate(5x) rotate(10deg)");

but without having to parse that string. That basically means the caller needs to provide an object representation of that thing on the right; more or less an AST of what you'd get from parsing the string.

In this particular case, it might be possible to elide the CSSTransformValue information (just like it's elided in the string syntax) if we know that the "transform" property takes a list of things. How that plays with other cases, I'm not sure.

@tabatkins
Copy link
Member

@domenic That's an extremely disappointing way to treat this, particularly since the thing you're talking about is a small aspect of one possible approach to solving this much larger overall problem with WebIDL. You do you tho.

@tabatkins
Copy link
Member

tabatkins commented Apr 19, 2017

So anyway, regardless of whether we decide to accept raw Arrays for things that are list-like (in addition to the bespoke objects), we still have the issue that, if the bespoke objects are Array-like but we can't intercept sets, we still need to verify the correctness of the data on every single set, whereas that's unnecessary for every single other object in the entire Typed OM.

(On that tangent - we do plan to accept doubles anytime we'd accept a CSS <number>, and DOMStrings anytime we'd accept a CSS keyword, so there's precedent supporting "accept an Array anytime we'd accept a list-valued property value".)

@tabatkins
Copy link
Member

tabatkins commented Apr 25, 2017

Sigh, new example of an Array-like in Typed OM (that I'm currently in the middle of speccing, so not visible yet in the spec): CSSMathValue. This replacement for CSSCalcValue represents a calc() as an expression tree, so calc(1 + 2 * 3) becomes CSSMathValue("+", 1, CSSMathValue("*", 2, 3)). It exposes operator and arguments attributes; the latter needs to be an Array-like that is limited to CSSNumericValue objects.

(Note that this is why we can't even depend on sequence<> to typecheck these array-likes at time-of-use; in this case, anything that ever uses a CSSMathValue will have to recursively typecheck its arguments values, because they can form an arbitrarily-deep tree of CSSMathValues that need to all be correct.)

@tabatkins
Copy link
Member

Okay, unblocking:

For now, these interfaces need to just use indexed getters/setters. This'll invoke more Proxy machinery than we need/want, but it'll give us the functionality we need for now, and is compatible with the functionality/syntax that a "proper" solution will have.

For later, I missed hitting the July TC39 meeting with the proposal for a limited proxy that just lets you intercept gets/sets on indexed properties, so I'll hit the September meeting instead. This proposal had good support when it was first mentioned back in 2009, and should be nice and cheap; once we get that thru, we can change the spec over to use that mechanism instead. There'll be no observable behavior change for authors (because that's the only part of the Proxy functionality we're using anyway), the objects should just get faster to manipulate (once browsers implement the new thing).

(Or rather, WebIDL will switch indexed getters/setters over to using this mechanism, instead of full Proxys.)

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

9 participants