[Android] Fix ImageButton Padding the third #25223
Conversation
|
Hey there @aheubusch! Thank you so much for your PR! Someone from the team will get assigned to your PR shortly and we'll get it reviewed. |
| platformButton.SetContentPadding((int)padding.Left, (int)padding.Top, (int)padding.Right, (int)padding.Bottom); | ||
| platformButton.SetPadding(0, 0, 0, 0); | ||
|
|
||
| // Just like before, the bugs are not done. This needs to trigger a re-calculation of |
There was a problem hiding this comment.
@mattleibow You have added this part (ShapeAppearanceModel) in #22298. I am not sure if I can just remove it. None of the tests that looked relevant failed on my machine.
There was a problem hiding this comment.
I am scared tbh. I forget the corner cases where this happens - and they yield is especially scary. I think I was going off the fixes the native side was doing.
The current test set may not be enough and we will have to go remove this and see what breaks manually, and then make your change to see if it fixes it.
|
/azp run |
|
Azure Pipelines successfully started running 3 pipeline(s). |
|
Not sure if the test failure are because of this PR. They all fail with a |
|
/rebase |
Remove ShapeableImageView Padding immediately after it is set in its onMeasure method to avoid double padding (ContentPadding + Padding).
github-actions
bot
force-pushed
the
android_imagebutton_padding
branch
from
0a0de1e to
8a3a074
Compare
|
Azure Pipelines successfully started running 3 pipeline(s). |
There was a problem hiding this comment.
I am super scared with this as the ImageButton was super fragile and I think I have seen padding issues but was super hard to repro in a test or I was not able to reliably repro.
I added tests as I did things, and TJ probably added more. But, I am not sure there are enough.
This comment does not help move the PR along, but I need to play a bit more and try and repro the issues that required the terrible hacks.
| platformButton.SetContentPadding((int)padding.Left, (int)padding.Top, (int)padding.Right, (int)padding.Bottom); | ||
| platformButton.SetPadding(0, 0, 0, 0); | ||
|
|
||
| // Just like before, the bugs are not done. This needs to trigger a re-calculation of |
There was a problem hiding this comment.
I am scared tbh. I forget the corner cases where this happens - and they yield is especially scary. I think I was going off the fixes the native side was doing.
The current test set may not be enough and we will have to go remove this and see what breaks manually, and then make your change to see if it fixes it.
|
Any chance to get this in for the next release? Both |
Description of Change
Improve the workaround (#22298) for the iffy Android ShapeImageView contentPadding/padding handling.
The underlying problem is that ShapeImageView updates Padding in onMeasure leading to a double padding. The previous workaround reset the Padding after one eventloop hoping that onMeasure was called in the meantime. This worked for most cases but not all. The "improved" workaround overrides onMeasure and resets the Padding as soon as it was set.
I ran all ImageButton tests I could find and they are all green. So thats a good sign.
Issues Fixed
Fixes #25201
Fixes #16713
Fixes #18001
Fixes #13101