Fix initial Skiko popup positioning - #3321
Conversation
| val canCalculatePosition by remember { | ||
| derivedStateOf { parentBoundsInWindow.value != null } | ||
| } | ||
| if (!canCalculatePosition) return@Content |
There was a problem hiding this comment.
It seems it breaks "showing in the same frame" behavior. Test failures seems to be related
There was a problem hiding this comment.
yes, wrote a comment about that: #3321 (comment) we need to choose which is preferred.
There was a problem hiding this comment.
Popup does not always depend on parent bounds and can have absolute positioning. In this case it seems we shouldn't delay showing it.
But I'm not sure if we can detect it here since it's just custom positioning logic from PoV of this code
|
The core problem is that the main scene's placement phase and the popup layer layout phase are independent, the frame timing is shared but there is no guarantee in the dependency between layers so that we can expect now there are two options if we don't want to tackle the dependency issue ATM:
Snapshot.withMutableSnapshot {
parentBoundsInWindow.value = IntRect(layoutPosition, layoutSize)
}This would restore the local snapshot apply during the main scene layout call (removed in #3272), and the popup scene can then consume the real anchor and render in the first frame.
|
|
The original code (and comments) assumed the popup and the parent are in the same scene. When that's not true, then indeed I don't think we can expect the popup to be shown in the same frame. I wouldn't completely remove my comments or tests though, but instead update them to document the two cases (same scene, different scenes). |
9a3b79f to
80b2f8b
Compare
80b2f8b to
da4b57e
Compare
|
I updated the comments and tests to document both cases based on the updated behavior. Alexander Maryanovsky (@m-sasha) can you please review? |
Alexander Maryanovsky (m-sasha)
left a comment
Alexander Maryanovsky (m-sasha)
left a comment
There was a problem hiding this comment.
This PR breaks a needed functionality: that popups are shown immediately.
This is needed for example, for initial sizing of a window that has a popup in it. Without this, it can't be sized to take into account the popup size.
I understand that this is not possible when the popup is in a separate native layer (maybe will be possible again after Ivan's planned shared recomposer change?), but can we keep it when the popup is in the same layer?
286aa33 to
5bbfbb5
Compare
5bbfbb5 to
62c14d9
Compare
|
Alexander Maryanovsky (@m-sasha) Ivan Matkov (@MatkovIvan), can you please review? |
4 participants
Fixes selection handles briefly shown at an offset.
Popup anchoring relied on the next snapshot apply (removed in #3272) during drawing to make parent bounds written during layout visible to the separate popup scene. As a result, the first popup layout used the initial
IntRect.Zeroanchor causing the issue.We now compose the Popup content only after a real parent anchor is available which prevents drawing the first zero-anchor frame. Similar approach is used on Android
compose-multiplatform-core/compose/ui/ui/src/androidMain/kotlin/androidx/compose/ui/window/AndroidPopup.android.kt
Line 635 in 9e18f11
Bug.mov
Fix.mov
Fixes CMP-10684 Selection handles blink on the view top
Fixes CMP-10714 [iOS] Dismissing a Popup leaves stale content when another Popup layer remains
Testing
This should be tested by QA
Release Notes
Fixes - iOS