[TextInputLayout] Add baseline alignment support #56
Conversation
|
@dsn5ft Mind taking a look at this one? |
|
@cketcham bump |
| @@ -795,6 +795,12 @@ public EditText getEditText() { | |||
| return editText; | |||
| } | |||
|
|
|||
| @Override | |||
| public int getBaseline() { | |||
| EditText text = getEditText(); | |||
There was a problem hiding this comment.
TextInputLayout already has an editText field. That field should be used instead.
| @Override | ||
| public int getBaseline() { | ||
| EditText text = getEditText(); | ||
| return text == null ? super.getBaseline() : text.getPaddingTop() + text.getBaseline(); |
There was a problem hiding this comment.
Returns statements in the codebase generally follow the opposite pattern for ordering null checks in ternary operations: editText != null ? ____ : ____ ;
| @Override | ||
| public int getBaseline() { | ||
| EditText text = getEditText(); | ||
| return text == null ? super.getBaseline() : text.getPaddingTop() + text.getBaseline(); |
There was a problem hiding this comment.
Have you verified that this works? getPaddingTop() alone won't provide the correct baseline, since TextInputLayout handles top spacing differently based in its boxBackgroundMode. calculateLabelMarginTop() is probably closer to what you want, although I don't think that will be enough to provide the correct baseline, either. You should also be aware that TextInputLayout adds spacing to the bottom of the box background, (boxBottomOffsetPx) which would throw off the calculation unless it's accounted for.
You should be able to get the correct baseline by offsetting the EditText's baseline by calculateLabelMarginTop(), getPaddingTop(), and the bottom offset.
There was a problem hiding this comment.
Are you sure all that is necessary? We've been using it for several months now in FirebaseUI without any problems. Also, both of these Stack Overflow questions found the same resolution.
If you think it won't work, would you mind explaining what boxBackgroundMode is and how I could activate it?
Example screenshot:
There was a problem hiding this comment.
Yep, all that is necessary. This solution would help with aligning legacy text fields with other legacy text fields, because legacy text fields only add top padding to make room for the label. The text field boxes do other calculations in order to align properly, so these calculations need to be taken into account.
The currently proposed getBaseline() produces this effect when put in a LinearLayout with baselineAligned set to "true":
And this is the effect of a getBaseline() method that takes those factors into account:
The getBaseline() method in the screenshot above uses the following code:
@Override
public int getBaseline() {
return editText != null
? editText.getBaseline() + calculateLabelMarginTop() + getPaddingTop() +
: super.getBaseline();
}
boxBottomOffsetPx should only be added if boxBackgroundMode != BOX_BACKGROUND_NONE.
There was a problem hiding this comment.
Ohhh, I see. I haven't used the Material theme yet so I didn't notice that. Thanks for the great catch!
|
@afohrman Addressed all your comments 👍 |
| @@ -798,7 +798,7 @@ public EditText getEditText() { | |||
| @Override | |||
| public int getBaseline() { | |||
| if (editText != null) { | |||
| return editText.getPaddingTop() + editText.getBaseline(); | |||
| return editText.getBaseline() + getPaddingTop() + calculateLabelMarginTop(); | |||
There was a problem hiding this comment.
Looking good, but boxBottomOffsetPx still needs to be accounted for. It should be added to the calculation only if boxBackgroundMode != BOX_BACKGROUND_NONE. You may want to split that logic out into a separate method.
There was a problem hiding this comment.
I'm not sure that's right. Here's a screenshot without the extra padding:
And here is with the extra padding:
BTW, here's my code:
int padding = getPaddingTop();
if (boxBackgroundMode != BOX_BACKGROUND_NONE) {
padding += boxCollapsedPaddingTopPx;
}There was a problem hiding this comment.
The extra offset to account for is boxBottomOffsetPx, not boxCollapsedPaddingTopPx.
There was a problem hiding this comment.
Nope, that will move the legacy widget down. The first screenshot was fine... what is there to fix?
There was a problem hiding this comment.
It shouldn't move the legacy widget down if the offset is only being added when boxBackgroundMode != BOX_BACKGROUND_NONE.
From what I can see in the first screenshot, the legacy text field is a little off. Can you add a screenshot that includes a TextView, EditText, and the TextInputLayout legacy and updated styles side by side?
There was a problem hiding this comment.
Cool, this looks great. I'll merge the change.
Thanks for contributing!
There was a problem hiding this comment.
Actually the baseline does still appear to still be a little off:
Could you look around in TextInputLayout and see what could be causing this?
There was a problem hiding this comment.
Aw, dang! Can you share the XML you used so I can repro?
There was a problem hiding this comment.
Oh never mind, that screenshot was using an earlier version of the code. I've verified that it works and I'll merge the changes.
Thank you!
There was a problem hiding this comment.
Phew! 😌 Can't wait to see this finally go through!!! 😁👏🎉
|
@afohrman anything holding you back from merging? |
|
@SUPERCILEX this has been submitted internally and will be synced out to GitHub soon. |
|
@dsn5ft Awesome! ❤️ |
|
@dsn5ft would you mind also closing https://issuetracker.google.com/issues/113439293? Thanks! |
|
I think there is some issue with this implementation and using ConstraintLayout's baseline alignment: |
Fixes #14, fixes #13
Simpler version of those above issues.