Fix: 16098 main composer is focused again when user enters key if the emoji picker is not open #17649

tienifr · 2023-04-19T10:52:17Z

Details

We should automatically focus on the Composer if we press on regular characters like a, b, c, d, ... (not non-character keys like Enter, Shift) when we're not typing on any input or textarea.

Fixed Issues

$ #16098
PROPOSAL: #16098 (comment)

Tests

Go to any chats
Click on any message
Press any character keys like a,b,c,d (not non-character keys like Enter, Shift)
Verify that the composer is focused again and the above character is entered in the composer
Open emoji picker
Press any character keys like a,b,c,d
Verify that the emoji composer is focused again and the above character is entered in this composer. And the main composer is not affected

Verify that no errors appear in the JS console

Offline tests

QA Steps

Go to any chats
Click on any message
Press any character keys like a,b,c,d (not non-character keys like Enter, Shift)
Verify that the composer is focused again and the above character is entered in the composer
Open emoji picker
Press any character keys like a,b,c,d
Verify that the emoji composer is focused again and the above character is entered in this composer. And the main composer is not affected

Verify that no errors appear in the JS console

PR Author Checklist

Screenshots/Videos

Web

Screen.Recording.2023-04-19.at.17.49.13.mp4

Mobile Web - Chrome

RPReplay_Final1680890370.mp4

Mobile Web - Safari

RPReplay_Final1680890255.mp4

Desktop

Screen.Recording.2023-04-08.at.01.07.15.mov

iOS

Screen-Recording-2023-04-08-at-01.06.13.mp4

Android

Screen.Recording.2023-04-08.at.01.27.23.mov

MelvinBot · 2023-04-19T10:59:16Z

@PauloGasparSv @rushatgabhane One of you needs to copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button]

cead22 · 2023-04-19T20:32:52Z

Can we update this PR's title to something that summarizes what's being done in it?

tienifr · 2023-04-24T02:32:23Z

@rushatgabhane @PauloGasparSv any updates?

rushatgabhane · 2023-04-24T13:17:10Z

replying to - #16098 (comment)
@tienifr sorry, but this feature can cause more trouble than it's worth.

I don't think we should implement this feature. It's a UX polish but is it even required?
@PauloGasparSv what do you think?

PauloGasparSv · 2023-04-27T18:16:49Z

Sorry for the delay here guys!

I don't think we should implement this feature. It's a UX polish but is it even required?
@PauloGasparSv what do you think?

I agree and I don't think we shouldn't implement this again at the moment : /

But I don't think we should dismiss this feature just yet! I'll bring this discussion over to slack in #expensify-open-source so more people can chime in and give their opinion on whether we should try to implement this again or not.

rushatgabhane · 2023-05-03T14:21:23Z

we can close this PR, thank you!

PauloGasparSv · 2023-05-03T14:27:17Z

we can close this PR, thank you!

Hey @rushatgabhane I'm confused! Aren't we going to test this?

PauloGasparSv · 2023-05-15T12:44:32Z

@tienifr would you mind fixing the conflicts?

bump @rushatgabhane for a review here please

PauloGasparSv · 2023-05-22T16:58:36Z

@tienifr can you fix the new conflict?

Also, what do you and @rushatgabhane think of using the following test steps? I tried to add all the regressions we had so far as tests here:

Typing should focus on the composer

Open any chat
Click on a message
Start typing alpha-numeric characters (like a,b,c,d… but not Enter, Shift…)
Verify that the composer is focused and the characters you type are inserted there

Typing in other inputs and modals

(Coming from here)

Open any chat
Open the Global Search and make sure the search is working
Blur the global search input and start typing alpha-numeric characters (like a,b,c,d… but not Enter, Shift…)
Make sure the characters you typed aren’t added to the search input or the Chat composer box
Open your profile then display name and make sure the “First name” and “Last name” are not focused then start typing alpha-numeric characters (like a,b,c,d… but not Enter, Shift…)
Go back to the chat and make sure those characters were not added to the composer box
Go to Settings > Workspaces
Open any workspace and go to the “Manage Members” page
With no input focused, start typing alpha-numeric characters (like a,b,c,d… but not Enter, Shift…) and make sure they are not added to the Chat composer box
Click on “Invite” and repeat the previous step

The composer Emoji Picker

(Coming from here)

Open any chat
Click on the Emoji Picker icon in the right side of the composer box
Make sure you can search inside the Emoji Picker but what you type isn’t added to the composer box
With the Emoji Picker opened, blur the search box and start typing alpha-numeric characters (like a,b,c,d… but not Enter, Shift…)
Make sure the focus goes back to the Emoji Picker search box and all characters are added there instead of the Chat composer box
Hover any message and click on the “Add reaction” icon
Repeat steps 3 to 5

Copying&Pasting - Web & Desktop only

(Coming from here and here

Open any chat
Select/Highlight the text from a message
Copy the text using CMD + C or CTRL + C
Click on the composer and paste the text using CMD + V or CTRL + V

PauloGasparSv · 2023-05-22T17:08:36Z

src/pages/home/report/ReportActionCompose/index.js

+     * @param {String} text
+     */
+    replaceSelectionWithInput(text) {
+        const newComment = this.comment.slice(0, this.state.selection.start) + text + this.comment.slice(this.state.selection.end, this.comment.length);


I think we could call ComposerUtils.insertText here right?

PauloGasparSv · 2023-05-22T17:30:35Z

@tienifr can you check why the following is happening after the workspaces page is accessed? The modal is still opened but the input is going to the composer.

Screen.Recording.2023-05-22.at.14.29.02.mov

rushatgabhane · 2023-05-23T01:41:30Z

Tests look good! They capture all the regressions we've faced

tienifr · 2023-05-23T09:55:15Z

can you check why the following is happening after the workspaces page is accessed? The modal is still opened but the input is going to the composer.

After checking our code, I found out that In

App/src/components/Modal/BaseModal.js

Lines 69 to 77 in 6946beb

    
           hideModal(callHideCallback = true) { 
        
               if (this.props.shouldSetModalVisibility) { 
        
                   Modal.setModalVisibility(false); 
        
               } 
        
               if (callHideCallback) { 
        
                   this.props.onModalHide(); 
        
               } 
        
               Modal.onModalDidClose(); 
        
           }

We always call Modal.setModalVisibility(false); even when we press back button (WS -> Setting) (this should not be called). That makes our checks false

 keydownListener(e) {
        if (
            this.state.isFocused ||
            this.state.isEmojiPickerVisible ||
            this.props.modal.isVisible || // false
            this.props.isSmallScreenWidth ||
            EmojiPickerAction.emojiPickerRef.current.state.isEmojiPickerVisible
        ) {
            return;
        }

It can be out of this scope, so I suggest to create another PR to fix this bug first.

My solution:

We should not call Modal.setModalVisibility(false) when we click back button

Change this line

App/src/components/Modal/BaseModal.js

Line 112 in 6946beb

onBackButtonPress={this.props.onClose}

to

onBackButtonPress={()=>{
                    this.shouldGoBack=true;
                    this.props.onClose()
                }}

and update these lines

App/src/components/Modal/BaseModal.js

Lines 70 to 72 in 6946beb

    
           if (this.props.shouldSetModalVisibility) { 
        
               Modal.setModalVisibility(false); 
        
           }

to

hideModal(callHideCallback = true) {
        if (this.props.shouldSetModalVisibility && this.shouldGoBack) {
            Modal.setModalVisibility(false);
        }

Please tell me what you think @PauloGasparSv @rushatgabhane

rushatgabhane · 2023-05-23T17:49:08Z

We should not call Modal.setModalVisibility(false) when we click back button

@tienifr let's try that but I think it'd cause regressions if other components are assuming the visibility to be false when back is pressed.

tienifr · 2023-05-24T02:31:07Z

let's try that but I think it'd cause regressions if other components are assuming the visibility to be false when back is pressed.

@rushatgabhane should we fix the visibility bug in other PR because it's not related to this PR?

PauloGasparSv · 2023-05-24T13:23:32Z

@rushatgabhane should we fix the visibility bug in other PR because it's not related to this PR?

I think we should try here as @rushatgabhane said!

@tienifr I also think we should treat this as related to the bug since we'll introduce the problem in this P.R. : /

Can we attempt that fix you proposed?
I'm confused on when shouldGoBack would be set to false in that logic and if it would cause regressions so I think we should try and test : )

tienifr · 2023-05-24T17:19:32Z

@rushatgabhane @PauloGasparSv I've updated my PR.
Sorry for this solution: #17649 (comment)
It doesn't work because onBackButtonPress is different from the one in HeaderView

I found out that we can lean on navigationRef.current.getState().index if it's 0, the modal is shown otherwise the modal is hidden.

 if (this.props.shouldSetModalVisibility && !navigationRef.current.getState().index) {
            Modal.setModalVisibility(false);
        }

Additional solution

We can use the following code in Navigation.js

navigationRef.current.addListener('state',(e)=>{
            if(e.data.state.index){
// set isVisible = true
}else{
// set isVisible = false
}
})

I tested many cases and there's no regression, but we should double check. Thanks

rushatgabhane · 2023-05-25T07:28:45Z

navigationRef.current.getState().index if it's 0, the modal

I don't think that's reliable enough. Because it's the index of focused route in the routes array.

https://reactnavigation.org/docs/navigation-state/#:~:text=index%20%2D%20Index%20of%20the%20focused%20route%20object%20in%20the%20routes%20array

tienifr · 2023-05-26T04:35:33Z

@rushatgabhane After checking our App, I can't find any cases that the index === 0 but the modal is still open. Can you help find this case? Or do you have any suggestions? Thanks so much

rushatgabhane · 2023-05-28T18:40:22Z

I can't find any cases that the index === 0 but the modal is still open

thing is, in future it could.
Because the docs suggest that routes array is - "List of route objects (screens) which are rendered in the navigator. It also represents the history in a stack navigator. There should be at least one item present in this array"

I think it's a workaround for finding if a modal is open.

tienifr · 2023-05-29T05:01:21Z

I spend much time, but I can't find the best solution, it can cause regression some where. Can you give me any advices? Thanks @rushatgabhane @PauloGasparSv

rushatgabhane · 2023-05-30T11:27:20Z

Okayy, I'll try to work on this today.
Appreciate the effort and time you've spent on this issue @tienifr

PauloGasparSv · 2023-05-31T21:04:32Z

Will investigate tomorrow : )

PauloGasparSv · 2023-06-02T15:01:55Z

Will investigate tomorrow : )

I'll actually have time for this today, couldn't focus on this at all this week. @rushatgabhane do you have any updates on your side?

PauloGasparSv · 2023-06-05T12:33:00Z

Bump @rushatgabhane

rushatgabhane · 2023-06-06T22:07:35Z

Hi, sorry I had looked into this but didn't find anything. I was out sick and I can't prioritize it right now.
What should we do?

PauloGasparSv · 2023-06-08T15:04:19Z

Hi, sorry I had looked into this but didn't find anything. I was out sick and I can't prioritize it right now.
What should we do?

Sorry for the delay and sorry to hear that @rushatgabhane, hope you are feeling better!

Should I re-assign a new C+ member to this issue/P.R.?

rushatgabhane · 2023-06-08T15:09:46Z

@PauloGasparSv yes please could you reassign it to another c+

Santhosh-Sellavel · 2023-06-08T18:10:50Z

@tienifr Please resolve conflicts.

Santhosh-Sellavel · 2023-06-20T16:43:55Z

Let's close this PR @tienifr thanks!

fix: 16098

3e63196

tienifr mentioned this pull request Apr 19, 2023

[HOLD for payment 2023-07-21] [$2000] Selecting a chat message and pressing any key focus the message instead of switching the focus to the composer. #16098

Closed

6 tasks

tienifr marked this pull request as ready for review April 19, 2023 10:59

tienifr requested a review from a team as a code owner April 19, 2023 10:59

melvin-bot bot requested review from PauloGasparSv and rushatgabhane and removed request for a team April 19, 2023 10:59

tienifr changed the title ~~fix: 16098~~ fix: 16098 main composer is focused again when user enters key if the emoji picker is not open Apr 20, 2023

cead22 changed the title ~~fix: 16098 main composer is focused again when user enters key if the emoji picker is not open~~ Fix: 16098 main composer is focused again when user enters key if the emoji picker is not open May 3, 2023

PauloGasparSv assigned tienifr May 15, 2023

merge main

dd477bd

s77rt mentioned this pull request May 19, 2023

[HOLD #16078][$2000] suggestion for Emoji remains even after sending message #15934

Closed

6 tasks

PauloGasparSv reviewed May 22, 2023

View reviewed changes

tienifr added 2 commits May 23, 2023 16:40

fix conflict

1458305

fix merge main

16237e4

fix lint

863af6c

tienifr added 2 commits May 24, 2023 23:07

fix conflict

4f57287

fix: reset modal visibility when index is 0

9ffc22c

PauloGasparSv requested a review from Santhosh-Sellavel June 8, 2023 15:21

fix conflict

d571cb6

tienifr closed this Jun 20, 2023

robertKozik mentioned this pull request Jun 26, 2023

16098 — selecting chat message and pressing any key focus the message instead of switch the focus to the composer #21583

Merged

57 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix: 16098 main composer is focused again when user enters key if the emoji picker is not open #17649

Fix: 16098 main composer is focused again when user enters key if the emoji picker is not open #17649

tienifr commented Apr 19, 2023 •

edited by PauloGasparSv

Loading

MelvinBot commented Apr 19, 2023

cead22 commented Apr 19, 2023

tienifr commented Apr 24, 2023

rushatgabhane commented Apr 24, 2023 •

edited

Loading

PauloGasparSv commented Apr 27, 2023 •

edited

Loading

rushatgabhane commented May 3, 2023

PauloGasparSv commented May 3, 2023

PauloGasparSv commented May 15, 2023

PauloGasparSv commented May 22, 2023

PauloGasparSv May 22, 2023

PauloGasparSv commented May 22, 2023

rushatgabhane commented May 23, 2023

tienifr commented May 23, 2023 •

edited

Loading

rushatgabhane commented May 23, 2023

tienifr commented May 24, 2023

PauloGasparSv commented May 24, 2023

tienifr commented May 24, 2023

rushatgabhane commented May 25, 2023

tienifr commented May 26, 2023

rushatgabhane commented May 28, 2023

tienifr commented May 29, 2023

rushatgabhane commented May 30, 2023 •

edited

Loading

PauloGasparSv commented May 31, 2023

PauloGasparSv commented Jun 2, 2023 •

edited

Loading

PauloGasparSv commented Jun 5, 2023

rushatgabhane commented Jun 6, 2023

PauloGasparSv commented Jun 8, 2023

rushatgabhane commented Jun 8, 2023

Santhosh-Sellavel commented Jun 8, 2023

Santhosh-Sellavel commented Jun 20, 2023

Fix: 16098 main composer is focused again when user enters key if the emoji picker is not open #17649

Fix: 16098 main composer is focused again when user enters key if the emoji picker is not open #17649

Conversation

tienifr commented Apr 19, 2023 • edited by PauloGasparSv Loading

Details

Fixed Issues

Tests

Offline tests

QA Steps

PR Author Checklist

Screenshots/Videos

MelvinBot commented Apr 19, 2023

cead22 commented Apr 19, 2023

tienifr commented Apr 24, 2023

rushatgabhane commented Apr 24, 2023 • edited Loading

PauloGasparSv commented Apr 27, 2023 • edited Loading

rushatgabhane commented May 3, 2023

PauloGasparSv commented May 3, 2023

PauloGasparSv commented May 15, 2023

PauloGasparSv commented May 22, 2023

Typing should focus on the composer

Typing in other inputs and modals

The composer Emoji Picker

Copying&Pasting - Web & Desktop only

PauloGasparSv May 22, 2023

Choose a reason for hiding this comment

PauloGasparSv commented May 22, 2023

rushatgabhane commented May 23, 2023

tienifr commented May 23, 2023 • edited Loading

rushatgabhane commented May 23, 2023

tienifr commented May 24, 2023

PauloGasparSv commented May 24, 2023

tienifr commented May 24, 2023

rushatgabhane commented May 25, 2023

tienifr commented May 26, 2023

rushatgabhane commented May 28, 2023

tienifr commented May 29, 2023

rushatgabhane commented May 30, 2023 • edited Loading

PauloGasparSv commented May 31, 2023

PauloGasparSv commented Jun 2, 2023 • edited Loading

PauloGasparSv commented Jun 5, 2023

rushatgabhane commented Jun 6, 2023

PauloGasparSv commented Jun 8, 2023

rushatgabhane commented Jun 8, 2023

Santhosh-Sellavel commented Jun 8, 2023

Santhosh-Sellavel commented Jun 20, 2023

tienifr commented Apr 19, 2023 •

edited by PauloGasparSv

Loading

rushatgabhane commented Apr 24, 2023 •

edited

Loading

PauloGasparSv commented Apr 27, 2023 •

edited

Loading

tienifr commented May 23, 2023 •

edited

Loading

rushatgabhane commented May 30, 2023 •

edited

Loading

PauloGasparSv commented Jun 2, 2023 •

edited

Loading