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

Prevent double-push of edit username screen #17735

Merged
merged 1 commit into from
Jan 10, 2022

Conversation

joshheald
Copy link
Contributor

@joshheald joshheald commented Jan 10, 2022

Fixes #17624

Description

The edit username screen on the SignupEpilogue screen was sometimes pushed twice when a user tapped on the username text field.

This navigation was triggered in the implementation of UITextFieldDelegate.textFieldShouldBeginEditing, which was under certain circumstances, called twice by the system. That delegate method is supposed to answer the question "should editing be allowed?" with a Bool, so navigating here could be considered a side-effect, which makes the delegate method unsafe to call more than once.

This change moves the responsibility for navigation to the TableViewController, in the didSelectRowAtIndex method.

The UITextField for username also needed to be disabled, to make it easier to tap the row. Without this change, the user would have to tap on the "Username:" label, or the disclosure indicator, and taps on the text field would be ignored. Prior to this commit, users would have to tap on the username value itself, taps elsewhere on the row were ignored.

To test:

  1. Launch app, make a new account
  2. On sign up epilogue, tap on the username field. The screen will only be pushed once.
  3. Tap back
  4. Tap on the Display Name
  5. Tap on the username field. The screen will still only be pushed once.
multi-push-fixed.mp4

Regression Notes

  1. Potential unintended areas of impact
    Editability of other SignupEpilogueCells – the cell is not used on any other screens
    Accessibility – action and label

  2. What I did to test those areas of impact (or what existing automated tests I relied on)
    Checked that Password and Display Name were still editable, before and after pushing the username selection screen (and multiple repeats in case of cell reuse)

Checked accessibility label was useful and showed that the row was selectable ("button"). Accessibility action still pushes username screen.

  1. What automated tests I added (or what prevented me from doing so)
    None – no existing tests found, idempotent testing of signup is a high barrier to entry for this small change.

PR submission checklist:

  • I have completed the Regression Notes.
  • I have considered adding unit tests for my changes.
  • I have considered adding accessibility improvements for my changes.
  • I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.

The edit username screen on the SignupEpilogue screen was sometimes pushed twice when a user tapped on the username text field.

This navigation was triggered in the implementation of `UITextFieldDelegate.textFieldShouldBeginEditing`, which was under certain circumstances, called twice by the system. That delegate method is supposed to answer the question "should editing be allowed?" with a Bool, so navigating here could be considered a side-effect, which makes the delegate method unsafe to call more than once.

This change moves the responsibility for navigation to the TableViewController, in the `didSelectRowAtIndex` method.

The `UITextField` for username also needed to be disabled, to make it easier to tap the row. Without this change, the user would have to tap on the "Username:" label, or the disclosure indicator, and taps on the text field would be ignored. Prior to this commit, users would have to tap on the username value itself, taps elsewhere on the row were ignored.
@joshheald joshheald added this to the 19.1 milestone Jan 10, 2022
@joshheald joshheald requested a review from momo-ozawa January 10, 2022 15:16
@joshheald joshheald self-assigned this Jan 10, 2022
@peril-wordpress-mobile
Copy link

You can trigger an installable build for these changes by visiting CircleCI here.

@peril-wordpress-mobile
Copy link

You can trigger optional UI/connected tests for these changes by visiting CircleCI here.

Copy link
Contributor

@momo-ozawa momo-ozawa left a comment

Choose a reason for hiding this comment

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

LGTM! Works as expected, thanks @joshheald!

This navigation was triggered in the implementation of UITextFieldDelegate.textFieldShouldBeginEditing, which was under certain circumstances, called twice by the system.

Nice catch

The UITextField for username also needed to be disabled, to make it easier to tap the row. Without this change, the user would have to tap on the "Username:" label, or the disclosure indicator, and taps on the text field would be ignored. Prior to this commit, users would have to tap on the username value itself, taps elsewhere on the row were ignored.

💯 ✨

@joshheald joshheald merged commit b2e64a5 into develop Jan 10, 2022
@joshheald joshheald deleted the issue/17624-double-push-edit-username-on-signup branch January 10, 2022 16:24
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.

Login: Signup Epilogue: Tapping on the change username field can result in it being pushed twice
2 participants