material-components / material-components-android

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

[Button] Double ripple with icon buttons in 1.7.0-alpha03 #2833

Open OxygenCobalt opened 2 years ago

OxygenCobalt commented 2 years ago

Description: When I use my custom Icon Button style in 1.7.0-alpha03, a strange behavior occurs where two ripples appear. One ripple will appear where I touch (expected behavior), but another will appear in the center.

Here is a demonstration in the catalog (with the tonal button modified to use my style and without the icon to see the ripple clearer):

https://user-images.githubusercontent.com/65370175/179551670-4bf8769c-0611-4bcd-9cb0-438534dad11d.mp4

Here is a close-up of the two ripples to show that I'm not just seeing things:

image

This was seemingly introduced by the update to AppCompat 1.5.0-beta01 in e968929c08228f475f0f09c302da437a97180284. Re-compiling the project with AppCompat 1.4.0 solves the issue.

Expected behavior: Only one ripple should appear at the point where I pressed the button.

Source code: I use this style for my button

<style name="Widget.Auxio.Button.PlayPause" parent="Widget.Material3.Button.IconButton.Filled.Tonal">
    <item name="android:minWidth">72dp</item>
    <item name="android:minHeight">72dpitem>
    <item name="iconSize">32dpitem>
    <item name="android:insetTop">0dp</item>
    <item name="android:insetBottom">0dp</item>
    <item name="android:insetLeft">0dp</item>
    <item name="android:insetRight">0dp</item>
    <item name="android:paddingStart">12dp<item>
    <item name="android:paddingEnd">12dp<item>
    <item name="android:paddingTop">12dp<item>
    <item name="android:paddingBottom">12dp</item>
    <item name="shapeAppearanceOverlay">
        @style/ShapeAppearanceOverlay.Material3.FloatingActionButton
    </item>
</style>

Android API version: Any API version

Material Library version: 1.7.0-alpha03

Device: OnePlus 7T on Android 11. Probably also happens on Android 12+, but I can't completely tell due to the new ripple style.

OxygenCobalt commented 2 years ago

Any update on this? I'm unable to update several dependencies now, such as coil, as they are all bringing in the newer AppCompat versions and causing this issue.

raajkumars commented 2 years ago

You mentioned that you are using a custom style. Does the issue occur without the custom style?

OxygenCobalt commented 2 years ago

Yes, it does @raajkumars. Even a MaterialButton with the default style will display this strange ripple behavior.

raajkumars commented 2 years ago

@OxygenCobalt Thanks for confirming. From what you describe this appears to issue in the AppCompat framework. We will look into this and post an update.

OxygenCobalt commented 2 years ago

By the way at @raajkumars, this bug only occurs from appcompat 1.5.0-beta01 onwards. So I guess you could take a look at these commits so see where the issue was introduced. I can't really identify where it could have shown up, and I can't really bisect on my end to find the issue.

OxygenCobalt commented 2 years ago

Found this issue in the appcompat issue tracker. No updates though.

OxygenCobalt commented 1 year ago

Still shocked this hasn't been resolved, albeit I get it given that AppCompat isn't given too much attention. I'm going to try to debug this myself and make a PR for the AppCompat library.

OxygenCobalt commented 1 year ago

Okay, I am almost certain that the issue is caused by this diff in androidx commit f2936e8c0b6a4b73d214b31edf8cde566a879dcb or e8769968f3530f3dc40a547c373bcfb00ad72c13. It is the only code remotely related to Drawables in the 1.5.0-beta01 creation, and there the class this is in (ResourceManagerInternal) has direct connections to AppCompatButton and thus MaterialButton.

Here's the combined diff.

@@ -27,6 +27,7 @@ import android.graphics.PorterDuff;
 import android.graphics.PorterDuffColorFilter;
 import android.graphics.drawable.Drawable;
 import android.graphics.drawable.Drawable.ConstantState;
+import android.graphics.drawable.LayerDrawable;
 import android.os.Build;
 import android.util.AttributeSet;
 import android.util.Log;
@@ -437,10 +438,22 @@ public final class ResourceManagerInternal {
     }

     static void tintDrawable(Drawable drawable, TintInfo tint, int[] state) {
-        if (DrawableUtils.canSafelyMutateDrawable(drawable)
-                && drawable.mutate() != drawable) {
-            Log.d(TAG, "Mutated drawable is not the same instance as the input.");
-            return;
+        int[] drawableState = drawable.getState();
+
+        boolean mutated = false;
+        if (DrawableUtils.canSafelyMutateDrawable(drawable)) {
+            mutated = drawable.mutate() == drawable;
+            if (!mutated) {
+                Log.d(TAG, "Mutated drawable is not the same instance as the input.");
+                return;
+            }
+        }
+
+        // Workaround for b/232275112 where LayerDrawable loses its state on mutate().
+        if (drawable instanceof LayerDrawable && drawable.isStateful()) {
+            // Clear state first, otherwise setState() is a no-op.
+            drawable.setState(new int[0]);
+            drawable.setState(drawableState);
         }

         if (tint.mHasTintList || tint.mHasTintMode) {

@raajkumars Can you please forward this to the androidx team? I've reported the issue here and have gotten no response whatsoever. I cannot report it the issue on the androidx github repository. My app has had to use obsoleted versions of appcompat, coil, and MDC for half a year now because of this visual error.

raajkumars commented 1 year ago

@OxygenCobalt Thanks for the detailed root cause analysis.

drchen commented 1 year ago

I can't reproduce it... (It doesn't seem that AppCompat has touched the code you pointed out though.)

Is this still reproducible if you update AppCompat version to 1.6.1?

OxygenCobalt commented 1 year ago

Hm. Were there any changes to MDC's ripple code between 1.7.0 and 1.8.0? I'm not able to test it right now since I am extremely busy, but I will get back to you as soon as possible. @drchen

drchen commented 1 year ago

I can't recall anything regarding the button or ripple implementation change. (But we do update our AppCompat dependency version, not sure if it's related.)

OxygenCobalt commented 1 year ago

Very strange then. I remember trying AppCompat 1.7.0-alpha01 and seeing that the bug could still be re-produced. I'll go check again with different versions of MDC and AppCompat when I finally get time.

SiddiquiUbaid commented 1 year ago

Helpful

OxygenCobalt commented 1 year ago

I can still reproduce it @drchen. You can tell because when you press the button, the ripple color appears more "intense" initially, eventually it disappears leaving the (correct) less-intense ripple highlight color. That is the double ripple I am referencing, however due to how fast it is it's easy to misinterpret it as WAI. I've attached a video demonstration.

With the bug:

https://user-images.githubusercontent.com/65370175/218358071-043ef5b2-39b9-4f72-9b65-247781e72d70.mp4

Without the bug:

https://user-images.githubusercontent.com/65370175/218358097-0b9a1dd2-d552-4d86-bf98-cd2288ffcb83.mp4

OxygenCobalt commented 1 year ago

Another update: I've confirmed that MDC's RippleDrawableCompat is not related to the issue. If you are any RippleDrawable on any AppCompatButton, it will have a double ripple. Still trying to find the exact process where ResourceManagerInternal breaks the drawable.

hunterstich commented 1 year ago

Hi @OxygenCobalt,

Is this still an issue related to Material? Any updates here?

OxygenCobalt commented 1 year ago

It is not an issue related to MDC @hunterstich. It's actually an issue where AppCompatButton mangles any RippleDrawable provided to it. Feel free to close this and forward people to the proper issuetracker entry.

fsgjlp commented 1 year ago

All material buttons are broken after the update and it's a pretty critical issue. Is there an ETA for when this will be fixed?

raajkumars commented 1 year ago

@OxygenCobalt Thanks for the detailed analysis of the issue. Since the issue is caused by AppCompat, I expect that the fix for this issue will be made available in an upcoming AppCompat update.

afohrman commented 1 year ago

Reopening this to explore a possible fix suggested in https://issuetracker.google.com/262784311#comment5. See https://github.com/material-components/material-components-android/issues/3605#issuecomment-1737977097 for more background about this.

fsgjlp commented 4 months ago

Are there any updates on this issue? It's been two years.