[HOLD for payment 2024-01-24] [$500] Web - Workspace - The initial selection applies to the new name too #33652

kbecciv · 2023-12-27T18:03:33Z

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!

Version Number: 1.4.17.1
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
Expensify/Expensify Issue URL:
Issue reported by: Applause - Internal Team
Slack conversation:

Action Performed:

Navigate to https://staging.new.expensify.com/
Log in with an expensifail account
Create a workspace
Create a room named "asd"
Rename the room by selecting "asd" and replacing it with this text from the clipboard "the user is able to select"

Expected Result:

The previous selection shouldn't be applied to the pasted text.

Actual Result:

The initial selection applies to the new name too.

Workaround:

Unknown

Platforms:

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

Screenshots/Videos

Add any screenshot/video evidence

Bug6326857_1703692738965.bandicam_2023-12-26_16-01-44-803.mp4

View all open jobs on GitHub

melvin-bot · 2023-12-27T18:03:37Z

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

melvin-bot · 2023-12-27T18:03:39Z

Triggered auto assignment to @JmillsExpensify (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.

melvin-bot · 2023-12-27T18:03:41Z

Bug0 Triage Checklist (Main S/O)

This "bug" occurs on a supported platform (ensure Platforms in OP are ✅)
This bug is not a duplicate report (check E/App issues and #expensify-bugs)
- If it is, comment with a link to the original report, close the issue and add any novel details to the original issue instead
This bug is reproducible using the reproduction steps in the OP. S/O
- If the reproduction steps are clear and you're unable to reproduce the bug, check with the reporter and QA first, then close the issue.
- If the reproduction steps aren't clear and you determine the correct steps, please update the OP.
This issue is filled out as thoroughly and clearly as possible
- Pay special attention to the title, results, platforms where the bug occurs, and if the bug happens on staging/production.
I have reviewed and subscribed to the linked Slack conversation to ensure Slack/Github stay in sync

melvin-bot · 2023-12-27T18:03:43Z

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

paultsimura · 2023-12-27T18:32:32Z

Proposal

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

When replacing the text in the room name, the selection remains.

What is the root cause of that problem?

This is happening because of the following code:

App/src/components/RoomNameInput/index.js

Lines 35 to 42 in 054eb86

    
           if (modifiedRoomName !== newRoomNameWithHash) { 
        
               const offset = modifiedRoomName.length - oldRoomNameWithHash.length; 
        
               const newSelection = { 
        
                   start: selection.start + offset, 
        
                   end: selection.end + offset, 
        
               }; 
        
               setSelection(newSelection); 
        
           }

It was intended to prevent the cursor from jumping to the end while adding a space (that should be replaced with - in the room name). However, this code did not consider the active selection. So similar bug can be observed when we select some symbol and press space – the selection remains, while we just should place the cursor at the end of the replaced selection.

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

There is no need to move the start of the selection along with the end of it. We should discard the selection and move the cursor to the end of the replaced text (as seen in the recording below):

        if (modifiedRoomName !== newRoomNameWithHash) {
            const offset = modifiedRoomName.length - oldRoomNameWithHash.length;
            const newSelection = {
-               start: selection.start + offset,
+               start: selection.end + offset,
                end: selection.end + offset,
            };
            setSelection(newSelection);
        }

Result:

Screen.Recording.2023-12-27.at.19.35.28.mov

What alternative solutions did you explore? (Optional)

yh-0218 · 2023-12-27T19:28:16Z

Proposal

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

When replacing the text in the room name, the selection remains.

What is the root cause of that problem?

This is happening because of the following code:

App/src/components/RoomNameInput/index.js

Lines 35 to 42 in 054eb86

    
           if (modifiedRoomName !== newRoomNameWithHash) { 
        
               const offset = modifiedRoomName.length - oldRoomNameWithHash.length; 
        
               const newSelection = { 
        
                   start: selection.start + offset, 
        
                   end: selection.end + offset, 
        
               }; 
        
               setSelection(newSelection); 
        
           }

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

We need this logic and add new.

if (modifiedRoomName !== newRoomNameWithHash) {
      const newSelection = {
          start: modifiedRoomName.length,
          end: modifiedRoomName.length,
      };
      setSelection(newSelection);
  }

What alternative solutions did you explore? (Optional)

getusha · 2023-12-29T11:10:45Z

@paultsimura your RCA makes sense, but i am not sure why this is happening on texts with space between words. i am trying to test the solution.

paultsimura · 2023-12-29T11:14:43Z

i am not sure why this is happening on texts with space between words

This is because, in the room names, the spaces (and iOS-specific dashes) get replaced with dashes:

App/src/libs/RoomNameInputUtils.ts

Lines 6 to 14 in 5c5c862

    
           function modifyRoomName(roomName: string): string { 
        
               const modifiedRoomNameWithoutHash = roomName 
        
                   .replace(/ /g, '-') 
        
                   // Replaces the smart dash on iOS devices with two hyphens 
        
                   .replace(/—/g, '--'); 
        
               return `${CONST.POLICY.ROOM_PREFIX}${modifiedRoomNameWithoutHash}`; 
        
           }

And because of this, only for the names with spaces, this condition becomes true:

App/src/components/RoomNameInput/index.js

Lines 35 to 42 in 5c5c862

    
           if (modifiedRoomName !== newRoomNameWithHash) { 
        
               const offset = modifiedRoomName.length - oldRoomNameWithHash.length; 
        
               const newSelection = { 
        
                   start: selection.start + offset, 
        
                   end: selection.end + offset, 
        
               }; 
        
               setSelection(newSelection); 
        
           }

melvin-bot · 2024-01-01T12:19:10Z

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

getusha · 2024-01-01T13:54:53Z

Testing proposals.

paultsimura · 2024-01-01T13:57:01Z

@getusha does this explanation make sense to you though? #33652 (comment)

getusha · 2024-01-01T13:59:51Z

And because of this, only for the names with spaces, this condition becomes true:

~~@paultsimura Can't we instead compare the length of the strings like if (modifiedRoomName.length !== newRoomNameWithHash.length) then?~~

paultsimura · 2024-01-01T14:08:54Z

Can't we instead compare the length of the strings

We must compare the string values here. If we enter "some text", it will be transformed into "some-text", so the length will be the same and the logic will not be executed. As a result, the cursor will jump to the end of the input every time a space is added, and this logic was added to fix exactly this:

Screen.Recording.2024-01-01.at.15.06.35.mov

The whole logic is correct, we just need to handle the selection start as I've described in the proposal.

JmillsExpensify · 2024-01-03T18:34:35Z

Discussing proposals

getusha · 2024-01-04T15:13:04Z

The proposal from @paultsimura looks good to me and works well, thanks for the patience!
🎀 👀 🎀
C+ Reviewed!

melvin-bot · 2024-01-04T15:13:11Z

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

marcaaron · 2024-01-05T22:45:40Z

Proposal looks ok. The bug feels very minor to me. I am surprised we have such complex logic for pasting text into an input.

paultsimura · 2024-01-06T15:50:36Z

Thanks. The PR is ready for review: #34076

melvin-bot · 2024-01-17T01:08:10Z

Reviewing label has been removed, please complete the "BugZero Checklist".

melvin-bot · 2024-01-17T01:08:14Z

The solution for this issue has been 🚀 deployed to production 🚀 in version 1.4.25-10 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

fix: Place cursor at the end of the replaced selection #34076

If no regressions arise, payment will be issued on 2024-01-24. 🎊

For reference, here are some details about the assignees on this issue:

@paultsimura requires payment (Needs manual offer from BZ)
@getusha requires payment (Needs manual offer from BZ)

melvin-bot · 2024-01-17T01:08:15Z

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:

[@getusha] The PR that introduced the bug has been identified. Link to the PR:
[@getusha] 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:
[@getusha] A discussion in #expensify-bugs 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:
[@getusha] Determine if we should create a regression test for this bug.
[@getusha] If we decide to create a regression test for the bug, please propose the regression test steps to ensure the same bug will not reach production again.
[@JmillsExpensify] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

JmillsExpensify · 2024-01-26T15:50:45Z

@getusha Mind helping get us kicked off with the BZ checklist?

JmillsExpensify · 2024-01-26T15:51:39Z

Payment summary:

Issue reporter: QA
Contributor: @paultsimura $500
Contributor+: @getusha $500

JmillsExpensify · 2024-01-26T15:54:26Z

Offers sent to both contributors!

paultsimura · 2024-01-26T16:18:15Z

Thanks, accepted

getusha · 2024-01-26T16:38:28Z

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:

[@getusha] The PR that introduced the bug has been identified. Link to the PR: https://github.com/Expensify/App/pull/

[@getusha] 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: https://github.com/Expensify/App/pull/12899/files#r1467873981

[@getusha] A discussion in #expensify-bugs 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: N/a

[@getusha] Determine if we should create a regression test for this bug. No, this is a very minor bug

[@getusha] If we decide to create a regression test for the bug, please propose the regression test steps to ensure the same bug will not reach production again.

JmillsExpensify · 2024-01-27T16:29:12Z

Thanks @getusha. Those make sense to me. I've processed your offer in Upwork so I'll close this issue.

kbecciv added External Added to denote the issue can be worked on by a contributor Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Dec 27, 2023

melvin-bot bot changed the title ~~Web - Workspace - The initial selection applies to the new name too~~ [$500] Web - Workspace - The initial selection applies to the new name too Dec 27, 2023

melvin-bot bot assigned JmillsExpensify Dec 27, 2023

melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Dec 27, 2023

melvin-bot bot assigned getusha Dec 27, 2023

melvin-bot bot added the Overdue label Jan 1, 2024

melvin-bot bot removed the Overdue label Jan 1, 2024

melvin-bot bot added the Overdue label Jan 3, 2024

melvin-bot bot removed the Overdue label Jan 3, 2024

melvin-bot bot assigned marcaaron Jan 4, 2024

marcaaron assigned paultsimura Jan 5, 2024

melvin-bot bot removed the Help Wanted Apply this label when an issue is open to proposals by contributors label Jan 5, 2024

paultsimura mentioned this issue Jan 6, 2024

fix: Place cursor at the end of the replaced selection #34076

Merged

52 tasks

melvin-bot bot added Reviewing Has a PR in review Weekly KSv2 and removed Daily KSv2 labels Jan 6, 2024

melvin-bot bot added Weekly KSv2 Awaiting Payment Auto-added when associated PR is deployed to production and removed Weekly KSv2 labels Jan 17, 2024

melvin-bot bot changed the title ~~[$500] Web - Workspace - The initial selection applies to the new name too~~ [HOLD for payment 2024-01-24] [$500] Web - Workspace - The initial selection applies to the new name too Jan 17, 2024

melvin-bot bot removed the Reviewing Has a PR in review label Jan 17, 2024

getusha mentioned this issue Jan 26, 2024

Fix: Cursor moves to end after adding space in Room Name #12899

Merged

98 tasks

JmillsExpensify closed this as completed Jan 27, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[HOLD for payment 2024-01-24] [$500] Web - Workspace - The initial selection applies to the new name too #33652

[HOLD for payment 2024-01-24] [$500] Web - Workspace - The initial selection applies to the new name too #33652

kbecciv commented Dec 27, 2023 •

edited

Loading

melvin-bot bot commented Dec 27, 2023

melvin-bot bot commented Dec 27, 2023

melvin-bot bot commented Dec 27, 2023

melvin-bot bot commented Dec 27, 2023

paultsimura commented Dec 27, 2023 •

edited

Loading

yh-0218 commented Dec 27, 2023

getusha commented Dec 29, 2023

paultsimura commented Dec 29, 2023 •

edited

Loading

melvin-bot bot commented Jan 1, 2024

getusha commented Jan 1, 2024

paultsimura commented Jan 1, 2024

getusha commented Jan 1, 2024 •

edited

Loading

paultsimura commented Jan 1, 2024

JmillsExpensify commented Jan 3, 2024

getusha commented Jan 4, 2024

melvin-bot bot commented Jan 4, 2024

marcaaron commented Jan 5, 2024

paultsimura commented Jan 6, 2024

melvin-bot bot commented Jan 17, 2024

melvin-bot bot commented Jan 17, 2024

melvin-bot bot commented Jan 17, 2024

JmillsExpensify commented Jan 26, 2024

JmillsExpensify commented Jan 26, 2024

JmillsExpensify commented Jan 26, 2024

paultsimura commented Jan 26, 2024

getusha commented Jan 26, 2024

JmillsExpensify commented Jan 27, 2024

[HOLD for payment 2024-01-24] [$500] Web - Workspace - The initial selection applies to the new name too #33652

[HOLD for payment 2024-01-24] [$500] Web - Workspace - The initial selection applies to the new name too #33652

Comments

kbecciv commented Dec 27, 2023 • edited Loading

Action Performed:

Expected Result:

Actual Result:

Workaround:

Platforms:

Screenshots/Videos

melvin-bot bot commented Dec 27, 2023

melvin-bot bot commented Dec 27, 2023

melvin-bot bot commented Dec 27, 2023

Bug0 Triage Checklist (Main S/O)

melvin-bot bot commented Dec 27, 2023

paultsimura commented Dec 27, 2023 • edited Loading

Proposal

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

What is the root cause of that problem?

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

Result:

What alternative solutions did you explore? (Optional)

yh-0218 commented Dec 27, 2023

Proposal

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

What is the root cause of that problem?

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

What alternative solutions did you explore? (Optional)

getusha commented Dec 29, 2023

paultsimura commented Dec 29, 2023 • edited Loading

melvin-bot bot commented Jan 1, 2024

getusha commented Jan 1, 2024

paultsimura commented Jan 1, 2024

getusha commented Jan 1, 2024 • edited Loading

paultsimura commented Jan 1, 2024

JmillsExpensify commented Jan 3, 2024

getusha commented Jan 4, 2024

melvin-bot bot commented Jan 4, 2024

marcaaron commented Jan 5, 2024

paultsimura commented Jan 6, 2024

melvin-bot bot commented Jan 17, 2024

melvin-bot bot commented Jan 17, 2024

melvin-bot bot commented Jan 17, 2024

JmillsExpensify commented Jan 26, 2024

JmillsExpensify commented Jan 26, 2024

JmillsExpensify commented Jan 26, 2024

paultsimura commented Jan 26, 2024

getusha commented Jan 26, 2024

JmillsExpensify commented Jan 27, 2024

kbecciv commented Dec 27, 2023 •

edited

Loading

paultsimura commented Dec 27, 2023 •

edited

Loading

paultsimura commented Dec 29, 2023 •

edited

Loading

getusha commented Jan 1, 2024 •

edited

Loading