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

Allow new password input keyboard to be dismissed #17938

Merged
merged 4 commits into from
Mar 16, 2022

Conversation

guarani
Copy link
Contributor

@guarani guarani commented Feb 10, 2022

Addresses #17927

To test

  1. Navigate to the welcome screen
  2. Tap the button to log-in/sign-up
  3. Enter a new email address
  4. Close the WordPress app and open your email client on the device
  5. Tap the link in the sign-up email and expect the app to open again
  6. On the screen that allows the user to set their Display Name, Username and Password:
    a. Tap the Password field
    b. Notice the keyboard "done" button is enabled and closed the keyboard when tapped (before this PR, the button was disabled)
    c. Interact with the other fields and verify they look and behave correctly

Regression Notes

  1. Potential unintended areas of impact

This change should only affect the Password field, but since it shares logic with the Display Name and Username fields, we should check for regressions there.

  1. What I did to test those areas of impact (or what existing automated tests I relied on)

SignupTests will help catch regressions related to setting a password.

  1. What automated tests I added (or what prevented me from doing so)

Writing an automated test here would be very challenging because the change involves the virtual keyboard which AFAIK is hard to control in tests.

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.

@guarani
Copy link
Contributor Author

guarani commented Feb 14, 2022

Hi @twstokes, I was wondering if this change would be a good improvement to the app as-is.
Basically, the issue this addresses is that on the "SignupEpilogue" screen (the screen that allows you to pick a displayname, username, and password), the keyboard from the password field covers the "Done" button. So it's not easy for the user to dismiss the keyboard and move on to the next screen.

There are a number of potential solutions, but this seems like a quick win, whereas the others were all more challenging to implement and none was a clear winner.

Before submitting this for review, I'd love to know where you think this is worth merging as-is given the high-priority of the issue.

@twstokes
Copy link
Contributor

👋 Hey @guarani! Thanks for evaluating this and coming up with potential solutions!

From my testing this certainly will allow the user to dismiss the keyboard in the event they want to keep a blank password. Since this is a high priority issue like you mentioned, I would feel good moving forward so that the user has at least one path to take.

I did want to share some observations:

This change allows the keyboard to be dismissed from any of the fields (not just Password, but also Display Name and Username). However, those fields are programmed to take the user to another screen, so this change shouldn't affect them in any way.

This is right on for Username, but I didn't have the same experience with Display Name. The new behavior allowed me to submit an empty value. My guess is that we don't allow empty display names to be saved because when I went to Account Settings -> My Profile the original still existed, unchanged. Do you think this is an issue?

Dragging down to hide the keyboard is a familiar gesture to users who use iOS' system Messages app

This behavior feels more natural to me in a setting like Messages where you have a long scroll view and you want to open up the vertical space. Since the sign-up view fits (for most devices, I assume) vertically, my gut was to tap outside the keyboard to dismiss it.

Some ideas for the future:

I wonder if we can override this property (or do something from the UITextFieldDelegate) for the password field only so that the keyboard "done" is enabled even when the string has zero characters and would allow a dismissal.

@guarani
Copy link
Contributor Author

guarani commented Feb 17, 2022

This is right on for Username, but I didn't have the same experience with Display Name. The new behavior allowed me to submit an empty value. My guess is that we don't allow empty display names to be saved because when I went to Account Settings -> My Profile the original still existed, unchanged. Do you think this is an issue?

Thanks for flagging this, I'd overlooked it. I went back to the current 19.2 beta and I can still set an empty password display name there if I "dismiss" the keyboard by going to the username screen and returning to this screen. I agree though that this change makes it slightly easier to do that by allowing the user to dismiss the keyboard more easily.

This behavior feels more natural to me in a setting like Messages where you have a long scroll view and you want to open up the vertical space. Since the sign-up view fits (for most devices, I assume) vertically, my gut was to tap outside the keyboard to dismiss it.

I agree with you here again, this screen isn't one that invites the user to scroll, so it might not be intuitive to most users that they can swipe down to close the keyboard.

I wonder if we can override this property (or do something from the UITextFieldDelegate) for the password field only so that the keyboard "done" is enabled even when the string has zero characters and would allow a dismissal.

Thanks for the suggestion, I'll try this one out and report back.

I'm not submitting this for review yet because it feels like the current improvement (swipe down to close keyboard) won't make things significantly easier for users.

@guarani
Copy link
Contributor Author

guarani commented Mar 10, 2022

I made a correction to a typo in my above comment. Instead of password, I meant "display name". I've struck out this but wanted to highlight it in a comment since it could have led to a different interpretation of my comment.

I'm currently exploring the UITextFieldDelegate to look at an alternative solution there.

@guarani
Copy link
Contributor Author

guarani commented Mar 11, 2022

I've had success with a different approach based on the enablesReturnKeyAutomatically property you mentioned above @twstokes, so I've removed the tableView.keyboardDismissMode = .interactive code.

Documentation states that this property is false by default, and I found that it was set to true in Interface Builder for the SignupEpilogueCell.xib:

Screen Shot 2022-03-10 at 17 43 48

This .xib is used for username, display name, and password fields on this screen and is not used anywhere else in the app.

After unchecking the property in Interface Builder, the "done" button on the password field's keyboard is now enabled - regardless of whether the field is populated - and closes the keyboard when tapped ✅. In testing, this change doesn't appear to have any negative effects with regard to the password field.

I think it makes sense to leave it true for the display name field, since I think we agree that field shouldn't allow the keyboard to be dismissed if the field is cleared.

Regarding the username field, I think this property has no effect (since tapping on that field navigates to the next screen). If that is the case, I think we should leave it with its default value (false) because any other value will create confusion as to why it was changed.

I'll do a bit more testing to check for regressions, push the change, and submit this for review.

@guarani guarani force-pushed the fix/inability-to-dismiss-keyboard branch from d7a1008 to 06ecfa7 Compare March 11, 2022 17:55
@guarani guarani marked this pull request as ready for review March 11, 2022 18:12
@wpmobilebot
Copy link
Contributor

wpmobilebot commented Mar 11, 2022

You can test the Jetpack changes on this Pull Request by downloading it from AppCenter here with build number: pr17938-a16e2a1. IPA is available here. If you need access to this, you can ask a maintainer to add you.

@wpmobilebot
Copy link
Contributor

wpmobilebot commented Mar 11, 2022

You can test the WordPress changes on this Pull Request by downloading it from AppCenter here with build number: pr17938-a16e2a1. IPA is available here. If you need access to this, you can ask a maintainer to add you.

guarani added 3 commits March 14, 2022 11:54
This change sets enablesReturnKeyAutomatically to false on the password field to allow users to dismiss the keyboard while the field is focused, even if the field is empty.
@guarani guarani force-pushed the fix/inability-to-dismiss-keyboard branch from 06ecfa7 to 2bd71b3 Compare March 14, 2022 15:00
@guarani guarani added this to the 19.5 milestone Mar 14, 2022
@guarani guarani requested a review from twstokes March 14, 2022 16:37
@guarani
Copy link
Contributor Author

guarani commented Mar 14, 2022

Hi @twstokes, this is now ready for review 🙇.

@twstokes
Copy link
Contributor

I think it makes sense to leave it true for the display name field, since I think we agree that field shouldn't allow the keyboard to be dismissed if the field is cleared.

Regarding the username field, I think this property has no effect (since tapping on that field navigates to the next screen). If that is the case, I think we should leave it with its default value (false) because any other value will create confusion as to why it was changed.

Thanks @guarani for explaining your logic here - totally agree.

I've tested these changes and they LGTM! :shipit: 🚀

@guarani
Copy link
Contributor Author

guarani commented Mar 16, 2022

Thanks for the review, @twstokes!

@guarani guarani enabled auto-merge March 16, 2022 01:20
@guarani guarani merged commit 80eb1ad into trunk Mar 16, 2022
@guarani guarani deleted the fix/inability-to-dismiss-keyboard branch March 16, 2022 01:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants