Material-components-android: [ShapeableImageView] Padding is applied to background, unlike normal ImageView

[drewhamilton]

Description: In ShapeableImageView, android:padding gets applied to both the image itself and the shaped background. This is unusual compared to most views (though maybe typical for Shapeables, not sure).

Expected behavior:
What the normal ImageView does: pads the image, but not the background.
Top: ShapeableImageView with padding. Bottom: normal ImageView with padding.

It seems like the current behavior should be achieved by margin rather than padding, and that padding should leave the background alone, like it does in most Views.

Source code:

<ShapeableImageView
    android:layout_width="48dp"
    android:layout_height="48dp"

    android:background="?colorSecondary"
    android:padding="8dp"

    app:shapeAppearance="?shapeAppearanceSmall"
    app:srcCompat="@drawable/my_icon" />

Android API version: 21-29

Material Library version: 1.2.0-rc01

Device: Pixel 3 emulator

Looking at the ShapeableImageView source, it appears to be clear where the shape is set to honor the padding. I'd like to know if the current behavior is expected for some reason. If it is, I'd like to know if the behavior I want can be accommodated by some new attribute or something.

If there's a clear path forward I'd be happy to open a PR. And if that PR could potentially land in 1.2, I'd be happy to open a PR quickly. :) Let me know!

[ymarian]

Hi the problem is that ShapeableImageView supports stroke like MaterialCardView which handles it the same way.
We rely on people to add padding if they need the stroke to fit the view bounds.

For your case if it's only icons you can work around it easily with an inset drawable?

<inset
  android:drawable="@drawable/my_icon"
  android:insetBottom="8dp"
         .....
/>

[drewhamilton]

Hmm. That's unfortunate. Would it not make sense to pad the background by half the stroke width automatically?

I'd argue that CardView was always a little "weird" about padding like this, but (Shapeable)ImageView has an existing behavior that it should aim to uphold. For this reason I would prefer that android:padding affect only the image while some custom attribute like app:backgroundPadding be created for the background.

But if consistency with MaterialCardView is more important than consistency with ImageView, can I offer a PR with the inverse? I.e. leave padding as is and add some app:imageInset attributes?

My goal is to ship some styles that match specific design guidelines without requiring a custom view, so the inset drawable solution is undesirable.

[drewhamilton]

OK so adding an app:contentPadding attribute in a way that makes sense is pretty difficult. Since ImageView's mDrawable is private, we can't manipulate it directly. And since a stable release now has the background being inset by android:padding, we can't change that behavior. I can think of 3 options:

1. In ShapeableImageView, override setDrawable methods to wrap the original Drawable inside an InsetDrawable. This has some pretty big problems. First, this means getDrawable will return a different-looking drawable than was provided to setDrawable. Second, this means always instantiating a new InsetDrawable whenever a Drawable is set, which is probably an undesirable performance impact. Third, I'm not sure if this would play nice with some of ImageView's internal logic, e.g. around recycling bitmap drawables or mutating the drawable to set a tint.
2. Add app:contentPadding to android:padding so that the image is automatically inset by the sum of both, but subtract app:contentPadding from the sum when insetting the background. This would make the getPadding methods inconsistent with the android:padding attribute which I'm sure is unacceptable. I could maybe add some getPadding overrides and super.getPadding hacks to ShapeableImageView to work around this problem.
3. Instead of anything like app:contentPadding, only add a boolean app:applyPaddingToBackground attribute. This would default to true to maintain the 1.2.0 behavior. It would provide less flexibility than the other options but would be sufficient for my use case at least.

Any more ideas and/or a preference on which direction to go with a PR?

[ymarian]

Hi, 2 sounds good to me if all the combinations work well with stroke and shadows. Otherwise don't see any harm on 3

[MarinKacaj]

I had the same problem, and ran into this issue.
IMO, option 2 is also better aligned with MaterialCardView, so it's less of a surprise for me as a consumer of this library.

[drewhamilton]

I believe I have this working on a local branch. But

1. It depends on #1769. Should I wait for that to be merged before I open a PR, or still open a PR with the same changes?
2. I'm having trouble getting the IDE to recognize the existing tests as test sources so I'm not sure how to go about writing tests for this. Any pointers?