ASInsetLayoutSpec inside Horizontal stack layout breaks line break of text nodes

[smyrgl]

It took me a while to nail down what was going on with this issue but I'll try to cover all the behaviors. I have seen this with a few components I have created but I will cover the simplest case.

This node is designed as a Horizontal stack layout with an ASDisplayNode and an ASTextNode in the horizontal stack. The layout spec looks like this:

    override func layoutSpecThatFits(constrainedSize: ASSizeRange) -> ASLayoutSpec! {
        textNode.flexShrink = true
        let insetSpec = ASInsetLayoutSpec(insets: BlockquoteInsets, child: textNode)
        sidebarNode.flexBasis = ASRelativeDimensionMakeWithPoints(2.0)
        return ASStackLayoutSpec(direction: .Horizontal,
            spacing: 0.0,
            justifyContent: .Center,
            alignItems: .Stretch,
            children: [sidebarNode, insetSpec])
    }

That produces this behavior:

Notice the lack of line breaking. It is like flexShrink isn't occurring. Looking at it in Reveal the container node that holds the textNode and owns the layout spec is sized fine but the text node itself is way oversized beyond the node bounds.

A few things to note about this:

1. This issue ALSO occurs with ASButtonNode which has a similar layout spec.
2. This is also occurs if you replace the ASTextNode with a wrapped UILabel ASDisplayNode.

Now here's what happens when I remove the inset. Here's the new layout spec:

    override func layoutSpecThatFits(constrainedSize: ASSizeRange) -> ASLayoutSpec! {
        textNode.spacingBefore = BlockquoteInsets.left
        textNode.spacingAfter = BlockquoteInsets.right
        textNode.flexShrink = true
        sidebarNode.flexBasis = ASRelativeDimensionMakeWithPoints(2.0)
        return ASStackLayoutSpec(direction: .Horizontal,
            spacing: 0.0,
            justifyContent: .Center,
            alignItems: .Stretch,
            children: [sidebarNode, textNode])
    }

Here's what that looks like:

You should be able to easily replicate this as it seems that any Horizontal stack spec that includes an inset spec as one of its children is going to have this problem.

[appleguy]

@smyrgl Thanks for the report. This is a known issue, I believe you simply need to set .flexShrink = YES on the Inset spec. I agree this is NOT intuitive, although note that it is the same behavior as ComponentKit.

@rcancro and @nguyenhuy have discussed this with me and we are trying to figure out the right behavior. Because the layout system is so configurable, there are pitfalls to most changes; for example I have a PR up to set flexShrink on by default for ASTextNode, because it seems more intuitive, but I haven't landed it yet as it does have a couple tradeoffs.

This isolated case may be easier to fix if we decide that the Inset should inherit flexGrow or flexShrink from its child. However, another case we've considered is when you have a vertical stack and every item (say they are ASTextNodes) has flexShrink set. If we did this for Inset and not for Stack, it could be confusing; but if we did it for both, there are other tradeoffs and it still wouldn't apply to custom specs.

Anyway, we'd be thrilled if you have any thoughts (read: value judgements - what seems like the "right" behavior for these subjective decisions?) to share with us. In fact, this specific issue and a small handful of related ones are the primary reason that we are at 1.9.x and not 2.0 yet. Let's make this the most intuitive implementation of Box Model around and get it shipped!

[rcancro]

Specifically with ASInsetLayoutSpec and ASRatioLayoutSpec, I've wondered if we should copy all of the child's options to the container spec by default.

For right now, I'd recommend using copyFromOptions: in ASLayoutOptions to make sure all the properties get propagated up:

let insetSpec = ASInsetLayoutSpec(insets: BlockquoteInsets, child: textNode)
insetSpec.layoutOptions.copyFromOptions(textNode.layoutOptions)

(I have no idea if that is valid Swift lol)

[smyrgl]

I think the copying makes sense since it is very easy to nest a ratio or inset spec in a stack layout and to do that as default behavior. However if it is decided not to be the default then I recommend updating the documentation to make it very explicit what needs to be done.

[smyrgl]

Oh and @appleguy thanks a lot for the pointer...I applied the change and everything was fixed instantly! It probably makes sense to keep this issue open for tracking until a decision on inset specs inheriting the flex rules from their child is reached (your call) but I do want to thank you for your help as this problem has been haunting me for a few weeks and it took me forever to isolate it enough that I could open a ticket on it. This was the only thing standing in my way of converting my whole project to the new layout system and now that this is done I've only got about 20% of my nodes left that need conversion.

[nguyenhuy]

While I believe that the documentation must better communicate that stack-related properties (flexShrink, flexGrow, flexBasis, etc) are only considered when set to immediate children of stack specs, I think the copying behaviour might be good idea and make the layout system more robust. But I agree with @appleguy that it would be confusing if we implemented flexShrink inheritance for some specs (say, inset and ratio specs) and not others. We need to consider the tradeoffs carefully.

I'm also thinking that maybe all layoutable objects should have flexShrink enabled by default. It actually is consistent with the Flexbox spec and might even improve the experience of new users. But it will be a big breaking change :(

[Adlai-Holler]

The immediate issue here is resolved – let's continue discussion about the underlying system at the linked issue. Cheers!