-
Notifications
You must be signed in to change notification settings - Fork 3.2k
[Due for payment 2025-04-14] [$250] Search - Pasting long text removes left padding & cursor focus is missing #58214
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
Comments
Triggered auto assignment to @isabelastisser ( |
🚨 Edited by proposal-police: This proposal was edited at 2025-03-11 13:47:54 UTC. ProposalPlease re-state the problem that we are trying to solve in this issue.Pasting long text removes left padding & cursor focus is missing What is the root cause of that problem?We apply and don't apply any padding right on App/src/components/Search/SearchAutocompleteInput.tsx Lines 223 to 224 in 22fed8e
What changes do you think we should make in order to solve the problem?Apply both padding left and right (padding horizontal) on and remove textInputContainerStyles={[styles.borderNone, styles.pb0, styles.ph3]}
inputStyle={[inputWidth, {lineHeight: undefined}]} Additionally, if we want to address the cursor focus issue, we can use const [cursorSelection, setCursorSelection] = useState<{start: number; end: number}>({start: 0, end: 0});
const handleSelectionChange = (e: NativeSyntheticEvent<TextInputSelectionChangeEventData>) => {
setCursorSelection(e.nativeEvent.selection);
};
useEffect(() => {
if (ref && typeof ref !== 'function' && ref.current) {
const input = ref.current as HTMLInputElement;
if (cursorSelection.end === value.length) {
scrollToRight(input);
}
}
}, [value, cursorSelection, ref]); and we should add <TextInput
...
textInputContainerStyles={[styles.borderNone, styles.pb0, styles.ph3]}
inputStyle={[inputWidth, {lineHeight: undefined}]}
...
onSelectionChange={handleSelectionChange}
/> What specific scenarios should we cover in automated tests to prevent reintroducing this issue in the future?None What alternative solutions did you explore? (Optional)New-Expensify.mp4 |
ProposalPlease re-state the problem that we are trying to solve in this issue.Pasting short text maintains proper left padding and keeps focus on the cursor. What is the root cause of that problem?When text is pasted into the App/src/hooks/useHtmlPaste/index.ts Lines 45 to 55 in 9b51ab8
Copied content is added to the text field only at the position where the cursor is currently located without automatically scrolling to the new cursor position. What changes do you think we should make in order to solve the problem?Add this logic to calculate current position of the cursor and auto scroll input that position.
This solution not only solve the problem of the paste action with Input Search on this screen but also in all other screen which use With the left padding we've already have a PR here Text in search input, bounces with every letter added to the search to set opacity of the loading so we don't need add more padding and when we autoscroll with above logic, padding left will be correct What specific scenarios should we cover in automated tests to prevent reintroducing this issue in the future?NA What alternative solutions did you explore? (Optional)Reminder: Please use plain English, be brief and avoid jargon. Feel free to use images, charts or pseudo-code if necessary. Do not post large multi-line diffs or write walls of text. Do not create PRs unless you have been hired for this job. |
Job added to Upwork: https://www.upwork.com/jobs/~021899895633486475589 |
Triggered auto assignment to Contributor-plus team member for initial proposal review - @mananjadhav ( |
Hi @mananjadhav, can you please review the proposals above? Thanks! |
I think we have similar solution of adding scrollToRight during the search. @nyomanjyotisa's proposal looks better to me. 🎀 👀 🎀 C+ reviewed |
Triggered auto assignment to @marcochavezf, see https://stackoverflow.com/c/expensify/questions/7972 for more details. |
@mananjadhav Can you help me explain why my proposal is not good? I think this is a global issue and needs to be addressed at all inputs. Screen.Recording.2025-03-13.at.17.34.29.mov |
@mananjadhav Eep! 4 days overdue now. Issues have feelings too... |
@mananjadhav, can you please follow up on the question? I will DM you for visibility. thanks! |
It’s waiting for review from @marcochavezf. @marcochavezf what do you think of the comment by #58214 (comment). I did a basic test of their proposal and it seems to work. But I haven’t stress tested. Meanwhile the earlier selected proposal solves it at one place. Let me know your thoughts. |
Oh, interesting. I'm inclined toward the solution that fixes it everywhere; otherwise, we might end up solving the issue again in other cases. Let's assign @thelullabyy, I think we can work on the edge cases (if any) in the PR 🚀 |
📣 @thelullabyy 🎉 An offer has been automatically sent to your Upwork account for the Contributor role 🎉 Thanks for contributing to the Expensify app! Offer link |
@mananjadhav Uh oh! This issue is overdue by 2 days. Don't forget to update your issues! |
|
The solution for this issue has been 🚀 deployed to production 🚀 in version 9.1.23-7 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue: If no regressions arise, payment will be issued on 2025-04-14. 🎊 For reference, here are some details about the assignees on this issue:
|
@mananjadhav @isabelastisser @mananjadhav The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed. Please copy/paste the BugZero Checklist from here into a new comment on this GH and complete it. If you have the K2 extension, you can simply click: [this button] |
BugZero Checklist:
Bug classificationSource of bug:
Where bug was reported:
Who reported the bug:
Regression Test Proposal Template
Regression Test ProposalPrecondition:Test:Do we agree 👍 or 👎 |
Payment summary: @mananjadhav requires payment through NewDot Manual Request. $250 for C+ review |
@mananjadhav, please complete the BZ checklist. Thanks! |
BugZero Checklist:
Bug classificationSource of bug:
Where bug was reported:
Who reported the bug:
I think it makes sense to add the regression test as it relates to a common component. Regression Test ProposalTest:
Do we agree 👍 or 👎 |
$250 approved for @mananjadhav |
All set! |
Uh oh!
There was an error while loading. Please reload this page.
If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!
Version Number: V9.1.11-0
Reproducible in staging?: Yes
Reproducible in production?: Yes
If this was caught on HybridApp, is this reproducible on New Expensify Standalone?: N/A
If this was caught during regression testing, add the test name, ID and link from TestRail: N/A
Email or phone of affected tester (no customers): [email protected]
Issue reported by: Applause Internal Team
Device used: Redminote 10s android 13
App Component: Search
Action Performed:
Expected Result:
The left padding should be consistent for both short and long pasted text.
After pasting long text, the focus should remain on the cursor.
Actual Result:
Pasting short text maintains proper left padding and keeps focus on the cursor.
Pasting long text removes left padding, making the text stick to the left margin.
Cursor focus is missing, requiring the user to scroll to bring it into view.
Workaround:
Unknown
Platforms:
Screenshots/Videos
bug.mp4
View all open jobs on GitHub
Upwork Automation - Do Not Edit
Issue Owner
Current Issue Owner: @mananjadhavThe text was updated successfully, but these errors were encountered: