Skip to content

Conversation

@JorgeMucientes
Copy link
Contributor

@JorgeMucientes JorgeMucientes commented Jan 10, 2025

Part of: #13285

Description

This PR fixes several issues related to ModalBottomSheetLayout composable.

We had a custom ModalBottomSheetLayout component with a workaround to apply the darker background overlay when a bottom sheet is opened over the StatusBar. In Android 15 some APIs that this workaround relies on, have been deprecated. The aim of this PR is to stop using the deprecated APIs on Android 15 onwards. For that the following changes have been applied:

  • ModalStatusBarBottomSheetLayout has been renamed to ModalBottomSheetLayoutWithStatusBarWorkAround to make it clearer what is different from the regular ModalBottomSheetLayout. The class has also been made private to avoid accidental usages.
  • Ensure all compose bottomsheets on the app are using WCModalBottomSheetLayout instead of ModalBottomSheetLayout or any other variant.
  • WCModalBottomSheetLayout will use regular ModalBottomSheetLayout for Android 15 and beyond, and ModalBottomSheetLayoutWithStatusBarWorkAround below Android 15.

Testing information

Test in Android 14 or below

  • Check the bottom sheet darker background overlays the status bar.

Test in Android 15 targeting API 35

Set the targetSdk = 35 and compile the app. Check bottom sheet darker background overlays the status bar.

NOTE: When targeting API 35 and running the app on Android 15 (API 35) then the content becomes edge to edge by default and we'll need to manually deal with window insets ourselves. Otherwise the nav bar will overlay the status bar as you'll see in the screen recording below.

tagetSdk35.mp4

NOTE: If we release the app only changing the compileSdk = 35 but not targetSdk=35 the status bar won't be overlayed by bottom sheet dark background in Android 15. It is not a big deal, but just a heads up on that.

The tests that have been performed

The above.

Reviewer (or Author, in the case of optional code reviews):

Please make sure these conditions are met before approving the PR, or request changes if the PR needs improvement:

  • The PR is small and has a clear, single focus, or a valid explanation is provided in the description. If needed, please request to split it into smaller PRs.
  • Ensure Adequate Unit Test Coverage: The changes are reasonably covered by unit tests or an explanation is provided in the PR description.
  • Manual Testing: The author listed all the tests they ran, including smoke tests when needed (e.g., for refactorings). The reviewer confirmed that the PR works as expected on big (tablet) and small (phone) in case of UI changes, and no regressions are added.

@dangermattic
Copy link
Collaborator

dangermattic commented Jan 10, 2025

1 Warning
⚠️ This PR is larger than 300 lines of changes. Please consider splitting it into smaller PRs for easier and faster reviews.

Generated by 🚫 Danger

@wpmobilebot
Copy link
Collaborator

wpmobilebot commented Jan 10, 2025

📲 You can test the changes from this Pull Request in WooCommerce-Wear Android by scanning the QR code below to install the corresponding build.
App Name WooCommerce-Wear Android
Platform⌚️ Wear OS
FlavorJalapeno
Build TypeDebug
Commit5c5aed7
Direct Downloadwoocommerce-wear-prototype-build-pr13285-5c5aed7.apk

@JorgeMucientes JorgeMucientes marked this pull request as draft January 10, 2025 14:38
@JorgeMucientes JorgeMucientes added compose Uses Jetpack Compose framework Tech Debt labels Jan 10, 2025
@wpmobilebot
Copy link
Collaborator

wpmobilebot commented Jan 10, 2025

📲 You can test the changes from this Pull Request in WooCommerce Android by scanning the QR code below to install the corresponding build.

App Name WooCommerce Android
Platform📱 Mobile
FlavorJalapeno
Build TypeDebug
Commit5c5aed7
Direct Downloadwoocommerce-prototype-build-pr13285-5c5aed7.apk

@kidinov kidinov self-requested a review January 13, 2025 09:53
@JorgeMucientes JorgeMucientes marked this pull request as ready for review January 14, 2025 11:22
@kidinov kidinov self-assigned this Jan 14, 2025
@kidinov kidinov changed the title 13269 android sdk update min sdk to 35 bottomsheet fixes [Android SDK update] Bottom sheet fixes Jan 14, 2025
@kidinov kidinov added this to the 21.5 milestone Jan 14, 2025
…oid-sdk-update-min-sdk-to-35-bottomsheet-fixes
…oid-sdk-update-min-sdk-to-35-bottomsheet-fixes
…oid-sdk-update-min-sdk-to-35-bottomsheet-fixes
…oid-sdk-update-min-sdk-to-35-bottomsheet-fixes
Copy link
Contributor

@kidinov kidinov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@JorgeMucientes huge thanks for your work here!

I noticed a weird glitch on Android 33 emulator. I can pull up the keyboard just by scrolling up. It doesn't happen on a real Android 35 device

Also, notice that the top toolbar padding is off. It's white for some reason too

01-14--16-29.mp4

@kidinov
Copy link
Contributor

kidinov commented Jan 14, 2025

@JorgeMucientes the scrolling part seems to be due to usage of imeNestedScroll

https://composables.com/foundation-layout/imenestedscroll

@codecov-commenter
Copy link

codecov-commenter commented Jan 14, 2025

Codecov Report

Attention: Patch coverage is 0% with 91 lines in your changes missing coverage. Please review.

Project coverage is 41.09%. Comparing base (ec83182) to head (5c5aed7).
Report is 19 commits behind head on 13269-android-sdk-update-min-sdk-to-35.

Files with missing lines Patch % Lines
...d/ui/compose/component/WCModalBottomSheetLayout.kt 0.00% 85 Missing ⚠️
...paignCreationAdDestinationParametersBottomSheet.kt 0.00% 5 Missing ⚠️
...orders/wooshippinglabels/address/AddressSection.kt 0.00% 1 Missing ⚠️
Additional details and impacted files
@@                             Coverage Diff                              @@
##             13269-android-sdk-update-min-sdk-to-35   #13285      +/-   ##
============================================================================
- Coverage                                     41.09%   41.09%   -0.01%     
  Complexity                                     6420     6420              
============================================================================
  Files                                          1321     1320       -1     
  Lines                                         77184    77193       +9     
  Branches                                      10645    10644       -1     
============================================================================
  Hits                                          31721    31721              
- Misses                                        42653    42662       +9     
  Partials                                       2810     2810              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@JorgeMucientes
Copy link
Contributor Author

@JorgeMucientes the scrolling part seems to be due to usage of imeNestedScroll
https://composables.com/foundation-layout/imenestedscroll

Great catch @kidinov! The purpose of it was to hide the keyboard when a bottomsheet is opened and swiped down. But the downside was the bug that you reported. To get better idea:

  • With imeNestedScroll(),
withImeScroll.mp4
  • Without it
notImenestedScrol.mp4

I ended up adding the imeNestedScroll qualifier to the only bottomsheet that required it. I still have second thoughs about keeping it at all. Because as you'll see there's some minor flickering when manually swiping down the bottom sheet when imeNestedScroll is enabled. I think is a minor detail, but let me me know if you think is worth removing it completely.

Copy link
Contributor

@kidinov kidinov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @JorgeMucientes LGTM!

@kidinov kidinov merged commit 27cd51f into 13269-android-sdk-update-min-sdk-to-35 Jan 15, 2025
15 checks passed
@kidinov kidinov deleted the 13269-android-sdk-update-min-sdk-to-35-bottomsheet-fixes branch January 15, 2025 11:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

compose Uses Jetpack Compose framework Tech Debt

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants