Open IuliiaHerets opened 2 weeks ago
Triggered auto assignment to @trjExpensify (Bug
), see https://stackoverflow.com/c/expensify/questions/14418 for more details. Please add this bug to a GH project, as outlined in the SO.
@trjExpensify FYI I haven't added the External label as I wasn't 100% sure about this issue. Please take a look and add the label if you agree it's a bug and can be handled by external contributors
Reproducible:
Job added to Upwork: https://www.upwork.com/jobs/~0166150593d62a36c2
Triggered auto assignment to Contributor-plus team member for initial proposal review - @alitoshmatov (External
)
The room description on Android is cut off when typein multiple lines.
function getAutoGrowHeightInputStyle
doesn't workout a correct height. Consider to use maxHeight: '100%'
instead, because we have clamp height on the container element.
https://github.com/Expensify/App/blob/9e82ed89b07c62ccb5d21a70398975bf0c37d11b/src/styles/index.ts#L1158
function getAutoGrowHeightInputStyle
doesn't workout a correct height. Consider to use maxHeight: '100%'
instead, because we have clamp height on the container element.
NA Reminder: Please use plain English, be brief and avoid jargon. Feel free to use images, charts or pseudo-code if necessary. Do not post large multi-line diffs or write walls of text. Do not create PRs unless you have been hired for this job.
Edited by proposal-police: This proposal was edited at 2023-10-01T00:00:00Z.
Room -On entering multiline text with header markdown, 2nd line is not visible
To fix scroll bar flickering issue on line breaks we are the text input height here
https://github.com/Expensify/App/blob/e66b1238e7ad0e69b0a12f429188d71bef09e35e/src/components/TextInput/BaseTextInput/index.tsx#L386-L389
So the input has a fixed height so to allow autoGrow of height for multiline we are setting the height of the container to textInputHeight
here
https://github.com/Expensify/App/blob/e66b1238e7ad0e69b0a12f429188d71bef09e35e/src/components/TextInput/BaseTextInput/index.tsx#L300
this textInputHeight
is determined by the layout of an invisible Text
here
https://github.com/Expensify/App/blob/e66b1238e7ad0e69b0a12f429188d71bef09e35e/src/components/TextInput/BaseTextInput/index.tsx#L512
But in our case the InputComponent
is RNMarkdownTextInput
which styles the text value based on the markdown and in this case when the user inputs header markdown it is bold and takes more space/width so jumps to the next line with fewer characters than where the normal invisible Text
component discussed above which we use to calculate the textInputHeight
.
The scroll-bar flickering problem is only for normal RNTextInput
and doesn't occur for RNMarkdownTextInput
so we need only to apply manual height for !isMarkdownEnabled
case
So change
https://github.com/Expensify/App/blob/e66b1238e7ad0e69b0a12f429188d71bef09e35e/src/styles/utils/index.ts#L1229-L1242
getAutoGrowHeightInputStyle: (textInputHeight: number, maxHeight: number, includeHeight = true): ViewStyle => {
if (textInputHeight > maxHeight) {
return {
...styles.pr0,
...styles.overflowAuto,
};
}
return {
...styles.pr0,
...styles.overflowHidden,
// maxHeight is not of the input only but the of the whole input container
// which also includes the top padding and bottom border
...(includeHeight && {['height']: maxHeight - styles.textInputMultilineContainer.paddingTop - styles.textInputContainer.borderBottomWidth}),
};
autoGrowHeightInputContainer: (textInputHeight: number, minHeight: number, maxHeight: number, includeHeight = true) =>
({
...(includeHeight && {height: lodashClamp(textInputHeight, minHeight, maxHeight)}),
...(!includeHeight && {maxHeight}),
minHeight,
} satisfies ViewStyle),
autoGrowHeight &&
styles.autoGrowHeightInputContainer(
textInputHeight,
variables.componentSizeLarge,
typeof maxAutoGrowHeight === 'number' ? maxAutoGrowHeight : 0,
!isMarkdownEnabled,
),
? [
StyleUtils.getAutoGrowHeightInputStyle(textInputHeight, typeof maxAutoGrowHeight === 'number' ? maxAutoGrowHeight : 0, !isMarkdownEnabled),
styles.verticalAlignTop,
]
and apply same for native files too Here is a test branch
The problem is that multi-line text is being cut off when you are using the heading (#
) markdown.
Now, there actually seems to be 2 different root causes for this issue.
1) The first one is, that the text with the heading markdown is bolder and, sometimes, the font size is larger, but the auto grow feature does not account for that in any way, since it is based on a hidden <Text>
component, which doesn't support live markdown.
https://github.com/Expensify/App/blob/537c62ad82c41d692cccc1770a30ab47f5c5afc5/src/components/TextInput/BaseTextInput/index.tsx#L485-L520
Although, fixing only this part of the issue is not enough. Here you can see that the auto grow feature works properly, but it is still cut off on the mobile version of the app:
https://github.com/user-attachments/assets/65f7942c-b372-4ee5-b5dd-6371bf7c1fd2
2) Here, notice that when I add the heading markdown, the text almost "shifts" down, which is causing this visual bug. So, the 2nd part of the issue is actually an issue in @expensify/react-native-live-markdown
node module. In the MarkdownUtils.java
, we can see that for the h1
or the heading markdown, it takes the current line-height and multiplies it by 1.5, which is causing the text's height to overflow and take up more space.
https://github.com/Expensify/react-native-live-markdown/blob/037a1b0107263314ad5cdc710fa862280f65ea32/android/src/main/java/com/expensify/livemarkdown/MarkdownUtils.java#L148-L157
After fixing the node module, we can see that it finally works properly:
https://github.com/user-attachments/assets/74ab7226-e3ee-4224-b8cc-7e076ecc1d1b
1) I fixed the first part of the issue by changing how the hidden <Text>
component behaves. I changed the code to extract every line from the inputted text, and wrap it in styles.textLarge
and styles.textBold
if it starts with the heading markdown (#
). That way, the hidden <Text>
component correctly represents the width/height of the user's input.
(here I unhid the hidden <Text>
component)
The changes I made:
{value?.split('\n').map((line, idx) => {
let formattedLine = line;
if (idx > 0) {
formattedLine = '\n\u200B' + line;
}
if (isMarkdownEnabled && line.startsWith("# ")) {
return <Text style={[styles.textLarge, styles.textBold]} key={idx}>{formattedLine}</Text>;
}
return <Text key={idx}>{formattedLine}</Text>;
})}
2) Fixing the second problem will require a separate issue/PR in the @expensify/react-native-live-markdown
repository. For this test, I commented out the problematic line, but the proper solution I'm thinking of is to either try to extrapolate the lineHeight
based on how much the fontSize
changed, but I think the best solution would be to remove the default behavior of forcing lineHeight
to 1.5x, while still allowing to provide a custom lineHeight
in the markdown style.
Also, there is a little typo:
(custom decription
)
which can be fixed here:
https://github.com/Expensify/App/blob/76f16bc578396765931ec40b522f47f842e58882/src/languages/en.ts#L1479
An alternative solution that we can take in case we don't want to alter the @expensify/react-native-live-markdown
repository is to remove the lineHeight
property from the styles on the app's side here:
https://github.com/Expensify/App/blob/76f16bc578396765931ec40b522f47f842e58882/src/styles/index.ts#L1213-L1222
(so, when it tries to 1.5x the lineHeight
, it's not going to have an effect, but I still think my initial solution would be better)
Auto grow height of markdown text input doesn't grow properly, Like in room description text input.
The TextInput doens't have autoGrow functionality which make the baseTexinput autoGrowHeight of baseTextInput use hidden Text
to grow the TextInput properly as shown in:
in onLayout property the height and width of text input is adjusted.
This only works for non markdown text input, but if the markdown is used, there is styling inside the displayed textinput which will make the the hidden Text
doesn't represent the correct representation of the displayed text. If Heading is used, the certain text will become large, if italic style is used the text grow smaller , etc. This will interfere with auto grow heightfunctionality of the markdown.
The react native live markdown has auto grow height functionality, we could use it and disable the usage of the hidden Text to detect the size of textInput widht/height. Some styling inside baseTextInput interfere with the auto grow of the markdown, so here is some modifications of the code:
We add variable : const isAutoGrowHeightMarkdown = isMarkdownEnabled && autoGrowHeight;
Then we add verticalPadding variable:
const verticalPadding = useMemo (() => {
const flattenedInputStyle = StyleSheet.flatten(inputStyle);
const topBottomPadding = (flattenedInputStyle.paddingTop ?? 0) + (flattenedInputStyle.paddingBottom ?? 0);
if (topBottomPadding !== 0) {
return topBottomPadding;
}
return flattenedInputStyle.padding ?? 0;
}, [inputStyle]);
This is needed, because the maxHeight style of the markdown doesn't include the padding, so we compute the vertical padding then subtract the maxAutoGrowHeight with the vertical padding.
The maxHeight
here enable the auto grow height to grow properly.
Then in :
https://github.com/Expensify/App/blob/fc849dda35628b8c6c8d6edfa66dd31b257e8e7a/src/components/TextInput/BaseTextInput/index.tsx#L300 We change it to:
(autoGrowHeight && !isAutoGrowHeightMarkdown && styles.autoGrowHeightInputContainer(textInputHeight, variables.componentSizeLarge, typeof maxAutoGrowHeight === 'number' ? maxAutoGrowHeight : 0))
here:
change it to:
<View style={[isAutoGrowHeightMarkdown ? undefined : styles.textInputAndIconContainer, isMultiline && hasLabel && styles.textInputMultilineContainer, styles.pointerEventsBoxNone]}>
Then below this line:
we set the maxHeight of markdown:
isAutoGrowHeightMarkdown? {maxHeight: maxAutoGrowHeight - verticalPadding} : undefined,
Then change the conditional in:
to be ...(autoGrowHeight && !isAutoGrowHeightMarkdown
Then we disable the hidden text input: In here:
change it to : {contentWidth && !isAutoGrowHeightMarkdown (
and in here:
change it to: {(!!autoGrow || autoGrowHeight) && !isAutoGrowHeightMarkdown && (
Then do similarly in baseTextinput.native.tsx For code changes reference I have made a test branch.
@trjExpensify, @alitoshmatov Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!
@alitoshmatov can you give these proposals a review, please?
@jp928 Thank you for your proposal, not sure what you mean exactly but maxHeight is used to calculate input height based on text, padding and other factors. Setting it to 100%
in getAutoGrowHeightInputStyle
function is not possible since it is expecting number and doing calculations on it.
@FitseTLT Thank you for your proposal, your RCA is correct. But your solution is not working on native app.
https://github.com/user-attachments/assets/f5f33928-4811-427c-89cc-35f7431614ff
@FitseTLT Thank you for your proposal, your RCA is correct. But your solution is not working on native app.
Simulator.Screen.Recording.-.iPhone.15.Pro.Max.-.2024-09-03.at.17.57.06.mp4
It's because Same change should be applied on .native files too that's why it is not working for native I have added comment on that @alitoshmatov
@FitseTLT I think I added all needed changes I even checkout out to your provided branch before applying changes. Maybe I missed something, can you make sure changes are working in you ios native app and if yes provide me with detailed change for native too or you can update your branch
@excile1 Thank you for your proposal, your RCA is correct. Your solution looks to be working fine, the 2nd bug you specified does not seem to be present in ios app. I just applied your first solution and ios app is working fine. Let me do some research for on the second bug and other platforms
@FitseTLT I think I added all needed changes I even checkout out to your provided branch before applying changes. Maybe I missed something, can you make sure changes are working in you ios native app and if yes provide me with detailed change for native too or you can update your branch
@alitoshmatov I didn't add the changes in native files in my branch, sorry that's why I included a note on my proposal to add it but now I have updated my test branch by include the same changes for native platforms 👍 U can check.
@excile1 Thank you for your proposal, your RCA is correct. Your solution looks to be working fine, the 2nd bug you specified does not seem to be present in ios app. I just applied your first solution and ios app is working fine. Let me do some research for on the second bug and other platforms
Yep. It looks like the second bug is specific to android, it doesn't seem to be consistent with other platforms
@FitseTLT Still not working, can you share a screen recording of it working on your ios app
edit: Just checked, it is not working in android either.
@tsa321 Thank you for your proposal, Your solution is working on ios fine, but have some issues on android
https://github.com/user-attachments/assets/5c0f19f4-503b-466b-a885-78e447b412ac
@tsa321 Thank you for your proposal, Your solution is working on ios fine, but have some issues on android https://github.com/user-attachments/assets/5c0f19f4-503b-466b-a885-78e447b412ac
@alitoshmatov might be a styling issue related to flex. I will try to revise my solution and inform you again.
@alitoshmatov, this issue is not only on the header styling but also with other markdown elements, such as code, italics, bold text, etc. Therefore, @excile1's solution might not work for cases other than heading markdown.
I think a better solution would be to let react-native-live-markdown
grow itself, without relying on a hidden text component to expand the text input.
Here is the test for other markdown styles:
https://github.com/user-attachments/assets/b2dd33ae-298e-4775-b09a-c2dea3d9957d
📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸
@trjExpensify, @alitoshmatov Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!
@alitoshmatov can you revert back here, please?
@trjExpensify, @alitoshmatov 6 days overdue. This is scarier than being forced to listen to Vogon poetry!
@excile1 Have you checked your solution with other markdown styles like bold and italic, they are causing the same issue
@tsa321 Have you revised your solution to consider https://github.com/Expensify/App/issues/48281#issuecomment-2326914498 and possibly other markdown styles
📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸
@trjExpensify @alitoshmatov this issue was created 2 weeks ago. Are we close to approving a proposal? If not, what's blocking us from getting this issue assigned? Don't hesitate to create a thread in #expensify-open-source to align faster in real time. Thanks!
Please wait, I am still trying to fix the issue in android.
Sounds good! Don't hesitate to use Slack to collab with some peeps if you need to.
@alitoshmatov @trjExpensify I have tried many methods to resolve the issue, but I am unable to fix it; it is more difficult than I thought. If my approach to solving this issue is preferred, I am open to assigning it to other people or an agency that can address the issue. I think this issue is open for a better proposal.
@alitoshmatov @trjExpensify I have tried many methods to resolve the issue, but I am unable to fix it; it is more difficult than I thought. If my approach to solving this issue is preferred, I am open to assigning it to other people or an agency that can address the issue. I think this issue is open for a better proposal.
I think it could be the same problem that I mentioned in my proposal (the 2nd root cause). I can't test it right now, but if it happens to be it I'd be open to working on this together
Great, can you let us know when you test it? Thanks!
@trjExpensify Yep, I applied @tsa321's solution for the FE and the same solution that I mentioned in my proposal for @expensify/react-native-live-markdown
(commenting out https://github.com/Expensify/react-native-live-markdown/blob/037a1b0107263314ad5cdc710fa862280f65ea32/android/src/main/java/com/expensify/livemarkdown/MarkdownUtils.java#L148-L157), rebuilt the app and it seems like it is fixed? @alitoshmatov if you could test it and if both you and @tsa321 agree, we could work on this together and split the bounty. I can open an issue and/or PR in the @expensify/react-native-live-markdown
repo to fix the line-height change, and then @tsa321 can submit the FE fix.
Here's a recording:
https://github.com/user-attachments/assets/bbb45e91-6cb9-4752-9e98-b48ce5b343d7
It’s fine, @excile1. If my proposal is accepted, you can fully work on this issue using my proposed code changes.
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: 9.0.26 Reproducible in staging?: Y Reproducible in production?: Y If this was caught during regression testing, add the test name, ID and link from TestRail: https://expensify.testrail.io/index.php?/tests/view/4904629 Issue reported by: Applause Internal Team
Action Performed:
Expected Result:
On entering multiline text with header markdown, 2nd line must be visible.
Actual Result:
On entering multiline text with header markdown, 2nd line is not visible.
Workaround:
Unknown
Platforms:
Screenshots/Videos
https://github.com/user-attachments/assets/8799e4d1-a06a-4eec-a6f9-74f06bb75f3a
View all open jobs on GitHub
Upwork Automation - Do Not Edit
Issue Owner
Current Issue Owner: @alitoshmatov