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

TAG review for CSS Typed OM #223

Closed
1 of 3 tasks
nainar opened this issue Jan 8, 2018 · 22 comments
Closed
1 of 3 tasks

TAG review for CSS Typed OM #223

nainar opened this issue Jan 8, 2018 · 22 comments
Assignees
Labels
Progress: pending external feedback The TAG is waiting on response to comments/questions asked by the TAG during the review

Comments

@nainar
Copy link

nainar commented Jan 8, 2018

Hello TAG!

I'm requesting a TAG review of:

We'd prefer the TAG provide feedback as (please select one):

  • open issues in our Github repo for each point of feedback
  • open a single issue in our Github repo for the entire review
  • leave review feedback as a comment in this issue and @-notify [github usernames]
@dbaron
Copy link
Member

dbaron commented Jan 26, 2018

I've started reading through the spec and filing some issues; I haven't gotten that far yet, though, but I plan to continue.

@dbaron
Copy link
Member

dbaron commented Jan 28, 2018

I'd also note that some of the open issues already pointed out in the spec are reasonably serious, such as pointing out that parts of the spec aren't written yet. That's a bit of an obstacle to getting interoperable implementations. Hopefully these will get sorted out, though.

@darrnshn
Copy link

Thanks for starting a review! Yes, we are hoping to resolve these issues by the end of this week.

@tabatkins
Copy link

(In particular, I'm in Sydney for the week specifically to burn thru the issues list.)

@foolip
Copy link

foolip commented Jan 30, 2018

FYI, there's a Intent to Ship: CSS Typed OM on blink-dev which links here.

@dbaron, do you expect to file more issues? (I see issues from 2 and 4 days ago, not clear if the latest was the last.)

@dbaron
Copy link
Member

dbaron commented Jan 30, 2018

I'm hoping to file more (or at least, hoping to read more of the spec), perhaps this afternoon. It's a pretty big spec!

@foolip
Copy link

foolip commented Jan 30, 2018

Sound good, thanks!

@dbaron dbaron self-assigned this Jan 31, 2018
@torgo torgo added this to the tag-f2f-london-2018-01-31 milestone Jan 31, 2018
@torgo
Copy link
Member

torgo commented Jan 31, 2018

Taken up at London F2F.

@slightlyoff
Copy link
Member

Per conversation at F2F, the motivation and design tradeoffs need to be documented. An Explainer or use-cases doc would resolve.

@annevk
Copy link
Member

annevk commented Jan 31, 2018

Note that whatwg/webidl#345 is worth reviewing here too. In particular the introduction of new objects that require a JavaScript Proxy to implement. As far as I know the last time we discussed this with TC39 they were still against that (and definitely far from consensus).

@nainar
Copy link
Author

nainar commented Feb 2, 2018

@slightlyoff - I have a PR for an explainer out here, in case you want to take a look.

Sorry it wasn't part of the original design review.

@foolip I will update the intent to ship thread once this is committed.

@slightlyoff
Copy link
Member

Thank you for the explainer!

@dbaron
Copy link
Member

dbaron commented Feb 2, 2018

@annevk, is #223 (comment) addressed at all by this change? If not, which objects are the relevant ones?

@slightlyoff
Copy link
Member

We're starting to review at the London F2F; some stream-of-consciousness questions:

  • Is CSSStyleValue really meant to be available in all Worker types? E.g., does it make sense in Service Workers?
  • Glad to see constructors on the concrete types!
  • Loving the static methods for shorthands (e.g. CSS.px()).
  • Don't understand how Example 7 works: it looks like the example should work with styleMap, but instead it uses attributeStyleMap, which doesn't seem like it should have values for 'object-position'
  • Should computedStyleMap() be synchronous? It seems to flush style on read...is that right? And does it need to be a function?
  • ...that gets us to a perhaps bigger question: should the OM be trying to help ensure performant read-back and separation of writes/reads? If this is a chance to go that way, it won't come around again. Has it been discussed?
  • to @annevk's point, it looks like CSSNumericArray is a duck-type now. 🤷
  • @plinss points out that [[tokens]] is a list of strings and not a list of actual tokens. Another way to look at this is that the tokenizer isn't exposed. Should it be?
  • Are changes to CSSVariableReferenceValues observable without polling?
  • Are there concrete performance numbers we can point to that show the benefits of the Typed OM approach?

Thanks!

@annevk
Copy link
Member

annevk commented Feb 2, 2018

@dbaron all objects that use getter/setter: CSSUnparsedValue, CSSNumericArray, and CSSTransformValue. See also whatwg/webidl#100 about more clearly marking getter/setter as legacy in IDL.

@slightlyoff
Copy link
Member

@annevk: this doesn't appear to require an proxy in implementations. Here's the Chrome code (auto-gen'd binding output), e.g.

@plinss plinss added Progress: pending external feedback The TAG is waiting on response to comments/questions asked by the TAG during the review and removed extra time labels Feb 2, 2018
@annevk
Copy link
Member

annevk commented Feb 2, 2018

@slightlyoff implementations have shortcuts, sure. This is assuming we'd use actual JavaScript to represent these objects, as per the extensible web principles.

@annevk
Copy link
Member

annevk commented Feb 2, 2018

Also, my objection is not that they require proxies were they to be implemented in JavaScript, my objection is that we've been explicitly asked to avoid introducing any such objects going forward by TC39.

@nainar
Copy link
Author

nainar commented Feb 5, 2018

@slightlyoff in response to your questions:

We're starting to review at the London F2F; some stream-of-consciousness questions:
Is CSSStyleValue really meant to be available in all Worker types? E.g., does it make sense in Service Workers?

Nope, it should be exposed to the Paint API only. Filing an issue on typed om: w3c/css-houdini-drafts#632

Don't understand how Example 7 works: it looks like the example should work with styleMap, but instead it uses attributeStyleMap, which doesn't seem like it should have values for 'object-position'

The example seems incorrect - it should be checking computedStyleMap(). Filed an issue here: w3c/css-houdini-drafts#633

Should computedStyleMap() be synchronous? It seems to flush style on read...is that right?

IIUC: when you read values of computedStyleMap() it causes a style recalc, so yes it is synchronous.

And does it need to be a function?

The CSSWG resolved to have it be a function, IIRC. For reasons:

  • To mirror getComputedStyle()
  • To reflect that it will return a new StylePropertyMap each time.
    @tabatkins

...that gets us to a perhaps bigger question: should the OM be trying to help ensure performant read-back and separation of writes/reads? If this is a chance to go that way, it won't come around again. Has it been discussed?

I don't understand the question here. Could you elaborate?

@plinss points out that [[tokens]] is a list of strings and not a list of actual tokens. Another way to look at this is that the tokenizer isn't exposed. Should it be?

Exposing a tokenizer was delayed to a Level 2 spec. Context here: w3c/css-houdini-drafts#193

Are changes to CSSVariableReferenceValues observable without polling?

IIUC. No, the CSSVariableReferenceValues objects aren't live, you have to get the new value again, if changed.

Are there concrete performance numbers we can point to that show the benefits of the Typed OM approach?

Since there is a lot of discussion about this, I am filing an issue to track the discussion: w3c/css-houdini-drafts#634

Thanks!

@RByers
Copy link

RByers commented Feb 14, 2018

Also, my objection is not that they require proxies were they to be implemented in JavaScript, my objection is that we've been explicitly asked to avoid introducing any such objects going forward by TC39.

@annevk can you link to any reference documenting the level of consensus on that? Is there a recommended alternative way already defined to achieve a similar design to existing web platform APIs (like TouchList)? Without there being a viable alternative ready today, I don't think I can reasonably deny requests to ship additional such APIs in blink.

@torgo
Copy link
Member

torgo commented Jul 25, 2018

@dbaron
Copy link
Member

dbaron commented Aug 14, 2018

I'm still concerned about seeing progress in w3c/css-houdini-drafts#718, but we're going to close this issue at this point because we don't feel we need to cycle back to it for further TAG discussion. Thanks for requesting TAG review.

@dbaron dbaron closed this as completed Aug 14, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Progress: pending external feedback The TAG is waiting on response to comments/questions asked by the TAG during the review
Projects
None yet
Development

No branches or pull requests

10 participants