-
Notifications
You must be signed in to change notification settings - Fork 905
[PM-27869] fix/[PM-26241] : draw out keyboard on talkback click #6129
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
base: main
Are you sure you want to change the base?
[PM-27869] fix/[PM-26241] : draw out keyboard on talkback click #6129
Conversation
|
Thank you for your contribution! We've added this to our internal tracking system for review. Details on our contribution process can be found here: https://contributing.bitwarden.com/contributing/pull-requests/community-pr-process. |
f26115f to
ef800d4
Compare
|
@bitwarden-bot @david-livefront Is there any update on this PR? |
ui/src/main/kotlin/com/bitwarden/ui/platform/components/field/BitwardenTextField.kt
Outdated
Show resolved
Hide resolved
ui/src/main/kotlin/com/bitwarden/ui/platform/components/field/BitwardenTextField.kt
Outdated
Show resolved
Hide resolved
ui/src/main/kotlin/com/bitwarden/ui/platform/components/field/BitwardenTextField.kt
Outdated
Show resolved
Hide resolved
e8cff8e to
6392086
Compare
ui/src/main/kotlin/com/bitwarden/ui/platform/components/field/BitwardenTextField.kt
Show resolved
Hide resolved
7410a73 to
8f3fe49
Compare
|
Great job! No new security vulnerabilities introduced in this pull request |
| focused = focusState.isFocused | ||
| if (focused) { | ||
| textFieldValueState = textFieldValueState.copy( | ||
| selection = TextRange(textFieldValueState.text.length), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like there are two tests failing because of the update to text selection here. 🤔
Can you update those
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're absolutely right, thanks for catching that.
The tests were written with an implicit assumption that new text would be inserted at the start of the field.
My fix for the focus behavior moves the cursor to the end, which caused that old test logic to fail. I've just pushed a commit to update the tests to align with this new, correct behavior. 👍
8f3fe49 to
da46c76
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #6129 +/- ##
=======================================
Coverage 84.99% 84.99%
=======================================
Files 723 723
Lines 52854 52854
Branches 7676 7676
=======================================
Hits 44925 44925
Misses 5246 5246
Partials 2683 2683 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|

🎟️ Tracking
#5951
📔 Objective
Test Updates
Context: The fix for the TalkBack focus behavior revealed that two unit tests were brittle. They were built on an incorrect assumption that text would always be inserted at the start of the field.
Solution: This PR also includes a commit to update these tests, making them more robust and aligning them with the new, correct cursor behavior.
📸 Screenshots
before.webm
after.webm
⏰ Reminders before review
🦮 Reviewer guidelines
:+1:) or similar for great changes:memo:) or ℹ️ (:information_source:) for notes or general info:question:) for questions:thinking:) or 💭 (:thought_balloon:) for more open inquiry that's not quite a confirmed issue and could potentially benefit from discussion:art:) for suggestions / improvements:x:) or:warning:) for more significant problems or concerns needing attention:seedling:) or ♻️ (:recycle:) for future improvements or indications of technical debt:pick:) for minor or nitpick changes