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

[TIMOB-24808]: iOS 11 Add support for Password AutoFill #9210

Closed
wants to merge 3 commits into from

Conversation

vijaysingh-axway
Copy link
Contributor

Copy link
Collaborator

@hansemannn hansemannn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Needs some more adjustments in order to be approved, but looking forward to get those solved easily. Thx!

- name: textContentType
summary: Provide the keyboard with extra information about the semantic intent of the text
document.
type: String
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Break summary into two lines.

document.
type: String
constants: Titanium.UI.iOS.CONTENT_TYPE_*
default: <NULL>
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do not specify the default value if none is set.

summary: Provide the keyboard with extra information about the semantic intent of the text
document.
type: String
constants: Titanium.UI.iOS.CONTENT_TYPE_*
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be [Titanium.UI.iOS.CONTENT_TYPE_*]

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think if the method accepts single constants it should be mentioned like Titanium.UI.iOS.CONTENT_TYPE_* rather [Titanium.UI.iOS.CONTENT_TYPE_*] . e.g. see "keyboardType" inside TextArea.yml.

default: <NULL>
platforms: [iphone, ipad]
since: "6.2.0"
osver: {ios: {min: "10.0"}}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same things as above (break into two lines, brackets in the constants value, no default value.

@@ -1699,6 +1699,231 @@ properties:
osver: {ios: {min: "9.1"}}
since: "5.1.0"

- name: CONTENT_TYPE_NAME
summary: Tell keyboard that content type is, name.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That sounds weird. Please use the Apple phrasing for all constants.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you mean it should be like TEXT_CONTENT_TYPE_NAME ? I just tried to make the constant name shorter.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, the summary should be proper english.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

- name: CONTENT_TYPE_PASSWORD
summary: Tell keyboard that content type is, password.
description: |
Use with <Titanium.UI.TextField.textContentType> or <Titanium.UI.TextArea.textContentType>.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should also have a paragraph about the default behavior of fields having both email-address keyboard appearance and password-mask enabled, because in that case the password-autofill will be enabled automatically.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I didn't get. Are you talking about to write a note about how to enable password autofill ?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If people use the keyboard-type "email" and the text-fied property "passwordMask", it will enable auto-fill automatically, even when you did not build for iOS 11. That should be noted as an own paragraph there.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We are not mentioning anything related password autofill in document. So I don't think we should write above detail here. Better is we should write a wiki for giving detail about autofill password.

-(void)setTextContentType_:(id)value
{
ENSURE_TYPE_OR_NIL(value, NSString);
if (@available(iOS 10.0, *)) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This whole method should be rewritten, since it has way too much manual logic and some missing guard inside it. Let try this something like this:

- (void)setTextContentType_:(id)value
{
    ENSURE_TYPE_OR_NIL(value, NSString);

    if (![TiUtils isIOS10OrGreater]) {
        NSLog(@"Setting a text-content-type is only available on iOS 10 and later");
        return;
    }

    [[self textWidgetView] setTextContentType:[TiUtils stringValue:value]];    
}

This has some advantages:

  • Constants are mapped to it's values automatically
  • There is no need for the Xcode 9-only @available syntax
  • There is no need for the (currently missing) #if IS_XCODE_9 macros
  • iOS 11 constants do not need to be checked manually.
  • Less error-prone due to no manual validation

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if (![TiUtils isiOS10OrGreater]) {
    NSLog(@"Setting a text-content-type is only available on iOS 10 and later");
    return;
}  , I agree. 

With [[self textWidgetView] setTextContentType:[TiUtils stringValue:value]], I have already tried but it didn't worked. When I saw the class type of [TiUtils stringValue:value], it gives value "NSTaggedPointerString " and for content type "__NSCFConstantString". This api accept only Constant string.
So we have to make the method large with manual logic.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can just cast the string (see similar to here), mapping everything manually cannot be the best solution.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can not convert NSstring to __NSCFConstantString. If we do-
NSString *str = @"test string", it get automatically converted. It is done at compile time. Either we can simply write -
textContentType = @"username"
or we write
textContentType = UITextContentTypeUsername
In both cases we have to use if else condition for differentiation. First option is not good as we have to get the string value of each UITextContentType and write there.

@@ -840,5 +840,36 @@ -(id)createFeedbackGenerator:(id)args
#endif
#endif

#if defined(USE_TI_UITEXTWIDGET) || defined(USE_TI_UITEXTAREA) || defined(USE_TI_UITEXTFIELD)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

USE_TI_UITEXTWIDGET is always defined when USE_TI_UITEXTAREA or USE_TI_UITEXTFIELD is defined, so I cannot think of a case where it is required to be checked manually.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

MAKE_SYSTEM_STR(CONTENT_TYPE_MIDDLE_NAME, UITextContentTypeMiddleName);
MAKE_SYSTEM_STR(CONTENT_TYPE_FAMILY_NAME, UITextContentTypeFamilyName);
MAKE_SYSTEM_STR(CONTENT_TYPE_NAME_SUFFIX, UITextContentTypeNameSuffix);
MAKE_SYSTEM_STR(CONTENT_TYPE_NICK_NAME, UITextContentTypeNickname);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be CONTENT_TYPE_NICKNAME , please also update inside the docs.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

MAKE_SYSTEM_STR(CONTENT_TYPE_CREDIT_CARD_NUMBER, UITextContentTypeCreditCardNumber);

#if IS_XCODE_9
MAKE_SYSTEM_STR(CONTENT_TYPE_USER_NAME, UITextContentTypeUsername);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be CONTENT_TYPE_USERNAME, please also update inside the docs and JIRA example-code.

@hansemannn hansemannn removed this from the 6.2.0 milestone Jul 20, 2017
@vijaysingh-axway
Copy link
Contributor Author

Closing in favor of #9368

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

Successfully merging this pull request may close these issues.

None yet

2 participants