-
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
[$1000] mweb - App does not refocus on compose box on message delete using edit mode on android chrome and app #22949
Comments
Triggered auto assignment to @tjferriss ( |
Bug0 Triage Checklist (Main S/O)
|
ProposalPlease restate the problem that we are trying to solve in this issue.when deleting the edit message main composer does not focus again. What is the root cause of that problem?main composer set is not render whe we edit message and we run show composer and focus without wait the composer render again. App/src/pages/home/report/ReportActionItemMessageEdit.js Lines 202 to 203 in 6e5b07c
What changes do you think we should make in order to solve the problem?we can simply wait until the composer is showing and then focus it using runAfterInteractions. ComposerActions.setShouldShowComposeInput(true);
InteractionManager.runAfterInteractions(() => {
ReportActionComposeFocusManager.focus();
}); What alternative solutions did you explore? (Optional)N/A Screen_Recording_20230716_004412_Chrome.mp4 |
ProposalPlease re-state the problem that we are trying to solve in this issue.On mobile web, closing the edit composer does not refocus back the main composer. What is the root cause of that problem?On a small-screen device, when we focus on an edit composer, the main composer will hide.
Either saving or closing the edit composer, it will call App/src/pages/home/report/ReportActionItemMessageEdit.js Lines 288 to 297 in ef3d8cd
However, at this point, the main composer is still hidden. We will show the main composer back when the active edit composer is unmounted (or blurred). App/src/pages/home/report/ReportActionItemMessageEdit.js Lines 192 to 208 in ef3d8cd
So, the focus always fails. What changes do you think we should make in order to solve the problem?When we save/cancel the edit composer, the component will be unmounted. So, we should focus back on the main composer when the component is unmounted and also wait for the Onyx process to complete by making
To make
|
ProposalPlease re-state the problem that we are trying to solve in this issue.Closing the edit composer does not re-focus the main composer on mweb. What is the root cause of that problem?As correctly pointed out by previous proposals, the root cause is that on small screens the main composer is hidden when the edit composer is visible. We're triggering focus when hiding the edit composer. However, It takes some time for the edit composer to hide and for the main composer to show up. This causes the main composer to be not focused. What changes do you think we should make in order to solve the problem?I think this problem is very common and we should queue our
Similarly, the
Lastly, the
ResultScreen.Recording.2023-07-16.at.7.17.05.PM.movWhat alternative solutions did you explore? (Optional)None |
I'm not able to reproduce the issue on the desktop app. The app correctly refocuses on the compose box. Screen.Recording.2023-07-17.at.10.37.12.mov |
Hi @tjferriss, the issue is specifically on android chrome and android app, sorry for confusion, it is not on desktop app |
@tjferriss This is still easily reproducible on android chrome. Can you please open this? |
Thanks @allroundexperts. I've re-opened so we can investigate. |
Job added to Upwork: https://www.upwork.com/jobs/~010ac4c166c4d90b1e |
Current assignee @tjferriss is eligible for the External assigner, not assigning anyone new. |
Triggered auto assignment to Contributor-plus team member for initial proposal review - @aimane-chnaif ( |
Added external label |
Update ProposalMy first proposal was based on the assumption that the composer would appear after some animation when using an interaction manager, and this should be enough to resolve the issue. However, to make the solution even better, we need to check if the composer is already mounted or not. We can do this by introducing new life cycle notification functions for ReportActionComposeFocusManager to handle composer mount and unmount events. The proposed functions are as follows: function focusOnMount() {
if (isMounted) {
focus();
return;
}
shouldFocusOnNextComposerMount = true;
}
function composerDidMount() {
isMounted = true;
if (!shouldFocusOnNextComposerMount) {
return;
}
focus();
shouldFocusOnNextComposerMount = false;
}
function composerWillUnmount() {
isMounted = false;
shouldFocusOnNextComposerMount = false;
} and we will call them in ReportActionCompose like this: componentDidMount() {
...
ReportActionComposeFocusManager.composerDidMount();
}
componentWillUnmount() {
...
ReportActionComposeFocusManager.composerWillUnmount()
} With these changes, we can easily call ComposerActions.setShouldShowComposeInput(true);
ReportActionComposeFocusManager.focusOnMount(); What alternative solutions did you explore? (Optional)1- The life cycle tracking can be separated into a new file. Instead of using 2- Instead of making life cycle notifications, we can move the function focusOnMount() {
shouldFocusOnNextComposerMount = true;
}
function shouldFocusOnMount(){
return shouldFocusOnNextComposerMount
}
onComposerFocus(callback) {
...
shouldFocusOnNextComposerMount = false;
} and we will call it in ReportActionCompose like this: componentDidMount() {
...
if (shouldFocusONMount()) focus()
} |
@tjferriss, @aimane-chnaif Eep! 4 days overdue now. Issues have feelings too... |
@aimane-chnaif can you take a look at this latest proposal when you have the time? |
Waiting for proposals to be reviewed. |
@aimane-chnaif following once more, any update on this one? |
Sorry missed this. I think we're waiting proposal based on latest codebase |
📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸 |
@aimane-chnaif any update here? |
Same #22949 (comment) |
ProposalPlease re-state the problem that we are trying to solve in this issue.There's inconsistency between native and mWeb on refocus behavior after deleting message using edit mode. What is the root cause of that problem?App/src/pages/home/report/ReportActionCompose/ComposerWithSuggestions/ComposerWithSuggestions.js Lines 498 to 501 in ef3d8cd
Here, we check willBlurTextInputOnTapOutside whether to refocus or not.On web/mWeb, it's always true but on native it's always false What changes do you think we should make in order to solve the problem?
We should avoid this on mWeb as well for consistency. Also confirmed that the fix doesn't cause regression of #13443 which introduced |
Updated my proposal |
@bernhardoj I am confused about your problem statement This is current behavior:
|
This isn't working currently Screen.Recording.2023-11-30.at.14.39.14.mov |
Is that mobile web? I thought iOS? |
Oops wrong recording, Screen.Recording.2023-11-30.at.14.43.14.mov |
What you recorded is different from OP - delete popup Btw, I don't think it's expected behavior to refocus upon cancelling edit mode on mobile. Keyboard re-popup really annoys users including me. |
The step is different but it's just the same. Deleting/canceling/saving a draft will call
I feel the same, but the focus logic is widely used on other components too. App/src/pages/home/report/ReportActionCompose/ComposerWithSuggestions/ComposerWithSuggestions.js Lines 411 to 420 in ded13be
If we change it, then it affects every component that uses it. (oh wait, do you mean do nothing with the issue?) |
Refocus still happens after deleting message on android chrome. This is bug to fix I think as native, mSafari doesn't refocus at all. |
It didn't refocus on me. Screen.Recording.2023-11-30.at.17.00.14.movIf we look at the code, we want to refocus the composer when deleting a draft. App/src/pages/home/report/ReportActionItemMessageEdit.js Lines 285 to 297 in 2febad0
The refocus will only work for web platform (willBlurTextInputOnTapOutside). App/src/pages/home/report/ReportActionCompose/ComposerWithSuggestions/ComposerWithSuggestions.js Lines 411 to 420 in 2febad0
But it's currently not working on mWeb (at least on my end) Maybe we should clarify the expected behavior here. If not, then we can remove the logic from the edit composer. |
I tested on physical device. From your video, I found another bug of glitch which means try to refocus again and failed |
reproducible on android emulator as well android.emulator.mov |
Looks like on my end (emulator and real device), it refocuses back to the edit composer. Anyway, we need to decide first whether to refocus back to main composer or not. |
📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸 |
I don't think it's expected behavior to refocus upon cancelling edit mode on mobile. Keyboard re-popup really annoys users including me. 🎀 👀 🎀 to confirm with engineer |
Triggered auto assignment to @yuwenmemon, see https://stackoverflow.com/c/expensify/questions/7972 for more details. |
I tend to agree with @aimane-chnaif synopsis above. I think we can safely close this issue and revisit it down the road. |
If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!
Action Performed:
Expected Result:
App should refocus on compose box on deleting message using edit mode as it does on web chrome
Actual Result:
App does not refocus on compose box on deleting message using edit mode
Workaround:
Unknown
Platforms:
Which of our officially supported platforms is this issue occurring on?
Version Number: 1.3.41-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
Notes/Photos/Videos: Any additional supporting documentation
refocus.on.compose.box.on.delete.using.edit.mp4
Screen_Recording_20230715_143240_Chrome.mp4
Expensify/Expensify Issue URL:
Issue reported by: @dhanashree-sawant
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1689363436779099
View all open jobs on GitHub
Upwork Automation - Do Not Edit
The text was updated successfully, but these errors were encountered: