[ShapeableImageView] Fix padding bugs #2280
Conversation
Test setting padding before measure. Fix logic errors in ShapeableImageView
lib/java/com/google/android/material/imageview/ShapeableImageView.java
Outdated
Show resolved
Hide resolved
lib/java/com/google/android/material/imageview/ShapeableImageView.java
Outdated
Show resolved
Hide resolved
| super.getPaddingBottom() - bottomContentPadding + bottom); | ||
| // If onMeasure hasn't adjusted padding yet, wait for it to be set to avoid double-applying the | ||
| // content padding in onMeasure: | ||
| if (hasAdjustedPaddingAfterLayoutDirectionResolved) { |
There was a problem hiding this comment.
Will this affect API >= 19?
There was a problem hiding this comment.
Yes, hasAdjustedPaddingAfterLayoutDirectionResolved doesn't get set until after onMeasure runs after isLayoutDirectionResolved (which is an API 19+ method) becomes true. (So really it affects 19+ more than it affects pre-19.)
There was a problem hiding this comment.
Got it. Don't you need to call setContentPadding() again when hasAdjustedPaddingAfterLayoutDirectionResolved is set to true? So the previous set content paddings can be applied?
There was a problem hiding this comment.
No. setContentPadding does two things:
- It calls
super.setPaddingaccording to this class's internal padding contract. This is skipped if this flag is not set, butsuper.setPaddingis also called immediately after the flag is set, so this call is covered either way. - It sets the private
contentPaddingvariables, regardless of whether that flag is set.
The private contentPadding variables are only used in the getContentPadding and getPadding getters. updateShapeMask uses the getPadding getters to set the shape size, so now I wonder if I need to force a call to updateShapeMask immediately after that flag is set too...
There was a problem hiding this comment.
That's a good catch. I think onSizeChanged() will be called after setting paddings, right? That should be fine I guess.
There was a problem hiding this comment.
I think onSizeChanged is not called in that case actually. I'll need to try to force an error this way to be sure, and add a related test if possible
| super.getPaddingBottom() - bottomContentPadding + bottom); | ||
| // If onMeasure hasn't adjusted padding yet, wait for it to be set to avoid double-applying the | ||
| // content padding in onMeasure: | ||
| if (hasAdjustedPaddingAfterLayoutDirectionResolved) { |
| if (hasAdjustedPaddingAfterLayoutDirectionResolved) { | ||
| // Super padding is equal to background padding + content padding. Adjust the content padding | ||
| // portion of the super padding here: | ||
| super.setPadding( |
There was a problem hiding this comment.
It looks like there might be a bug if content padding are set twice, and the first time hasAdjustedPaddingAfterLayoutDirectionResolved is false and the second time it's true.
At the second call, super's paddings are not actually updated with content paddings yet, so subtracting the content paddings will causes extra paddings being subtracted. Can you also add test cases for this situation after you fix it?
There was a problem hiding this comment.
At your second call to setContentPadding, super's paddings have been updated, because setPadding is called in onMeasure immediately after hasAdjust... was set, and setPadding then called super.setPadding with the added differences.
In any case I'll look to add tests that check this.
| super.getPaddingBottom() - bottomContentPadding + bottom); | ||
| // If onMeasure hasn't adjusted padding yet, wait for it to be set to avoid double-applying the | ||
| // content padding in onMeasure: | ||
| if (hasAdjustedPaddingAfterLayoutDirectionResolved) { |
There was a problem hiding this comment.
That's a good catch. I think onSizeChanged() will be called after setting paddings, right? That should be fine I guess.
| private static final Class<CutCornerTreatment> CUT_CORNER_FAMILY_CLASS = CutCornerTreatment.class; | ||
|
|
||
| // Valid measureSpec values copied from a debugging session: | ||
| private static final int WIDTH_MEASURE_SPEC = 0x4000_03da; |
There was a problem hiding this comment.
Q) Can't we just use UNSPECIFIED here?
| forceResolveLayoutDirection(imageView); | ||
| imageView.onMeasure(WIDTH_MEASURE_SPEC, HEIGHT_MEASURE_SPEC); | ||
|
|
||
| assertThat(imageView.getContentPaddingStart()).isEqualTo(5); |
There was a problem hiding this comment.
Can we also add tests to check if super.getPaddingTop/Bottom/Start/End() returns correct values?
Thanks for starting a pull request on Material Components!
Don't forget:
[Buttons] Updated documentationcloses #1234Fixes #2063.
Ensures that content padding values are not propagated to their respective
superpadding values until afteronMeasurehas completed withisLayoutDirectionResolved() == trueat least once. This was already the case insideonMeasure, but not the case insetContentPaddingandgetPadding*methods.The internal logic required to make this work is hard to reason about, so I added
ShapeableImageViewTestto test that both types of padding work as expected on multiple Android SDKs.