CSS Layout API

[bfgeek]

Hello TAG!

I'm requesting a TAG review of:

• Name: CSS Layout API
• Specification URL: https://drafts.css-houdini.org/css-layout-api/
• Explainer, Requirements Doc, or Example code: https://github.com/w3c/css-houdini-drafts/blob/master/css-layout-api/EXPLAINER.md
• Tests: No tests yet, but will be upstreamed to wpt during initial development.
• Primary contacts: @bfgeek

Further details (optional):

• I have read and filled out the Self-Review Questionnare on Security and Privacy. The assessment is here.
• I have reviewed the TAG's API Design Principles

You should also know that...

This is an early review, so things might be difficult to understand and/or out of date in the specification and explainer. I'll ping this issue when large changes have been made / clarified, and when some initial code can be used in chromium.

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]

[torgo]

Discussed at London f2f. We will reach out to someone to have a dedicated call slot to discuss this before next f2f.

[dbaron]

We discussed at London in a breakout. Had a bit of discussion; I'm going to try to review in more detail prior to our February 13th call and then make suggestions as to whether we need further review.

[slightlyoff]

Stream of consciousness: It's sort of odd that there's a Fragment interface hierarchy, and something Fragment-ish is returned from layout methods...but it's not an actual Fragment and the Fragment types don't appear to have constructors. This seems like a place where the usual JS style would be to make Fragment constructable.

[travisleithead]

Took a pretty detailed look at this on the plane ride over :-).

Overall, I'm very excited to see this API--it really has the opportunity to be a game-changer.

Starting with some nits (mostly editorial):

• Missing "or" clause: "A LayoutChild represents either a CSS generated box before layout has occured."
• Extra 'the': "A LayoutConstraints object is passed into the layout method which represents the all the constraints for the current layout to perform layout inside."

Other thoughts (in sequential order as I read the spec top-to-bottom):

(-)
LayoutFragment's inlineSize and blockSize might be renamed to reflect (as noted in the spec) that they are relative to the border-box, e.g., inlineBorderBoxSize and blockBorderBoxSize…

(-)
Example 1 comes too early in a linear reading of the spec--using lots of things unexplained yet. A simpler or more focused example (or better yet: a picture illustration) would be much more helpful in understanding and far less distracting--especially when dumped into JavaScript code that uses generator functions in a class definition--sheesh!

Example 1: Why the yeild at that particular spot (right after executing the map operation? Is the native Layout engine going to know how to handle an array of IntrinsicSizeRequests?
(+)
Example 1: I don't believe I read what the shape of the objects are that may be returned or yeilded from the intrinsicSizes/layout generator functions? It should be made super-clear or obvious somewhere up-front what signature the author-defined generators are supposed to have (for input) and what their expected output is supposed to be. E.g., there is a lot of inconsistency in the doc right now about whether an IntrinsicSizeRequest, or LayoutFragmentRequest is supposed to be returned, or something from section 5.6 (FragmentResultOption/IntrinsicSizeResultOption -- only at the end did I finally get it). Postscript: these intermediate results appear to be converted to a list if not a list already in the run a generator API, but you have to read the algorithm very carefully to figure this out. These type of I/O boundaries should be clearly spelled out [outside of an algorithm].
(+)
In create a layout fragment algorithm, what happens if the StructuredDeserialize operation fails? Is is the "otherwise null" result, or something else? Larger question: can Layout throw? is there a "global" error handler for a WorkletGlobalScope? What happens in the Layout Engine if there is a problem--does it keep trying over and over? Assume 'yes' but this is not written down.
(+)
IntrinsicSizes object has readonly attributes. This is an interface (not a dictionary). The interface has an internal slot for the request that generated it. If the developer stashes an IntrinsicSizes object in the WorkerGlobalScope, and reviews it later--in a follow-up layout pass, are the values the same, or has the UA re-purposed the object to reflect different values? Should this be a dictionary instead where the values are 'readonly' by virtue of not being data-bound to any internal getters?
(+)
The Note about "The parent layout should expect this [layout fragments that exceed the layout available size] to occur and deal with it appropriately." Nice tease! Please eventually link this to an example for how to do this!
(+)
layoutNextFragment takes a LayoutConstraints IDL object in all cases--however, in many of the examples, you pass a LayoutConstraintsOptions dictionary into the method--this is not allowed per the IDL--you'll either want to new-up a LayoutConstraints object wrapped around your dictionary, or just change the API to take the dictionary instead for simplicity. This might be an early opportunity to throw if the wrong data-types would be passed back.

Nit & open question: "The LayoutConstraints object has percentageInlineSize and percentageBlockSize attributes. These represent the size that a layout percentages should be resolved against while performing layout." The "a" is weird in the sentence structure, and I'm lost as to the meaning of how "layout percentages should be resolved against" said number means? A follow-up example might help?

Will LayoutEdgeSizes match the CSS Typed OM for the varous box properties like margin, padding?

This sentence could really use a picture! "The LayoutEdgeSizes object represents the width in CSS pixels of an edge in each of the abstract dimensions (inlineStart, inlineEnd, blockStart, blockEnd)."

invalidate layout functions algorithm appears to re-acquire the values of input properties and chld input properties each time an invalidation happens--is that intensional? I wouldn't expect these values to be changing all the time, wouldn't it make more sense to cache them internally and re-use? (Seems more similar to a customElements.define() one-time setup to me...

5.3 -- the CSS object is now a namespace (rather than an interface) I believe...

On Generators--this model relies on the benevolent developer to call yield in order to achive the layout parallelism/asynchronicity desired. It doesn't seem quite right--the break-points should be more deterministic and based on API calls--hense Promises would seem better suited to the task. But putting that aside, the author cannot expect to write a generator function without a crisp understanding of how that generator function will be used (e.g., how the code that uses the generator's iterator will use it, when it will be expecting intermediate results (if any), and with what parameters to next() the generator will be resumed with (if any). The spec doesn't contain much in the way of explaining these details, which makes it really hard to know where to put yield (if at all) in the generator function definitions...

Example 6 (the JavaScript Layout engine) needs to come much earlier in the spec! It helps frame what is going on for the other examples.

Example 6, layoutFragment method tries to get the fragmentRequest.layoutChild but that doesn't exist (I think it's supposed to be box instead).

Example 6, the layout API is invoked with arguments in the wrong positions, per the previous definition (e.g., constraints is not the first argument)

The spec needs to be much crisper in defining what the layout generator function's expected parameter will be, e.g., make is super-clear that the first parameter to the generator is a sequence<LayoutChild>, etc. Not sure if there's a way to formally spec this (e.g., the WebIDL callback syntax is for regular functions, not generator functions, but maybe it would work?) Note: this is mentioned only once just above the definition for the layoutNextFragment algorithm.

The paragraph about LayoutFragmentRequest objects notes that if they are yielded, then the layout engine will [eventually] produce a fragment for it (returned to the generator). What happens if the generator yields something else? Does it throw? How is the result of the Frament request resolve later? (E.g., this is a job for a Promise...)

In reflecting about the recursive nature of how a given pass through the layout generator function can recursively call layoutNextFragment on it's children, it seems to me that layoutNextFragment should really be called layout instead; this would appear to match intrinsicSizes() which is already named the same as the generator. However, I understand one reason for the name mismatch is because you don't want to imply that the return value from such a call would be an iterator (which is what the generator gives to the actual Layout Engine code). This is just further evidence to me that the Generator model is not what is desired, but a Promise-base model.

In 5.6, why does the FragmentResultOptions have a place to return childFragments? Is this an optimization? Surely the browser will already know about any child fragments from previous calls to layoutNextFragment already made in the function (where a fragment was already returned), or will be calculated later in a subsequent call to layout by the Layout Engine?

[torgo]

We will collaborate on what issues to open up relating to this feedback.

[torgo]

@travisleithead to file substantive parts of his comment as issues on the spec based on discussion at tokyo f2f.

[tabatkins]

Issue brought up in the CSSWG meeting:

Using a generator here as a poor-man's async is bad. It's not actually an iterator; while layout() can be called multiple times, that's not done via the iterator-ness of the function. Instead, the iterator-ness is used solely to allow the engine to pause and do the children's layout asynchronously - in childFragment = yield child.layoutNextFragment(childConstraints);, the lNF() call doesn't return a fragment, it creates a LayoutFragmentRequest, which is yielded out to the engine, and then the completed fragment is pushed back into the function as the return value of the yield expression.

Semantically, this should be an async function, with lNF() returning a promise for a completed fragment, and await used to pause the function and pull out the completed fragment when it fulfills. Using iterators to achieve this semantic is how some userland libraries created async functions in the past, but it was always a somewhat-confusing hack; we shouldn't reproduce this hack in browser-provided APIs when we have the appropriate primitive available.

[dbaron]

The generator vs. promise discussion at CSS is recorded in w3c/css-houdini-drafts#750.

[travisleithead]

Looks like this was reviewed recently and good progress is being made. We don't have any additional feedback at this time, but would love to take a second look when further spec revisions have landed. Please keep us in the loop, and thanks for flying TAG!