Repository navigation
Right Click Popover -> Edit Comment does not focus on the Edit Comment Textbox #3017
Description
Coming from https://github.com/Expensify/Expensify/issues/163577
Problem
If you right-click in the chat app to edit a comment, the edit comment textbox does not automatically get focused:
However this does not happen if you use the hover menu:
Reproducible Steps
• Sign in to the e.chat staging web app
• Right-click text and choose Edit Comment
• Textbox does not automatically get focused
Why is this important?
The two flows should be consistent.
Solution
TBD
Upwork post - https://www.upwork.com/jobs/~015ee115217a359f1b
Metadata
Metadata
Assignees
Labels
No labels
Activity
MelvinBot commentedon May 19, 2021
Triggered auto assignment to @Julesssss (
Exported), see https://stackoverflow.com/c/expensify/questions/7972 for more details.parasharrajat commentedon May 19, 2021
There are multiple factors stealing the focus away from the input box.
Context Menu itself
I didn't find any strong reason why it is stealing the focus but It is. If I remove all the manual focus logic from the code still focus is set back to the page
bodytag. A factor could be that context menu's hiding action is async.https://github.com/Expensify/Expensify.cash/blob/85b084c0d2c79c08d6e6fad7eea08e00ca0c8a85/src/pages/home/report/ReportActionContextMenu.js#L119
To fix this so far, I think the best would be to just use a
setTimeoutand it does not change UX at all. Just wrap this call in asetTimeouthttps://github.com/Expensify/Expensify.cash/blob/85b084c0d2c79c08d6e6fad7eea08e00ca0c8a85/src/pages/home/report/ReportActionContextMenu.js#L120
Composer
Composer is meant to take focus when any modal is closed as per, what could I say more I wrote this logic earlier.
https://github.com/Expensify/Expensify.cash/blob/85b084c0d2c79c08d6e6fad7eea08e00ca0c8a85/src/pages/home/report/ReportActionCompose.js#L141-L144
Now to fix this, we have many ways and we have to find the best.
We could just add one more condition check here if there is any draft message action just don't set the focus, but it's not that simple. We store the draft Messages in a common collection so we will have to get all drafts from all reports and then filter those to the current report and then Check if we just trigger the draft action.
We somehow let the app announce that the user has triggered an edit message and thus, we skip the focus in Composer. I am not sure that do we have something like this and how can this be achieved.
Easiest of all and I think does not have any side-effect. As we set the
modalVisibleto false when the context menu is closed and further set the focus on Composer. We just don't take Context Menu as Modal. In other words, we don't triggermodalVisibleOnyx changes on Context Menu.Julesssss commentedon May 20, 2021
Sorry @kevinksullivan, but I need to reassign
Exportedtoday, I'm completely overrun with tasks and will not be able to make any progress on the multiple N5 blocking IOU issues if I don't reassign some of them 😕MelvinBot commentedon May 20, 2021
Triggered auto assignment to @deetergp (
Exported), see https://stackoverflow.com/c/expensify/questions/7972 for more details.parasharrajat commentedon May 21, 2021
Proposal
Please refer to this comment #3017 (comment).
deetergp commentedon Jun 1, 2021
@parasharrajat Apologies for not seeing this sooner. Your proposals for the Context Menu sound good, and as far as the options for Composer, if the "simple with no side effects" option works, then that is always the option I will choose!
parasharrajat commentedon Jun 4, 2021
@kevinksullivan Am I good to start the PR?
Julesssss commentedon Jun 7, 2021
@kevinksullivan FYI an emoji reaction won't trigger a notification, so it's better to confirm as a comment
parasharrajat commentedon Jun 8, 2021
Question:
deetergp commentedon Jun 8, 2021
It would be a regression if your fix for this issue causes the other behavior to break. We need a solution that works for the "Edit" selection sending focus to the Edit textbox, without preventing the others from sending focus back to the main composer textbox when performed.
parasharrajat commentedon Jun 8, 2021
Ok. But before delving into that I would like to ask that whether we need that behaviour "Focus the composer on Context menu Close" at all. cc: @roryabraham @shawnborton .
roryabraham commentedon Jun 8, 2021
Yeah, I think we want to keep that behavior. It would also be great if the composer is refocused whenever the edit is made or cancelled:
ArrowUpto edit your last messageEscto cancel the editReportActionComposeis not focused, but it should be.That's additional scope though, so could be handled in a follow-up.
parasharrajat commentedon Jun 8, 2021
We focus on the composer when the menu(which is a modal internally) is closed. But currently, there is no way to tell that a user-initiated
Edit commentaction. If we know this, then we can block the focus on the context-menu close over the composer.So Onyx's way to do this would be to set a key and then reset it automatically which will cause an event like a trigger via onyx. And we can subscribe to that key in the composer. But This is not the correct way, I would say.
Another way would be,
a) Create a lib with two methods like
onComposerFocus& 'requestFocus' . We will not needunsubscribeas callingonComposerFocuswill replace the previous callback.b)
onComposerFocuswill be used to register a callback from theReportActionComposer, typicallythis.focus.c) Now whenever we call
requestFocusfrom anywhere in the app, a registered callback will be called.I feel like we can remove the modal visibility-based autofocus from 'componentDidUpdate' on the Composer and call this method to requestFocus.
I think technique two would be useful when we want to set focus on the close of the Edit box. Also, it will work for multiple Edit boxes. I don't see any drawback from this approach. Any thoughts @roryabraham?
roryabraham commentedon Jun 8, 2021
I definitely agree we don't want to put this data (whether the main
ReportActionComposeshould be focused) in Onyx. I think your solution sounds good, except to keep it clean I would make a couple small tweeks, including an "unsubscribe" function. Something like this:ReportActionComposeFocusManager, and export three functions:onComposerFocus,focus, andclearReportActionCompose::componentDidMountregister theonComposerFocuscallback, then incomponentWillUnmountcallclearto reset the callback to a no-op. That will ensure we're never trying to focus an element that's been unmounted.ReportActionComposecomponent, callfocusAll that said, I don't understand your use of
setTimeoutin your original proposal and how it solves the problem of focusing the "edit comment" input w/ the selection position at the end of the input.parasharrajat commentedon Jun 9, 2021
Thanks for the input. I have migrated from the use of SetTimeout in the PR. My actual solution does not need settimeout now We needed it so that we can wait for the context modal to close. But in PR, I delayed the calling of editComment so now we don't need settimeout. I am trying to get away from settimeout as much as possible.
parasharrajat commentedon Jun 11, 2021
Issue resolved but I'm still waiting for the Offer to be sent. 🙄
kevinksullivan commentedon Jun 14, 2021
Offer sent.
kevinksullivan commentedon Jun 22, 2021
Paid un upwork!