App icon indicating copy to clipboard operation
App copied to clipboard

[$250] mWeb - Group - Name input is displayed in the middle while editing a large group name

Open lanitochka17 opened this issue 1 year ago • 15 comments

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: 9.0.66-0 Reproducible in staging?: Y Reproducible in production?: Y 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: https://expensify.testrail.io/index.php?/tests/view/5256940&group_by=cases:section_id&group_order=asc&group_id=229067 Email or phone of affected tester (no customers): [email protected] Issue reported by: Applause - Internal Team

Action Performed:

  1. Open the staging.new.expensify.com website
  2. Tap on the FAB and select "Start Chat"
  3. Add several users to group chat
  4. Tap on "Next"
  5. Tap on the group´s name
  6. Verify the name´s input is displayed at the end of the name

Expected Result:

When opening group´s name tab to edit it, the input should be displayed at the end of the group´s name

Actual Result:

When trying to edit a group´s long name, the input shows the name in the middle and the user has to scroll to the right to reach the end of the name and the cursor

Workaround:

Unknown

Platforms:

Which of our officially supported platforms is this issue occurring on?

  • [ ] Android: Standalone
  • [ ] Android: HybridApp
  • [x] Android: mWeb Chrome
  • [ ] iOS: Standalone
  • [ ] iOS: HybridApp
  • [ ] iOS: mWeb Safari
  • [ ] MacOS: Chrome / Safari
  • [ ] MacOS: Desktop

Screenshots/Videos

Add any screenshot/video evidence

https://github.com/user-attachments/assets/50f318b3-a69a-477d-a83c-0cc22b01a44b

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~021863445665286781404
  • Upwork Job ID: 1863445665286781404
  • Last Price Increase: 2024-12-02
  • Automatic offers:
    • rojiphil | Reviewer | 105224860
    • daledah | Contributor | 105224861
Issue OwnerCurrent Issue Owner: @rojiphil

lanitochka17 avatar Nov 23 '24 14:11 lanitochka17

Triggered auto assignment to @RachCHopkins (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details. Please add this bug to a GH project, as outlined in the SO.

melvin-bot[bot] avatar Nov 23 '24 14:11 melvin-bot[bot]

Proposal

Please re-state the problem that we are trying to solve in this issue.

When opening group´s name tab to edit it, the input should be displayed at the end of the group´s name

What is the root cause of that problem?

  1. The underlying component for TextInput accepts a prop selection={{start, end}} to control the cursor position, ensuring it is displayed at the end of the text value. This prop needs to be passed from the GroupChatNameEditPage to achieve the desired behaviour https://github.com/Expensify/App/blob/3ebe8520d74c57a2fe9548b7d4307206e2b7c673/src/pages/GroupChatNameEditPage.tsx#L102

  2. In addition to this, the selection prop is only supported in the native environment. For the web environment, additional logic is required to achieve the desired behaviour. This can be implemented by setting the selectionRange and scrollLeft on the inputRef explicitly to ensure the cursor is positioned at the end of the text value. https://github.com/Expensify/App/blob/3ebe8520d74c57a2fe9548b7d4307206e2b7c673/src/components/TextInput/BaseTextInput/index.tsx#L32

What changes do you think we should make in order to solve the problem?

  1. Add the selection prop with the value selection={{start: currentChatName?.length ?? 0, end: currentChatName?.length ?? 0}} to the InputWrapper in the GroupChatNameEditPage https://github.com/Expensify/App/blob/3ebe8520d74c57a2fe9548b7d4307206e2b7c673/src/pages/GroupChatNameEditPage.tsx#L102

  2. Add a useEffect in the web implementation of BaseTextInput to handle text selection and scrolling. The effect listens for changes to isFocused and inputProps.selection. When triggered, it validates the selection props and sets the cursor or selection range using setSelectionRange. Additionally, it adjusts the scrollLeft property of the input element to ensure the text is scrolled to the end if necessary. https://github.com/Expensify/App/blob/3ebe8520d74c57a2fe9548b7d4307206e2b7c673/src/components/TextInput/BaseTextInput/index.tsx#L32

    useEffect(() => {
        const inputElement = input.current;
        // Set selection range and handle scrolling
        inputElement.setSelectionRange(inputProps.selection.start, inputProps.selection.end);
        inputElement.scrollLeft = inputElement.scrollWidth;
    }, [isFocused, inputProps.selection]);
    

What alternative solutions did you explore? (Optional)

prakashbask avatar Nov 23 '24 18:11 prakashbask

Works fine on iOS. I will need Android to test this properly

RachCHopkins avatar Nov 25 '24 21:11 RachCHopkins

Proposal

Please re-state the problem that we are trying to solve in this issue.

When trying to edit a group´s long name, the input shows the name in the middle and the user has to scroll to the right to reach the end of the name and the cursor

What is the root cause of that problem?

  • This issue is caused by the TextInputClearButton component. It works fine when shouldShowClearButton is false.

  • In Chrome (both macOS and Android), the input cursor automatically scrolls to the end when the input is focused. However, upon focusing on the input, there is logic to display the TextInputClearButton component: https://github.com/Expensify/App/blob/2d5479e4a416da6a4617476dbfba67541adeed68/src/components/TextInput/BaseTextInput/index.tsx#L430 This causes the input's width to reduce because the TextInputClearButton occupies some space, resulting in the input cursor becoming hidden.

What changes do you think we should make in order to solve the problem?

  • We need to have a logic to scroll the input cursor to the end when the shouldShowClearButton is displayed. So we can update:

https://github.com/Expensify/App/blob/2d5479e4a416da6a4617476dbfba67541adeed68/src/components/TextInput/BaseTextInput/index.tsx#L430

to:

                            {isFocused && !isReadOnly && shouldShowClearButton && !!value && (
                                <View
                                    onLayout={() => {
                                        if (!didScrollToEndRef.current) {
                                            input.current.scrollLeft = input.current?.scrollWidth;
                                            didScrollToEndRef.current = true;
                                        }
                                    }}
                                >
                                    <TextInputClearButton onPressButton={() => setValue('')} />
                                </View>
                            )}

In the example above, I added an onLayout function that scrolls the input cursor to the end only the first time (when the input is auto-focused). For subsequent focuses, scrolling isn't necessary as those are triggered manually.

What alternative solutions did you explore? (Optional)

Alternative solution 1:

  • We can add:
    useEffect(() => {
        if (!didScrollToEndRef.current && isFocused && !isReadOnly && shouldShowClearButton && !!value) {
            input.current.scrollLeft = input.current?.scrollWidth;
            didScrollToEndRef.current = true;
        }
    }, [isFocused, shouldShowClearButton, value, isReadOnly]);

to this place instead of using onLayout.

Alternative solution 2:

  • We can add:
        setTimeout(()=>{
            inputRef.current.scrollLeft = inputRef.current?.scrollWidth;
        }, 300)

to this place: https://github.com/Expensify/App/blob/2d5479e4a416da6a4617476dbfba67541adeed68/src/hooks/useAutoFocusInput.ts#L32

It will auto-scroll the input cursor to the end after 300ms

daledah avatar Nov 26 '24 09:11 daledah

Got my hands on an android, will repro on Monday.

RachCHopkins avatar Nov 29 '24 05:11 RachCHopkins

Finally got it tested on Android and I finally understand the issue. The group name shows in the middle but the cursor is off-screen to the right end of the name field.

RachCHopkins avatar Dec 02 '24 04:12 RachCHopkins

Job added to Upwork: https://www.upwork.com/jobs/~021863445665286781404

melvin-bot[bot] avatar Dec 02 '24 04:12 melvin-bot[bot]

Triggered auto assignment to Contributor-plus team member for initial proposal review - @rojiphil (External)

melvin-bot[bot] avatar Dec 02 '24 04:12 melvin-bot[bot]

@rojiphil, @RachCHopkins Whoops! This issue is 2 days overdue. Let's get this updated quick!

melvin-bot[bot] avatar Dec 05 '24 09:12 melvin-bot[bot]

Will review today

rojiphil avatar Dec 05 '24 09:12 rojiphil

Thanks for all the proposals. As TextInputClearButton is displayed later, the input's width readjustment causes the cursor to be hidden. @daledah proposal to use onLayout to scroll the input cursor after width readjustment LGTM. 🎀👀🎀 C+ reviewed

rojiphil avatar Dec 06 '24 15:12 rojiphil

Triggered auto assignment to @carlosmiceli, see https://stackoverflow.com/c/expensify/questions/7972 for more details.

melvin-bot[bot] avatar Dec 06 '24 15:12 melvin-bot[bot]

📣 @rojiphil 🎉 An offer has been automatically sent to your Upwork account for the Reviewer role 🎉 Thanks for contributing to the Expensify app!

Offer link Upwork job

melvin-bot[bot] avatar Dec 06 '24 15:12 melvin-bot[bot]

📣 @daledah 🎉 An offer has been automatically sent to your Upwork account for the Contributor role 🎉 Thanks for contributing to the Expensify app!

Offer link Upwork job Please accept the offer and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻 Keep in mind: Code of Conduct | Contributing 📖

melvin-bot[bot] avatar Dec 06 '24 15:12 melvin-bot[bot]

@carlosmiceli @rojiphil @RachCHopkins @daledah this issue was created 2 weeks ago. Are we close to approving a proposal? If not, what's blocking us from getting this issue assigned? Don't hesitate to create a thread in #expensify-open-source to align faster in real time. Thanks!

melvin-bot[bot] avatar Dec 07 '24 10:12 melvin-bot[bot]

This issue has not been updated in over 15 days. @carlosmiceli, @rojiphil, @RachCHopkins, @daledah eroding to Monthly issue.

P.S. Is everyone reading this sure this is really a near-term priority? Be brave: if you disagree, go ahead and close it out. If someone disagrees, they'll reopen it, and if they don't: one less thing to do!

melvin-bot[bot] avatar Jan 02 '25 09:01 melvin-bot[bot]

PRs have been merged.

carlosmiceli avatar Jan 02 '25 13:01 carlosmiceli

BugZero Checklist:

  • [x] [Contributor] Classify the bug:
Bug classification

Source of bug:

  • [x] 1a. Result of the original design (eg. a case wasn't considered)
  • [ ] 1b. Mistake during implementation
  • [ ] 1c. Backend bug
  • [ ] 1z. Other:

Where bug was reported:

  • [x] 2a. Reported on production
  • [ ] 2b. Reported on staging (deploy blocker)
  • [ ] 2c. Reported on a PR
  • [ ] 2z. Other:

Who reported the bug:

  • [ ] 3a. Expensify user
  • [ ] 3b. Expensify employee
  • [ ] 3c. Contributor
  • [x] 3d. QA
  • [ ] 3z. Other:
  • [x] [Contributor] The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake.

    Link to comment

  • [x] [Contributor] If the regression was CRITICAL (e.g. interrupts a core flow) A discussion in #expensify-open-source has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner.

    Link to discussion: Not Required

  • [x] [Contributor] If it was decided to create a regression test for the bug, please propose the regression test steps using the template below to ensure the same bug will not reach production again.

Regression Test Proposal

Precondition:

Test:

  1. Tap on the FAB and select "Start Chat"
  2. Add several users to group chat
  3. Tap on "Next"
  4. Tap on the group´s name
  5. Verify the name´s input is displayed at the end of the name

Do we agree 👍 or 👎

rojiphil avatar Jan 02 '25 15:01 rojiphil

Oh! Not sure why the automation failed here. And I think we need a daily label here

@RachCHopkins This is due payment as the PR is already in production since 18 Dec.
I have completed BZ checklist and accepted offer too. Thanks.

rojiphil avatar Jan 02 '25 15:01 rojiphil

@RachCHopkins Gentle bump on the comment here

rojiphil avatar Jan 09 '25 13:01 rojiphil

Payment Summary:

  • Contributor: @daledah to be paid $250 via Upwork
  • Contributor+: @rojiphil to be paid $250 via Upwork

Upwork job here

RachCHopkins avatar Jan 10 '25 01:01 RachCHopkins

Contributors have been paid, the contracts have been completed, and the Upwork post has been closed.

RachCHopkins avatar Jan 10 '25 01:01 RachCHopkins