Closed kbecciv closed 1 year ago
@Christinadobrzyn, it's external, I'll post a review today Apologies for the delay everyone
Summary:
This is a known bug in Mobile Chrome. It was reported a year ago, there seems to be no activity or plans to resolve it.
Since this is pretty severe and will not be fixed in Chrome soon, I think we should apply a workaround.
type="search"
. The downside is, this will negatively affect accessibilityI think we need to fix this for all inputs throughout the app. The bug is present for all numeric inputs, not just the magic input (not the screen jumping, but the password manager bar is present while it shouldn't)
In my opinion, i dont think applying workaround to all of numeric inputs (that does not efffect UX by rendering password bar) is necessary considering the accessibility tradeoffs, as both are merely workaround and root cause is unfixable. I would recommend selectively applying the workaround to only input that's causing bad UX like Magiccodeinputs on 2fa screen.
Current assignee @Christinadobrzyn is eligible for the External assigner, not assigning anyone new.
Current assignee @eVoloshchak is eligible for the External assigner, not assigning anyone new.
@eVoloshchak, Thank you checking the proposal I have added. I would like to add the point in the reference to what you have mentioned here:
- @ydhandha's proposal is to set textInput's type the only type that isn't affected:
type="search"
. The downside is, this will negatively affect accessibility
Furthermore, if we use multiline true it will convert into textarea, I believe it will impact the accessibility and screenreaders and for the same below are the differences:
Multiline Text:
Screen Readers: Screen readers typically announce
Assistive Technology: Users who rely on screen readers or other assistive technologies might find
Resizing: Users can often resize the
============== Input:
Single-Line Text: input elements are primarily used for single-line text input, such as usernames, emails, search queries, or short descriptions.
Screen Readers: Screen readers announce elements differently, often indicating the type of input expected
Compact: For single-line input, elements tend to be more compact and visually fit well in various layouts.
Specific Input Types: Different type attributes of can be used to provide specialized input fields, such as email, number, search, and more.
Input or textarea both are built for taking text from user, we by design accepts only one number so user won't be able to enter multiline text or any other text other than a single digit. As you have said text-area are resizable but resizing to text input is disabled by default in the app, Screen readers might associate type=search
to a search input which I believe is a relatively bigger tradeoff for accessibility than replacing input from textarea.
Yes, that's what I am trying to highlight in here. Since we have everything in place that we require, we can simply change the type to search but since, you have provided to use multiline which converts input to textarea, I have checked the same on my local using multiline prop whilst using the screen reader(NVDA) and below is the output as mentioned:
Input (using: type="search"):
Textarea (using: multiline prop)
Furthermore you may check the above using screen readers for checking both the scenario as well as for the accessibility and I would suggest if required, not only on our react-app but you may check the screen readers anywhere(even a raw file which consists of simply textarea and input with any type) to compare how screen reader will read for a user for a textarea as well as for an input.
To state further - Even if we use aria-label attribute which is, specifically, for screen reading, in the end(as a suffix), it would still reads the "Edit Blank" for an input and "Edit Multiline Blank" for textarea specifically in a case where no placeholder value is passed otherwise it would read the placeholder value as well
Example: Consider aria-label="Add OTP here" and we are using textarea and input
Screen reader Output for Input: Add OTP here Edit Blank Screen reader Output for Textarea: Add OTP here Edit Multiline Blank
I hope I have cover everything in here to explain how it will work in both the scenario.
π£ It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? πΈ
Re-posting summary from my comment above:
This is a known bug in Mobile Chrome. It was reported a year ago, there seems to be no activity or plans to resolve it. Since this is pretty severe and will not be fixed in Chrome soon, I think we should apply a workaround.
Testing this with a screen reader, having it say "Edit text. Search field" is better than "Text Area", so @ydhandha's proposal is better from an accessibility standpoint (but it depends on the specific screen reader I'd imagine)
I think we should proceed with @ydhandha's proposal. Changing textinput to a textarea as suggested by @ishpaul777 is a much bigger change and it could potentially introduce some regressions, since there are differences in how different devices/browsers handle textinput and textarea.
πππ C+ reviewed!
Triggered auto assignment to @hayata-suenaga, see https://stackoverflow.com/c/expensify/questions/7972 for more details.
I'm focusing on Wave 5 right now, but I'll come back to this issue to review proposals later this week π
π£ It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? πΈ
@ydhandha
This above strategy certainly targets to solve the issue on platform-specific rather than changing it globally as well as this created function can be imported in other cases where, we may want to use the same to pass some custom attribute.
I like only applying the text input type change to web, but creating a platform specific files for this seems a little bit overwork π€
Anyway, I think setting attribute to native text input through useRef
won't affect iOS and Android native text input components... maybe?
Anyway, let's go with your proposal π
π£ @eVoloshchak Please request via NewDot manual requests for the Reviewer role ($1000)
π£ @ydhandha You have been assigned to this job! Please apply to the Upwork job and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review π§βπ» Once you apply to this job, your Upwork ID will be stored and you will be automatically hired for future jobs! Keep in mind: Code of Conduct | Contributing π
Hey @ydhandha let us know when you have a PR for review!
Hey @hayata-suenaga / @Christinadobrzyn ,
Could you help me with the Upwork Job link? I am seeing job is not available. Neither am I seeing invitation nor can I apply since Job is no longer available. So I believe, once I receive the invitation / I am able to apply, I can work further.
@ydhandha, it's normal, the upwork job has just expired. You can go ahead with raising the PR, usually, the whole upwork process is finalized before payment is due, long after the PR was merged We treat GitHub as the only source of truth, so as long as you're assigned to the issue on GitHub - you're hired and can create the PR
@eVoloshchak, Thank you for providing details.
I'll create the PR max by tomorrow and will be ready for review by then.
Hi @eVoloshchak , PR is ready please check at your end.
Based on my calculations, the pull request did not get merged within 3 working days of assignment. Please, check out my computations here:
On to the next one π
Reviewing
label has been removed, please complete the "BugZero Checklist".
The solution for this issue has been :rocket: deployed to production :rocket: in version 1.3.71-12 and is now subject to a 7-day regression period :calendar:. Here is the list of pull requests that resolve this issue:
If no regressions arise, payment will be issued on 2023-09-27. :confetti_ball:
After the hold period is over and BZ checklist items are completed, please complete any of the applicable payments for this issue, and check them off once done.
For reference, here are some details about the assignees on this issue:
As a reminder, here are the bonuses/penalties that should be applied for any External issue:
BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:
Looks like no speed bonus is applied here.
@eVoloshchak should we have a regression test for this?
Regression Test Proposal
(mobile Chrome only)
Do we agree π or π
Can I get a payment summary for this issue?
Sure:
Contributor: @ydhandha $1000 to be paid via Upwork C+: @eVoloshchak $1000 to be paid via manual request
@ydhandha please apply here
@bfitzexpensify, I have applied to the job link given. Please check and let me know if anything else is needed from my end!
Hey @ydhandha I sent an offer to you, can you accept and I'll pay it out? Thanks!
Hi @Christinadobrzyn, I have accepted the offer.
Regression test GH: https://github.com/Expensify/Expensify/issues/321481
Thanks @ydhandha! I paid you in Upwork based on this payment summary
I think this is good to close?
Thanks @Christinadobrzyn. I can see the payment is done!
$1,000 payment for @eVoloshchak approved based on BZ summary.
If you havenβt already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!
Action Performed:
Expected Result:
The code input stays on focus and there are no unusual jumps
Actual Result:
The screen jumps when removing digits
Workaround:
Unknown
Platforms:
Which of our officially supported platforms is this issue occurring on?
Version Number: 1.3.45-3 Reproducible in staging?: y Reproducible in production?: y If this was caught during regression testing, add the test name, ID and link from TestRail: Email or phone of affected tester (no customers): Logs: https://stackoverflow.com/c/expensify/questions/4856 Notes/Photos/Videos: Any additional supporting documentation
https://github.com/Expensify/App/assets/93399543/e0ee3b91-6a9c-4e7d-8bc4-c891c4947366
Expensify/Expensify Issue URL: Issue reported by: Applause - Internal Team Slack conversation:
View all open jobs on GitHub
Upwork Automation - Do Not Edit