-
Notifications
You must be signed in to change notification settings - Fork 3k
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
[HOLD #23959] Web - RHN - Opening RHN after opening the emoji picker will keep the composer focused #25584
Comments
Triggered auto assignment to @mallenexpensify ( |
Bug0 Triage Checklist (Main S/O)
|
ProposalPlease re-state the problem that we are trying to solve in this issue.Web - RHN - Opening RHN after opening the emoji picker will keep the composer focused What is the root cause of that problem?When emoji picker popover is closed it is automatically focusing to composer, and this line is responsible for this: App/src/pages/home/report/ReportActionCompose.js Line 1234 in cea152b
Hiding modal comes before pressing event and expected behavior should be hide modal -> focus composer -> press event(which should unfocus composer) but since we are focusing in InteractionManager.runAfterInteractions it is causing focus event to be executed lastly like so hide modal -> press event -> focus composer https://github.com/Expensify/App/blob/f600d338ac1778ee7bc8383d691b04f720783446/src/libs/focusWithDelay/focusWithDelay.js#L22C1-L22C1 What changes do you think we should make in order to solve the problem?I think this logic causes a lot of unexpected focuses in composer, like it is not just RHN but pressing anything after opening emoji picker results in the same. For example pressing search after emoji picker causes focus to be in composer and not in search page, or pressing FAB you can't use up/down buttons because focus is still in composer. That is why, I think we should completely get rid of this logic. App/src/pages/home/report/ReportActionCompose.js Line 1235 in cea152b
So that when emoji is picked we focus back in composer What alternative solutions did you explore? (Optional)One more solution is to execute focus event immediately not inside |
ProposalPlease re-state the problem that we are trying to solve in this issue.Opening RHN after opening the emoji picker will keep the composer focused What is the root cause of that problem?When the emoji picker closes, the focus the composer: App/src/pages/home/report/ReportActionCompose.js Lines 1232 to 1236 in cea152b
However, we are not checking What changes do you think we should make in order to solve the problem?If the composer is not visible, we should not continue to focus the composer. In ReportActionCompose.js: - focusWithDelay(textInputRef.current)(shouldDelay);
+ focusWithDelay(textInputRef.current)(shouldDelay, () => composerVisibility.current); In focusWithDelay.js: function focusWithDelay(disableDelay = false) {
...
- (shouldDelay = false) => {
+ (shouldDelay = false, shouldFocusAfterInteractions = () => true) => {
...
InteractionManager.runAfterInteractions(() => {
- if (!textInput) {
+ if (!textInput || !shouldFocusAfterInteractions()) {
return;
}
... What alternative solutions did you explore? (Optional)N/A |
ProposalPlease re-state the problem that we are trying to solve in this issue.Opening RHN after opening the emoji picker will keep the composer focused What is the root cause of that problem?
App/src/pages/home/report/ReportActionCompose.js Lines 1232 to 1235 in 4a7558c
What changes do you think we should make in order to solve the problem?We use |
The root cause is we are focusing the composer with a delay in closing the emoji picker. We have a plan to remove the delay in this issue, but we are waiting for upstream PRs which would take an unknown amount of time. If waiting is fine, we can hold this one. |
@mallenexpensify Whoops! This issue is 2 days overdue. Let's get this updated quick! |
@mallenexpensify Still overdue 6 days?! Let's take care of this! |
Thanks @bernhardoj for the context |
@mallenexpensify this issue was created 2 weeks ago. Are we close to a solution? Let's make sure we're treating this as a top priority. Don't hesitate to create a thread in #expensify-open-source to align faster in real time. Thanks! |
Updated title and moved to weekly |
Holding on upstream |
Holding |
Held issue got downgraded to Monthly, so this one might be a while. Adding to #vip-vsb cuz it doesn't involve money. |
Bumped to monthly cuz held issue was bumped to monthly |
Holdin' |
Waiting for feedback on |
PR for issue we're held on is waiting review |
UI is drastically changed since this was reported over a year ago, closing, unable to reproduce |
If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!
Action Performed:
Expected Result:
The composer should not be focused.
Actual Result:
The composer is focused and can type on it.
Workaround:
Unknown
Platforms:
Which of our officially supported platforms is this issue occurring on?
Version Number: v1.3.55-7
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
Notes/Photos/Videos: Any additional supporting documentation
Screen.Recording.2023-08-11.At.12.36.44.Pm.mp4
20230821_200321.mp4
Expensify/Expensify Issue URL:
Issue reported by: @getusha
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1691746780206579
View all open jobs on GitHub
The text was updated successfully, but these errors were encountered: