Skip to content
This repository has been archived by the owner on Mar 7, 2021. It is now read-only.

Remove abort() call to prevent crash on select clear #553

Merged
merged 2 commits into from
Nov 15, 2016
Merged

Conversation

jeffleeismyhero
Copy link
Collaborator

When a select field is cleared using the "clear" button, subsequent access causes - (BOOL)identifierIsEqualTo:(id)identifier to hit the else condition which calls abort().

This removes the else condition and allows the method to return NO.

@3lvis
Copy link
Owner

3lvis commented Nov 12, 2016

@jeffleeismyhero could you add some steps to reproduce? Can I reproduce it in the current demo? Would be nice so we can add some unit tests for unit.

@jeffleeismyhero
Copy link
Collaborator Author

It seems like the select field buttons were modified in d06d8a9. I was working on version 3.12.0. Since those buttons are no longer present, this isn't a problem --- for the iPad idiom.

However, the code for the clear button action is still present for the iPhone idiom. The issue occurs with the following steps:

  1. Click a select field
  2. Press the clear button
  3. Click the same select field again

This is illustrated in the attached screen recording.

form-clear-abort

@jeffleeismyhero
Copy link
Collaborator Author

This could also be resolved by checking to see if self.valueID is nil at the same point we check identifier:

- (BOOL)identifierIsEqualTo:(id)identifier {
    if (!identifier || !self.valueID) return NO;

    if ([self.valueID isKindOfClass:[NSString class]]) {
        return [self.valueID isEqualToString:identifier];
    } else if ([self.valueID isKindOfClass:[NSNumber class]]) {
        return [self.valueID isEqualToNumber:identifier];
    } else if ([self.valueID isKindOfClass:[NSDate class]]) {
        return [self.valueID isEqualToDate:identifier];
    } else {
        abort();
    }

    return NO;
}

@3lvis
Copy link
Owner

3lvis commented Nov 14, 2016

@jeffleeismyhero the nil check makes more sense here, although I'm not sure why is nil now.

@jeffleeismyhero
Copy link
Collaborator Author

I've switched this to check self.valueID for nil.

It's interesting that it's nil since the clear button sets it to @NO. I'll look into it a bit more tomorrow.

@3lvis
Copy link
Owner

3lvis commented Nov 15, 2016

Gonna merge this since it fixes a crash, we'll look more into it in a different PR.

# for free to subscribe to this conversation on GitHub. Already have an account? #.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants