material-components / material-components-android

Modular and customizable Material Design UI components for Android
Apache License 2.0
16.37k stars 3.07k forks source link

[BottomSheetDialog] Broken after last commit for edge to edge #2733

Closed Tolriq closed 2 years ago

Tolriq commented 2 years ago

Description: Edge to edge is broken with last commit

image

I've checked drawEdgeToEdge is properly set to true and WindowCompat.setDecorFitsSystemWindows(window, !drawEdgeToEdge) is properly called with false but this triggers the state shown above. The call should have made the window edgetoedge and the statusbar should have been taken in account via padding in the rest of the code.

Reverting that commit fix the issue

image

Expected behavior: The container should be edge to edge it is not.

Source code: Bug introduced by https://github.com/material-components/material-components-android/commit/2d3024ea1092b55d46e7785527853186e124d5aa

Minimal sample app repro:

Android API version: Android 12L + Androidx core 1.8.0

Material Library version: Snapshot

Device: P6 Pro last patches

Ping @drchen

To help us triage faster, please check to make sure you are using the latest version of the library.

We also happily accept pull requests.

drchen commented 2 years ago

Thanks for reporting this. Will take a look.

drchen commented 2 years ago

Do you have your sample code? Like how you create a bottom sheet dialog and show it? From your screenshots I can't tell which part is the dialog. Is the FrameLayout you highlighted the root view of the activity?

Tolriq commented 2 years ago

The highlighted view is the rootview of the BottomSheetDialog, the root of the activity is the other one the main_drawer and is properly edge to edge as you can see on both screenshots.

The height of the containerview (from the dialog) is wrong in the first case and does not go under the bottom navigation bar as it should and do in the second case.

There's nothing special in the code just an app that is globally edge to edge and using Androidx Core 1.8.0 and not the 1.7.0 that MDC link to.

https://developer.android.com/jetpack/androidx/releases/core#core-1.8.0-alpha06 they did change a few things about insets and windows that could be the underlying cause of this.

drchen commented 2 years ago

I believe internally we are using Androidx Core 1.8.0 as well, but I couldn't reproduce the issue, at least with our Catalog demo.

It will be helpful if you can provide how you set up your BottomSheetDialog - like the themes/styles you are using - and how its content view is structured.

Edge-to-edge feature is unfortunately something kind of easy to break. It's hard to tell what's really going on solely by the screenshots when I could reproduce it easily.

drchen commented 2 years ago

Actually I probably find a way to reproduce a similar behavior. Does this only happen in the expanded state? Or this happens with the collapsed state as well?

Tolriq commented 2 years ago

I lack time but will try to reproduce then and another issue with appbarlayout wrong offset that still baffles me.

From the screenshot it's simple, the call WindowCompat.setDecorFitsSystemWindows(window, !drawEdgeToEdge) just does not work the window that contains the dialog does not fit the window neither at top or at bottom. The phone is in gesture mode so the navigation bar is small but you can see that the window leaves space at the bottom and you see the main activity behind.

When reverting, the dialog correctly draw under the navigation bar as the code request and it works.

I think that the issue is that the WindowCompat touch the wrong window, but I lack time to fully debug more, I can't get composite builds to work with Gradle 7.5 on Windows :(

Tolriq commented 2 years ago

I only use full expanded see the base code I use below.

It's possible that some of things I do here are no more needed in current version of the BottomSheets but it should not impact this specific issue.

open class BaseBottomSheetDialogFragment : BottomSheetDialogFragment() {

    override fun onCreateDialog(savedInstanceState: Bundle?): Dialog {
        val dialog = super.onCreateDialog(savedInstanceState) as BottomSheetDialog
        dialog.setOnShowListener {
            context?.run {
                if (resources == null) {
                    return@setOnShowListener
                }
                val width = resources.getDimensionPixelSize(R.dimen.bottom_sheet_width)
                try {
                    dialog.window?.setLayout(if (width > 0) width else ViewGroup.LayoutParams.MATCH_PARENT, ViewGroup.LayoutParams.MATCH_PARENT)
                } catch (ignore: Exception) {
                    // Ignore
                }
                dialog.findViewById<FrameLayout>(com.google.android.material.R.id.design_bottom_sheet)?.let { frameLayout ->
                    BottomSheetBehavior.from(frameLayout).apply {
                        state = BottomSheetBehavior.STATE_EXPANDED
                        isHideable = false
                    }
                }
            }
        }
        return dialog
    }

    override fun onViewCreated(view: View, savedInstanceState: Bundle?) {
        super.onViewCreated(view, savedInstanceState)
        dialog?.window?.attributes?.windowAnimations = com.google.android.material.R.style.Animation_Design_BottomSheetDialog
    }

    override fun onViewStateRestored(savedInstanceState: Bundle?) {
        super.onViewStateRestored(savedInstanceState)
        if (haveQApi) {
            view?.findViewById<View>(R.id.bottom_sheet_navbar)?.run {
                ViewCompat.setOnApplyWindowInsetsListener(this) { v, insets ->
                    v.updateLayoutParams { height = insets.getInsets(WindowInsetsCompat.Type.systemBars()).bottom }
                    insets
                }
            }
        }
    }

    override fun onCreate(savedInstanceState: Bundle?) {
        setStyle(STYLE_NO_TITLE, DisplayHelper.currentDialogThemeId(activeContext))
        super.onCreate(savedInstanceState)
    }
}
drchen commented 2 years ago

Ah I managed to reproduce the issue. Yeah it's sort of broken by the change. Let me think about what should be the proper fix. Thanks for the info you provided!!

Tolriq commented 2 years ago

It's nice to see that there's actually someone reading the issues (Cf your comment on a previous 2020 issue :p)

I have another very strange issue with appbarlayout to try to repro and report (also edge to edge issue and some magic 5dp variation in the offset making the scrim to not be visible in pin mode) I hope you'll be available to look into it so I don't repro for nothing.

drchen commented 2 years ago

I'll try my best. Usually reproducing the issue is the hardest part if it's not a prominent one. So it will be much easier for me if you can have an easy repro. : )

Tolriq commented 2 years ago

I still have opened issue here with the exact code and the fix from 2020 but I've seen you assigned yourself yesterday. So let's start fresh and I'll report the issues for my M3 migration instead of local fixes. Thanks for being active.

drchen commented 2 years ago

So the fix for the other issue was just merged. : )

Regarding this issue - I tried to revert my change but the issue was still reproducible under the reproducing scenario so now I'm not sure what's going on again... ^^;

Are you setting anything related to system bar visibility in your themes/styles?

Tolriq commented 2 years ago

Welcome to the joys of edge to edge :) Even more fun when dealing with samsung.

Anyway sorry for the delay, different time zone.

Nothing special as I'm starting fresh on the new M3 themes

parent="Theme.Material3.Light.NoActionBar" and

        <!-- Material mandatory -->
        <item name="emojiCompatEnabled">false</item>
        <item name="enableEdgeToEdge">true</item>

        <!-- Android mandatory -->
        <item name="layout_keyline">-1</item>
        <item name="windowActionBar">false</item>
        <item name="windowActionModeOverlay">true</item>
        <item name="windowNoTitle">true</item>
        <item name="android:windowSoftInputMode">adjustPan|stateAlwaysHidden</item>
        <item name="android:soundEffectsEnabled">false</item>

I'll try to find some times next week to repro both things

Tolriq commented 2 years ago

Ok so found some times today to play a little.

1) The catalog app force things via windowPreferencesManager.applyEdgeToEdgePreference(bottomSheetDialog.getWindow()); in BottomSheetMainDemoFragment that kinda hide what is really happening and tests. When removing that line and relying on the normal way in the BottomSheetDialog there's many cases where it does not work properly.

2) The catalog app does not use BottomSheetDialogFragment but directly the BottomSheetDialog

3) For my issue: Changing

  public Dialog onCreateDialog(@Nullable Bundle savedInstanceState) {
    return new BottomSheetDialog(getContext(), getTheme());
  }

to

  public Dialog onCreateDialog(@Nullable Bundle savedInstanceState) {
    return new BottomSheetDialog(getContext());
  }

in BottomSheetDialogFragment (removing the getTheme) mostly fix the issue as it allows edge to edge but it also automatically add some padding at the bottom that is unwanted.

image

The blue bar is my code adding it based on the received insets, the padding at the end is unwanted and did never occur before the last commit.

This looks like some strange theming / timing issues that are a little too complicated for me.

I could use a local version of BottomSheetDialogFragment but BottomSheetDialog.removeDefaultCallback() is package private so can't :(

drchen commented 2 years ago

Yeah. I also found windowPreferencesManager.applyEdgeToEdgePreference(bottomSheetDialog.getWindow()); is what makes Catalog works last Friday. But even if I reverted my change, bottom sheet edge-to-edge doesn't work correctly without that line. (And from its implementation, I feel the behavior makes sense because it checks if navigation bar is translucent to decide if it wants to go edge-to-edge....I.e., its behavior is "passive" and it only goes edge-edge when the window is already sort of edge-to-edge. I'm not really sure if this does make sense or not...)

I need to confirm with the team what should be the expected behavior and see how we can fix it or if we really want to fix it.

As for your case - calling new BottomSheetDialog(getContext(), getTheme()) will override the ?attr/bottomSheetDialogTheme settings provided in the main theme. I think that's the main reason why edge-to-edge doesn't really work in your case?

If you need a custom theme you may want to extend it from ThemeOverlay.Material3.BottomSheetDialog to make sure the default behavior will be preserved.

As for the extra padding - it looks like working as intended because BottomSheetDialog will wrap your content and pad it with the insets of navigation bar.

In your case maybe the best solution for you is disabling ?attr/enableEdgeToEdge in your bottom sheet dialog theme and handle it by yourself?

Tolriq commented 2 years ago

There's a few different things here.

enableEdgeToEdge is mandatory in the theme to have the window of the bottomsheet go edge to edge and the dialog to remove the fitsystemwindow (Something I did manually before). So I can't disable it actually as it's needed, but yes I want to manage all the rest by myself.

After some tests I figured out that paddingBottomSystemWindowInsets does not work when applied to the global theme, It was necessary to do

   <item name="bottomSheetDialogTheme">@style/ThemeOverlay.App.BottomSheetDialog</item> 
    <style name="ThemeOverlay.App.BottomSheetDialog" parent="ThemeOverlay.Material3.BottomSheetDialog">
        <item name="bottomSheetStyle">@style/ModalBottomSheetDialog</item>
    </style>

    <style name="ModalBottomSheetDialog" parent="Widget.Material3.BottomSheet.Modal">
        <item name="paddingBottomSystemWindowInsets">false</item>
    </style>

That's not very intuitive or documented but maybe it's just me badly reading the docs.

As for your case - calling new BottomSheetDialog(getContext(), getTheme()) will override the ?attr/bottomSheetDialogTheme settings provided in the main theme. I think that's the main reason why edge-to-edge doesn't really work in your case?

This is actually what the Material BottomSheetDialogFragment do :) But yes this was (I don't know why) the source of the issue from a dialog theme overlay that should not have impacted anything and that was working perfectly before that last commit. Honestly I'm still not 100% sure how everything is supposed to be handled in Material3 but while I've figured out way to have it working for my use case that seems to be the normal case, the fact that the catalog does not work without the added call that normal apps should not have to do (and can't easily in the case of BottomSheetDialogFragment) seems to indicate there's still something odd somewhere.

drchen commented 2 years ago

I agree the whole theming structure of bottom sheet dialogs is kind of confusing. (And I agree we may need to improve our dev doc description.) And yes, paddingBottomSystemWindowInsets needs to be set inside your ?attr/bottomSheetStyle instead of the app theme. (It probably also works if you set it in ?attr/bottomSheetDialogTheme but I'm not 100% sure.

Regarding the edge-to-edge mode, I confirmed that it's documented that bottom sheet dialog will only make it edge-to-edge when enableEdgeToEdge=true and the navigation bar is translucent: https://github.com/material-components/material-components-android/blob/master/docs/components/BottomSheet.md#handling-insets-and-fullscreen

In other words, clients need to "trigger" edge-to-edge mode by setting navigation bar color to a translucent color. It can be a bit confusing but I think it's more or less a standard practice.

I'm going to close the issue for now. Let me know if you have more questions. : )