-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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
16098 — selecting chat message and pressing any key focus the message instead of switch the focus to the composer #21583
16098 — selecting chat message and pressing any key focus the message instead of switch the focus to the composer #21583
Conversation
@Santhosh-Sellavel Please 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] |
Moved here |
@robertKozik Tests mentioned here work well, please include the linked #17649 (comment) steps here as well, it would be easy for everyone! |
@Santhosh-Sellavel Sure, pasted the testes 👌🏼 |
…y-key-focus-the-message-instead-of-switch-the-focus-to-the-composer
Bug: Pressing copy paste is broken Steps:
Data is pasted multiple times Comparision between staging & devScreen.Recording.2023-06-29.at.1.23.06.AM.mov |
Resolve conflicts and look into the above bug I will continue testing tomorrow! |
@Santhosh-Sellavel @PauloGasparSv I want to clarify one thing about this issue/PR. From this issue: #17411 (linked in the repro steps) I assumed that copy&pasting function which will focus composer from anywhere should be implemented. But I missed that in this issue that was pointed out as |
I think yes, we should do it here. Actually, implementation works, but there is a breakage |
With new changes from last commit everything should be fine. Looks like after redirecting to the new page, the composer is not unmounting. Because of it, I bind the lifecycle of the listeners with the navigation focus/blur, so we wont register multiple ones - which was causing the breakage. |
@robertKozik Now the Typing does not focus on the composer Screen.Recording.2023-06-30.at.4.42.11.AM.mov |
@robertKozik Also fix the lint |
@Santhosh-Sellavel Can you double check if you were using my branch? I checked it locally and It's working on my end Screen.Recording.2023-06-30.at.01.24.02.mov |
@robertKozik Try merging with the latest main, seems it fails after merging please check! |
…t-message-and-pressing-any-key-focus-the-message-instead-of-switch-the-focus-to-the-composer
@Santhosh-Sellavel @PauloGasparSv I've moved the ref for the |
src/pages/home/sidebar/SidebarScreen/FloatingActionButtonAndPopoverUtils.js
Outdated
Show resolved
Hide resolved
Suggestion Resolved |
Sorry @robertKozik I missed this component earlier, can we have some other hook to detect visible popups? Screen.Recording.2023-07-10.at.1.33.39.AM.mov |
It for sure can be done in a similar manner. Do we know if there can be more such a popups? If so I'll opt for coming up with more generic approach to them, if all the popovers are using the same component. But as it's only the second one to check, it can be done in same way. |
@Santhosh-Sellavel I've switched the way of detecting such a modals to use onyx data ( |
Can you please resolve conflicts? |
…y-key-focus-the-message-instead-of-switch-the-focus-to-the-composer
I'm starting tests here and Web is looking fine : D I'll resume and finish testing in ~2hours |
LGTM Tests well! Screen.Recording.2023-07-11.at.9.08.56.PM.mov |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM tests well!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
Web
Web.mov
Mobile Web - Chrome
Skipped, still having problems with chrome in the android emulator
Mobile Web - Safari
iosMweb.mov
Desktop
Deskto.mov
iOS
iOs.mov
Android
Android.mov
🚀 Deployed to staging by https://github.com/PauloGasparSv in version: 1.3.40-0 🚀
|
🚀 Deployed to production by https://github.com/Julesssss in version: 1.3.40-5 🚀
|
// If the key pressed is non-character keys like Enter, Shift, ... do not focus | ||
if (e.key.length > 1) { | ||
return; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We forgot to consider the space
as the key which also shouldn't focus the composer. This lead to issue #23508
} | ||
|
||
this.focus(); | ||
this.replaceSelectionWithText(e.key, false); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This call was redundant. Later it caused #33710.
More details about the root cause: #33710 (comment)
Details
This PR creates new document-scoped key listener which is connected with composer lifecycle. Listener will be mounted only when composer is rendered. It listens on any alphanumeric key event, and focuses the composer accordingly
Also there is added a fix for the ROUTES method
parseReportRouteParams
as it was not suited to theNavigation.getActiveRoute
returend path - it don't have slash at the beggining.the copy&paste listener is now document-scoped rather than connected to textInput and restricted in the same manner as new key listner
Fixed Issues
$ #16098
PROPOSAL: #16098 (comment)
Tests
As the test steps also the regression tests pasted by @PauloGasparSv in previous PR can be used :
Typing should focus on the composer
Typing in other inputs and modals
(Coming from here)
The composer Emoji Picker
(Coming from #17413)
The FAB popover menu
+
) in the bottom left of the screen"Meeting" popup
three dot popup
Copying&Pasting - Web & Desktop only
(Coming from #17397 and #17411)
Offline tests
Same as test steps
QA Steps
Same as test steps
PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)myBool && <MyComponent />
.src/languages/*
files and using the translation methodWaiting for Copy
label for a copy review on the original GH to get the correct copy.STYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)/** comment above it */
this
properly so there are no scoping issues (i.e. foronClick={this.submit}
the methodthis.submit
should be bound tothis
in the constructor)this
are necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);
ifthis.submit
is never passed to a component event handler likeonClick
)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)ScrollView
component to make it scrollable when more elements are added to the page.main
branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTest
steps.Screenshots/Videos
Web
web.mov
Mobile Web - Chrome
chrome.mov
Mobile Web - Safari
safari.mov
Desktop
desktop.mov
iOS
iOS.mov
Android
android.mov