Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

iOS: Expanded ModalBottomSheet: semi-transparent black background doesn't occupy complete screen. #3701

Closed
sdzshn3 opened this issue Sep 18, 2023 · 4 comments · Fixed by JetBrains/compose-multiplatform-core#826
Closed

iOS: Expanded ModalBottomSheet: semi-transparent black background doesn't occupy complete screen. #3701

sdzshn3 opened this issue Sep 18, 2023 · 4 comments · Fixed by JetBrains/compose-multiplatform-core#826
Assignees
@MatkovIvan
Labels
bug Something isn't working ios layers: popup/dialog reproduced

Comments

@sdzshn3
Copy link

sdzshn3 commented Sep 18, 2023

Describe the bug
In iOS, when ModalBottomSheet is expanded the semi transparent black background doesn't occupy complete screen.

Simulator Screenshot - iPhone 14 Pro - 2023-09-18 at 14 13 51

Affected platforms
Select one of the platforms below:

  • iOS

Versions

  • Kotlin version*: 1.9.0
  • Compose Multiplatform version*: 1.5.10-beta01
  • OS version(s)* (required for Desktop and iOS issues): iOS Emulator 16.4 (20E247)
  • OS architecture (x86 or arm64): No Idea

To Reproduce
Steps and/or the code snippet to reproduce the behavior:
Using the above following Compose version, implement a ModelBottomSheet and make sure it doesn't fill entire height of screen. (Either set a max height or have a content which doesn't fill whole height.)

Expected behavior
That semi-transparent black bacground should fill the entire screen just like how it works in Android.
@sdzshn3 sdzshn3 added bug Something isn't working submitted labels Sep 18, 2023
@MatkovIvan MatkovIvan self-assigned this Sep 18, 2023
@MatkovIvan
Copy link
Member

MatkovIvan commented Sep 18, 2023 •
edited
Loading

It looks like combination of two issues. The first obvious part is wrong usage of insets (safe-area) inside Popup (used under the hood in this component). I'll take care of fixing it.

But the second part is the current way that ModalBottomSheet is implemented (not in our fork, but in androidx repo from where we picked up implementation). It has similar issue on Android with applications that draws content outside safe area.
Here is the same reproducer, but on Android (original Google's Jetpack Compose):

Screen_recording_20230918_134958.mp4

Expected behavior
That semi-transparent black bacground should fill the entire screen just like how it works in Android.

So, it kinda already works like Android 😄
For this part I'd suggest open an issue in Google's issue tracker. Passing zeros in windowInsets parameter solves it

@sdzshn3
Copy link
Author

sdzshn3 commented Sep 18, 2023

I was able to reproduce what you told for Android. It happens only when we draw content outside safe area, not otherwise.

So, is this issue in iOS dependent on the issue in original androidx repo?

@MatkovIvan
Copy link
Member

It happens only when we draw content outside safe area, not otherwise.

Yep, it was what I meant

is this issue in iOS dependent on the issue in original androidx repo?

Partially. Currently on iOS there is additional problem - it applies safe area incorrectly on low-level component (Popup). I'm investigating this problem

This was referenced Sep 19, 2023
Merged
Merged
MatkovIvan added a commit to JetBrains/compose-multiplatform-core that referenced this issue Sep 19, 2023
@MatkovIvan
Fix RootLayout platform padding usage (#825)
9951692 
## Proposed Changes

- Apply more insets in `RootLayout` without assuming that it will be
centered
- Center relatively content area instead of screen (matches Android now)

Test | Before | After
--- | --- | ---
`Popup` / max size / zero offset | <img
src="https://github.com/JetBrains/compose-multiplatform-core/assets/1836384/a3bfef0f-002c-4dab-ad85-58f4a8e88060"
height="600"> | <img
src="https://github.com/JetBrains/compose-multiplatform-core/assets/1836384/ec8dd32c-f12b-4e5b-a987-00631678d24c"
height="600">
`Dialog` / max size / center | <img
src="https://github.com/JetBrains/compose-multiplatform-core/assets/1836384/ce216f77-5be4-43fa-ae78-df666ee96517"
height="600"> | <img
src="https://github.com/JetBrains/compose-multiplatform-core/assets/1836384/8e9820b4-99cd-44a1-80fc-68d7b4887b53"
height="600">

## Testing

Test: run mpp on iOS or try to use maximized `Popup` on device with
safe-area

## Issues Fixed

Fixes (partially)
JetBrains/compose-multiplatform#3701
@MatkovIvan MatkovIvan mentioned this issue Sep 20, 2023
Merged
elijah-semyonov pushed a commit to JetBrains/compose-multiplatform-core that referenced this issue Sep 21, 2023
@MatkovIvan @elijah-semyonov
Fix RootLayout platform padding usage (#825)
a02c7bd 
## Proposed Changes

- Apply more insets in `RootLayout` without assuming that it will be
centered
- Center relatively content area instead of screen (matches Android now)

Test | Before | After
--- | --- | ---
`Popup` / max size / zero offset | <img
src="https://github.com/JetBrains/compose-multiplatform-core/assets/1836384/a3bfef0f-002c-4dab-ad85-58f4a8e88060"
height="600"> | <img
src="https://github.com/JetBrains/compose-multiplatform-core/assets/1836384/ec8dd32c-f12b-4e5b-a987-00631678d24c"
height="600">
`Dialog` / max size / center | <img
src="https://github.com/JetBrains/compose-multiplatform-core/assets/1836384/ce216f77-5be4-43fa-ae78-df666ee96517"
height="600"> | <img
src="https://github.com/JetBrains/compose-multiplatform-core/assets/1836384/8e9820b4-99cd-44a1-80fc-68d7b4887b53"
height="600">

## Testing

Test: run mpp on iOS or try to use maximized `Popup` on device with
safe-area

## Issues Fixed

Fixes (partially)
JetBrains/compose-multiplatform#3701
MatkovIvan added a commit to JetBrains/compose-multiplatform-core that referenced this issue Sep 21, 2023
@MatkovIvan
Fix applying WindowInsets inside Popup/Dialog (#832)
2be70cf 
## Proposed Changes

- Unwrap unnecessary state for `LocalSafeArea`/`LocalLayoutMargins`
- Override  `LocalSafeArea`/`LocalLayoutMargins` for separate owners.
- Replace `IOSInsets` to `PlatformInsets` - [Don't use data classes in
an
API](https://kotlinlang.org/docs/jvm-api-guidelines-backward-compatibility.html#don-t-use-data-classes-in-an-api)

## Changes in API

```diff
-androidx.compose.ui.uikit.IOSInsets
+androidx.compose.ui.platform.PlatformInsets
```
```diff
-androidx.compose.ui.uikit.LocalSafeAreaState
+androidx.compose.ui.platform.LocalSafeArea
```
```diff
-androidx.compose.ui.uikit.LocalLayoutMarginsState
+androidx.compose.ui.platform.LocalLayoutMargins
```

## Testing

```kt
Box(modifier = Modifier
    .fillMaxSize()
    .windowInsetsPadding(WindowInsets.systemBars)
    .background(Color.Green)
)
Popup {
    Box(modifier = Modifier
        .fillMaxSize()
        .windowInsetsPadding(WindowInsets.systemBars)
        .background(Color.Red)
    )
}
```

Android | iOS (Before) | iOS (After)
--- | --- | ---
<img
src="https://github.com/JetBrains/compose-multiplatform-core/assets/1836384/8fe492a6-007e-4cbf-9c8a-fa6eea9bb056"
height="600"> | <img
src="https://github.com/JetBrains/compose-multiplatform-core/assets/1836384/3168efba-0072-47aa-853a-1c44bd68e4e0"
height="600"> | <img
src="https://github.com/JetBrains/compose-multiplatform-core/assets/1836384/3845efe8-eb8e-4c71-87e2-ea44dd73a91e"
height="600">

## Issues Fixed

Fixes (partially)
JetBrains/compose-multiplatform#3701
MatkovIvan added a commit to JetBrains/compose-multiplatform-core that referenced this issue Sep 21, 2023
@MatkovIvan
Fix ModalBottomSheetPopup parameters (#826)
955281b 
## Proposed Changes

- Position `ModalBottomSheet` not relative to parent
- Exclude default `Popup`'s insets

Before | After
--- | ---
<img
src="https://github.com/JetBrains/compose-multiplatform-core/assets/1836384/fe7b4b1b-8c12-49d7-9f4c-9c72af6d05cd"
height="600"> | <img
src="https://github.com/JetBrains/compose-multiplatform-core/assets/1836384/176a56ec-9469-4603-b0e8-153fa6c9318a"
height="600">

## Testing

Test: run mpp on iOS or try to use `ModalBottomSheetPopup` on device
with safe-area

## Issues Fixed

Fixes JetBrains/compose-multiplatform#3701,
JetBrains/compose-multiplatform#3703
igordmn pushed a commit to JetBrains/compose-multiplatform-core that referenced this issue Jan 30, 2024
@MatkovIvan @igordmn
Fix ModalBottomSheetPopup parameters (#826)
05b651b 
## Proposed Changes

- Position `ModalBottomSheet` not relative to parent
- Exclude default `Popup`'s insets

Before | After
--- | ---
<img
src="https://github.com/JetBrains/compose-multiplatform-core/assets/1836384/fe7b4b1b-8c12-49d7-9f4c-9c72af6d05cd"
height="600"> | <img
src="https://github.com/JetBrains/compose-multiplatform-core/assets/1836384/176a56ec-9469-4603-b0e8-153fa6c9318a"
height="600">

## Testing

Test: run mpp on iOS or try to use `ModalBottomSheetPopup` on device
with safe-area

## Issues Fixed

Fixes JetBrains/compose-multiplatform#3701,
JetBrains/compose-multiplatform#3703
@okushnikov
Copy link
Collaborator

Please check the following ticket on YouTrack for follow-ups to this issue. GitHub issues will be closed in the coming weeks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@MatkovIvan MatkovIvan

Labels
bug Something isn't working ios layers: popup/dialog reproduced
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Fix ModalBottomSheetPopup parameters JetBrains/compose-multiplatform-core
3 participants
@MatkovIvan @sdzshn3 @okushnikov