fix(TokenCompleteTextView): let the tab key move focus out of the field - #11413
Conversation
|
The Everything else is waiting on workflow approval. Locally the module's tests, lint, spotless and detekt pass, along with |
|
✅ Validation Passed: All report and feature-flag labels are correctly set. |
onKeyDown consumed the tab key together with enter and dpad centre, so the event never reached the framework's focus navigation and the focus could not leave a recipient field with a hardware keyboard. Shift+tab kept working because hasNoModifiers() is false for it, which is why the field could only be left backwards. AutoCompleteTextView already handles tab the way it should: it consumes the key while the completion list is open and leaves it alone otherwise, and TextView returns it unhandled precisely so that focus can move. So stop handling tab in onKeyDown and let the superclass decide. That leaves AutoCompleteTextView's habit of completing immediately on tab, which picks a suggestion for the user as soon as there are several. Handle the key in onKeyUp instead: a single suggestion is completed and the focus stays put, while several suggestions move the selection into the list so it can be walked with tab or the arrow keys and confirmed with enter. The drop down hides its selection, and ignores keys, until something is selected, so selecting the first suggestion is what makes it navigable at all. Enter and dpad centre keep the behaviour they had. The field shouldFocusNext gated is renamed to completeOnKeyUp, since completing is what it actually does, and it is now drained at the top of onKeyUp so that a tab key up arriving while enter is still held cannot leave it armed to fire on a later, unrelated key up. Fixes thunderbird#8231
634166b to
b95022c
Compare
|
Pushed two follow-ups to this branch. The first is a real bug. The second is a test for the real key sequence. The tab tests called I should be straight about those two. They pass against the unpatched widget as well, so they are guards, not proof. I could not get Robolectric to reproduce the device path that tells them apart, because the drop down there does not end up in the same state. The discriminating coverage is still the four original tab tests plus the new drain test, and the device check below. I re-verified on an Android 16 emulator, since the reordering changes when |
|
@snowykte0426, could you please create a more concise summary? There is too much information there that makes it hard to follow. |
|
Thanks for your contribution! Your pull request has been merged and will be part of Thunderbird 24. We appreciate the time and effort you put into improving Thunderbird. If you haven’t already, you’re welcome to join our Matrix chat for contributors. It’s where we discuss development and help each other out. https://matrix.to/#/#tb-android-dev:mozilla.org |
…ected Moving through the recipient completion list with a keyboard left no trace on screen, so there was no way to tell which suggestion was about to be picked. The list selector cannot do this reliably. `setListSelection()` reaches `ListView.layoutChildren`, which positions the selector with `positionSelector(INVALID_POSITION, sel)`; `AbsListView.positionSelector` then computes `positionChanged = position != mSelectorPosition` and skips the block that refreshes the selector's drawable state, because both sides are -1. The selector is therefore painted with an empty state set on the first selection. Its state is the list's own state as well, and `DropDownListView` reports itself as focused for as long as the popup exists, so a stateful selector cannot tell a selected row from a cleared one either. The row does not have that problem: `ListView.setupChild` calls `child.setSelected(selected && shouldShowSelector())`, which is correct from the first selection onwards. So give the row a background that draws the selection, and leave the list selector alone so touch feedback is unchanged. `colorSecondaryContainer`, the Material token for a selected list item, is not usable here: the rows take their text colour from `textColorPrimaryRecipientDropdown`, which is `@android:color/primary_text_light` and pinned to black in the light theme, and that only reaches 3.3:1 on `lightSecondaryContainer`. `colorSurfaceContainerHighest` keeps the text at 16:1 but is itself only 1.22:1 against the popup background, so the selected row also gets a `colorPrimary` outline: 7.7:1 in the light theme and 14.1:1 in the dark one, which meets WCAG 1.4.11 for a state indicator. This has no visible effect on its own. The completion list ignores the arrow keys until something is selected, so thunderbird#11413, which makes the tab key select the first suggestion, is what reaches this code path.
2 participants
Contribution Summary
Linked Issue/Ticket: Fixes #8231
RFC / Technical Design (if applicable): n/a, behaviour agreed in this comment
Description
With a hardware keyboard the focus could not leave To, Cc or Bcc.
TokenCompleteTextView.onKeyDownconsumed the tab key along with enter and dpad centre.ViewRootImplonly moves focus for a key that comes back unhandled, so consuming it trapped the focus. Shift+tab kept working, becausehasNoModifiers()is false for it. That is the "only way out is backwards" from the first comment on the issue.AutoCompleteTextViewalready gets tab right. It consumes the key while the completion list is open and leaves it alone otherwise, andTextViewdeliberately returns it unhandled so that focus can move. So tab is no longer touched inonKeyDown.It is handled in
onKeyUpinstead. Left alone,AutoCompleteTextViewcompletes on tab immediately, which picks a suggestion for the user as soon as there is more than one to pick from. Now: with the list closed the key falls through and the framework moves the focus on. With a single suggestion tab completes it and the focus stays in the field. With several, tab selects into the list, and you walk it with tab or the arrow keys and confirm with enter.Enter and dpad centre behave as they did.
Screen Shots
Verified on an Android 16 emulator with a real tab key (
adb shell input keyevent 61), and two contacts so the number of suggestions could be controlled. Before, tab did nothing at all. After, it walks To, the Cc/Bcc expander, Subject and then Message text; a single suggestion gets completed; and tab, tab, enter commits the second of two.Tests are in
TokenCompleteTextViewBehaviorTest, on the Robolectric harness that was already there. Five of them fail against the current code.Two things worth knowing. Tab no longer hides the soft keyboard, which used to happen through
handleDone(). With an external keyboard there is nothing to hide, and once tab lands on Subject you want the keyboard up anyway, so I left it that way; say the word if you would rather have the old behaviour back. The other is that the selected suggestion still is not highlighted. That turned out to be a separate problem in the drop down's styling, and #11416 fixes it.AI Disclosure
Select one of the following (mandatory)
Contribution Checklist
gradlew spotlessCheckto check andgradlew spotlessApplyto format your source code; will be checked by CI).gradlew testDebugUnitTest; will be checked by CI).The Kotlin box is unticked because this changes the vendored
token-auto-completewidget, which is Java, and its tests sit alongside it.