Skip to content
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

Closed
2 of 6 tasks
kbecciv opened this issue Jul 15, 2023 · 72 comments
Assignees
Labels
Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 Engineering External Added to denote the issue can be worked on by a contributor Help Wanted Apply this label when an issue is open to proposals by contributors

Comments

@kbecciv
Copy link

kbecciv commented Jul 15, 2023

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:

  1. Open the app
  2. Open any report
  3. Send any message
  4. Edit the message and remove all text
  5. Remove the text and click on green tick
  6. In delete popup, click on delete and observe that app does not refocus on compose box (works fine on web chrome)

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?

  • Android / native
  • Android / Chrome
  • iOS / native
  • iOS / Safari
  • MacOS / Chrome / Safari
  • MacOS / Desktop

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
  • Upwork Job URL: https://www.upwork.com/jobs/~010ac4c166c4d90b1e
  • Upwork Job ID: 1681531735404843008
  • Last Price Increase: 2023-12-02
@kbecciv kbecciv added Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Jul 15, 2023
@melvin-bot
Copy link

melvin-bot bot commented Jul 15, 2023

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

@melvin-bot
Copy link

melvin-bot bot commented Jul 15, 2023

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

@ahmdshrif
Copy link
Contributor

ahmdshrif commented Jul 15, 2023

Proposal

Please restate the problem that we are trying to solve in this issue.

when deleting the edit message main composer does not focus again.
also when canceling the edit 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.

ComposerActions.setShouldShowComposeInput(true);
ReportActionComposeFocusManager.focus();

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

@bernhardoj
Copy link
Contributor

bernhardoj commented Jul 16, 2023

Proposal

Please 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.

setShouldShowComposeInputKeyboardAware(false);

Either saving or closing the edit composer, it will call deleteDraft which will try to focus back to the main composer (ReportActionComposeFocusManager.focus()).

const deleteDraft = useCallback(() => {
InputFocus.callback(() => setIsFocused(false));
InputFocus.inputFocusChange(false);
debouncedSaveDraft.cancel();
Report.saveReportActionDraft(props.reportID, props.action, '');
if (isActive()) {
ReportActionComposeFocusManager.clear();
ReportActionComposeFocusManager.focus();
}

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).

return () => {
// Skip if the current report action is not active
if (!isActive()) {
return;
}
if (EmojiPickerAction.isActive(props.action.reportActionID)) {
EmojiPickerAction.clearActive();
}
if (ReportActionContextMenu.isActiveReportAction(props.action.reportActionID)) {
ReportActionContextMenu.clearActiveReportAction();
}
// Show the main composer when the focused message is deleted from another client
// to prevent the main composer stays hidden until we swtich to another chat.
setShouldShowComposeInputKeyboardAware(true);
};

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 setShouldShowComposeInputKeyboardAware as a Promise.

setShouldShowComposeInputKeyboardAware(true).then(() => {
    ReportActionComposeFocusManager.clear();
    ReportActionComposeFocusManager.focus();
});

To make setShouldShowComposeInputKeyboardAware as a Promise,

  1. Return the Onyx set in here
    function setShouldShowComposeInput(shouldShowComposeInput: boolean) {
    Onyx.set(ONYXKEYS.SHOULD_SHOW_COMPOSE_INPUT, shouldShowComposeInput);
    }
  2. In setShouldShowComposeInputKeyboardAware/index, return the promise from Composer.setShouldShowComposeInput
    const setShouldShowComposeInputKeyboardAware: SetShouldShowComposeInputKeyboardAware = (shouldShow) => {
    Composer.setShouldShowComposeInput(shouldShow);
    };
  3. In setShouldShowComposeInputKeyboardAwareBuilder, return a new Promise object which will be resolved by Composer.setShouldShowComposeInput promise.
    const setShouldShowComposeInputKeyboardAwareBuilder: (keyboardEvent: KeyboardEventName) => SetShouldShowComposeInputKeyboardAware =
    (keyboardEvent: KeyboardEventName) => (shouldShow: boolean) => {
    if (keyboardEventListener) {
    keyboardEventListener.remove();
    keyboardEventListener = null;
    }
    if (!shouldShow) {
    Composer.setShouldShowComposeInput(false);
    return;
    }
    // If keyboard is already hidden, we should show composer immediately because keyboardDidHide event won't be called
    if (!Keyboard.isVisible()) {
    Composer.setShouldShowComposeInput(true);
    return;
    }
    keyboardEventListener = Keyboard.addListener(keyboardEvent, () => {
    Composer.setShouldShowComposeInput(true);
    keyboardEventListener?.remove();
    });
    };
=> new Promise(resolve => {
    ...

    if (!shouldShow) {
        Composer.setShouldShowComposeInput(false).then(resolve);
    ...

    if (!Keyboard.isVisible()) {
        Composer.setShouldShowComposeInput(true).then(resolve);
    ...

    keyboardEventListener = Keyboard.addListener(keyboardEvent, () => {
        Composer.setShouldShowComposeInput(true).then(resolve);
})

@allroundexperts
Copy link
Contributor

Proposal

Please 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 focus requests to the main composer. We already have a ReportActionComposeFocusManager and we can use it to queue the focus action. In order to do this, we can create a new variable called pendingFocus and set its value to false initially. Then we can change our current focus function inside ReportActionComposeFocusManager to be:

function focus() {
    if (!_.isFunction(focusCallback)) {
        pendingFocus = true;
        return;
    }

    focusCallback();
    pendingFocus = false;
}

Similarly, the clear function would be changed to:

function clear() {
    focusCallback = null;
    pendingFocus = false;
}

Lastly, the onComposerFocus function would be changed to:

function onComposerFocus(callback) {
    focusCallback = callback;
    if (pendingFocus) {
        callback();
        pendingFocus = false;
    }
}

Result

Screen.Recording.2023-07-16.at.7.17.05.PM.mov

What alternative solutions did you explore? (Optional)

None

@tjferriss
Copy link
Contributor

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

@tjferriss tjferriss added the Needs Reproduction Reproducible steps needed label Jul 17, 2023
@dhanashree-sawant
Copy link

Hi @tjferriss, the issue is specifically on android chrome and android app, sorry for confusion, it is not on desktop app

@allroundexperts
Copy link
Contributor

@tjferriss This is still easily reproducible on android chrome. Can you please open this?

@tjferriss tjferriss reopened this Jul 18, 2023
@tjferriss
Copy link
Contributor

Thanks @allroundexperts. I've re-opened so we can investigate.

@tjferriss tjferriss added the External Added to denote the issue can be worked on by a contributor label Jul 19, 2023
@melvin-bot melvin-bot bot changed the title mweb - App does not refocus on compose box on message delete using edit mode on android chrome and app [$1000] mweb - App does not refocus on compose box on message delete using edit mode on android chrome and app Jul 19, 2023
@tjferriss tjferriss removed the Needs Reproduction Reproducible steps needed label Jul 19, 2023
@melvin-bot
Copy link

melvin-bot bot commented Jul 19, 2023

Job added to Upwork: https://www.upwork.com/jobs/~010ac4c166c4d90b1e

@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Jul 19, 2023
@melvin-bot
Copy link

melvin-bot bot commented Jul 19, 2023

Current assignee @tjferriss is eligible for the External assigner, not assigning anyone new.

@melvin-bot
Copy link

melvin-bot bot commented Jul 19, 2023

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

@tjferriss
Copy link
Contributor

Added external label

@ahmdshrif
Copy link
Contributor

Update Proposal

My 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 focusOnMount in [ReportActionItemMessageEdit.js] like this:

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 focusOnMount, we can use onComposerMount, and any component can set its callback for focus or other actions, but I prefer to keep it simple for now.

2- Instead of making life cycle notifications, we can move the composerWillUnmount and composerDidMount functionality inside the ReportActionItemMessageEdit and just add a function to return if we should focus on mount or not. But I think separating the code in focus manager is better and can track the life cycle better.

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()
}

@melvin-bot melvin-bot bot added the Overdue label Jul 21, 2023
@melvin-bot
Copy link

melvin-bot bot commented Jul 24, 2023

@tjferriss, @aimane-chnaif Eep! 4 days overdue now. Issues have feelings too...

@tjferriss
Copy link
Contributor

@aimane-chnaif can you take a look at this latest proposal when you have the time?

@melvin-bot melvin-bot bot removed the Overdue label Jul 24, 2023
@tjferriss
Copy link
Contributor

Waiting for proposals to be reviewed.

@tjferriss
Copy link
Contributor

@aimane-chnaif following once more, any update on this one?

@aimane-chnaif
Copy link
Contributor

Sorry missed this. I think we're waiting proposal based on latest codebase

Copy link

melvin-bot bot commented Nov 25, 2023

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

@tjferriss
Copy link
Contributor

@aimane-chnaif any update here?

@aimane-chnaif
Copy link
Contributor

Same #22949 (comment)

@mkhutornyi
Copy link
Contributor

mkhutornyi commented Nov 30, 2023

Proposal

Please 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.
So always refocus on mWeb while not on native

What is the root cause of that problem?

// We want to focus or refocus the input when a modal has been closed or the underlying screen is refocused.
// We avoid doing this on native platforms since the software keyboard popping
// open creates a jarring and broken UX.
if (!(willBlurTextInputOnTapOutside && !isNextModalWillOpenRef.current && !modal.isVisible && isFocused && (prevIsModalVisible || !prevIsFocused))) {

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 avoid doing this on native platforms since the software keyboard popping 
 // open creates a jarring and broken UX. 

We should avoid this on mWeb as well for consistency.
So replace willBlurTextInputOnTapOutside with canFocusInputOnScreenFocus.
After this fix, the behavior is: always refocus on web/desktop, don't refocus on native/mWeb.

Also confirmed that the fix doesn't cause regression of #13443 which introduced willBlurTextInputOnTapOutside logic

@melvin-bot melvin-bot bot added Daily KSv2 and removed Weekly KSv2 labels Nov 30, 2023
@bernhardoj
Copy link
Contributor

Updated my proposal

@aimane-chnaif
Copy link
Contributor

On mobile web, closing the edit composer does not refocus back the main composer.

@bernhardoj I am confused about your problem statement

This is current behavior:

On mobile web, closing the edit composer refocus back the main composer.
On native, closing the edit composer does not refocus back the main composer.

@bernhardoj
Copy link
Contributor

On mobile web, closing the edit composer refocus back the main composer.

This isn't working currently

Screen.Recording.2023-11-30.at.14.39.14.mov

@aimane-chnaif
Copy link
Contributor

On mobile web, closing the edit composer refocus back the main composer.

This isn't working currently

Screen.Recording.2023-11-30.at.14.39.14.mov

Is that mobile web? I thought iOS?

@bernhardoj
Copy link
Contributor

Oops wrong recording,

Screen.Recording.2023-11-30.at.14.43.14.mov

@aimane-chnaif
Copy link
Contributor

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.
@tjferriss what do you think?

@bernhardoj
Copy link
Contributor

bernhardoj commented Nov 30, 2023

The step is different but it's just the same. Deleting/canceling/saving a draft will call deleteDraft.

Keyboard re-popup really annoys users including me.

I feel the same, but the focus logic is widely used on other components too.

const setUpComposeFocusManager = useCallback(() => {
// This callback is used in the contextMenuActions to manage giving focus back to the compose input.
ReportActionComposeFocusManager.onComposerFocus(() => {
if (!willBlurTextInputOnTapOutside || !isFocused) {
return;
}
focus(false);
}, true);
}, [focus, isFocused]);

If we change it, then it affects every component that uses it.

(oh wait, do you mean do nothing with the issue?)

@aimane-chnaif
Copy link
Contributor

Refocus still happens after deleting message on android chrome. This is bug to fix I think as native, mSafari doesn't refocus at all.

@bernhardoj
Copy link
Contributor

It didn't refocus on me.

Screen.Recording.2023-11-30.at.17.00.14.mov

If we look at the code, we want to refocus the composer when deleting a draft.

/**
* Delete the draft of the comment being edited. This will take the comment out of "edit mode" with the old content.
*/
const deleteDraft = useCallback(() => {
InputFocus.callback(() => setIsFocused(false));
InputFocus.inputFocusChange(false);
debouncedSaveDraft.cancel();
Report.saveReportActionDraft(props.reportID, props.action, '');
if (isActive()) {
ReportActionComposeFocusManager.clear();
ReportActionComposeFocusManager.focus();
}

The refocus will only work for web platform (willBlurTextInputOnTapOutside).

const setUpComposeFocusManager = useCallback(() => {
// This callback is used in the contextMenuActions to manage giving focus back to the compose input.
ReportActionComposeFocusManager.onComposerFocus(() => {
if (!willBlurTextInputOnTapOutside || !isFocused) {
return;
}
focus(false);
}, true);
}, [focus, isFocused]);

But it's currently not working on mWeb (at least on my end)

Maybe we should clarify the expected behavior here.
Do we want to refocus back to the main composer after closing the edit composer (web only)?

If not, then we can remove the logic from the edit composer.

@aimane-chnaif
Copy link
Contributor

It didn't refocus on me.

I tested on physical device. From your video, I found another bug of glitch which means try to refocus again and failed

@aimane-chnaif
Copy link
Contributor

reproducible on android emulator as well

android.emulator.mov

@bernhardoj
Copy link
Contributor

From your video, I found another bug of glitch which means try to refocus again and failed

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.

Copy link

melvin-bot bot commented Dec 2, 2023

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

@melvin-bot melvin-bot bot added the Overdue label Dec 2, 2023
@aimane-chnaif
Copy link
Contributor

I don't think it's expected behavior to refocus upon cancelling edit mode on mobile. Keyboard re-popup really annoys users including me.
Refocus happens after deleting message on edit mode on android chrome. This is inconsistency bug to fix I think, as native and mSafari don't refocus at all.

🎀 👀 🎀 to confirm with engineer

Copy link

melvin-bot bot commented Dec 2, 2023

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

@melvin-bot melvin-bot bot added the Overdue label Dec 5, 2023
@tjferriss
Copy link
Contributor

I tend to agree with @aimane-chnaif synopsis above. I think we can safely close this issue and revisit it down the road.

@melvin-bot melvin-bot bot removed the Overdue label Dec 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 Engineering External Added to denote the issue can be worked on by a contributor Help Wanted Apply this label when an issue is open to proposals by contributors
Projects
None yet
Development

No branches or pull requests

9 participants