Fix judder on scroll with disappearing scrollBehavior

[dfabulich]

Fixes #295

EDIT: Updated Feb 17

This PR looks small, but it feels like major surgery in the very heart of the NavigationStack, converting it from a Column containing the topBar, content, and bottomBar into a Box where the topBar is in its own Box aligned at the top, the bottomBar is aligned at the bottom, and the content appears in its own Box, filled to max size, relying on safe area insets to avoid clobbering the top and bottom bars.

Root cause of #295 was the safe area and the Column getting out of sync

Navigation.swift computes an updated safe area (based on top bar size and bottom bar size, topBarBottomPx and bottomBarTopPx) as it composes the top bar, content, and bottom bar.

The tricky part is that we have to compute topBarBottomPx and bottomBarTopPx asynchronously, with onGloballyPositionedInWindow callbacks. We don't necessarily know the current actual values as we compose the content.

In the old implementation, this could cause us to pass a stale safe area to the content. It would eventually get updated when the onGloballyPositionedInWindow callback fired, but that left enough time to show incorrect layout as the callback settles.

Worse, because we were incorrectly laying out a container as it scrolls, the actual scroll position was temporarily incorrect, causing the navigation bar itself to judder instead of smoothly scrolling away.

In the new Box implementation, we always layout the content based on topBarBottomPx and bottomBarTopPx, ensuring that the content never gets out of sync with the safe area.

FWIW, I suspect that this code may improve performance in NavigationStack. In my repro for #295, this PR fixing the layout shift/judder during scroll naturally resulted in fewer recompositions happening during scroll.

Testing

I tested this with the following playgrounds:

• ButtonPlayground: An ordinary ScrollView in a NavigationStack
• ToolbarPlayground: Tested with Add a Toolbar playground that actually hides all bars skipapp-showcase#56 I tested hiding the navigation bar and hiding both bars (navigation bar, tab bar).
• SafeAreaPlayground: Tested with Add test using safe area padding skipapp-showcase#57 I used the "Geometry padding" sub-playground, where I tested all eight combinations of hidden/visible navigation bar, bottom bar, and tab bar.

I ran all of these tests manually, once on the old Column-based layout implementation, and then again on the new Box-based implementation. In the old implementation, you can see a visible layout shift as you tap "Hide Navigation Bar"… this layout shift is gone now in my implementation.

Screenshots

Before and after screenshots, scrolled to top and bottom, all identical

ButtonPlayground

ToolbarPlayground: Hide Navigation Bar

ToolbarPlayground: Hide Bars

SafeAreaPlayground --> Geometry Padding: All bars showing

SafeAreaPlayground --> Geometry Padding: All bars hidden

SheetPlayground

I also tested every combination of bars hidden/showing, but it's too laborious to prepare that many screenshots. LMK if you want/need more screenshots.

Skip Pull Request Checklist:

• REQUIRED: I have signed the Contributor Agreement
• REQUIRED: I have tested my change locally with swift test
• OPTIONAL: I have tested my change on an iOS simulator or device
• OPTIONAL: I have tested my change on an Android emulator or device

[marcprux]

Agreed, we'll need to look pretty closely at this. Did you run through all the navigation playgrounds in Showcase as well?

In the past with risky overhauls like this (like #238), we've offered a backwards-compatibility setting as an escape hatch. I wonder if you could re-use the .layoutImplementationVersion() and bump the default value to 2, and only use the new navigation layout if it is higher than that. It might be a lot of refactoring (or a lot of code duplication), but it would ease our minds about providing a way to avoid large-scale breakage.

skip-ui/Sources/SkipUI/SkipUI/Environment/EnvironmentValues.swift

Lines 700 to 704 in 6377088

	/// Allow users to revert to previous layout behavior.
	var _layoutImplementationVersion: Int {
	get { builtinValue(key: "_layoutImplementationVersion", defaultValue: { 1 }) as! Int }
	set { setBuiltinValue(key: "_layoutImplementationVersion", value: newValue, defaultValue: { 1 }) }
	}

[dfabulich]

The latest version of this PR now adds a layoutImplementationVersion 2. I've tested that .layoutImplementationVersion(1) reverts back to the Column-based layout. (I copied and pasted a bunch of code rather than trying to deduplicate, because I figure anybody who wants to opt in to the old algorithm wants it exactly the way it used to be. If/when we make further changes to the Box-based layout, those can and should diverge from the old way.)

I also added in a layout fix which arose when I tested the navigation playgrounds. 😬 Good on us for finding it! But IMO that's also a bad sign… where else do I still need to check??

[aabewhite]

Really appreciate the patch! See my two comments. So action items are:

1. Restore the safeDrawing insets for cases when the bars are hidden.
2. Simplify the new Box code to pad the content Box based on the heights of the top and bottom bars.

Hopefully I'm correct in how this is all working and not leading you in the wrong direction.

[dfabulich]

OK! I've tested the current version against the NavigationStack playground, the Toolbar playground, and the SafeArea playground, including a new Toolbar subplayground skiptools/skipapp-showcase#56 and a new SafeArea subplayground skiptools/skipapp-showcase#57 that I used to repro the issues that @aabewhite reported.

I think this should be clear to merge now.

[aabewhite]

Hey Dan - in this version of the patch you're no longer padding the content Box when top/bottom bars are visible, so the content ends up under the bars.

I'm not sure if there was a problem with pushing the wrong code... but it's broken everywhere.

It's also possible that I've screwed something up applying the patch, but the code itself looks wrong to me too: it only adds non-zero top/bottom padding to the content Box when the bars are hidden.

[dfabulich]

nope, I screwed it up somehow. I'll try it again.

[dfabulich]

I've fixed the open issues in this PR and updated the description with my testing strategy and a ton of screenshots. I think think this is now ready to merge.

The CI build is failing CI was fixed in a rerun.

[dfabulich]

I discovered a bug in SheetPlayground with this branch, now fixed. (I also updated skiptools/skipapp-showcase#57 to add a sheet-based version of its playground.)

I've been using this branch in my code for weeks now, and I'm pretty sure it's bulletproof. I think the thing to do now would be to identify any missing tests that could exist (I think we're covered now!) or to finally merge this thing.

[marcprux]

OK, I think we've looked at this closely enough, and everything looks/feels OK in my testing. And we have _layoutImplementationVersion to fallback in case anything breaks for anyone, so I'll go ahead and merge.

Thanks for all the hard work on this!