Expensify / App

Welcome to New Expensify: a complete re-imagination of financial collaboration, centered around chat. Help us build the next generation of Expensify by sharing feedback and contributing to the code.
https://new.expensify.com
MIT License
3.54k stars 2.89k forks source link

[$500] Chat - The composer jumps when editing a message #36374

Closed m-natarajan closed 7 months ago

m-natarajan commented 9 months 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!


Version Number: 1.4.40-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 Expensify/Expensify Issue URL: Issue reported by: Applause internal team Slack conversation:

Action Performed:

  1. Navigate to a chat with a lot of text messages
  2. Edit the text closest to the composer

Expected Result:

It shouldn't shake or jump

Actual Result:

The composer jumps when editing a message - see this video for an example

Workaround:

unknown

Platforms:

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

Screenshots/Videos

Add any screenshot/video evidence

https://github.com/Expensify/App/assets/38435837/f101889f-6e39-46c6-9ceb-9bcea89cd234

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~011ebf704e63731d68
  • Upwork Job ID: 1757147558004047872
  • Last Price Increase: 2024-04-01
s77rt commented 8 months ago

@brunovjk

Main composer appears even when we still have an edit composer open. Steps to reproduce in the proposal.

That's expected since no edit composer is focused

shawnborton commented 8 months ago

I'd be keen to find a solution that prevents any kind of jumping/movement. I can see where the animated approach feels nicer, but it doesn't feel like it really solves the issue.

jeremy-croff commented 8 months ago

I'd be keen to find a solution that prevents any kind of jumping/movement. I can see where the animated approach feels nicer, but it doesn't feel like it really solves the issue.

I reviewed the experience when the keyboard is open and it's much less jarring.

The primary reason for asking whether the keyboard should pop up here: https://github.com/Expensify/App/issues/36374#issuecomment-1993144489

Is because in native we do pop up the keyboard, and it's a smooth transition as both the keyboard pushes up, and the editable composer pushes towards it.

We could force a similar behavior for web, to open the keyboard. Especially cause in web the edit composer IS focused ( green ring ) but I'm surprised the keyboard diddn't pop up here.

However I would like some UX confirmation

jeremy-croff commented 8 months ago

Native:

https://github.com/Expensify/App/assets/157416545/aebcf26f-4c12-431d-a574-1ece46853349

s77rt commented 8 months ago

@jeremy-croff Interesting! I thought the keyboard didn't show up just because I was on a simulator (hardware keyboard). I agree for consistency the keyboard should show up when editing a message.

s77rt commented 8 months ago

The keyboard does not show up only on Safari. We had many similar bugs regarding focus. Safari won't open the keyboard unless it's a user action and within a limited time frame (I think it's 1sec).

jeremy-croff commented 8 months ago

The keyboard does not show up only on Safari. We had many similar bugs regarding focus. Safari won't open the keyboard unless it's a user action and within a limited time frame (I think it's 1sec).

I confirmed also in IOS Chrome it doesn't pop up on my real device. However on Simulators it does. So developing or debugging this is going to be tricky for me.

edit: I'm afraid this a can of worms: https://github.com/Expensify/App/pull/23922

brandonhenry commented 8 months ago

@s77rt @shawnborton in my opinon, the keyboard popping up is only a side effect of this and essentially hides the real issue. I don't think we should try to focus on making they keyboard pop up since that seems like a bandaide. if for any reason the keyboard does not open up, the user will see the jarring experience.

my root cause correctly identifies why we're seeing the jarring. the way the code is currently written, the jarring will always happen, whether the keyboard pops up or not. This is true, because we see the issue on all devices, not just mWeb Safari.

I believe we need some type of animation (or any styling really) so we are not just remove and showing the component with no styling.

brunovjk commented 8 months ago

@brunovjk Thanks for the update. Good catch with the multiline issue. Can you please outline how we will fix that without PRs / code diffs?

@s77rt Thanks for the review. Thank you. Sure. We should calculate the numberOfLines from the draft in ReportActionItemMessageEdit and then pass it as a props to the Composer Component.

brunovjk commented 8 months ago

@brunovjk

Main composer appears even when we still have an edit composer open. Steps to reproduce in the proposal.

That's expected since no edit composer is focused

I understand, it makes sense, we need the main composer back when we are not focusing on anything, but doesn't that seem strange to you?

Strange behavior observed in the main composer when focusing and blurring edit composers. https://github.com/Expensify/App/assets/95647348/06007af0-280a-46cc-9d2d-e189758d7984

I believe I can improve the logic how we deal with ONYXKEYS.SHOULD_SHOW_COMPOSE_INPUT when editing messages, this should mainly prevents any kind of jumping/movement. I'll be back when there's an update.

s77rt commented 8 months ago

@jeremy-croff

However on Simulators it does. So developing or debugging this is going to be tricky for me.

On the simulator you need to turn off the hardware keyboard: I/O > Keyboard > Connect Hardward Keyboard to be able to reprouduce (if it's off, turn it on then back off)

I'm afraid this a can of worms

Safari issues were never straightforward 😅

s77rt commented 8 months ago

@brandonhenry The keyboard should open when editing a message. If we are going to add animation here we should take the keyboard animation into account. Animation is also a bandaide here. the optimal solution is to prevent the jump behaviour and not to just smooth it out.

s77rt commented 8 months ago

@brunovjk

We should calculate the numberOfLines from the draft

Can you elaborate? How can we calculate the number of lines given a text input? Wouldn't we need to render the text first? (to measure its width)

Strange behavior observed in the main composer when focusing and blurring edit composers.

That's actually a bug, we fixed that here https://github.com/Expensify/App/issues/23908 (Case 2) but looks like some recent change broke it. Let's not focus on that since it's a different bug.

brunovjk commented 8 months ago

How can we calculate the number of lines given a text input?

@s77rt What do you think?

// ReportActionItemMessageEdit.tsx
...
    const numberOfLines = useMemo(() => {
        if (!draft) {
            return 1; // Return a default value if draft is empty
        }

        // Get the number of newline characters in the draft content
        const newlineCount = (draft.match(/\n/g) || []).length;

        // Add 1 to account for the last line
        const calculatedNumberOfLines = newlineCount + 1;
        return calculatedNumberOfLines;
    }, [draft]);
...
s77rt commented 8 months ago

@brunovjk The text may wrap into multiple lines even if there is no LF char (\n).

brunovjk commented 8 months ago

Hmm... I see, I have to dig further then. @s77rt do you have any idea?

brandonhenry commented 8 months ago

@s77rt could you help me understand the purpose of hiding the main composer when we edit? I cannot seem to find any details on why we are doing that?

The root cause of the problem is the sudden removal of the main composer from the component hierarchy when transitioning to the editing mode. This abrupt change in the component structure causes the content below to reposition itself instantly, resulting in a jarring visual effect.

So, in my opinion, we have just a couple options here:

  1. Always show the main composer, even during editing. This approach maintains a consistent layout and aligns with user expectations based on other chat applications like Slack. By keeping the main composer visible, we prevent unexpected layout shifts and provide a more stable user experience.

  2. Instead of hiding the entire composer, we can keep the composer's View container visible and only gray out the children content whenever an edit composer is being used. This solution preserves the layout structure while still indicating that the main composer is temporarily disabled during editing (which I am guessing is the reason we are hiding it?).

I believe these approaches would significantly enhance the user experience without impacting the app's performance. Additionally, I noticed that the current behavior allows the main composer to be open simultaneously with the edit composer, which might be confusing for users. I think it's worth discussing whether we want to keep the composer hidden at all times when an edit composer is open or if it doesn't matter and we can always keep the main composer open.

I'd love to hear your thoughts. I'm open to refining the approach based on feedback and working together to address this issue effectively.

s77rt commented 8 months ago

@brunovjk

Hmm... I see, I have to dig further then. @s77rt do you have any idea?

Not really but I think we should work on the focus and animation issues first

s77rt commented 8 months ago

@brandonhenry We don't want the main composer to show up when editing a message for two reasons:

  1. Not to the confuse the user (can't tell where he is typing)
  2. Get more space
With main composer Without main composer
2022-05-18_14-02-07 2022-05-18_14-01-13

I noticed that the current behavior allows the main composer to be open simultaneously with the edit composer

As long as no edit composer is focused (the keyboard not visible) then the main composer is expected to be visible

melvin-bot[bot] commented 7 months ago

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

s77rt commented 7 months ago

Still looking for proposals

melvin-bot[bot] commented 7 months ago

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

jliexpensify commented 7 months ago

Hello @sakluger - I am headed OOO from the 21st to 31st March so have assigned you as a buddy here.

Summary:

mvtglobally commented 7 months ago

Issue not reproducible during KI retests. (First week)

jliexpensify commented 7 months ago

@mvtglobally can you share a video please? Thanks!

sakluger commented 7 months ago

Sounds like this may be fixed. Is anyone due payment? My assumption is no, @s77rt can you please confirm.

s77rt commented 7 months ago

@sakluger If this is fixed already then no one would be due payment

brunovjk commented 7 months ago

@s77rt I believe we've made significant progress in mitigating the 'shake or jump' issue by ensuring simultaneous rendering/hiding of the composer and text. I actually don't know how it was done, but it seems like that's what happened hehe. However, a small 'shake or jump' persists due to the '2 lines' issue. I couldn't pinpoint the root cause, but it seems related to passing numberOfLines=undefined and multiline=true in the composer at ReportActionItemMessageEdit. As calculating numberOfLines upfront remains challenging, setting numberOfLines=1 has resolved the issue, but I wonder about potential regressions. I'm questioning the use of 0 as the default value in src/components/Composer/index.tsx. Since I can't think of a scenario where 0 would be appropriate, changing it to 1 would also resolve the 'two-line composer issue'. Do you think this is worth fixing? If yes, here? Or should we create another issue? Thank you for your time.

s77rt commented 7 months ago

@brunovjk If the fix will only cover the case of 2 lines then it may not be worth it as it will only sound like a workaround (issue would still be reproducible where we have multiple lines)

brunovjk commented 7 months ago

Update:

@s77rt please take a look https://github.com/Expensify/App/pull/30964#issuecomment-1808403267: "We set the default value of numberOfLines in ReportActionItemMessageEdit composer to be undefined. This is the same as not setting the rows property of textarea. If we don't set the rows property, textarea will have 2 rows by default."

I didn't find any related issue. I'm still working on the function to calculate numberOfLines before render the composer.

melvin-bot[bot] commented 7 months ago

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

s77rt commented 7 months ago

Still looking for proposals. Though it has been reported that the behaviour is no longer that buggy but there is still room for improvements

jliexpensify commented 7 months ago

Back, thanks Sasha - unassigning you!

melvin-bot[bot] commented 7 months ago

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

brunovjk commented 7 months ago

@s77rt what do you think about closing this current issue and opening another one? Regarding the 'Number of Lines when Editing a Message', I believe it would be cool to be similar to Slack:

https://github.com/Expensify/App/assets/95647348/7dcada21-55f2-4ef6-8dcb-cda860eed84e

I couldn't find a solution, maybe another issue will bring the right eyes to the problem.

s77rt commented 7 months ago

Since the original bug does not seem to be reproducible anymore it makes sense to close this issue cc @jliexpensify

s77rt commented 7 months ago

@brunovjk I don't think that issue is important enough given that bug reports are limited but feel free to raise on Slack