Fix iOS WindowInsets over-reported on non-fullscreen / safe-area-filt…#3062
Open
Jacob Rhoda (jadar) wants to merge 1 commit into
Open
Fix iOS WindowInsets over-reported on non-fullscreen / safe-area-filt…#3062Jacob Rhoda (jadar) wants to merge 1 commit into
Jacob Rhoda (jadar) wants to merge 1 commit into
Conversation
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
Vladimir Mazunin (mazunin-v-jb)
requested review from
Andrei Salavei (ASalavei) and
Vendula Švastalová (svastven)
|
Hello, Jacob Rhoda (@jadar)! |
Author
|
Vladimir Mazunin (@mazunin-v-jb) I signed it and re-scanned multiple times to no avail. 🤷♂️ |
…ered surfaces Fixes [CMP-10230](https://youtrack.jetbrains.com/issue/CMP-10230) `UIKitWindowInsetsManager` per-edge `union()`s safe-area insets from the local `_overlayView` with those from `window.rootViewController.view`. This was added in JetBrains#2946 to cover CMP-9931 (missing `UINavigationBar` height) and CMP-8922 (iPad Stage Manager traffic-light controls), but over-reports whenever the Compose surface is positioned below window- level chrome that does not intersect `_overlayView`, or whose ancestor chain filters safe-area propagation (`UIHostingController.safeAreaRegions = .keyboard`, parent `additionalSafeAreaInsets`, manual frame placement). The local view correctly reports zero for the affected edge, but the window root reports the global value and `max()` wins. This commit: 1. Removes the `window.rootViewController.view` source from `windowInsetsViews`. Standard UIKit safe-area propagation already incorporates ancestor chrome into a child view's `safeAreaInsets` for typical hosting setups, so the CMP-9931 navbar case should still be covered. Worth re-validating against the original repro. 2. Adds a dedicated `onSafeAreaInsetsDidChange` callback on `BackgroundInputView` (parallel to the existing `onLayoutSubviews`) wired to a new `internal fun onSafeAreaInsetsDidChange()` on the mediator. The callback is invoked directly from `safeAreaInsetsDidChange`, without going through `setNeedsLayout`, so Compose re-reads insets even when the parent reposition leaves the input view's local frame unchanged. 3. Adds an Obj-C bridge on `CMPViewController` to expose `viewSafeAreaInsetsDidChange` for Kotlin override (matches the existing `userInterfaceStyleDidChange` pattern). The new `safeAreaInsetsDidUpdate` hook is implemented in `ComposeHostingViewController` and forwards through `container.onSafeAreaInsetsDidChange()`. This is defense-in-depth alongside the view-level hook. Each path is a no-op when the value hasn't changed (`mutableStateOf` only triggers recompose on actual change), so redundant delivery is cheap. The existing `WindowInsetsPaddingTest.testWindowInsetsPaddingAppliedTo NonFullscreenComposeUIViewContent` encoded the union semantics; it has been renamed and inverted to assert the new contract (no padding on a non-fullscreen embedded `ComposeUIView`). The fullscreen-outer test is unchanged. Known limitation not addressed by this commit: iOS does not deliver `safeAreaInsetsDidChange` (UIView) or `viewSafeAreaInsetsDidChange` (UIViewController) to a view/VC occluded by a presented sheet, including during interactive sheet dismissal ("peek"). Confirmed empirically with both hooks wired. Per-frame `CADisplayLink` polling does not help because the read returns the same stale value iOS is caching; geometric inset computation from window-space position handles peek but over-subtracts the legitimate navbar contribution in standard `UINavigationController` setups. ## Release Notes ### Fixes - iOS - Fix `WindowInsets` over-reported on Compose surfaces positioned below window-level chrome that does not intersect the host view (e.g. under a `UIHostingController` with `safeAreaRegions = .keyboard`).
Jacob Rhoda (jadar)
force-pushed
the
cmp-10230-ios-host-view-insets
branch
from
06b71f8 to
2b87d60
Compare
Author
|
Vladimir Mazunin (@mazunin-v-jb) looks like it went through! |
| // WindowInsets.statusBars should only represent the insets at the top in portrait orientation | ||
| val topSafeAreaInsetsDp = viewController.view.safeAreaInsets.useContents { top }.dp | ||
|
|
||
| assertEquals( |
There was a problem hiding this comment.
I think this behavior was added intentionally because it mimics the Android one.
Please, correct me Vendula Švastalová (@svastven) .
There was a problem hiding this comment.
Yes, that is right. I have added a comment to the task reporting this issue.
| container.updateUserInterfaceStyle(traitCollection.userInterfaceStyle) | ||
| } | ||
|
|
||
| override fun safeAreaInsetsDidUpdate() { |
There was a problem hiding this comment.
You already added the observer to safe area insets in the BackgroundInputView. Why another one is needed here?
|
|
||
| // Insets are read from the host view only. Adding `window.rootViewController.view` | ||
| // (the union introduced in #2946 for CMP-9931) over-reports when CMP is positioned | ||
| // below window-level chrome that does not intersect `_overlayView`. |
There was a problem hiding this comment.
I would avoid such comments - they are related to the change, not to the resulting code.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
UIKitWindowInsetsManagerper-edgeunion()s safe-area insets from the local_overlayViewwith those fromwindow.rootViewController.view. This was introduced in #2946 to cover CMP-9931 and CMP-8922 but causes over-reporting whenever the Compose surface is positioned below window-level chrome that does not intersect_overlayView, or whose ancestor chain filters safe-area propagation (UIHostingController.safeAreaRegions = .keyboard, parentadditionalSafeAreaInsets, manual frame placement). In those configurations the local view correctly reports zero for the affected edge but the window root reports the global value, andmax()wins.This MR:
Removes the
window.rootViewController.viewsource so insets come only from the immediate hosting view. Standard UIKit safe-area propagation already incorporates ancestor chrome (status bar, navigation bar, tab bar) into a child view'ssafeAreaInsetsfor typical hosting setups, so the navbar case from CMP-9931 should still be covered. Worth re-validating against the original CMP-9931 reproduction.Adds a dedicated
onSafeAreaInsetsDidChangecallback path so Compose re-reads insets the moment iOS reports a change, rather than via asetNeedsLayoutindirection that can be deferred. Two parallel entry points land at the samemediator.onSafeAreaInsetsDidChange():onSafeAreaInsetsDidChangeconstructor parameter onBackgroundInputView, invoked from itssafeAreaInsetsDidChangeoverride.CMPViewController(matches the existinguserInterfaceStyleDidChangepattern), declaredsafeAreaInsetsDidUpdateand overridden inComposeHostingViewControllerto forward through the container.Both paths are defense-in-depth. Each is a no-op when the value hasn't changed (
mutableStateOfonly triggers recompose on actual change).The existing
WindowInsetsPaddingTest.testWindowInsetsPaddingAppliedToNonFullscreenComposeUIViewContentencoded the union semantics; it has been renamed and inverted to assert the new contract. The fullscreen-outer test is unchanged.Known limitation not addressed: iOS does not deliver
safeAreaInsetsDidChange(UIView) orviewSafeAreaInsetsDidChange(UIViewController) to a view/VC occluded by a presented sheet, including during interactive sheet dismissal ("peek"). CachedsafeAreaInsetspersist until the view is fully visible again. Confirmed empirically with both hooks wired. Per-frameCADisplayLinkpolling does not help (returns the same stale value); geometric inset computation from window-space position handles peek but over-subtracts the legitimate navbar contribution in standardUINavigationControllersetups. Documenting as a known UIKit quirk.Alternative considered: rather than removing the window-root source, expose it as a
ComposeUIViewControllerConfiguration.windowInsetsStrategyflag with valuesHostingViewOnlyandUnionWithWindow, defaulting to the latter to preserve current behavior. Happy to take that route if maintainers prefer for backwards-compat.Fixes CMP-10230.
Testing
WindowInsetsPaddingTestruns green on iOS simulator.ComposeUIViewControllerembedded under aUIHostingControllerwithsafeAreaRegions = .keyboard; content is no longer double-padded.ComposeUIViewControllerpushed ontoUINavigationController, navbar height accounted for) and CMP-8922 (iPad Stage Manager traffic-light controls).This should be tested by QA.
Release Notes
Fixes - iOS
WindowInsetsover-reported on Compose surfaces positioned below window-level chrome that does not intersect the host view (e.g. under aUIHostingControllerwithsafeAreaRegions = .keyboard). Fixes CMP-10230.