Re: [w3ctag/design-reviews] CSS Layout API (#224)

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 'this': "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.

* 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`?

`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.

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?


-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/w3ctag/design-reviews/issues/224#issuecomment-378824908