Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

TextField looks drastically different after Swift rewrite #568

Closed
reidmain opened this issue Oct 26, 2016 · 55 comments
Closed

TextField looks drastically different after Swift rewrite #568

reidmain opened this issue Oct 26, 2016 · 55 comments
Assignees

Comments

@reidmain
Copy link

I am in the process of updating our app to use Swift 3 and Material is one of the dependencies I've had to update. I am using release 2.2.5.

There are three big issues I am seeing:

  1. Placeholder text does not appear until some text is entered into the text field.

  2. placeholderActiveColor is being ignored. The color of the placeholder text does not change if the text field becomes the first responder.

  3. The placeholder label now appears to be in a different position. We use snapshot tests to ensure our UI does not change accidentally and all of our uses of Material's TextField seem to have the placeholder label shift up by about 20 pixels.

I am currently using Xcode 8.1 GM and building against the iOS 10 SDK.

I will continue to investigate to try and figure out what the issue could be. These seem like core components of the TextField so I have to imagine it isn't behaving like this for everyone but looking at our code we are not doing anything unique with our text fields.

@daniel-jonathan
Copy link
Member

daniel-jonathan commented Oct 26, 2016

Have you looked at the Samples repo ? All the latest examples are there. Also, can you share some code ?

@reidmain
Copy link
Author

I did but it wasn't exactly clear to me what it's purpose was. I opened the workspace and tried to run the "TextField" scheme and immediately got the build error No such module 'Material' which makes sense because I don't see the Material framework included anywhere. The repo seems to just be example code with no way to include Material as a dependency.

Ah nevermind. Before I posted this I decided to sanity check one more time and see that you need to go into the directory of the scheme you want to build and run pod install for each scheme you want to run.

Of course pod install is not working and even after running it I still get the error that I can't find the module. I'll dig into this.

@daniel-jonathan
Copy link
Member

They should work with pod install. You could also use the workspace, but you need to add the Material framework to it. I made it possible for both styles to try the samples because of the different frameworks being used.

So let's go forward with some error messages you are getting and sample code.

@reidmain
Copy link
Author

I have tired opening up both CosmicMind.xcworkspace and TextField.xcworkspace.

The former gives the No such module 'Material error even after I have run pod install.

The later gives a linker error which then displays ld: framework not found Material when you click into it.

Looking into the "Build Phases" I see Pods_TextField.framework under the "Link Binary With Libraries" section so I am actually quite puzzled why this is not working.

@daniel-jonathan
Copy link
Member

What version of CocoaPods are you using ?

@reidmain
Copy link
Author

1.1.1

Also I think this is a Xcode 8.1 issue. I converted the project to the "recommended settings" and it suddenly compiled.

@daniel-jonathan
Copy link
Member

daniel-jonathan commented Oct 26, 2016

Okay, so that tackles that. What are the other issues ?

  1. Placeholder text does not appear until some text is entered into the text field.

  2. placeholderActiveColor is being ignored. The color of the placeholder text does not change if the text field becomes the first responder.

  3. The placeholder label now appears to be in a different position. We use snapshot tests to ensure our UI does not change accidentally and all of our uses of Material's TextField seem to have the placeholder label shift up by about 20 pixels.

@reidmain
Copy link
Author

And like I suspected the TextField seems to be working fine in the sample app.

There must be something we are doing differently in our code but it is just so bizarre because TextField worked perfect under Swift 2 and iOS 9.

Could creating the TextField inside a nib make a difference?

@daniel-jonathan
Copy link
Member

Also, you must have made a big jump. A lot of the components and underlying libraries were rewritten for Material 2. So I can help you catch up.

It could, but shouldn't be an issue. At the end of the day, the TextField is a subclass of UITextField. What I think the issue is that the API has been updated and you are not setting the correct values. Do you mind sharing your setup code?

Thank you!

@reidmain
Copy link
Author

Sure but first let me prune out some code and make sure the problem is still occurring with the most basic of setup code. Should take me about 5 minutes.

@reidmain
Copy link
Author

reidmain commented Oct 26, 2016

OK I have reduced the code to the absolute bare minimum and am still seeing the issue.

Basically the scenario is as follows. I have a nib file for a UITableViewCell subclass. We add a label, a button and two text fields to the content view.

Inside the awakeFromNib method we call an extension method we added to TextField: setFontStyle.

func setFontStyle(_ text: String? = nil,
                        placeholder: String? = nil,
                        detail: String? = nil,
                        accessiblity: String? = nil) {
    self.text = text
    font = FontStyle.Subheading1
    textColor = Colors.Black90
    self.placeholder = placeholder
    placeholderNormalColor = Colors.Black60
    placeholderActiveColor = Colors.Indigo
    placeholderVerticalOffset = 0
    dividerColor = Colors.Black30
    dividerActiveColor = Colors.Indigo
    dividerActiveHeight = Stylesheet.deviceScaleSize * 2
    detailLabel.text = detail
    detailLabel.font = FontStyle.Caption1
    detailColor = Colors.Red
    detailLabel.isHidden = true
    accessibilityLabel = accessiblity
    if let image = R.image.icon_clear_all_small() {
        setCustomIconImage(image)
    }
}

and that's it. Other than the fact that the text fields are created inside the nibs (or that they are inside a table view cell) everything looks normal.

I think I'll go and create a quick view controller that adds text fields to it programmatically and see if I have any better luck.

Oh and I just want to reiterate that I changed nothing about this nib and custom cell class during the Swift 2 to 3 migration other than the changes the migrator made which look benign.

@daniel-jonathan
Copy link
Member

I can see API issues that you have in your code, such as dividerColor should be dividerNormalColor. If you create a sample project I can help you from there. Send it to [email protected].

@reidmain
Copy link
Author

So I created a quick view controller, programmatically created a TextField and then called the setFontStyle method on it and stuff was busted again. After a bit of experimentation I think I have founds the issues.

  1. Setting the text property to nil causes the placeholder text to go AWOL. If I don't change the text property on the text field then the placeholder text works fine. If I set it to nil or the empty string then it disappears.

This occurs in the sample app as well. Go into the ViewController.swift file and add nameField.text = nil at line 78 and you can reproduce it.

  1. placeholderNormalColor and placeholderActiveColor don't behave like I was expecting them to. I thought the removal of placeholderColor meant that if previously you were changing the placeholderColor when the text field became first responder you should now use placeholderActiveColor instead. But we should just go back placeholderNormalColor and modify it when the text field becomes first responder.

  2. The placeholder label now seems to move to the top of the frame of the TextField. I believe our TextFields are now taller than they actually need to be and so when text is entered the placeholder label now animates higher than it should. In the Swift 3 version of Material did the text field's placeholder get positioned above the entered text?

@daniel-jonathan
Copy link
Member

I really need code to see what you are doing. Please share the sample app.

@reidmain
Copy link
Author

reidmain commented Oct 26, 2016

@DanielDahan I have no sample app to share. I've just been working with a scrap piece of code like:

let viewController = UIViewController()

viewController.view.backgroundColor = Colors.Black10

let textField = TextField(frame: CGRect(x: 25, y: 150, width: 300, height: 100))
textField.setFontStyle(nil, placeholder: "Name", detail: nil, accessiblity: "Bah")

viewController.view.addSubview(textField)

present(viewController, animated: true, completion: nil)

and playing with the innards of the setFontStyle method to try to get it to behave like it was in the Swift 2 days.

So far I think the only bug is setting nil on the text property seems to cause the placeholder label to disappear which you can see for yourself in the sample app or by adding textField.text = nil to the code above.

The other issue which I am not confident is a bug is the fact that the placeholder label is now laid out in a different spot. Did that get changed during the Swift 3 migration?

@reidmain
Copy link
Author

reidmain commented Oct 26, 2016

Below are some screenshots and the code used to generate them. You can see when you set the text to nil then the placeholder label disappears. In the Swift 2 days this code would generate the same screenshot.

let viewController = UIViewController()

viewController.view.backgroundColor = Color.lightGray

let textField = TextField(frame: CGRect(x: 25, y: 150, width: 300, height: 50))
//textField.setFontStyle(nil, placeholder: "Name", detail: nil, accessiblity: "Test")
textField.placeholder = "Name"
textField.backgroundColor = Color.lightGreen.base
textField.dividerNormalColor = Color.red

viewController.view.addSubview(textField)

present(viewController, animated: true, completion: nil)

name not set

let viewController = UIViewController()

viewController.view.backgroundColor = Color.lightGray

let textField = TextField(frame: CGRect(x: 25, y: 150, width: 300, height: 50))
//textField.setFontStyle(nil, placeholder: "Name", detail: nil, accessiblity: "Test")
textField.placeholder = "Name"
textField.backgroundColor = Color.lightGreen.base
textField.dividerNormalColor = Color.red
textField.text = nil

viewController.view.addSubview(textField)

present(viewController, animated: true, completion: nil)

name set to nil

@daniel-jonathan
Copy link
Member

Okay, I will need to review this. I notice you are using API calls that I have not necessarily mapped out. I will need to double check. Thank you for bringing this to our attention.

@reidmain
Copy link
Author

@DanielDahan no problem. I'll update the code for the screenshots above so they don't use the setFontStyle method so they can be easily copy and pasted.

@daniel-jonathan
Copy link
Member

@reidmain possibly update them, but leave the ones you are using commented out. I can then make sure it all works and ship an update :) I have one coming up.

@reidmain
Copy link
Author

@DanielDahan don't worry I know our extension on TextField is busted and needs to be updated so I'm not gonna try to salvage it. For me the only real concerns are setting the text property to nil and the position of the placeholder label when text has been entered.

@daniel-jonathan
Copy link
Member

@reidmain cool :)

@reidmain
Copy link
Author

So here is an example of how the positioning of the placeholder label changes based on the height of the text field.

let viewController = UIViewController()

viewController.view.backgroundColor = Color.lightGray

let textField = TextField(frame: CGRect(x: 25, y: 150, width: 300, height: 50))
textField.placeholder = "Name"
textField.text = "Reid"
textField.backgroundColor = Color.lightGreen.base
textField.dividerNormalColor = Color.red

viewController.view.addSubview(textField)

present(viewController, animated: true, completion: nil)

height 50

let viewController = UIViewController()

viewController.view.backgroundColor = Color.lightGray

let textField = TextField(frame: CGRect(x: 25, y: 150, width: 300, height: 100))
textField.placeholder = "Name"
textField.text = "Reid"
textField.backgroundColor = Color.lightGreen.base
textField.dividerNormalColor = Color.red

viewController.view.addSubview(textField)

present(viewController, animated: true, completion: nil)

height 100

You can see how the placeholder label in the second screenshot is much higher than the previous

To see this in practice in our app I have the two screenshots taken from our failed snapshot tests.

Swift 2 which is what we want
swift2

Swift 3 which is what we are currently seeing
swift3

You can see how the placeholder labels jumped up a bunch but the text is in the same place.

@reidmain
Copy link
Author

reidmain commented Oct 26, 2016

Actually @DanielDahan could you confirm something for me? I would expect placeholderNormalColor to be the color of the placeholder label when the text field is not the first responder. I would then expect placeholderActiveColor to be the color of the placeholder label when the text field becomes the first responder. Currently it looks like placeholderActiveColor just corresponds to the color of the caret that appears when the text field is first responder.

@daniel-jonathan
Copy link
Member

It should be the former, where the colors correspond to the different states.

@reidmain
Copy link
Author

So this screenshot from the samples app indicates a bug then? The "Name" text should also be pink?

placeholder color

@daniel-jonathan
Copy link
Member

It should, I will look into it. I will make this issue as a bug.

@reidmain
Copy link
Author

I'm now confident that the placement of the placeholder label is also a bug. Say you have a textfield whose top is flush with the bottom of some other view, the placeholder label will now overlap that other view. You'd have to put padding into between the text field and the other view and that padding would need to be a function of the height of the text field which, in my opinion, is not ideal. Your text field should be sized such that it is the right bounds to contain all of it elements that it could be displaying.

@reidmain
Copy link
Author

reidmain commented Nov 1, 2016

@DanielDahan totally understand. It's not blocking anything on our end at this moment. We put in some workarounds and the second you have a release out we'll update.

@daniel-jonathan
Copy link
Member

Perfect!

@daniel-jonathan
Copy link
Member

I am relating an issue that came in #576

Let's tackle this today :) I will post to development a proposed fix, and if possible, please test.

@Arkezis
Copy link

Arkezis commented Nov 3, 2016

Sure, I'll test it this evening or tomorrow :) (GMT+2 here ;))

@daniel-jonathan
Copy link
Member

Adding this as well #582

@daniel-jonathan
Copy link
Member

Adding this as well #583

@daniel-jonathan
Copy link
Member

Adding this as well CosmicMind/Samples#15

@daniel-jonathan
Copy link
Member

@reidmain Hey, so I made these updates to the TextField, can you verify:

  • issue-568: fixed issue where placeholderActiveColor was not being set correctly when TextField was in an active state.
  • issue-568: fixed issue where setting text to nil broke the TextField layout.

@reidmain
Copy link
Author

reidmain commented Nov 7, 2016

@DanielDahan I'll update to the development branch and report back after all of our tests have run.

@reidmain
Copy link
Author

reidmain commented Nov 7, 2016

Looks good from my end. The placeholder text field shows if nil is set and the placeholder text is coloured properly when in the active state.

@reidmain
Copy link
Author

reidmain commented Nov 7, 2016

Oh sorry I noticed one issue. If you become active and type in some text it works fine. The divider line and placeholder text are the active color. But if you move to the inactive state and then back to the active the placeholder text is the current color but the divider line is not.

@daniel-jonathan
Copy link
Member

@reidmain I will check this scenario out within an hour :)

@daniel-jonathan
Copy link
Member

@reidmain Sorry, I got majorly sidetracked with an issue I am looking at for Material. I will have to look into this more tomorrow.

@reidmain
Copy link
Author

@DanielDahan no rush man. We already shipped a release with the workarounds I detailed and tomorrow we should be preparing a release with the 2.3.3 version. Everything is solid.

@daniel-jonathan
Copy link
Member

@reidmain amazing :) Thank you

@daniel-jonathan
Copy link
Member

I tired reproducing this after I made some updates. I am basically tabbing to different fields and all seems well. I am going to make a release now with some updates. Please test this out and let me know :) Thanks buddy! If you become active and type in some text it works fine. The divider line and placeholder text are the active color. But if you move to the inactive state and then back to the active the placeholder text is the current color but the divider line is not.

@reidmain
Copy link
Author

reidmain commented Nov 15, 2016

So I updated from 2.3.3 to 2.3.10 and unfortunately the divider line is still having the issue where it doesn't become the active colour when it regains focus. Below are some images showing exactly what I am seeing.

Here is the screen when it is first presented. You can see the two text fields are in their placeholder state.

Then I focus on the first text field and you can see the divider line is the correct colour.

I select an entry from the picker and everything continues to look good.

I then tab over to the other text field and enter some text into it and everything looks good.

But when I tab back to the previous text field you can see that the divider line for it no longer becomes the active colour.

I cleaned my project and reset my simulator just to ensure that I was building and using the absolute latest version of Material.

@daniel-jonathan
Copy link
Member

Your app looks pretty :) All seems to be working for me. Do you mind showing me your latest setup code?

@reidmain
Copy link
Author

Your "all seems to be working for me" made me go back and double check and I realized that this table view cell is a unique case. In another scenario (where we are taking in a credit card number) the text fields seem to be working as intended.

I suspect that in the delegate methods of UITextField we are doing something strange that is confusing the state of TextField. I will delve deeper and report back if I think there is anything TextField is doing but at this point in time it looks to be working as intended. Thank you very much for all your hard work.

@daniel-jonathan
Copy link
Member

Thank you, and if you can, even privately at [email protected], you can share code with me on the setup and I can help :)

@jbaez
Copy link
Contributor

jbaez commented Jun 5, 2018

I just updated Material library in our App (it's been a while since we haven't updated), and also noticed the placeholder label was positioning higher than before (overlapping with some other elements in some cases).
We have some custom components that where using the same logic as the Material.TextField for showing the placeholder, detail label and other material styling. Looks like what has changed is using bounds instead of frame for y position:
New TextField in layoutPlaceholderLabel line 574:
placeholderLabel.frame.origin.y = -placeholderLabel.bounds.height + placeholderVerticalOffset
With the old logic it would have been:
placeholderLabel.frame.origin.y = -placeholderLabel.frame.size.height + placeholderVerticalOffset

screen shot 2018-06-04 at 18 06 05

Above, the new TextField placeholder showing too high. Below, is our custom component using the old logic which shows the placeholder right up of the TextField.

Reverting line 574 to use frame instead of bounds fixes this issue.

@jbaez
Copy link
Contributor

jbaez commented Jun 6, 2018

@DanielDahan I created a fork to fix this issue so it works as before in our App. Let me know if you want me to send a pull request.

jbaez@2d147b3

@daniel-jonathan
Copy link
Member

@jbaez what was the issue? I understand it doesn't work like before, but that may a settings change that is needed. Can you send a PR, I'd be happy to review it. Thank you!

@OrkhanAlikhanov
Copy link
Contributor

OrkhanAlikhanov commented Jun 7, 2018

@DanielDahan and @jbaez I created a separate issue for this. #1092

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

No branches or pull requests

5 participants