[HOLD for payment 2023-06-21] [$1000] Chat - Height of edit field is different for mWeb and native app @thesahindia

[kbecciv]

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. Launch the app
2. Navigate to any chat
3. Send a message
4. Long press on it > Edit
5. Keep pressing enter and check the max height

Expected Result:

The height should be consistent

Actual Result:

The height isn't consistent

Workaround:

Unknown

Platform:

Where is this issue occurring?

• Web
• iOS
• Android
• Desktop App
• Mobile Web

Version Number: 1.2.12.2

Reproducible in staging?: Yes

Reproducible in production?: Yes

Email or phone of affected tester (no customers): any

Logs: https://stackoverflow.com/c/expensify/questions/4856

Notes/Photos/Videos: Any additional supporting documentation

Expensify/Expensify Issue URL:

Issue reported by: @thesahindia

Slack conversation: https://expensify.slack.com/archives/C01GTK53T8Q/p1664197609260269

View all open jobs on GitHub

[melvin-bot[bot]]

Triggered auto assignment to @strepanier03 (AutoAssignerTriage), see https://stackoverflow.com/c/expensify/questions/4749 for more details.

[Puneet-here]

Proposal

We need to keep the max lines 6 for isSmallScreenWidth and 16 when it's false

https://github.com/Expensify/App/blob/f84651934726d25e720047839c4ed750eea111b6/src/pages/home/report/ReportActionItemMessageEdit.js#L201

maxLines={this.props.isSmallScreenWidth ? 6 : 16}

we can keep add them in CONST.js

I think we should also check if it's a touch screen by canUseTouchScreen() we should also check the same here https://github.com/Expensify/App/blob/f84651934726d25e720047839c4ed750eea111b6/src/pages/home/report/ReportActionCompose.js#L144

[strepanier03]

Triage notes:

This issue is repeatable.

Tested this on my Android using the new.expensify mobile app.

I was able to have a height much greater than 6 or 16, it seemed near infinite and I could keep increasing the height repeatedly.

[strepanier03]

I think even with a proposal, this should go to Eng according ot the linked SO. There's multiple moving parts to our transition to BZ and I think this Triage process will go away in it's current form soon, a DD was just released for reviews regarding it.

Based on this thread in Slack, I think I should still triage as normal for now, with this going to eng who can put the external label on it as needed??

[melvin-bot[bot]]

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

[melvin-bot[bot]]

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

[melvin-bot[bot]]

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

[melvin-bot[bot]]

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

[roryabraham]

@Puneet-here's proposal looks good to me, but I'll let @rushatgabhane buddy-check it. I'm not sure if we need to check canUseTouchScreen or if just screen width is sufficient here.

[rushatgabhane]

@Puneet-here question: In message edit, why is the max line height 6 for native? Shouldn't it be 16 because we're setting it over here?

https://github.com/Expensify/App/blob/f84651934726d25e720047839c4ed750eea111b6/src/pages/home/report/ReportActionItemMessageEdit.js#L201

Also, I'm curious why we should use canUseTouchScreen(), is it because it's mWeb only?

[Puneet-here]

@Puneet-here question: In message edit, why is the max line height 6 for native? Shouldn't it be 16 because we're setting it over here?

It's probably because of the styling

Also, I'm curious why we should use canUseTouchScreen(), is it because it's mWeb only?

I think it's good to use canUseTouchScreen() and isSmallScreenWidth because this way the height of composer won't change for desktop when we decrease the screen size

[rushatgabhane]

It's probably because of the styling

@Puneet-here I think we need to dive deeper then. Like which specific style is changing maxLine behavior for native only?

I think it's good to use canUseTouchScreen() and isSmallScreenWidth because this way the height of composer won't change for desktop when we decrease the screen size

Oh, I get you. Desktop app is rarely resized to small width. And I think it's okay to change the height on desktop when the screen size is reduced to simplify the code a bit.

[sonialiap]

Job is posted to Upwork

[Puneet-here]

@Puneet-here I think we need to dive deeper then. Like which specific style is changing maxLine behavior for native only?

I was a little bit busy, I will check this today.

[Puneet-here]

Apologies, I just realised that I had closed the tab without really pressing the comment button earlier.

So we have platform specific file for composer and because we are passing maxHeight, passing maxLines has no effect on it https://github.com/Expensify/App/blob/83676c1036299ffecbf82f4b829a7f8cbd741cd7/src/components/Composer/index.ios.js#L104 https://github.com/Expensify/App/blob/83676c1036299ffecbf82f4b829a7f8cbd741cd7/src/components/Composer/index.android.js#L99

[rushatgabhane]

You're right! maxLines internally sets the numberOfLines. And numberOfLines is an android only prop (docs).

So for Native, we're using maxHeight.

We need to keep the max lines 6 for isSmallScreenWidth and 16 when it's false

Sounds good.

@roryabraham @Puneet-here's proposal LGTM! :) 🎀 👀 🎀 C+ reviewed

[rushatgabhane]

@roryabraham do you think a cleaner solution would be to implement maxLines for native using maxHeight?

This way, Composer will have a single interface to change it's maximum number of lines.

[rushatgabhane]

bump on https://github.com/Expensify/App/issues/11694#issuecomment-1291451773 @roryabraham

[melvin-bot[bot]]

@sonialiap, @rushatgabhane, @roryabraham Whoops! This issue is 2 days overdue. Let's get this updated quick!

[roryabraham]

@roryabraham do you think a cleaner solution would be to implement maxLines for native using maxHeight?

I think a cleaner solution would probably be to implement the maxLines prop in React Native for iOS and use maxLines consistently across platforms.

[rushatgabhane]

Perfect, thanks @roryabraham

@Puneet-here feel free to submit an update proposal applying this suggestion

[roryabraham]

Overall I feel like:

1. The platform differences of the Composer have diverged more than they should have, such that the differences between the platforms aren't clear
2. There is a lot of custom logic in the Composer component, much of which is probably intentional, but a lot of the context behind that logic is not very transparent. Some automated tests which cover the behaviors that we expect would go a long way into preserving the intention of the component.
3. What is even happening with isComposerFullSize?
   • Why do we have three separate values for isFullComposerAvailable, setIsFullComposerAvailable, and isComposerFullSize?
   • isComposerFullSize is actually persisted in Onyx on a report level: https://github.com/Expensify/App/blob/0e147426759c9cf4878479f3ffeb69e54249aeb6/src/pages/home/ReportScreen.js#L341-L343, is that something we actually want to persist between sessions or are we just using that as a way to manage UI state?
   • Do we actually need a separate value for isFullComposerAvailable? It seems like it's just shadowing isComposerFullSize.
4. It seems like all of this boils down to maxLines, which we could use directly across all platforms if it was implemented in React Native iOS.
5. Why is maxLines smaller for small screens? Is this because we have a software keyboard that takes up screen height? If so, why don't we determine maxLines based on whether the software keyboard is open, rather than based on platform or screen width?

I think the wisest course of action would be:

• To do a Full Throttle (FT) or Slack meeting for the Composer component across platforms, to identify the behaviors we expect.
• (optional, but ideal) Write some automated tests to cover the expected behavior
• Update the Composer component to address this issue and some of the above concerns with as simple and cross-platform an implementation as possible, implementing maxLines in React Native for iOS if needed.

What do people think?

[melvin-bot[bot]]

@sonialiap, @rushatgabhane, @roryabraham Whoops! This issue is 2 days overdue. Let's get this updated quick!

[roryabraham]

Didn't have time to do a deep-dive on this myself yet, but I did take the opportunity to do one pass on cleanup of the Composer component in this PR. It's not much, but it's an easy win and a step in the right direction.

Long-term for this specific bug I think we probably want to implement maxLines on iOS as well as Android in RN.

[sonialiap]

Price doubled 🙌

[roryabraham]

This PR turned out to be not quite ready yet. An upstream fix is needed, but then it will be ready

[tjferriss]

Following instructions for the weekly update chore:

@roryabraham this is one of the oldest issues in the /App repo. To help us clear out the large backlog of bugs, can you:

• Decide whether any proposals currently meet our guidelines and can be approved as-is
• For any that can't, please take this issue internal and treat it as one of your highest priorities
• If you have any questions, don't hesitate to start a discussion in #bug-zero

[trjExpensify]

Ah, just commented here in thread. I’m changing the label to Internal and removing Help wanted.

[melvin-bot[bot]]

@sonialiap, @rushatgabhane, @roryabraham Huh... This is 4 days overdue. Who can take care of this?

[roryabraham]

Ah, just commented here in thread. I’m changing the label to Internal and removing Help wanted.

Sorry, I think there was a miscommunication here @trjExpensify. The PR I put up was a step in the right direction, but not a solution to this issue.

Also the upstream fix was merged in our fork, but in the actual react-native-web upstream, the maintainer has stopped accepting pull requests in the VirtualizedList components. I think the plan is to move VirtualizedList into a separate package of React Native. We need to have a conversation about what to do here.

[JmillsExpensify]

Going to float this one to Margelo today.

[roryabraham]

Still waiting for proposals that fix this by implementing numberOfLines for React Native TextInput on iOS (currently an Android-only prop), such that it can be used consistently across platforms.

[sonialiap]

Price doubled 🥳

[sonialiap]

waiting for proposals

[roryabraham]

We're getting some help from @Szymon20000 on the upstream numberOfLines prop.

[Szymon20000]

I added native support for numberOfLines and maximumNumberOfLines on iOS. Here is a pr where you can test it. https://github.com/Expensify/App/pull/13367

[JmillsExpensify]

@sonialiap Please note that @Szymon20000 qualifies for the bug bounty. Thanks!

[Szymon20000]

https://github.com/Expensify/react-native/pull/37 Here is the pr introducing above 2 props to Expensify/react-native fork.

[rushatgabhane]

can't review this today or on weekend

[roryabraham]

@sonialiap Please note that @Szymon20000 qualifies for the bug bounty. Thanks!

Sorry for the multiple miscommunications here @JmillsExpensify @sonialiap et al, but I think @Szymon20000 should just bill us separately for his work in the React Native fork. Completing https://github.com/Expensify/react-native/pull/37 is a prerequisite of fixing this bug but will not fix it outright, so we'll want to hire someone separately to use the new feature to fix this bug. Hopefully that makes sense. Either that or just hire @Szymon20000 to take it over the finish line with an E/App PR, which I'd also be fine with (and then he'd qualify for the bounty on top of his hourly rate).

[melvin-bot[bot]]

@Szymon20000, @sonialiap, @rushatgabhane, @roryabraham Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

[rushatgabhane]

PR in draft, feel free to unassign me if this needs a review anytime this week.

[trjExpensify]

👋 Should https://github.com/Expensify/react-native/pull/37 be in review, not a draft?

[Szymon20000]

👋 Should Expensify/react-native#37 be in review, not a draft?

It works in paper architecture but in Fabric Sth is wrong when you already reached out maximum height and add a new line. I haven't yet found what's causing it. Will take a look today/tomorrow. However, you can already review it :)

[Szymon20000]

After long hours of uneven struggle, I found a strange bug in NSContainer, which turned out to be visible not only in Fabric. However, in paper architecture it was only visible in text. Long story short when you set some properties in a wrong order than adding new lines causes text/text input to grow even though we set maximumNumberOfLines. Here is the commit: https://github.com/Expensify/react-native/pull/37/commits/5db9c6f77c265c7a5bed0e4c22b3a7fb5706f6e1

I will clean up the pr on Monday morning and would be very happy if someone could test it.

[trjExpensify]

Nice! Thanks for getting that up, Szymon! @roryabraham @rushatgabhane are you guys taking the reviews and testing on that? Let's get you added to the PR as reviewers now if so!

Also, where did we land on this comment? @Szymon20000 did you want to take the App PR as well? Given the age of this issue, we should find an internal volunteer for it if not @roryabraham. Either way, this issue is held on the Expensify/react-native PR which is a prerequisite to fixing the bug this issue was created for. I'm updating the title to reflect the dependency.

[melvin-bot[bot]]

@Szymon20000, @sonialiap, @rushatgabhane, @roryabraham Whoops! This issue is 2 days overdue. Let's get this updated quick!

[roryabraham]

Left a comment on the react-native PR

[Szymon20000]

Nice! Thanks for getting that up, Szymon! @roryabraham @rushatgabhane are you guys taking the reviews and testing on that? Let's get you added to the PR as reviewers now if so!

Also, where did we land on https://github.com/Expensify/App/issues/11694#issuecomment-1344968442? @Szymon20000 did you want to take the App PR as well? Given the age of this issue, we should find an internal volunteer for it if not @roryabraham. Either way, this issue is held on the Expensify/react-native PR which is a prerequisite to fixing the bug this issue was created for. I'm updating the title to reflect the dependency.

Sure, I can open a pr fixing this issue in the App repo once the RN pr is merged. It's 5min fix then.

[Szymon20000]

Just switched the base branch of RN pr to Expensify-0.71.0-alpha1 branch + opened a twin pull-request in react-native repo.