[css-layout-api] Assorted TAG feedback

[travisleithead]

This is a summary of the TAG Review comments for the CSS Layout API.

The major issue of feedback is already covered by #750 (where we asked why a Promise-style design was not used).

Constructors for Fragments

We generally try to enable constructors for objects returned by the platform (like Events) to enable JS authors to synthesize the same behavior. As such, we were wondering why there's no way to construct the Fragment types returned?

Expectations for thrown exceptions

In create a layout fragment algorithm, what happens if the StructuredDeserialize operation fails? Is it the "otherwise null" result, or something else? Larger question: what is the expectation if the script powering the layout function throws? Does it keep trying over and over? Or does is abandon the script and stop executing it?

IntrinsicSizes object as a dictionary type

Is the IntrinsicSizes object reused across layout passes? If the developer caches an IntrinsicSizes object and retrieves it in a later layout pass, will it have the same data? If not, should this be a dictionary structure instead, as it appears to be purely for containing data w/no operations...

layoutNextFragment -> layout?

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 function required at registration. If layout is too general, then maybe layoutFragment?

IDL/usage bugs

• 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.
• Example 6, the 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 definition (e.g., constraints is not the first argument).

Readability

In general, this is a complex spec--there's no way around that. IMHO, the conceptual contents of section 5 (Layout) including some much-needed diagrams, would be a better lead-in to the spec than diving into section 3 right away.

[dbaron]

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 function required at registration. If layout is too general, then maybe layoutFragment?

I think there is a distinction here worth emphasizing, though, since intrinsic sizes are a layout function of all fragments, and layoutNextFragment is a request to lay out just one fragment.

[css-meeting-bot]

The Working Group just discussed TAG feedback, and agreed to the following:

• RESOLVED: No change in behavior, add a note as to why constructors are not exposed
• RESOLVED: Add a new LayoutFragmentResult object with a constructor.
• RESOLVED: Keep the layoutNextFragment name.

The full IRC log of that discussion

<heycam> Topic: TAG feedback
<heycam> github: https://github.com//issues/760
<heycam> iank_: this is a bunch of assorted TAG feedback
<heycam> ... a lot of editorial stuff, but 3 or 4 issues we can talk about
<heycam> ... the first one is having a constructor for layout fragments
<heycam> ... in summary, currently today the only thing that can produce a layout fragment is the rendering engine
<heycam> ... call on the child, hey lay out this fragment, it does some work, and gives back a fragment
<heycam> ... the useful information is the width and height of the fragment, and you can return it in your list of children
<heycam> ... do we want a constructor for this?
<heycam> ... we can add this, I'm not objecting, but the constraint is that this isn't a valid fragment you could return to the engine
<heycam> ... just a placeholder for testing
<heycam> Rossen: the current design has a farily rigid assumption of who can produce fragments
<heycam> ... and be responsible for their lifteime
<heycam> ... and it's not the script, it's the layout engine
<heycam> ... exposing a constructor, you are fooling the user that they have control over it
<heycam> ... so I would push back on this issue with that reasoning
<heycam> eae: unless it's a valid fragment, it would be more confusing than it's worth
<heycam> Rossen: again, it would lead down the path of considering those as user space objects when they're actually browser objects
<heycam> dbaron: what would it mean to have a real constructor for a fragment?
<heycam> iank_: what Travis brings up here is events in the platform
<heycam> ... synthesizing an event through the system
<heycam> ... this is quite often used for testing
<heycam> ... it's not a real event
<heycam> ... they're already second class citizens
<heycam> ... for us, I'm struggling to come up with use cases for synthesizing events like that
<heycam> Rossen: again, the fragment projection we give back to the user is just an interace with a bunch of functionality to interrogate properties
<heycam> ... ti's meant to be a black box that you can move around and that's it
<heycam> iank_: the things you can read from it are teh width, height, baseline info (in the future0, and basically just position it
<heycam> ... if the child is also a custom layout it can pass information up via structured cloning
<heycam> ... but that's basically it
<heycam> iank_: dbaron or plinss anythign to add?
<heycam> plinss: that's reasonable
<heycam> Rossen: probably something that they didn't grok entirely, and they look similar on the surface but they're not
<heycam> plinss: in general the TAG likes to see constructors on everything
<heycam> ... but I agree here
<heycam> astearns: would it make sense to add a note saying usually we have constructors but not here?
<heycam> Rossen: if it's lacking explanation let's add that
<heycam> RESOLVED: No change in behavior, add a note as to why constructors are not exposed
<heycam> iank_: the second point, is that we return an options bag from layout
<heycam> ... which does some magic
<heycam> ... explicitly you return the child fragments, auto block size, some other info
<heycam> ... the engine goes and synthesizes a fragment fro you with correct internal representation
<heycam> ... shoudl that operation have a constructor, so you can create a new layout result fragment
<heycam> ... and check if that operation fails
<heycam> ... there are some cases where that operation could fail
<heycam> s/shoudl/should/
<heycam> ... for example, you pass it auto block size, create a new result fragment, atm it's just an options bag
<heycam> ... and then inspect what your output box size is going to be, maybe do something different based on that
<heycam> ... the other thing that can fail is when you deserialize the extra data you can pass up the tree
<heycam> ... when you try to structured clone that, like adding a SharedArrayBuffer or something
<heycam> ... currently we just say it threw an exception
<heycam> ... with a constructor the user could detect that it threw an exception
<heycam> ... I think it's reasonable, but would need to work out what the constructor's option bag argument would be
<heycam> ... it's not entirely clear to me yet
<heycam> frremy: when I tried it, something I found frustrating is you can have a big option you want to give, only a few props on the object can't be serialized
<heycam> ... then you have to clone the entire object yourself, by removing the stuff you can't serialize
<heycam> ... maybe would be nicer to set null for props you can't serialize for example
<heycam> iank_: I think that's a bigger ask
<heycam> ... structured clone is painful for people to use
<heycam> ... would that have been useful? there's a bit out of your control
<heycam> ... would it be useful to ensure that operation doesn't fail?
<heycam> frremy: something like JSON.stringify, with a callback to handle the unserializable stuff
<heycam> iank_: that would be a slightly larger change in the HTML spec
<heycam> frremy: I don't think having a constructor would change anything
<heycam> ... only solution is to clone
<heycam> iank_: would it have been useful to just get say the auto block size?
<heycam> frremy: not sure
<heycam> iank_: I think that's a reasonable ask, shouldn't be too much work, not sure many people would use it
<heycam> ... can always add it and use count it, remove if it's unused
<heycam> iank_: we need to add a constructor, not jsut returning a FragmentResultOptions, but a constructor for a type like FragmentResult
<heycam> ... then run all the steps for userland layout, structure clone, sanitize the output
<heycam> ... by running the constructor you could catch an error that would happen during that process
<heycam> Rossen: as to the larger question, about expectations of what happens if the script starts throwing?
<heycam> iank_: I think we just fall back
<heycam> ... I think that larger question is just a misunderstanding, editorial
<heycam> Rossen: objections to adding the new LayoutFragmentResult object?
<heycam> RESOLVED: Add a new LayoutFragmentResult object with a constructor.
<heycam> iank_: final piece of discussable feedback here
<heycam> ... confusion about layoutNextFragment name
<heycam> ... do we want to go back to layout?
<heycam> Rossen: no
<heycam> ... that's like call everything object
<heycam> iank_: David or Peter?
<heycam> dbaron: I already disagreed with Travis on that one
<heycam> frremy: I like that "next fragment" knowing that you're getting a fragment
<heycam> ... "layout" is too general
<heycam> iank_: Rossen and David want layout
<heycam> plinss: I tend to prefer verbosity
<heycam> iank_: Ian and Francois don't care
<heycam> Rossen: other opinions or objections?
<heycam> RESOLVED: Keep the layoutNextFragment name.

[css-meeting-bot]

The Houdini Task Force just discussed layoutNextFragment.

The full IRC log of that discussion

<iank_> topic: layoutNextFragment
<emilio> github: https://github.com//issues/760
<emilio> iank_: so we previously resolved to keep layoutNextFragment, but the more I use it it feels very long
<emilio> iank_: so I want to rename to layout
<emilio> iank_: so this LayoutChild.layoutNextFragment will become just layout, which also matches the callback name
<emilio> dbaron: I guess I think short names are good. I'm also a little hesitant about making people less aware of fragmentation. But I suppose that in many cases you don't fragment, so as long as when you do what happens is documented it's ok
<emilio> Rossen: initially in the early stages of houdini we thought that this was mostly for middleware / framework authors and they need to understand it and that's why we chose to be consistent with layoutNextFragment
<emilio> Rossen: I'm leaning about keeping it, but I won't object to renaming it
<emilio> Rossen: so you haven't made any of the changes yet
<emilio> iank_: nope, one other option is to rename this to layoutFragment and rename the callback to layoutFragment, which is a bit clearer and shorter
<emilio> Rossen: and layoutFragment will be important when you start doing inline layout
<emilio> Rossen: so you wanted to revisit it based on feedback you got, you haven't made the changes yet, so the options is renaming layoutNextFragment to layout, or changing both the callback and the idl to layoutFragment
<emilio> Rossen: thughts?
<emilio> surma: I lean to have the same name for both
<bkardell_> q+
<emilio> Rossen: oh sure, if we don't communicate that fragmentation can and will happen people won't care
<bkardell_> ?+
<emilio> iank_: third proposal is layoutNextFragment for both of them
<emilio> heycam: I'm not actually super-sure about the symmetry since it seems like calling into layoutFragment to your child would call directly into the callback, but that's not what it's happening right?
<emilio> iank_: right, though we have that symmetry for inline sizing
<Rossen> q?
<Rossen> q?
<emilio> cbiesinger: I think I really like layoutNextFragment, it's clearer
<Rossen> q?
<fantasai> s/clearer/clearer that there might be more than one/
<fantasai> +1 to cbiesinger
<Rossen> q?
<cbiesinger> s/inline sizing/intrinsic sizing/
<emilio> Rossen: I think first choice... do we want to insist for symmetry and keeping the same name? Since it's not really the same, so there needs not to be a symmetry between the two, which then leaves the question of just renaming layoutNextFragment
<emilio> smfr: layoutFragment is slightly ambiguous because I maybe interpret it as a name
<bkardell_> if there is not symmetry here, for reasons - I think we need to be careful to follow that example in all the places
<emilio> smfr: where is the magic layout function used?
<emilio> iank_: it's on registerLayout
<emilio> iank_: there's a bug in WebIDL to handle classes instead of https://drafts.css-houdini.org/css-layout-api/#dom-layoutworkletglobalscope-registerlayout
<emilio> chrishtr: so if there are two fragments you may get called twice?
<emilio> iank_: yeah, if you return a break token you get called multiple times
<emilio> chrishtr: so layout() calls layoutNextFragment and that calls layout() potentially multiple times on the child right?
<emilio> chrishtr: so that example is not really exercising it right?
<emilio> iank_: yeah the line-by-line example exercises it
<emilio> iank_: it's calling layoutNextFragment on the child multiple time until it's done
<emilio> chrishtr: I think layoutNextFragment makes more sense since it indicates an iterator pattern
<emilio> Rossen: that was the initial intent yeah
<emilio> iank_: first question is should the callback have the same name
<emilio> surma: layout doesn't return fragments right?
<emilio> iank_: yeah it does
<emilio> Rossen: layout() is called once for each fragment
<emilio> chrishtr: I'd keep the `Next`
<emilio> Rossen: so, keeping layoutNextFragment and renaming layout() to be either layoutFragment or layoutNextFragment?
<dbaron> I'm leaning towards the idea that it's better for the names to be different so that it's clearer that one isn't calling the other, though I don't feel that strongly right now...
<emilio> Rossen: can we live with layoutFragment? That'd make TabAtkins a little bit happier?
<emilio> TabAtkins: a third less happy
<bkardell_> I think layoutFragment/layoutNextFragment is less confusing actually
<emilio> surma: I think the symmetry helps it to be less confusing
<emilio> plinss: having the same name I think it's more confusing
<emilio> plinss: seems like it's calling the function directly instead of going through the engine
<bkardell_> +1 to what plinss said actually
<emilio> TabAtkins: you can think a bit over it and ping us on a call or something
<gregwhitworth> I agree with bkardell_ and plinss (layoutFragment and layoutNextFragment)
<emilio> Rossen: so no resolution?
<emilio> Rossen: what else is keeping the module from going forward?
<emilio> iank_: the precision stuff
<iank_> https://github.com//issues/796