[HOLD for payment] [$2000] Wrong chat composer highlighted after clicking out of emoji modal

kavimuru commented 1 year ago

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:

Open app, access a report
Press the edit message icon
Press the emoji icon in the edit composer to open the emoji modal
Exit the emoji model by pressing outside

Expected Result:

The main composer is not highlighted, and the keyboard is focused on the editing composer

Actual Result:

The main composer is highlighted, and the keyboard is not focused on the editing composer

Workaround:

unknown

Platforms:

Which of our officially supported platforms is this issue occurring on?

[x] iOS / Safari

Version Number: 1.2.65-0 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:

https://user-images.githubusercontent.com/43996225/216879422-75923acb-656c-4b27-8a56-d15e7ec68a2c.mp4

https://user-images.githubusercontent.com/43996225/216879478-34c57868-40a7-4ec3-be7e-88380da3f4b8.MP4

Expensify/Expensify Issue URL: Issue reported by: @Tienifr

Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1675481157349539

View all open jobs on GitHub

Upwork Automation - Do Not Edit

Upwork Job URL: https://www.upwork.com/jobs/~01aa19f44ae1aa2e68
Upwork Job ID: 1623817661403475968
Last Price Increase: 2023-02-16

melvin-bot[bot] commented 1 year ago

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

MelvinBot commented 1 year ago

@davidcardoza Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

davidcardoza commented 1 year ago

I was able to reproduce the issue. Going to bring this out to a contributor.

MelvinBot commented 1 year ago

Triggered auto assignment to @NikkiWines (Engineering), see https://stackoverflow.com/c/expensify/questions/4319 for more details.

MelvinBot commented 1 year ago

Job added to Upwork: https://www.upwork.com/jobs/~01aa19f44ae1aa2e68

MelvinBot commented 1 year ago

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

MelvinBot commented 1 year ago

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

MelvinBot commented 1 year ago

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

cristipaval commented 1 year ago

Why did Melvin assign both me and @NikkiWines ? 👀

allroundexperts commented 1 year ago

Proposal

Please re-state the problem that we are trying to solve in this issue.

When the emoji selector popup is opened from the edit message composer and then closed, the main chat composer gets the focus instead of the edit message composer.

What is the root cause of that problem?

The main composer has autoFocus property partially set to true. That is, whenever the main composer mounts, it gets focused due to the presence of this property.

On smaller screens, when the second composer (one which comes up when you edit the message) gets the focus, we're un-mounting the main composer. When the secondary composer looses focus (by a click outside), we then mount the main composer again. Since the main composer has auto-focus property setup, the main composer gets focused. This can be seen in the ReportActionItemMessageEdit file. The function used to show or hide the main composer is toggleReportActionComposeView.

When the emoji popup gets closed by clicking outside the secondary composer, it gets blur for a split second. At the same instance, the emoji popup also calls onModalHide since it has been closed. The two events happen almost at the same time. The first event (secondary composer blur) causes the primary composer to show up and be focused. The second event (onModalHide) causes the secondary composer to focus. The first event takes more time to complete (since it mounts a whole component) and thus, causes the main composer to be in a focused state.

What changes do you think we should make in order to solve the problem?

We can do the following:

We can delay the callback executed in the onModalHide function in the emoji picker popup. This will make sure that the secondary composer focus happens AFTER the main composer has finished its focus. For an example, we can use setTimeout function for this.

Alternate Solutions

We could probably remove this functionality of hiding the main composer when the secondary composer gets focus on small screens altogether. It's more of a design / UX decision though.
We can also remove autofocus property of the main composer. This will cause the onModalHide event to always execute which is what we want in this case.

tienifr commented 1 year ago

Proposal

Please re-state the problem that we are trying to solve in this issue.

After closing the emoji picker when editing message, the wrong composer appeared and was focused instead of the edit composer

What is the root cause of that problem?

After the emoji picker is closed, the focus was returned to the body element, at the same time we try to execute focus on the edit composer. This causes a race condition, when the event focus was returned to the body element happens after the edit composer is focused, it causes the edit composer to immediately be blurred and triggered the main composer to appear and gain focus.

What changes do you think we should make in order to solve the problem?

In src/components/EmojiPicker/EmojiPicker.js, we need to add a new function (only for web) for onModalHide that wraps the function passed in to the EmojiPicker. This new function will create a one-time listener for the focusin event of the body element in the DOM and only run the original onModalHide after the body is already focused, thus preventing the race condition.

Here's the pseudo code of the wrapped function:

wrappedOnModalHide(onModalHide) {
        return () => {
            if (document) {
                document.addEventListener('focusin', (event) => {
                    if (event.target.localName === 'body') {
                        onModalHide();
                    }

                }, { once: true });
            }
        }
    }

We can optionally consider putting that same logic down to the Modal component instead if we want this to apply to all modals.

What alternative solutions did you explore? (Optional)

We can use timeout but generally if there's a working solution that doesn't use timeout, we should prioritize that solution.

Result

Working well after the fix:

https://user-images.githubusercontent.com/113963320/218247085-e8e424a5-d1d0-48b5-9573-b35caf53901c.mov

s77rt commented 1 year ago

Proposal

Please re-state the problem that we are trying to solve in this issue.

Closing the emoji modal does not focus the corresponding composer.

What is the root cause of that problem?

The onModalHide callback is being called too early (i.e. before actually hiding the modal). This is a bug in react-native-modal. Focusing the composer before hiding the modal is illegal where focus trap kicks in and shifts the focus back to the modal.

What changes do you think we should make in order to solve the problem?

Fix the bug upstream: https://github.com/react-native-modal/react-native-modal/pull/733

What alternative solutions did you explore? (Optional)

Reach out to RN to implement a onHide prop and wait for RNW to adopt it, then use that new prop on react-native-modal to determine when to call the onModalHide callback.

allroundexperts commented 1 year ago

@s77rt Is this really a bug upstream? Aren't you just delaying the callback as well but in a different way? (Rendering twice vs using setTimeout)? Also, aren't we not allowed to create a PR without being assigned the issue?

s77rt commented 1 year ago

@allroundexperts Thanks for taking the time to consider my proposal. I believe we can handle such discussion on slack. I have created this thread. Feel free to post your concerns there.

ntdiary commented 1 year ago

Proposal

Please re-state the problem that we are trying to solve in this issue.

Close the emoji modal opened via the edit composer, the main composer is highlighted, get focus and doesn't pull up the keyboard (only IOS platform).

Note: need to confirm, do we really want to pull up the keyboard in IOS ? (maybe we need to make some trade-offs.)

What is the root cause of that problem?

https://github.com/Expensify/App/blob/39d9b660e387b28688a221b976f0fbf52873cc67/src/pages/home/report/ReportActionItemMessageEdit.js#L260 This line will call requestIdleCallback

the main composer is shown and highlighted On IOS web platform, requestIdleCallback is still an experimental feature and is disabled by default. react-native-web replaces it with setTimeout(() => {}, 1). In addition the rendering of modal children is controlled by isRendering and visible, so there are multiple macro tasks here. The following image is the execution timeline record, composerInput.focus(Timer 334) is fired before the ModalFocusTrap is unmounted. and then refocusing from ModalFocusTrap will trigger composerInput.onBlur event. Finally toggleReportActionComposeView is executed. (Click on the image for a larger view):

On Android chrome, requestIdleCallback will defer composerInput.focus until the react macro tasks are completed. So it can work well.

focus doesn't pull up the keyboard On IOS web platform, element.focus will only pull up the keyboard if there is user interaction within the same event loop. No such restriction on Android chrome.

What changes do you think we should make in order to solve the problem?

Anyway, if we sure we want to pull up the keyboard, disabling focus trap should be required. (What we have been trying to solve is to fix the highlight part, so ignore keyboard part here).

replace InteractionManager.runAfterInteractions with setImmediate. https://github.com/Expensify/App/blob/d196a1217754ffab2e10ba28ba3c88f0b6391bf5/src/pages/home/report/ReportActionItemMessageEdit.js#L302.

It can reliably defer the textInput.focus() until after the inactivation of the focus trap, because it also uses MessageChannel on the web platform, like react's internal code. And InteractionManager.runAfterInteractions is also calling setImmediate in the native app because there is no interaction when the onModalHide is called. And This also works in react 18 concurrent mode.

What alternative solutions did you explore? (Optional)

Add a prop to Modal to allow us to disable trap. (This is an initial idea, we are not sure that the maintainers would to adopt it)
Create a new manager file, migrate some functions from the Modal/index.js into it, and export this file. With these functions (such as removeActiveModal/addActiveModal...), we can not only disable the trap manually, but also integrate it with other third party modals more easily.
Add a prop to Modal (i.e. focusAfterClosed), which would allow us to be able to specify which element gets focus when the modal is closed. I think its current implementation is a little weird: when the modal is unmounted, it forces a refocus on the element that triggered the modal. IMO, it is more appropriate as a default behavior rather than a mandatory one. And to comply with the w3c APG, we also need this parameter.
Just wait to upgrade to react-native-web 0.19.x and enable concurrent mode, (this will cause regression, I think it can be fixed in a separate issue).

mollfpr commented 1 year ago

Thanks guys! I’ll start reviewing the proposal.

mollfpr commented 1 year ago

@allroundexperts We should avoid using the setTimeout for a solution. Also, we already delay the input focus with InteractionManager.runAfterInteractions, which already works fine for other platforms.

@s77rt The upstream already has the issue reported, but it's closed, and in the other issue, the maintainer recommends to use InteractionManager.runAfterInteractions. Before opening a PR in E/App or the upstream library, we should discuss the solution first. Although your PR is solving the issue logically, we still need to determine if the maintainer will agree with the solution when it looks like a hack.

@tienifr Your result is promising, and the solution seems straightforward, but I still need to figure out if this is the best we can do.

@ntdiary Surprisingly, onDismiss works! Is there any explanation for why it works?

allroundexperts commented 1 year ago

@mollfpr We're making use of this at several places to solve this same problem. See here: https://github.dev/Expensify/App/blob/f40f9c9d81547292a53da4e48c23c2151a9d7fb7/src/pages/home/report/ReportActionCompose.js#L353

Also, runAfterInteractions is not a delay to be precise. The above mentioned function uses this inside of the runAfterInteractions as well.

mollfpr commented 1 year ago

@allroundexperts Yes, that will be the last option to consider. For now, it's too early to decide using setTimeout.

s77rt commented 1 year ago

@mollfpr Thanks for the review! I believe that using runAfterInteractions is just a hack and the input should be focusable even without it. @davidcardoza @cristipaval Can you help us get a response from the maintainers for this PR.

ntdiary commented 1 year ago

https://github.com/necolas/react-native-web/blob/e8098fd029102d7801c32c1ede792bce01808c00/packages/react-native-web/src/exports/Modal/ModalAnimation.js#L43-L77

@mollfpr, The children of ModalAnimation are removed immediately after onDismiss is executed (same loop). So we can safely use runAfterInteractions to delay the input.focus() until the next loop, or use queueMicrotask/Promise to delay the input.focus() until the children are removed, thus avoiding the refocusing behavior of ModalFocusTrap.

tienifr commented 1 year ago

@ntdiary just curious how you were able to make it work consistently with onDismiss? 😁

I tried (by changing onModalHide to onDismiss in src/components/Modal/BaseModal.js) but it seems to work some of the times and not work in some others.

tienifr commented 1 year ago

@s77rt I think if anything needs to be fixed upstream the fix should be on react-native-web rather than react-native-modal.

react-native-modal doesn't know anything about ModalFocusTrap for example, so I'm not sure if react-native-modal should be updated due to an implementation detail of another library (react-native-web). I think react-native-modal works well on native Android, iOS where it's originally intended.

s77rt commented 1 year ago

@tienifr Thanks for your input. Would you please take that to Slack. Let's keep GH for proposals only and Slack for the discussion.

ntdiary commented 1 year ago

I tried (by changing onModalHide to onDismiss in src/components/Modal/BaseModal.js)

@tienifr hi, I just tried this approach, no problems found either. If you can still reproduce it, could you please share your timeline json file? I'm interested in it too. 😂

https://user-images.githubusercontent.com/8579651/219347489-c6ede632-e156-4eb6-b1da-43e45121f5f4.mov

We need to make sure these records of attemptFocus will disappear from the timeline. (We can also analyze some other relevant records).

And we can also discuss these on slack if needed. : )

mollfpr commented 1 year ago

@ntdiary just curious how you were able to make it work consistently with onDismiss? 😁

I tried (by changing onModalHide to onDismiss in src/components/Modal/BaseModal.js) but it seems to work some of the times and not work in some others.

@tienifr This is interesting. Could you share the step you use? Is it happening on an iOS safari simulator or a physical device?

@mollfpr, The children of ModalAnimation are removed immediately after onDismiss is executed (same loop).

@ntdiary I'm unsure what ModalAnimation need to do to make onDismiss work well than onModalHide. Could you elaborate?

@ntdiary I see you have a step on the focus trap issue in RNW https://github.com/necolas/react-native-web/issues/2483. Let's say that they agree to have a prop to control the focused element after the modal it's closed; I assume that the update we need to do is just add the element we want to focus on. Is it right?

ntdiary commented 1 year ago

I'm unsure what ModalAnimation need to do to make onDismiss work well than onModalHide. Could you elaborate?

@mollfpr , use the react-native-web-example for a demonstration, and add some breakpoints and logs:

https://user-images.githubusercontent.com/8579651/219674687-5504317e-a45b-4030-b3da-bf76a324f0f4.mp4

In ModalAnimation, the visible update and the isRendering update are scheduled as two tasks. And looking at the logs, we can see that the onDismiss execution and the ref update are in the same task stack, so micro-task can also help us delay the job until the trapElementRef.current is null.

        if (
          trapElementRef.current == null ||
          focusRef.current.trapFocusInProgress ||
          !active
        ) {
          return;

I see you have a step on the focus trap issue in RNW https://github.com/necolas/react-native-web/issues/2483. Let's say that they agree to have a prop to control the focused element after the modal it's closed; I assume that the update we need to do is just add the element we want to focus on. Is it right?

yeh. One more thing, if we want to pull up the keyboard on the IOS safari, we also need to disable the focus trap when closing the modal. Although we can pull up the keyboard with focus() on other platforms, but there are limitations on IOS safari.

tienifr commented 1 year ago

I tried (by changing onModalHide to onDismiss in src/components/Modal/BaseModal.js) but it seems to work some of the times and not work in some others.

@mollfpr @ntdiary I'm no longer able to reproduce this issue, perhaps I missed out something, pls ignore it 🙏

mollfpr commented 1 year ago

@ntdiary Thanks for the clear explanation!

@cristipaval We can go with @ntdiary proposal, but only replacing onModalHide with onDismiss for the web.

🎀 👀 🎀 C+ reviewed!

cristipaval commented 1 year ago

Thank you @mollfpr for triaging the proposals! I am happy with your choice, @ntdiary please go ahead with the fix 🚀

s77rt commented 1 year ago

@cristipaval Can you assign @ntdiary to this issue? (to remove the "Help Wanted" label)

MelvinBot commented 1 year ago

📣 @ntdiary You have been assigned to this job by @davidcardoza! Please apply to this job in Upwork and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻 Keep in mind: Code of Conduct | Contributing 📖

davidcardoza commented 1 year ago

Done.

situchan commented 1 year ago

PR which fixes this issue caused regression - https://github.com/Expensify/App/issues/15559

cristipaval commented 1 year ago

PR which fixes this issue caused regression - #15559

Thanks @situchan !

We discussed a bit here on Slack. The workaround that I was thinking about is to set shouldUseOnDismiss by default as it is set in the @ntdiary's PR, but also make it possible to set another value for it in specific situations where we see at testing that it doesn't work with the default value. In our regression, if we override the default value and set shouldUseOnDismiss as false, it works as expected. cc @s77rt

As mentioned on Slack, we prefer to avoid workarounds.

MelvinBot commented 1 year ago

⚠️ Looks like this issue was linked to a Deploy Blocker here

If you are the assigned CME please investigate whether the linked PR caused a regression and leave a comment with the results.

If a regression has occurred and you are the assigned CM follow the instructions here.

If this regression could have been avoided please consider also proposing a recommendation to the PR checklist so that we can avoid it in the future.

ntdiary commented 1 year ago

I think we can just use shouldUseOnDismiss for EmojiPicker.js on the web platform if needed, and keep it false in all other cases, like this:

how the regression happened: The browser will check whether there is user interaction before showing the file picker.

void FileInputType::handleDOMActivateEvent(Event& event)
{
    ...
    if (!UserGestureIndicator::processingUserGesture())
        return;
    showPicker();

In this case, it is triggered by fileInput.click() in the AttachmentPicker/index.js.

With the previous PR(#15298), after we click on Add attachment, the call to fileInput.click() was deferred to the second react task (react scheduler uses MessageChannel to perform its work). Whereas safari will only hold the user interaction state for requestAnimationFrame/setTimeout(also expires after 1 second), not for MessageChannel.^1

So after the change, although fileInput.click is still executed, the file picker won't be displayed.

Chrome does this better than safari, and this is a demo: https://codesandbox.io/s/file-picker-truo1r

https://user-images.githubusercontent.com/8579651/222097229-3afa3b76-15d6-485a-a988-f6a03b379420.mp4

s77rt commented 1 year ago

@ntdiary Sorry to see things didn't go as expected. If I may I'd like to re-propose my solution. @mollfpr @cristipaval Given that the chosen solution is no longer generic, I'd appreciate a second look on this one.

mollfpr commented 1 year ago

@s77rt Thank you!

I tested your PR for RNM, there's a significant delay before the input focus compared to the @ntdiary solution.

@ntdiary	@s77rt

s77rt commented 1 year ago

Thanks for testing @mollfpr I'm not really sure why you are experiencing such delay, maybe it's unrelated to the change, can you test more?

The upstream bug is that the callback is being called too early, so it's natural that the fix will introduce some delay, but in your video the delay looks to be too much. If it may help try remove the runAfterInteractions call here and focus directly. runAfterInteractions is not required here especially since it's a hack, but maybe we should keep it for native only.

tienifr commented 1 year ago

@mollfpr if we're considering an alternate solution, maybe we can reconsider my earlier proposal as well. It's quite straight forward and clean. cc @cristipaval

mollfpr commented 1 year ago

@s77rt Yeah, I just removed runAfterInteractions, and there's no delay anymore.

Your PR on RNM has been running for about 2 weeks, and has yet to receive a comment from the maintainer, and we are still determining if they will approve your solution. So it will take longer to solve this issue if we go with your solution.

There's also a chance that we will re-evaluate the solution if they do not approve your solution.

@tienifr We have yet to decide, but if we want to re-evaluate the proposal. I will review all the proposals again.

mollfpr commented 1 year ago

@cristipaval I'm still thinking the onDismiss is still a valid solution and @ntdiary explain have a solid point for what we have missing.

MelvinBot commented 1 year ago

Reviewing label has been removed, please complete the "BugZero Checklist".

MelvinBot commented 1 year ago

The solution for this issue has been :rocket: deployed to production :rocket: in version 1.2.77-4 and is now subject to a 7-day regression period :calendar:. Here is the list of pull requests that resolve this issue:

If no regressions arise, payment will be issued on 2023-03-09. :confetti_ball:

After the hold period is over and BZ checklist items are completed, please complete any of the applicable payments for this issue, and check them off once done.

[ ] External issue reporter
[ ] Contributor that fixed the issue
[ ] Contributor+ that helped on the issue and/or PR

As a reminder, here are the bonuses/penalties that should be applied for any External issue:

Merged PR within 3 business days of assignment - 50% bonus
Merged PR more than 9 business days after assignment - 50% penalty

MelvinBot commented 1 year ago

BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:

[ ] [@mollfpr / @cristipaval] The PR that introduced the bug has been identified. Link to the PR:
[ ] [@mollfpr / @cristipaval] The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment:
[ ] [@mollfpr / @cristipaval] A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion:
[ ] [@ntdiary] Propose regression test steps to ensure the same bug will not reach production again.
[ ] [@davidcardoza] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

ntdiary commented 1 year ago

Hi, @mollfpr, I think we're still waiting for @cristipaval 's inputs if he has the bandwidth, right? 🙂

mollfpr commented 1 year ago

@ntdiary Yup, we will get the answer shortly!

cristipaval commented 1 year ago

It sounds like @ntdiary 's solution is more like a workaround instead of a fix. If I'm getting it right, we add something which doesn't always work and we enable it only when it proves to work in our testing during development. Let's suppose we develop a new feature and we add a new modal. How do I know if I have to set shouldUseOnDismiss to true or false, other than testing and then praying it will also work in production as I expect?

On the other hand, I do understand that the upstream fix proposed by @s77rt takes a while to get merged and it isn't sure it will get merged.

Hmm.. hard decision.. I'll open an internal discussion with the team later today.

tienifr commented 1 year ago

@cristipaval what do you think about my earlier proposal here? It's a straight forward fix and doesn't require any upstream changes.

Expensify / App