material-components / material-components-android

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

[TopAppBar] Spacing does not match M3 specs #2742

Open OxygenCobalt opened 2 years ago

OxygenCobalt commented 2 years ago

Is your feature request related to a problem? Please describe. According to the Material Design 3 docs, this is how a Top App Bar should be laid out:

image

There should be 16dp of spacing from the edge of the screen to the icon, and each icon should be spaced 24dp apart.

MaterialToolbar (which is based on AppCompat Toolbar) is inconsistent with this design for three reasons:

  1. The overflow icon has a minWidth of 36dp. It should be 48dp.
  2. The navigation icon is also sized strangely, with it taking up 56dp instead of 48dp.
  3. The toolbar does not pad correctly. While the navigation icon seems to be padded right, the action buttons have no padding whatsoever. The Toolbar does apply padding on tablet layouts, but not only is it 8dp, it is also applied to the already-padded navigation icon.

Describe the solution you'd like I want the library to override the Toolbar styles to remove these inconsistencies. I would bring this over to the core AppCompat libraries, but I imagine that these quirks exist due to compat reasons, so I feel like it would be better if such was overridden in the material design library.

Describe alternatives you've considered I've figured out how to implement this myself using some ugly hacks.

To resize the overflow icon, I redefined actionOverflowButtonStyle to this:

<style name="Widget.MyApp.Button.Overflow" parent="@style/Widget.AppCompat.ActionButton.Overflow">
    <!-- 48dp + no padding hacks -->
    <item name="android:minWidth">48dp</item>
    <item name="android:minHeight">48dp</item>
    <item name="android:paddingStart">0dp</item>
    <item name="android:paddingEnd">0dp</item>
</style>

To resize the navigation icon, I redefined toolbarNavigationButtonStyle to this:

<style name="Widget.MyApp.Button.Navigation" parent="@style/Widget.AppCompat.Toolbar.Button.Navigation">
    <!-- Can't change the height, but we can change the width -->
    <item name="android:minWidth">48dp</item>
</style>

To resolve the padding issue, I defined a custom toolbar style to this in a plain values folder:

<style name="Widget.MyApp.Toolbar" parent="">
    <!-- Navigation icon already pads correctly, do not pad it -->
    <item name="android:layout_marginEnd">4dp</item>
</style>

And then defined the same style as this in values-sw600dp:

<style name="Widget.MyApp.Toolbar" parent="">
    <!-- Remove the 8dp navigation padding (navigation icon already pads correctly) -->
    <item name="android:layout_marginStart">-8dp</item>
    <!-- Remove half of the 8dp navigation padding (keep 4dp) -->
    <item name="android:layout_marginEnd">-4dp</item>
</style>

This causes the toolbar to go from this: image

To this:

image

Which I feel is much more consistent with the Material Design guidelines.

Additional context I don't know what the reasoning is behind this. I hope it is simply some legacy cruft that this library can clear out. If this isn't possible, I'm fine with my hacks to fix this.

OxygenCobalt commented 2 years ago

Update: I've been able to simplify the margin logic somewhat with this class:

/** [MaterialToolbar] that automatically fixes padding in order to align with the M3 specs. */
class M3Toolbar : MaterialToolbar {
    constructor(context: Context) : super(context)

    constructor(context: Context, attrs: AttributeSet?) : super(context, attrs)

    constructor(
        context: Context,
        attrs: AttributeSet?,
        @AttrRes defStyleAttr: Int
    ) : super(context, attrs, defStyleAttr)
    init {
        val tinySpacing = TypedValue.applyDimension(TypedValue.COMPLEX_UNIT_DIP, 4f)

        if (navigationIcon != null) {
            updatePaddingRelative(start = tinySpacing)
        }

        if (menu != null) {
            updatePaddingRelative(end = tinySpacing)
        }
    }
}

This assumes the overflow button and navigation button fixes are in place.

dsn5ft commented 2 years ago

Hey @OxygenCobalt, I spent some time looking into this and we can match the new spec by making the following style updates:

  <style name="Widget.MyApp.Toolbar" parent="Widget.Material3.Toolbar">
    <item name="maxButtonHeight">48dp</item>
    <item name="android:paddingLeft">4dp</item>
    <item name="android:paddingStart">4dp</item>
    <item name="android:paddingRight">4dp</item>
    <item name="android:paddingEnd">4dp</item>
  </style>
  <style name="Widget.MyApp.Toolbar.Button.Navigation" parent="Widget.AppCompat.Toolbar.Button.Navigation">
    <!-- Note: height is handled via maxButtonHeight attribute in Toolbar style. -->
    <item name="android:minWidth">48dp</item>
  </style>
  <style name="Widget.MyApp.ActionButton.Overflow" parent="Widget.AppCompat.ActionButton.Overflow">
    <item name="android:minWidth">48dp</item>
    <item name="android:minHeight">48dp</item>
  </style>

And then in your app theme:

    <item name="toolbarStyle">@style/Widget.MyApp.Toolbar</item>
    <item name="toolbarNavigationButtonStyle">@style/Widget.MyApp.Toolbar.Button.Navigation</item>
    <item name="actionOverflowButtonStyle">@style/Widget.MyApp.ActionButton.Overflow</item>

You shouldn't need your M3Toolbar class or any custom margin logic. Here's a before and after with the above changes:

Before After
image image

I'm working on submitting these changes to the library, but it will take some time due to internal migrations/issues. Please give it a try and let me know what you think and if you find any issues.

OxygenCobalt commented 2 years ago

Works well aside from one issue @dsn5ft: You never fixed the padding that the overflow icon applied to compensate for it's smaller width. You can fix it with this:

  <style name="Widget.MyApp.ActionButton.Overflow" parent="Widget.AppCompat.ActionButton.Overflow">
    <item name="android:minWidth">48dp</item>
    <item name="android:minHeight">48dp</item>
    <item name="android:paddingStart">0dp</item>
    <item name="android:paddingEnd">0dp</item>
  </style>

Otherwise, it works perfectly with the existing material-components style system. Don't know why this isn't a simple "plug-and-play" operation for the team. Thanks for actually finding a way to clamp the navigation icon's height. Thought that was impossible.

dsn5ft commented 2 years ago

@OxygenCobalt thanks for testing.

You never fixed the padding that the overflow icon applied to compensate for it's smaller width.

Hmm gotcha. That doesn't seem to matter though, right? Since the padding is only 6dp on each side, and the icon is probably 24dp, that totals to 36dp which is smaller than the 48dp min width anyway.

Don't know why this isn't a simple "plug-and-play" operation for the team.

It affects a lot of apps using Material internally, so it just takes a while to land the update.

Thanks for actually finding a way to clamp the navigation icon's height. Thought that was impossible.

👍

OxygenCobalt commented 2 years ago

Theoreticaly it should not have an effect @dsn5ft, but in practice, it does effect the visuals of the icon:

This is what it looks like without the fix:

image

And this is what it looks like with the fix:

image

The padding still shifts the icon to the start just slightly towards the right. Hence why I think it should be removed.

This also makes sense, as in the androidx code, paddingStart is 6dp, yes, but paddingEnd is 10dp, which would cause it to be misaligned as such.

dsn5ft commented 2 years ago

🤦‍♂️ ughhh you are totally right. Now I shall restart the process of landing this change... Thanks for finding this!

OxygenCobalt commented 2 years ago

Any updates on this @dsn5ft?

RequestPrivacy commented 1 year ago

Can confirm that this is still an issue at the end of year 2023. Applying the fix from above solves it though...