search

tidev / titanium-sdk

🚀 Native iOS and Android Apps with JavaScript
https://titaniumsdk.com/
Other
2.75k stars 1.21k forks source link

Android: Ti.UI.TextField does not handle "Ti.UI.INPUT_BORDERSTYLE_ROUNDED" borderStyle correctly #13278

Open hansemannn opened 2 years ago

hansemannn commented 2 years ago

I have searched and made sure there are no existing issues for the issue I am filing

Description

When setting a text-field's borderStyle to Ti.UI.INPUT_BORDERSTYLE_ROUNDED, there are two major issues that prevent the API's from being used:

  1. It cannot handle a fixed size (the padding breaks, since it's seems to try to calculate with a larger height internally)
  2. It cannot handle custom background colors (in that case, the border style is not applied at all - possibly the background color is applied to the wrong sub view internally)

Expected Behavior

Actual behavior

Reproducible sample

const window = Ti.UI.createWindow();
window.add(Ti.UI.createTextField({
    top: 20,
    height: 40,
    borderStyle: Ti.UI.INPUT_BORDERSTYLE_ROUNDED,
    hintText: 'Enter value …'
}));
window.open();

Steps to reproduce

Run the above code.

Platform

Android

SDK version you are using

10.1.1.GA, 10.2.0 (master)

m1ga commented 2 years ago

setting hintType: Ti.UI.HINT_TYPE_ANIMATED will change the padding size:

https://github.com/appcelerator/titanium_mobile/blob/db1a83a45529993e2d694e55181a881091cfa849/android/modules/ui/src/java/ti/modules/titanium/ui/widget/TiUIText.java#L968-L972

That at least allows you to make the textfield smaller: Screenshot_20220222-131308 Screenshot_20220222-131319

Still has some issues when you want to type into it.

Looking at the "Layout Inspector" it looks like the padding is applied to the outer TextInputLayout container (blue line in the middle) and not the TiUiEditText. So the Titanium padding is pushing the TextField smaller. Screenshot_20220222_131745

So it might be better to apply the padding to the TiUIEditText instead of the outer layout. Looks like that is causing some issues. Setting this.tv.setPadding(0, 0, 0, 0); before line 149 looks better.

Screenshot_20220222-133413 (yes, left/right padding is gone but that's on purpose for the test).

About the background: I think it's because the "border" is a drawable and when you set a color it will override the drawable with the new color. So maybe the color should be set in the theme (https://material.io/components/text-fields/android#theming-text-fields).

m1ga commented 2 years ago

Ok, it works a lot better when you set a default padding to 28 (all sides, before it was 44 left/right) and then use tv instead of textInputLayout in: https://github.com/appcelerator/titanium_mobile/blob/db1a83a45529993e2d694e55181a881091cfa849/android/modules/ui/src/java/ti/modules/titanium/ui/widget/TiUIText.java#L310-L334

gives you a nice default, you can set a height and if your height is smaller you can compensate it by setting a smaller padding (<28) in your Ti code.

Have to do some more tests of course with the other layouts. But it looks and feels a lot better

m1ga commented 2 years ago

A first draft version https://github.com/appcelerator/titanium_mobile/pull/13279 Still has the default "wrong" padding if you don't need the animated hint text but you can compensate it with the padding property now

jquick-axway commented 2 years ago

In my opinion, this isn't a bug. Setting a TextField's height like this is bad practice in a cross-platform UI framework. The issue with this is you're making bad assumptions about the default height of the border, the default font size, and other UI decorations such as the top hint text (and bottom error/description text which isn't currently supported).

The only way to make hard-coding the height of a TextField work without clipping the text is to set up the font to auto-size itself to fit the height of the field. Back when I used to work on the Corona SDK, I added a resizeFontToFitHeight feature to do this... with the negative being that the font might be too small to read if the borders are thicker than expected... but it can also scale the font bigger if you set the field height larger than normal which is a positive thing. I also added a resizeHeightToFitFont for devs who hard-coded font sizes. I blogged about this here back in 2014 and implemented it on ALL platforms.

In regards to Titanium, perhaps the simplest solution is to automatically scale the font if the field height is set to a value. You may not need a new property like I did up above. And I think TextField is already set up to do the reverse where setting the font size will auto-size the field height (but if you set the font too small then things will be clipped). However, if you set both the field height and the font size, then you're screwed and you should probably go borderless.

m1ga commented 2 years ago

Thanks for the explanation :+1: Have to check that. Still there is a small bug setting the top padding to 48 even if there is no animated hintText doesn't look right to me. But I'll check that too. Even if it might not be right it is a bit easier to understand for the user if you can change the padding correctly (in the inner view)

jquick-axway commented 2 years ago

Yeah, Google's TextInputLayout wants to add padding for the animated hint text whether you use it or not. I had to work-around it for devs who don't want to use the animated version of hint text (iOS style). https://github.com/appcelerator/titanium_mobile/blob/master/android/modules/ui/src/java/ti/modules/titanium/ui/widget/TiUIText.java#L956-L973