[HOLD #15598] Make "save changes" and "cancel" buttons visible when editing comment

[puneetlath]

When you are editing a comment, depending on how you are scrolled, the "save changes" and "cancel" buttons aren't always visible.

This can be true on all platforms, but is especially likely on mobile where the keyboard that comes up can cover the buttons.

Let's update this so that whenever we enter editing mode on a message, the buttons are visible.

@eVoloshchak would you like to C+ this since you were the C+ on this issue where we originally came to this conclusion.

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~01c04802076c08f73f
• Upwork Job ID: 1640745904641236992
• Last Price Increase: 2023-03-28

[MelvinBot]

Current assignee @puneetlath is eligible for the Bug assigner, not assigning anyone new.

[MelvinBot]

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

[eVoloshchak]

@eVoloshchak would you like to C+ this since you were the C+ on #15303 where we originally came to this conclusion.

Absolutely!

[gedu]

Taking a look

[MelvinBot]

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

[MelvinBot]

Current assignee @eVoloshchak is eligible for the Internal assigner, not assigning anyone new.

[gedu]

I am exploring the use of ViewPort and scrollToIndex along with the viewOffset to ensure that the list item is centered as much as possible within the list.

[gedu]

I am exploring the onFocus event from the input element to retrieve viewport data and determine whether the input element might be covered by the keyboard. If this is the case, I plan to shift the FlatList element so that the input element has more visibility, along with the buttons.

[MelvinBot]

@puneetlath, @gedu, @eVoloshchak Whoops! This issue is 2 days overdue. Let's get this updated quick!

[gedu]

Chatting internally a proposal

[gedu]

@puneetlath

Proposal

Please restate the problem that we are trying to solve in this issue.
Issue: The “Save” and “Cancel” buttons can get obscured by the keyboard.

What is the root cause of that problem?
The list does not always scroll to the focused edit input, and sometimes when it does, it does not do it correctly. One of the factors could be the “getItemLayout” math that is not exact and makes the scroll behave weirdly. Additionally, having multiple edit modes at the same time makes the scrolling to the input behave strangely.

What changes do you think we should make in order to solve the problem?
Without UI changes: When a message is selected to be edited, all the messages older than that message will be hidden, making the edited message the top message on the screen. This gives us the chance to shortcut the list and use the “scrollToIndex” more effectively (focusing and placing the item always at the top). If the user cancels, all the messages will be shown again. This approach also enables us to handle only one edit message at a time.

What alternative solutions did you explore? (Optional)
With UI changes: We could copy how Slack and Discord handle it. When a message is selected to be edited, that text is placed into the main chat input, and the “Save” and “Cancel” button UI is shown. This way, we avoid any scrolling logic, data list manipulation, and list item render changes.

[puneetlath]

@gedu I'm not 100% following. I have some follow-up questions.

One of the factors could be the “getItemLayout” math that is not exact and makes the scroll behave weirdly.

Would you mind explaining this? What is not exact about it?

Additionally, having multiple edit modes at the same time makes the scrolling to the input behave strangely.

What are the multiple edit modes? I'm not familiar. I thought there is only one way to edit a message.

Without UI changes: When a message is selected to be edited, all the messages older than that message will be hidden, making the edited message the top message on the screen. This gives us the chance to shortcut the list and use the “scrollToIndex” more effectively (focusing and placing the item always at the top). If the user cancels, all the messages will be shown again. This approach also enables us to handle only one edit message at a time.

I'm having a hard time imagining how this would look. Would there just be blank space below the message being edited if there are no messages after it?

With UI changes: We could copy how Slack and Discord handle it. When a message is selected to be edited, that text is placed into the main chat input, and the “Save” and “Cancel” button UI is shown. This way, we avoid any scrolling logic, data list manipulation, and list item render changes.

That's interesting. However, I notice that they have different behavior on mobile vs desktop. I think we'd prefer to have the behavior consistent across platforms if possible.

[gedu]

@puneetlath

Would you mind explaining this? What is not exact about it?

Sorry for the vague explanation earlier. I'm not entirely sure if react-native-web has limitations, but I've noticed that some functions from the list aren't working as expected. For example, when I use scrollToIndex to go to index 35, it doesn't take me there. The same issue occurs with scrollToOffset, which misses by a couple of pixels and becomes unpredictable when the keyboard is showing or not. I suspect that the problem lies with the getItemLayout function. Although the code makes sense, I believe that something is not matching with the actual behavior of the app. This issue would require further investigation.

What are the multiple edit modes? I'm not familiar. I thought there is only one way to edit a message.

I mean this

I'm having a hard time imagining how this would look. Would there just be blank space below the message being edited if there are no messages after it?

Ideally, the app should show one or two messages above the edited message. If the edited message is closer to the start of the chat, it should show more items to ensure that the chat is always filled with messages, if possible.
https://user-images.githubusercontent.com/1676818/230032693-0a079b17-59f0-471d-881a-f50e3f65ba78.mp4

If there is no deadline or urgency on this issue, I can give another round of tests on this and chat with the team.

[eVoloshchak]

With UI changes: We could copy how Slack and Discord handle it. When a message is selected to be edited, that text is placed into the main chat input, and the “Save” and “Cancel” button UI is shown. This way, we avoid any scrolling logic, data list manipulation, and list item render changes.

This feels like the most intuitive approach to me, however that would take away the ability to edit multiple messages in different tabs, since composer is synced between tabs, while the edit message composer isn't

[puneetlath]

If there is no deadline or urgency on this issue, I can give another round of tests on this and chat with the team.

Yes, no problem. Let's try to find the best solution, even if it takes a little longer.

[MelvinBot]

@puneetlath, @gedu, @eVoloshchak Whoops! This issue is 2 days overdue. Let's get this updated quick!

[MelvinBot]

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

[eVoloshchak]

Not overdue, waiting for proposals

[MelvinBot]

@puneetlath @gedu @eVoloshchak 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!

[gedu]

Exploring the possibility of using BaseInvertedFlatList in our favor given it has a map of all the item height

[puneetlath]

Another option was raised here of putting the buttons inside the input, like they are in the regular compose box. Would that solve the problems we're talking about? https://expensify.slack.com/archives/C049HHMV9SM/p1677476774053969

[gedu]

I think it will help make the buttons always visible. Should we focus on the scrolling behavior? or that UI change will fix this issue? @puneetlath

[gedu]

should we talk in that thread about scrolling to the on-edit message? and how should behave when the keyboard shows? because in the last video, the keyboard is always hidden

[puneetlath]

Yeah that's a good idea @gedu. Let's bring this up in that thread.

[MelvinBot]

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

[eVoloshchak]

Not overdue, looks like discussion is still in progress on slack

[MelvinBot]

@puneetlath @gedu @eVoloshchak this issue is now 3 weeks old. There is one more week left before this issue breaks WAQ. What needs to happen to get a PR in review this week? Please create a thread in #expensify-open-source to discuss. Thanks!

[puneetlath]

Putting on hold as we discussed in Slack until #15598 is done.

[gedu]

We will hold on this until #15598 you can read here

[puneetlath]

Still on hold.

[puneetlath]

Ok I think this issue has been made obsolete by moving the buttons inline.

I'm going to go ahead and close, but reopen or comment if you disagree!