feat: flexible SwiftUI preferredContentSize calc [UI-7665] #320

watt · 2025-01-28T22:02:09Z

This is a possible solution for preferredContentSize of SwiftUI content, for discussion.

A summary of the problem:

The system provided PCS calculation is unconstrained and not useful to us. We need to calculate this manually.
SwiftUI ScrollViews greedily fill the space available, but will report the natural content size when given an infinite size.

This solution finds an appropriate PCS for the general case by measuring in a few different constraints and combining the results. This accommodates content with no scroll view, as well as content with scroll views in either axis or both.

~~If we like this I'll follow up with tests that validate PCS in various permutations of size and scrolling axes.~~ tests added!

Checklist

Unit Tests
UI Tests
Snapshot Tests (iOS only)
I have made corresponding changes to the documentation

kyleve · 2025-01-28T22:49:28Z

WorkflowSwiftUI/Sources/ObservableScreen.swift

+            // deduce the natural size of content that scrolls in either direction, or both, or
+            // neither.
+
+            let fixedResult = view.sizeThatFits(fixedSize)


Any idea what the perf cost is for these repeated measurements? I wonder if we could introduce our own sizing options that would allow us to do fewer calls based on more specific sizing configurations.

also, is the new logic specifically for scroll views? if so, do we want to do some of these extra calculations conditionally? or does it have to account for them being embedded as descendant views?

Any idea what the perf cost is for these repeated measurements

Unknown, but my suspicion is it's fine due to SwiftUI layout being much better than what we're used to.

also, is the new logic specifically for scroll views? if so, do we want to do some of these extra calculations conditionally? or does it have to account for them being embedded as descendant views?

We do need to calculate PCS regardless. The multiple measurements are to account for scroll views. Unfortunately we have to account for there being a scroll view anywhere in the content, and there's just no way to know.

Unfortunately we have to account for there being a scroll view anywhere in the content, and there's just no way to know.

It'd be a science experiment, but I wonder how feasible it'd be to push the measurements onto the SwiftUI side with a custom Layout and then bubble up context like 'hasScrollView'/'hasMultilineText' via preferences so we could be slightly smarter with our measurements.

I think I'd rather spend the time submitting feedback to Apple to hopefully make this better directly out of the box with the hosting controller though!

jamieQ · 2025-01-29T19:37:38Z

WorkflowSwiftUI/Sources/ObservableScreen.swift

@@ -146,21 +147,31 @@ private final class ModeledHostingController<Model, Content: View>: UIHostingCon

        defer { hasLaidOutOnce = true }

-        if #available(iOS 16.0, *) {


was the availability change intentional?

Yep. Previously we were letting UIHostingController handle PCS on iOS 16+, except for the first layout. This if handled the first layout case, and the else was for ≤15. Now we're handling PCS in all cases.

jamieQ · 2025-01-29T22:41:56Z

WorkflowSwiftUI/Sources/ObservableScreen.swift

+            let size = CGSize(
+                width: min(fixedResult.width, unboundedHorizontalResult.width),
+                height: min(fixedResult.height, unboundedVerticalResult.height)
+            )


any chance we need to (or should) handle weird/unexpected values here? is sizeThatFits guaranteed to return sensical results? i guess the concern would be if we end up assigning a preferred content size that doesn't 'make sense' – would that matter?

any chance we need to (or should) handle weird/unexpected values here? is sizeThatFits guaranteed to return sensical results? i guess the concern would be if we end up assigning a preferred content size that doesn't 'make sense' – would that matter?

It's not guaranteed to do anything by API contract, but based on observation it seems like SwiftUI will return sensible values.

jamieQ · 2025-01-29T22:47:54Z

WorkflowSwiftUIExperimental/Sources/SwiftUIScreen.swift

-            }
-        } else if swiftUIScreenSizingOptions.contains(.preferredContentSize) {
-            let size = view.sizeThatFits(view.frame.size)
+        if swiftUIScreenSizingOptions.contains(.preferredContentSize) {


if this is the path we want to go, shall we extract the duplicate calculation logic?

I'm following the rule of 3 here, since these two don't currently share any common place to extract to. Plus we'll probably deprecate SwiftUIScreen once ObservableScreen is more established.

jamieQ · 2025-01-29T22:49:52Z

WorkflowSwiftUI/Sources/ObservableScreen.swift

+            // deduce the natural size of content that scrolls in either direction, or both, or
+            // neither.
+
+            let fixedResult = view.sizeThatFits(fixedSize)


also, is the new logic specifically for scroll views? if so, do we want to do some of these extra calculations conditionally? or does it have to account for them being embedded as descendant views?

johnnewman-square

After trying out these changes in the Market Catalog app, SwiftUI Screens size themselves much better than before 🎉. I also didn't experience performance hiccups on a physical device with dialogs and sheets. I think this is a good solution to merge in!

jamieQ

changes all look reasonable to me!

robmaceachern

Thanks for working through this!

robmaceachern · 2025-01-29T14:22:14Z

WorkflowSwiftUI/Sources/ObservableScreen.swift

@@ -133,7 +134,7 @@ private final class ModeledHostingController<Model, Content: View>: UIHostingCon
        super.viewDidLoad()

        // `UIHostingController`'s provides a system background color by default. In order to
-        // support `ObervableModelScreen`s being composed in contexts where it is composed within another
+        // support `SwiftUIScreen`s being composed in contexts where it is composed within another


Revert this line?

Whoops. I feel like this line is self-explanatory but apparently not.

robmaceachern · 2025-01-30T22:22:06Z

WorkflowSwiftUI/Tests/PreferredContentSizeTests.swift

+private struct TestView: View {
+    var store: Store<StateAccessor<State>>
+
+    var body: some View {
+        WithPerceptionTracking {
+            if store.axes.isEmpty {
+                box
+            } else {
+                ScrollView(store.axes) {
+                    box
+                }
+            }
+        }
+    }


I'd love if multiline text was also included in the test cases.

Would be hard to do without making the test fragile.

robmaceachern · 2025-01-30T22:44:17Z

WorkflowSwiftUI/Sources/ObservableScreen.swift

+            // deduce the natural size of content that scrolls in either direction, or both, or
+            // neither.
+
+            let fixedResult = view.sizeThatFits(fixedSize)


Unfortunately we have to account for there being a scroll view anywhere in the content, and there's just no way to know.

It'd be a science experiment, but I wonder how feasible it'd be to push the measurements onto the SwiftUI side with a custom Layout and then bubble up context like 'hasScrollView'/'hasMultilineText' via preferences so we could be slightly smarter with our measurements.

I think I'd rather spend the time submitting feedback to Apple to hopefully make this better directly out of the box with the hosting controller though!

feat: flexible swiftui preferredContentSize calc

c1bf3c8

watt requested review from a team as code owners January 28, 2025 22:02

kyleve reviewed Jan 28, 2025

View reviewed changes

jamieQ reviewed Jan 29, 2025

View reviewed changes

test: swiftui preferredcontentsize

7ee2295

watt force-pushed the awatt/swiftui-screen-pcs branch from 6fc3869 to 7ee2295 Compare January 30, 2025 01:29

johnnewman-square approved these changes Jan 30, 2025

View reviewed changes

jamieQ approved these changes Jan 30, 2025

View reviewed changes

mjohnson12 approved these changes Jan 30, 2025

View reviewed changes

robmaceachern approved these changes Jan 30, 2025

View reviewed changes

chore: update comment about background color

47cc3b6

watt enabled auto-merge (squash) January 31, 2025 00:52

watt merged commit 8ef3944 into main Jan 31, 2025
7 checks passed

watt deleted the awatt/swiftui-screen-pcs branch January 31, 2025 00:58

		@@ -146,21 +147,31 @@ private final class ModeledHostingController<Model, Content: View>: UIHostingCon

		defer { hasLaidOutOnce = true }

		if #available(iOS 16.0, *) {

feat: flexible SwiftUI preferredContentSize calc [UI-7665] #320

feat: flexible SwiftUI preferredContentSize calc [UI-7665] #320

Uh oh!

Conversation

watt commented Jan 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

johnnewman-square left a comment

Choose a reason for hiding this comment

Uh oh!

jamieQ left a comment

Choose a reason for hiding this comment

Uh oh!

robmaceachern left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

watt commented Jan 28, 2025 •

edited

Loading