Skip to content

Fix: Enter triggers OK without extra press #18354

New issue

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

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

Already on GitHub? # to your account

Merged
merged 1 commit into from
Jun 4, 2025

Conversation

samirsuroshe18
Copy link
Contributor

@samirsuroshe18 samirsuroshe18 commented May 21, 2025

Purpose / Description

When an alert dialog appears after entering incorrect credentials in the sync page, pressing the Enter key only selects the "OK" button instead of triggering it. This PR modifies the behavior so that pressing Enter directly triggers the positive button action, making the user experience more intuitive.

Fixes

Approach

Added an extension function setupEnterKeyHandler() to handle Enter key presses in AlertDialogs
Modified the existing show() extension function to apply this handler to all dialogs created through the builder pattern
The handler detects Enter key presses and simulates a click on the positive button if it exists and is enabled

How Has This Been Tested?

Manually verified with the following steps:

Without an AnkiWeb account on AnkiDroid
Attempted to sync
From the sync page, entered an email and wrong password
When error message appeared, pressed Enter key

🎥 Before the Fix

Click to expand
failure.webm

🎥 After the Fix

Click to expand
success.mp4

Before the fix, Enter key only selected the "OK" button requiring a second press.
After the fix, Enter key immediately dismisses the dialog.

Learning

The default behavior of AlertDialogs in Android is to only select but not trigger buttons when Enter is pressed. This is a common UI pattern that needs explicit handling to improve user experience

Checklist

  • You have a descriptive commit message with a short title (first line, max 50 chars).
  • You have commented your code, particularly in hard-to-understand areas
  • You have performed a self-review of your own code
  • UI changes: include screenshots of all affected screens (in particular showing any new or changed strings)
  • UI Changes: You have tested your change using the Google Accessibility Scanner

Copy link

welcome bot commented May 21, 2025

First PR! 🚀 We sincerely appreciate that you have taken the time to propose a change to AnkiDroid! Please have patience with us as we are all volunteers - we will get to this as soon as possible.

@samirsuroshe18 samirsuroshe18 marked this pull request as ready for review May 22, 2025 05:36
@@ -144,7 +145,27 @@ fun AlertDialog.Builder.cancelable(cancelable: Boolean): AlertDialog.Builder = t
*/
inline fun AlertDialog.Builder.show(block: AlertDialog.Builder.() -> Unit): AlertDialog {
this.apply { block() }
return this.show()
val dialog = this.show()
return dialog.setupEnterKeyHandler()
Copy link
Member

Choose a reason for hiding this comment

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

For reviewer 2 to confirm, but I'd rather explicitly opt-in to this on a per-dialog basis, OR find a way to make this a standard default via accessibility

This use of setOnKeyListener overrides any listeners which we set in calling code, which isn't intuitive to me.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the feedback! You're absolutely right about setOnKeyListener potentially overriding existing listeners — that could definitely lead to unexpected behavior.

To address this, I’m planning to go with an opt-in approach by modifying the show() function to accept an optional parameter:

fun AlertDialog.Builder.show(enableEnterKeyHandler: Boolean = false)

This way, setupEnterKeyHandler() will only be applied when explicitly requested, preserving backward compatibility and avoiding side effects in existing dialogs.

Let me know if this direction works for you, and I’ll update the PR accordingly.

Copy link
Member

Choose a reason for hiding this comment

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

Sounds great!

@david-allison david-allison added Needs Author Reply Waiting for a reply from the original author Needs Review and removed Needs Author Reply Waiting for a reply from the original author labels Jun 3, 2025
Copy link
Member

@david-allison david-allison left a comment

Choose a reason for hiding this comment

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

Perfect, cheers!

@david-allison david-allison added Needs Second Approval Has one approval, one more approval to merge squash-merge Squash & force push by author/maintainers requested and removed Needs Review labels Jun 3, 2025
Copy link
Member

@mikehardy mikehardy left a comment

Choose a reason for hiding this comment

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

LGTM - pending merge, needs squash so has to wait for merge queue to be empty

@mikehardy mikehardy added Pending Merge Things with approval that are waiting future merge (e.g. targets a future release, CI wait, etc) and removed Needs Second Approval Has one approval, one more approval to merge labels Jun 3, 2025
- add support for optional Enter key handler in error dialog
@mikehardy mikehardy force-pushed the fix/enter-key-sync-alert branch from 32bce7d to e2b8b1c Compare June 3, 2025 23:48
@mikehardy
Copy link
Member

I'm in the middle of a merge festival and the isn't going to empty until long after I've moved to other things so I rebased this one to squash it locally then force-pushed that, so I can put it in the auto queue

@mikehardy mikehardy enabled auto-merge June 3, 2025 23:49
@mikehardy mikehardy removed the squash-merge Squash & force push by author/maintainers requested label Jun 3, 2025
@mikehardy mikehardy added this pull request to the merge queue Jun 4, 2025
Merged via the queue into ankidroid:main with commit d88b9db Jun 4, 2025
9 checks passed
@github-actions github-actions bot added this to the 2.21 release milestone Jun 4, 2025
@github-actions github-actions bot removed the Pending Merge Things with approval that are waiting future merge (e.g. targets a future release, CI wait, etc) label Jun 4, 2025
# for free to join this conversation on GitHub. Already have an account? # to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Wrong password require to "enter" tap to dismiss
3 participants