[ShapeableImageView] Fix padding bugs by drewhamilton · Pull Request #2280 · material-components/material-components-android

drewhamilton · 2021-07-01T12:01:43Z

Thanks for starting a pull request on Material Components!

Don't forget:

Identify the component the PR relates to in brackets in the title.
[Buttons] Updated documentation
Link to GitHub issues it solves. closes #1234
Sign the CLA bot. You can do this once the pull request is opened.

Fixes #2063.

Ensures that content padding values are not propagated to their respective super padding values until after onMeasure has completed with isLayoutDirectionResolved() == true at least once. This was already the case inside onMeasure, but not the case in setContentPadding and getPadding* methods.

The internal logic required to make this work is hard to reason about, so I added ShapeableImageViewTest to test that both types of padding work as expected on multiple Android SDKs.

Test setting padding before measure. Fix logic errors in ShapeableImageView

lib/java/com/google/android/material/imageview/ShapeableImageView.java

drchen · 2021-07-13T20:01:08Z

lib/java/com/google/android/material/imageview/ShapeableImageView.java

-        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) {


Will this affect API >= 19?

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.)

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?

No. setContentPadding does two things:

It calls super.setPadding according to this class's internal padding contract. This is skipped if this flag is not set, but super.setPadding is also called immediately after the flag is set, so this call is covered either way.

It sets the private contentPadding variables, 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...

That's a good catch. I think onSizeChanged() will be called after setting paddings, right? That should be fine I guess.

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

drchen · 2021-07-13T20:01:19Z

lib/java/com/google/android/material/imageview/ShapeableImageView.java

-        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) {


drchen · 2021-07-14T13:48:47Z

lib/java/com/google/android/material/imageview/ShapeableImageView.java

+    if (hasAdjustedPaddingAfterLayoutDirectionResolved) {
+      // Super padding is equal to background padding + content padding. Adjust the content padding
+      //  portion of the super padding here:
+      super.setPadding(


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?

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.

drchen · 2021-07-14T13:55:06Z

lib/java/com/google/android/material/imageview/ShapeableImageView.java

-        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) {


That's a good catch. I think onSizeChanged() will be called after setting paddings, right? That should be fine I guess.

drchen · 2021-08-17T16:46:37Z

lib/javatests/com/google/android/material/imageview/ShapeableImageViewTest.java

+  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;


Q) Can't we just use UNSPECIFIED here?

drchen · 2021-08-18T14:29:40Z

lib/javatests/com/google/android/material/imageview/ShapeableImageViewTest.java

+    forceResolveLayoutDirection(imageView);
+    imageView.onMeasure(WIDTH_MEASURE_SPEC, HEIGHT_MEASURE_SPEC);
+
+    assertThat(imageView.getContentPaddingStart()).isEqualTo(5);


Can we also add tests to check if super.getPaddingTop/Bottom/Start/End() returns correct values?

drewhamilton added 3 commits June 30, 2021 15:04

Fix incorrectly refactored onMeasure logic in ShapeableImageView

9f4d553

Only apply super padding after layout direction is resolved

7d8ed42

Add ShapeableImageViewTest

3e6a663

Test setting padding before measure. Fix logic errors in ShapeableImageView

google-cla bot added the cla: yes label Jul 1, 2021

drewhamilton commented Jul 1, 2021

View reviewed changes

lib/java/com/google/android/material/imageview/ShapeableImageView.java Outdated Show resolved Hide resolved

drchen reviewed Jul 13, 2021

View reviewed changes

drewhamilton added 2 commits July 14, 2021 08:55

Revert to original logic structure in ShapeableImageView#onMeasure

e010ee5

Revert unwanted comment and format changes

199d595

drchen reviewed Jul 14, 2021

View reviewed changes

drchen reviewed Aug 18, 2021

View reviewed changes

drchen added the Review in progress label Nov 3, 2021

Conversation

drewhamilton commented Jul 1, 2021

Thanks for starting a pull request on Material Components!

Don't forget:

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants