cashapp / redwood

Multiplatform reactive UI for Android, iOS, and web using Kotlin and Jetpack Compose
https://cashapp.github.io/redwood/0.x/docs/
Apache License 2.0
1.67k stars 73 forks source link

iOS UIKit needs to propagate size changes up the DOM #1152

Closed colinrtwhite closed 1 year ago

colinrtwhite commented 1 year ago

On Android changing a property that changes the size of a view will re-layout all its parents in the view tree automatically. On iOS UIKit this is not the case! We need to manually notify that parent (and parent's parent, etc.) that the child view has changed size.

Currently we hack around this in a few places by doing something like:

func text(text: String) {
    root.text = text
    root.superview?.setNeedsLayout()
}

However, this isn't foolproof since it only re-lays out the parent and not the parent's parent, which also might have changed size due to its child's child changing its text.

How do we solve this? I think we have two main options:

Automatic

We already handle changes similar to this through the Widget.Children.onModifierUpdated method, however this is only called when a child's modifier is updated. I propose we generalize it to:

public fun change(index: Int, property: String)

This would be called for all property changes and would pass the property name so Android (and similar platforms) can skip any change where property != "modifier".

Manual

Optionally, we could have the child widget notify its parent that its size should be recomputed. This has the benefit of the child not having to recompute its size for every property change. For example, the parent doesn't have to recompute the child's size when its text colour changes. The downside to this solution is it's manual and could be error-prone.

JakeWharton commented 1 year ago

So as far as I can tell we should not have to do this manually. I've been reading and watching WWDC videos on how iOS layout works since you posted this issue, and all signs point to this being able to be made to work using the built-in mechanisms. I also chatted with @efirestone about it who also believes that it should be something else causing the incorrect behavior.

From your example,

func text(text: String) {
    root.text = text
    root.superview?.setNeedsLayout()
}

I would say two things:

  1. Whatever type is root seems broken. Writes to its text property should automatically be invoking setNeedsLayout() on its parent, no? Controls like UILabel do this.
  2. If for some reason there's a really good justification for point 1 to NOT be implemented that way, having the widget wrapper perform the action is an okay solution.

However, this isn't foolproof since it only re-lays out the parent and not the parent's parent, which also might have changed size due to its child's child changing its text.

This also seems extremely suspect to me. That would mean in a traditional iOS application using UIKit changes to any property affecting layout on any view would have to manually traverse the ancestral chain calling setNeedsLayout on each.

Is there a reproducer of the problematic behavior in the samples that I can debug? Or is there one somewhere else internally?

JakeWharton commented 1 year ago

Looking at UIViewChildren,

https://github.com/cashapp/redwood/blob/546aa52bd2d07126d4febd57d084e569f02dde07/redwood-widget/src/iosMain/kotlin/app/cash/redwood/widget/UIViewChildren.kt#L69-L72

This seems incorrect. setNeedsDisplay is requesting a draw pass, not a layout pass. And, to the best of my understanding, invalidateIntrinsicContentSize invalidates constraints but does not request a layout pass itself (plus we don't actually use the constraints system).

Looking at UIViewFlexContainer,

https://github.com/cashapp/redwood/blob/546aa52bd2d07126d4febd57d084e569f02dde07/redwood-layout-uiview/src/commonMain/kotlin/app/cash/redwood/layout/uiview/UIViewFlexContainer.kt#L124-L126

We're probably missing a call to super here to propagate the desire for layout into the UI toolkit itself.

JakeWharton commented 1 year ago

Going to close since I think we've seen that this can be accomplished through the toolkit directly and does not require such a mechanism as proposed. We just need to ensure we're doing this properly, and probably need more tests the the like. There may be more we need to do, but if we have problematic patterns we need to figure out how to get them into the Redwood repo as a sample or unit test so that we can be sure to not regress them.