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.

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

[bottomsheet] do not remove corners when bottomsheet is not fully expanded #437

Closed
wants to merge 1 commit into from

Conversation

hrach
Copy link
Contributor

@hrach hrach commented Jul 13, 2019

Currently, when bottomsheet is in expanded state and its content is wrapped (fitToContents) then the corners are animated to 0dp. This leads to quite unwanted behavior. This PR fixes this to check fitToContents variable.

Screenshots attached from catalog app. (large shape corner size in settings required):

Before:

device-2019-07-13-121306

After:

device-2019-07-13-121429

@ikim24
Copy link
Contributor

ikim24 commented Jul 22, 2019

Please create a GitHub issue for tracking this - it will help us prioritize reviewing this. Thanks!

@ldjcmu
Copy link
Contributor

ldjcmu commented Sep 30, 2019

I have bad news and good news.

Bad news: Our design team is strongly opinionated that rounded corners indicate scrollable content while flat corners indicate that there is no additional content. As such, they do no want us to add this change with fitToContents.

Good news: You should be able to fix this for your case by setting up a callback and overriding the shapeAppearance in the expanded state.

@ldjcmu ldjcmu closed this Sep 30, 2019
@hrach
Copy link
Contributor Author

hrach commented Sep 30, 2019

Please, which callback do you mean?

@ldjcmu
Copy link
Contributor

ldjcmu commented Sep 30, 2019

I believe BottomSheetBehavior#addBottomSheetCallback.

@ldjcmu
Copy link
Contributor

ldjcmu commented Oct 1, 2019

I just realized this method wasn't on github until just now. Sorry about that.

@iPaulPro
Copy link

Bad news: Our design team is strongly opinionated that rounded corners indicate scrollable content while flat corners indicate that there is no additional content. As such, they do no want us to add this change with fitToContents.

This is such an arbitrary decision. The API should default to the designers' opinions, but not lock them in. Design is fleeting, and these types of restrictions don't stop customization, they just make it more difficult. The whole concept of "it can't be customized because our designers feel strongly about it" is short-sighted, at best.

Some other things Google's Android designers have felt strongly about but eventually changed their minds on:

  • There shouldn't be two ways to go "back" on screen.
  • App Bar titles should not be center-aligned.
  • Tabs belong at the top of the screen.
  • Action Bar belongs at the top of the screen.
  • Use dashboard pattern for top-level app navigation.
  • Use Action Bar Spinner for top-level app navigation.
  • Use nav drawer for top-level app navigation.
  • Main action should go in the Action Bar.
  • App logo/icon should go in the Action Bar.
  • Buttons shouldn't have rounded corners.
  • Launcher icons should have distinct silhouettes.
  • Glyphs should be flat and filled (not outlined).
  • Over-scrolling should not be visually indicated.

That's just to name a few.

Please consider adding a proper configuration option to control this behavior. "Designers said no" is simply not a valid reason for omission.

@ZacSweers
Copy link
Contributor

@ldjcmu can you give an example of how that workaround would work? Especially considering only one callback can be set per behavior, it's fairly inconvenient to have to use that for every sheet if this is something we need to work around app-wide.

@akhbulatov
Copy link

I agree with @iPaulPro , please add this as an option so that we don't have to write workaround like now.

@vbuberen
Copy link

Would like to bump this issue.

Another point here - can these so called opinions be added into official Material Guideline, so there will be less misunderstandings? Because this repo already has some issues with some feedback from design team.
If something needs an additional explanation it means that official Material Guidelines fail to do its job on explaining vision of the Material design team.

@alex-signal
Copy link

alex-signal commented May 4, 2020

As an example. Note that you still need to set the shapeAppearnceOverlay in your bottomSheetStyle.

  @Override
  public @NonNull Dialog onCreateDialog(Bundle savedInstanceState) {
    BottomSheetDialog     dialog               = (BottomSheetDialog) super.onCreateDialog(savedInstanceState);
    ShapeAppearanceModel  shapeAppearanceModel = ShapeAppearanceModel.builder()
                                                                     .setTopLeftCorner(CornerFamily.ROUNDED, ViewUtil.dpToPx(requireContext(), 8))
                                                                     .setTopRightCorner(CornerFamily.ROUNDED, ViewUtil.dpToPx(requireContext(), 8))
                                                                     .build();
    MaterialShapeDrawable dialogBackground     = new MaterialShapeDrawable(shapeAppearanceModel);

    dialogBackground.setTint(ThemeUtil.getThemedColor(requireContext(), R.attr.dialog_background_color));

    dialog.getBehavior().addBottomSheetCallback(new BottomSheetBehavior.BottomSheetCallback() {
      @Override
      public void onStateChanged(@NonNull View bottomSheet, int newState) {
        if (newState == BottomSheetBehavior.STATE_EXPANDED) {
          ViewCompat.setBackground(bottomSheet, dialogBackground);
        }
      }

      @Override
      public void onSlide(@NonNull View bottomSheet, float slideOffset) {
      }
    });

    return dialog;
  }

@proninyaroslav
Copy link

@ldjcmu
Our design team is strongly opinionated. Could you really have come up with a different excuse? This is ridiculous.

@gabrielemariotti
Copy link
Contributor

I totally agree with @iPaulPro.
@ldjcmu You could evaluate the possibility to introduce an option to override the default behavior since it doesn't break any guidelines (and code is just written in the PR).

Just to improve the code posted above by @alex-signal:

  @NonNull @Override public Dialog onCreateDialog(@Nullable Bundle savedInstanceState) {
    Dialog dialog = super.onCreateDialog(savedInstanceState);


    ((BottomSheetDialog)dialog).getBehavior().addBottomSheetCallback(new BottomSheetBehavior.BottomSheetCallback() {

      @Override public void onStateChanged(@NonNull View bottomSheet, int newState) {
        if (newState == BottomSheetBehavior.STATE_EXPANDED) {
          //In the EXPANDED STATE apply a new MaterialShapeDrawable with rounded cornes
          MaterialShapeDrawable newMaterialShapeDrawable = createMaterialShapeDrawable(bottomSheet);
          ViewCompat.setBackground(bottomSheet, newMaterialShapeDrawable);
        }
      }

      @Override public void onSlide(@NonNull View bottomSheet, float slideOffset) {

      }
    });

    return dialog;
  }

  @NotNull private MaterialShapeDrawable createMaterialShapeDrawable(@NonNull View bottomSheet) {
    ShapeAppearanceModel shapeAppearanceModel =

      //Create a ShapeAppearanceModel with the same shapeAppearanceOverlay used in the style
      ShapeAppearanceModel.builder(getContext(), 0, R.style.ShapeAppearanceBottomSheetDialog_Rounded)
        .build();

      //Create a new MaterialShapeDrawable (you can't use the original MaterialShapeDrawable in the BottomSheet)
      MaterialShapeDrawable currentMaterialShapeDrawable = (MaterialShapeDrawable) bottomSheet.getBackground();
      MaterialShapeDrawable newMaterialShapeDrawable = new MaterialShapeDrawable((shapeAppearanceModel));

      //Copy the attributes in the new MaterialShapeDrawable
      newMaterialShapeDrawable.initializeElevationOverlay(getContext());
      newMaterialShapeDrawable.setFillColor(currentMaterialShapeDrawable.getFillColor());
      newMaterialShapeDrawable.setTintList(currentMaterialShapeDrawable.getTintList());
      newMaterialShapeDrawable.setElevation(currentMaterialShapeDrawable.getElevation());
      newMaterialShapeDrawable.setStrokeWidth(currentMaterialShapeDrawable.getStrokeWidth());
      newMaterialShapeDrawable.setStrokeColor(currentMaterialShapeDrawable.getStrokeColor());
      return newMaterialShapeDrawable;
  }

with:

  <style name="ShapeAppearanceBottomSheetDialog_Rounded" parent="">
    <item name="cornerFamily">rounded</item>
    <item name="cornerSizeTopRight">16dp</item>
    <item name="cornerSizeTopLeft">16dp</item>
    <item name="cornerSizeBottomRight">0dp</item>
    <item name="cornerSizeBottomLeft">0dp</item>
  </style>

@SamStenner
Copy link

Notwithstanding the fact that this excuse is clearly ridiculous, if we actually spend a moment to think about what they said, the whole thing becomes even more ridiculous. Rounded corners indicate scrollable content? What? If my bottom sheet has rounded corners, and then the user fully expands the sheet until it stops, they very clearly understand that the sheet is expanded. Keeping rounded corners on that wouldn't affect that. I've used dozens of apps that have permanent rounded corners on the bottom sheet, not been a problem, and those designers don't think so either.

So not only is your excuse quite bizarre, but the justification is equally bizarre.

@seanghay
Copy link
Contributor

Bad news: Our design team is strongly opinionated that rounded corners indicate scrollable content while flat corners indicate that there is no additional content. As such, they do no want us to add this change with fitToContents.

I'm surprised! 😲

@seanghay
Copy link
Contributor

Actually, there's an option for us to disable animation by calling disableShapeAnimations() on BottomSheetBehavoir.

+   @SuppressLint("RestrictedApi")
    override fun onCreateDialog(savedInstanceState: Bundle?): BottomSheetDialog {
        val bottomSheetDialog = super.onCreateDialog(savedInstanceState) as BottomSheetDialog
+       bottomSheetDialog.behavior.disableShapeAnimations()
        return bottomSheetDialog
    }

However, the API is still restricted, but you can suppress it by putting @SuppressLint("RestrictedApi"). I'm using com.google.android.material:material:1.3.0-alpha02

Example

import android.annotation.SuppressLint
import android.os.Bundle
import com.google.android.material.bottomsheet.BottomSheetDialog
import com.google.android.material.bottomsheet.BottomSheetDialogFragment

abstract class BottomSheetFragment : BottomSheetDialogFragment() {

    @SuppressLint("RestrictedApi")
    override fun onCreateDialog(savedInstanceState: Bundle?): BottomSheetDialog {
        val bottomSheetDialog = super.onCreateDialog(savedInstanceState) as BottomSheetDialog
        bottomSheetDialog.behavior.disableShapeAnimations()
        return bottomSheetDialog
    }

}

@eneim
Copy link

eneim commented Sep 12, 2020

Have not tried yet, but I'm afraid using disableShapeAnimations will cause the shape to be initialized without corners after configuration change. One of another practice I'm testing is to use reflection to get the ValueAnimator and set a custom Evaluator.

@Fishfield
Copy link

I just stumbled upon another solution to keep the rounded corners. If you just set cornerSize, the rounded corners stay as they are when the bottom sheet is expanded. I hope this helps some of you in order to avoid those hacky workarounds

<style name="ShapeAppearance.App.LargeComponent" parent="">
	<item name="cornerFamily">rounded</item>
	<item name="cornerSize">@dimen/corner_radius_s</item>
</style>

@CharlyLafon37
Copy link

I have bad news and good news.

Bad news: Our design team is strongly opinionated that rounded corners indicate scrollable content while flat corners indicate that there is no additional content. As such, they do no want us to add this change with fitToContents.

Good news: You should be able to fix this for your case by setting up a callback and overriding the shapeAppearance in the expanded state.

Screenshot of Youtube app on Android :

Screenshot_20201105-235346_2

Google's design team seems to have very fickle design opinions 🤔

@ldjcmu Please reconsider this PR or at least provide us a stable and reliable way to achieve what so many developers here want. We would be very grateful.

@dptsolutions
Copy link

Used @seanghay 's workaround and it seems to work properly. Really hope this just becomes a public API. My designer's do not like being boxed in by the opinion's of Material Design, and this is the path of least resistance.

@gmk57
Copy link

gmk57 commented Jan 10, 2021

If you just set cornerSize, the rounded corners stay as they are when the bottom sheet is expanded.

Unfortunately this does not work for me. But custom drawable seems to work fine.

Maybe it's worth voting for corresponding issue on Google Issue Tracker.

@rickchristie
Copy link

If you need fitToContents and rounded corner, I tried the solution by hooking into onStateChanged, but it flickers from the straight to rounded corners once the dialog reaches EXPANDED state. It's quite annoying.

I want to share the workaround I found.

Looking at the code of BottomSheetBehavior we find:

  /** True if Behavior has a non-null value for the @shapeAppearance attribute */
  private boolean shapeThemingEnabled;

Turns out if shape theming is disabled, MaterialShapeDrawable will not be used. We find this in BottomSheetBehavior.onLayout():

// Only set MaterialShapeDrawable as background if shapeTheming is enabled, otherwise will
// default to android:background declared in styles or layout.
if (shapeThemingEnabled && materialShapeDrawable != null) {
  ViewCompat.setBackground(child, materialShapeDrawable);
}

Defaulting to android:background is exactly what we need, as this means complete control on how the background is rendered.

We can disable material theming by creating a separate style and setting shapeAppearance and shapeAppearanceOverlay to null:

<style name="Theme.YourApp.NoShapeBottomSheetDialog" parent="Theme.MaterialComponents.BottomSheetDialog">
  <item name="bottomSheetStyle">@style/Theme.YourApp.NoShapeButtonSheet</item>
</style>

<style name="Theme.YourApp.NoShapeButtonSheet" parent="Widget.MaterialComponents.BottomSheet.Modal">
  <item name="shapeAppearance">@null</item>
  <item name="shapeAppearanceOverlay">@null</item>
</style>

Extend BottomSheetDialogFragment and override onCreateDialog:

public Dialog onCreateDialog(@Nullable Bundle savedInstanceState) {
  return new BottomSheetDialog(requireContext(),
                R.style.Theme_Grupin_NoShapeBottomSheetDialog);
}

The bottom sheet is now naked, without any background at all. So we can add any background we want, no animation will be triggered anymore.

@CharlyLafon37
Copy link

Nice solution @rickchristie thanks a lot !

@avocadochif
Copy link

I'm agree with people above, just add this feature to public API and let us think out of box exclude your Material Design team.

@a-szotyori
Copy link

+1

@wixeless
Copy link

Try this
class MyBottomSheer: BottomSheetDialogFragment() {

override fun onViewCreated(view: View, savedInstanceState: Bundle?) {
    super.onViewCreated(view, savedInstanceState)
    disableShapeAnimation()
}

@SuppressLint("RestrictedApi", "VisibleForTests")
private fun disableShapeAnimation() {
    try {
        val dlg = dialog as BottomSheetDialog
        dlg.behavior.disableShapeAnimations()
    } catch (ex: Exception) {
        Log.e("BaseBottomSheet", "disableShapeAnimation Exception:", ex)
    }
}

}

jonnyandrew added a commit to element-hq/element-android that referenced this pull request Oct 26, 2022
jonnyandrew added a commit to element-hq/element-android that referenced this pull request Oct 26, 2022
jmartinesp pushed a commit to element-hq/element-android that referenced this pull request Oct 26, 2022
* Add new attachments selection dialog

* Add rounded corners to bottom sheet dialog.

Note these are currently only visible in the collapsed state.
- [Google issue](https://issuetracker.google.com/issues/144859239)
- [Rejected PR](material-components/material-components-android#437)
- [Github issue](material-components/material-components-android#1278)

* Add changelog entry

* Remove redundant call to superclass click listener

* Refactor to use view visibility helper

* Change redundant sealed class to interface

* Remove unused string

* Revert "Add rounded corners to bottom sheet dialog."

This reverts commit 17c43c9.

* Remove redundant view group

* Remove redundant `this`

* Update rich text editor to latest

* Update rich text editor version

* Allow toggling rich text in the new editor

* Persist the text formatting setting

* Add changelog entry
jmartinesp pushed a commit to element-hq/element-android that referenced this pull request Nov 2, 2022
* Add new attachments selection dialog

* Add rounded corners to bottom sheet dialog.

Note these are currently only visible in the collapsed state.
- [Google issue](https://issuetracker.google.com/issues/144859239)
- [Rejected PR](material-components/material-components-android#437)
- [Github issue](material-components/material-components-android#1278)

* Add changelog entry

* Remove redundant call to superclass click listener

* Refactor to use view visibility helper

* Change redundant sealed class to interface

* Remove unused string

* Revert "Add rounded corners to bottom sheet dialog."

This reverts commit 17c43c9.

* Remove redundant view group

* Remove redundant `this`

* Update rich text editor to latest

* Update rich text editor version

* Allow toggling rich text in the new editor

* Persist the text formatting setting

* Add changelog entry
@mikekudzin
Copy link

Starting from 1.8.0-alpha2 shouldRemoveExpandedCorners style attribute is available to override default behaviour.
Starting from 1.10.0-alpha2 setShouldRemoveExpandedCorners exposed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.