Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[$250] Android - Room -On entering multiline text with header markdown, 2nd line is not visible #48281

Open
6 tasks done
IuliiaHerets opened this issue Aug 29, 2024 · 57 comments
Assignees
Labels
Bug Something is broken. Auto assigns a BugZero manager. External Added to denote the issue can be worked on by a contributor Reviewing Has a PR in review Weekly KSv2

Comments

@IuliiaHerets
Copy link

IuliiaHerets commented Aug 29, 2024

If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] 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:

  1. Launch app
  2. Tap on a room chat
  3. Tap header -- description
  4. Enter a multiline text with header markdown

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:

  • Android: Native
  • Android: mWeb Chrome
  • iOS: Native
  • iOS: mWeb Safari
  • MacOS: Chrome / Safari
  • MacOS: Desktop

Screenshots/Videos

Bug6586126_1724906639755.Screenrecorder-2024-08-29-10-08-19-950_compress_1.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~0166150593d62a36c2
  • Upwork Job ID: 1829289632232865933
  • Last Price Increase: 2024-09-19
  • Automatic offers:
    • alitoshmatov | Reviewer | 104139938
    • tsa321 | Contributor | 104139940
Issue OwnerCurrent Issue Owner: @tsa321
@IuliiaHerets IuliiaHerets added Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Aug 29, 2024
Copy link

melvin-bot bot commented Aug 29, 2024

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.

@IuliiaHerets
Copy link
Author

@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

@trjExpensify
Copy link
Contributor

Reproducible:
image

@trjExpensify trjExpensify added the External Added to denote the issue can be worked on by a contributor label Aug 29, 2024
@melvin-bot melvin-bot bot changed the title Android - Room -On entering multiline text with header markdown, 2nd line is not visible [$250] Android - Room -On entering multiline text with header markdown, 2nd line is not visible Aug 29, 2024
Copy link

melvin-bot bot commented Aug 29, 2024

Job added to Upwork: https://www.upwork.com/jobs/~0166150593d62a36c2

@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Aug 29, 2024
Copy link

melvin-bot bot commented Aug 29, 2024

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

@jp928
Copy link
Contributor

jp928 commented Aug 30, 2024

Proposal

Please re-state the problem that we are trying to solve in this issue.

The room description on Android is cut off when typein multiple lines.

What is the root cause of that problem?

height: maxHeight - styles.textInputMultilineContainer.paddingTop - styles.textInputContainer.borderBottomWidth,

function getAutoGrowHeightInputStyle doesn't workout a correct height. Consider to use maxHeight: '100%' instead, because we have clamp height on the container element.

autoGrowHeightInputContainer: (textInputHeight: number, minHeight: number, maxHeight: number) =>

What changes do you think we should make in order to solve the problem?

function getAutoGrowHeightInputStyle doesn't workout a correct height. Consider to use maxHeight: '100%' instead, because we have clamp height on the container element.

What alternative solutions did you explore? (Optional)

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.

@FitseTLT
Copy link
Contributor

FitseTLT commented Aug 30, 2024

Edited by proposal-police: This proposal was edited at 2023-10-01T00:00:00Z.

Proposal

Please re-state the problem that we are trying to solve in this issue.

Room -On entering multiline text with header markdown, 2nd line is not visible

What is the root cause of that problem?

To fix scroll bar flickering issue on line breaks we are the text input height here

// Stop scrollbar flashing when breaking lines with autoGrowHeight enabled.
...(autoGrowHeight
? [StyleUtils.getAutoGrowHeightInputStyle(textInputHeight, typeof maxAutoGrowHeight === 'number' ? maxAutoGrowHeight : 0), styles.verticalAlignTop]
: []),

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
autoGrowHeight && styles.autoGrowHeightInputContainer(textInputHeight, variables.componentSizeLarge, typeof maxAutoGrowHeight === 'number' ? maxAutoGrowHeight : 0),

this textInputHeight is determined by the layout of an invisible Text here
setTextInputHeight(e.nativeEvent.layout.height);

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.

What changes do you think we should make in order to solve the problem?

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

App/src/styles/utils/index.ts

Lines 1229 to 1242 in e66b123

getAutoGrowHeightInputStyle: (textInputHeight: number, maxHeight: number): 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
height: maxHeight - styles.textInputMultilineContainer.paddingTop - styles.textInputContainer.borderBottomWidth,

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}),
        };

App/src/styles/index.ts

Lines 1158 to 1162 in e66b123

autoGrowHeightInputContainer: (textInputHeight: number, minHeight: number, maxHeight: number) =>
({
height: lodashClamp(textInputHeight, minHeight, maxHeight),
minHeight,
} satisfies ViewStyle),

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),

autoGrowHeight &&
                            styles.autoGrowHeightInputContainer(
                                textInputHeight,
                                variables.componentSizeLarge,
                                typeof maxAutoGrowHeight === 'number' ? maxAutoGrowHeight : 0,
                                !isMarkdownEnabled,
                            ),

? [StyleUtils.getAutoGrowHeightInputStyle(textInputHeight, typeof maxAutoGrowHeight === 'number' ? maxAutoGrowHeight : 0), styles.verticalAlignTop]

 ? [
                                              StyleUtils.getAutoGrowHeightInputStyle(textInputHeight, typeof maxAutoGrowHeight === 'number' ? maxAutoGrowHeight : 0, !isMarkdownEnabled),
                                              styles.verticalAlignTop,
                                          ]

and apply same for native files too
Here is a test branch

What alternative solutions did you explore? (Optional)

@excile1
Copy link

excile1 commented Aug 30, 2024

Proposal

Please re-state the problem that we are trying to solve in this issue.

The problem is that multi-line text is being cut off when you are using the heading (#) markdown.

What is the root cause of that problem?

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.
    {/*
    Text input component doesn't support auto grow by default.
    We're using a hidden text input to achieve that.
    This text view is used to calculate width or height of the input value given textStyle in this component.
    This Text component is intentionally positioned out of the screen.
    */}
    {(!!autoGrow || autoGrowHeight) && (
    // Add +2 to width on Safari browsers so that text is not cut off due to the cursor or when changing the value
    // Reference: https://github.com/Expensify/App/issues/8158, https://github.com/Expensify/App/issues/26628
    // For mobile Chrome, ensure proper display of the text selection handle (blue bubble down).
    // Reference: https://github.com/Expensify/App/issues/34921
    <Text
    style={[
    inputStyle,
    autoGrowHeight && styles.autoGrowHeightHiddenInput(width ?? 0, typeof maxAutoGrowHeight === 'number' ? maxAutoGrowHeight : undefined),
    styles.hiddenElementOutsideOfWindow,
    styles.visibilityHidden,
    ]}
    onLayout={(e) => {
    if (e.nativeEvent.layout.width === 0 && e.nativeEvent.layout.height === 0) {
    return;
    }
    let additionalWidth = 0;
    if (Browser.isMobileSafari() || Browser.isSafari() || Browser.isMobileChrome()) {
    additionalWidth = 2;
    }
    setTextInputWidth(e.nativeEvent.layout.width + additionalWidth);
    setTextInputHeight(e.nativeEvent.layout.height);
    }}
    >
    {/* \u200B added to solve the issue of not expanding the text input enough when the value ends with '\n' (https://github.com/Expensify/App/issues/21271) */}
    {value ? `${value}${value.endsWith('\n') ? '\u200B' : ''}` : placeholder}
    </Text>
    )}
    </>
    );

    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:
rec1.mp4
  1. 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:
rec2.mp4

What changes do you think we should make in order to solve the problem?

  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)

Screenshot 2024-08-30 at 22 16 01

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>;
})}
  1. 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:
    image
    (custom decription)
    which can be fixed here:
    explainerText: 'Set a custom decription for the room.',

What alternative solutions did you explore? (Optional)

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:

App/src/styles/index.ts

Lines 1213 to 1222 in 76f16bc

baseTextInput: {
...FontUtils.fontFamily.platform.EXP_NEUE,
fontSize: variables.fontSizeNormal,
lineHeight: variables.lineHeightXLarge,
color: theme.text,
paddingTop: 23,
paddingBottom: 8,
paddingLeft: 0,
borderWidth: 0,
},

(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)

@tsa321
Copy link
Contributor

tsa321 commented Sep 1, 2024

Proposal

Please re-state the problem that we are trying to solve in this issue.

Auto grow height of markdown text input doesn't grow properly, Like in room description text input.

What is the root cause of that problem?

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:

{(!!autoGrow || autoGrowHeight) && (
// Add +2 to width on Safari browsers so that text is not cut off due to the cursor or when changing the value
// Reference: https://github.com/Expensify/App/issues/8158, https://github.com/Expensify/App/issues/26628
// For mobile Chrome, ensure proper display of the text selection handle (blue bubble down).
// Reference: https://github.com/Expensify/App/issues/34921
<Text
style={[
inputStyle,
autoGrowHeight && styles.autoGrowHeightHiddenInput(width ?? 0, typeof maxAutoGrowHeight === 'number' ? maxAutoGrowHeight : undefined),
styles.hiddenElementOutsideOfWindow,
styles.visibilityHidden,
]}
onLayout={(e) => {
if (e.nativeEvent.layout.width === 0 && e.nativeEvent.layout.height === 0) {
return;
}
let additionalWidth = 0;
if (Browser.isMobileSafari() || Browser.isSafari() || Browser.isMobileChrome()) {
additionalWidth = 2;
}
setTextInputWidth(e.nativeEvent.layout.width + additionalWidth);
setTextInputHeight(e.nativeEvent.layout.height);
}}

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.

What changes do you think we should make in order to solve the problem?

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 :

autoGrowHeight && styles.autoGrowHeightInputContainer(textInputHeight, variables.componentSizeLarge, typeof maxAutoGrowHeight === 'number' ? maxAutoGrowHeight : 0),

We change it to:

(autoGrowHeight && !isAutoGrowHeightMarkdown && styles.autoGrowHeightInputContainer(textInputHeight, variables.componentSizeLarge, typeof maxAutoGrowHeight === 'number' ? maxAutoGrowHeight : 0))

here:

<View style={[styles.textInputAndIconContainer, isMultiline && hasLabel && styles.textInputMultilineContainer, styles.pointerEventsBoxNone]}>

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:

{(!!autoGrow || autoGrowHeight) && (

change it to: {(!!autoGrow || autoGrowHeight) && !isAutoGrowHeightMarkdown && (

Then do similarly in baseTextinput.native.tsx
For code changes reference I have made a test branch.

What alternative solutions did you explore? (Optional)

@melvin-bot melvin-bot bot added the Overdue label Sep 1, 2024
Copy link

melvin-bot bot commented Sep 2, 2024

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

@trjExpensify
Copy link
Contributor

@alitoshmatov can you give these proposals a review, please?

@alitoshmatov
Copy link
Contributor

@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.

@melvin-bot melvin-bot bot removed the Overdue label Sep 3, 2024
@alitoshmatov
Copy link
Contributor

@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

@FitseTLT
Copy link
Contributor

FitseTLT commented Sep 3, 2024

@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

@alitoshmatov
Copy link
Contributor

@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
Copy link
Contributor

@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
Copy link
Contributor

FitseTLT commented Sep 3, 2024

@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
Copy link

excile1 commented Sep 3, 2024

@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

@alitoshmatov
Copy link
Contributor

alitoshmatov commented Sep 3, 2024

@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.

@alitoshmatov
Copy link
Contributor

alitoshmatov commented Sep 3, 2024

@tsa321 Thank you for your proposal, Your solution is working on ios fine, but have some issues on android

Screen.Recording.2024-09-03.at.9.04.41.PM.mov

@tsa321
Copy link
Contributor

tsa321 commented Sep 3, 2024

@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.

@melvin-bot melvin-bot bot added the Overdue label Sep 13, 2024
@tsa321
Copy link
Contributor

tsa321 commented Sep 15, 2024

It’s fine, @excile1. If my proposal is accepted, you can fully work on this issue using my proposed code changes.

Copy link

melvin-bot bot commented Sep 16, 2024

@trjExpensify, @alitoshmatov Eep! 4 days overdue now. Issues have feelings too...

@trjExpensify
Copy link
Contributor

@alitoshmatov let us know your thoughts on next steps. 👍

@alitoshmatov
Copy link
Contributor

#48281 (comment)

Looks like a good plan. Based on this we can continue with @excile1.

Though I think we get third opinion for solution with react-native-live-markdown.

@melvin-bot melvin-bot bot removed the Overdue label Sep 17, 2024
@excile1
Copy link

excile1 commented Sep 17, 2024

#48281 (comment)

Looks like a good plan. Based on this we can continue with @excile1.

Though I think we get third opinion for solution with react-native-live-markdown.

I agree, I was planning to open a separate issue in the @expensify/react-native-live-markdown repo to get someone to look at it and find out how exactly we should address this

@alitoshmatov
Copy link
Contributor

@tomekzaw Looks like you have worked on react-native-live-markdown's part that concerns us and also maintaining this package. So, would be great if we could get your opinion on this one

TLDR: The problem is in android where you can see little jump when heading markdown is written in the input.

Screen.Recording.2024-09-03.at.9.04.41.PM.mov

The solution proposed is:

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.

https://github.com/Expensify/react-native-live-markdown/blob/037a1b0107263314ad5cdc710fa862280f65ea32/android/src/main/java/com/expensify/livemarkdown/MarkdownUtils.java#L148-L157

@alitoshmatov
Copy link
Contributor

alitoshmatov commented Sep 17, 2024

@excile1 I just noticed in the video you provided, jittering effect is still there when the line is full and goes to the next line

That was the main issue for me with @tsa321 's solution

@tomekzaw
Copy link
Contributor

tomekzaw commented Sep 17, 2024

Hi, author of react-native-live-markdown here. We're aware of this issue and we're working on a fix.

This problem appears when both react-native-live-markdown and react-native-reanimated are used. That's because each of these libraries has its own commit hook. It looks like RNLiveMarkdown's commit hook is registered first and RNReanimated's commit hook is registered as a second. Note that this problem would most likely not happen if the order of commit hooks was different (however we cannot rely on that and the solution needs to be order-agnostic).

In RNLiveMarkdown there's a MarkdownCommitHook that calls AndroidTextInputShadowNode::setTextLayoutManager method to change the algorithm used for calculating MarkdownTextInput's height so that it reflects Markdown formatting. Unfortunately, this is information is effectively lost because in next step, this AndroidTextInputShadowNode is cloned again by ReanimatedCommitHook. When shadow node is cloned, AndroidTextInputComponentDescriptor::adopt method is cloned which also calls setTextLayoutManager with the original text layout manager that doesn't take into account Markdown formatting.

At this point we're still not sure why AndroidTextInputShadowNode is cloned again as a result of ReanimatedCommitHook call even if there are no running animations. We're currently investigating that. I also confirmed that this is indeed the root cause of the issue because the height is correct when I comment out ReanimatedCommitHook.

TL;DR This issue needs to be fixed in C++ codebase.

As for little jump, this is actually a problem with React Native's TextInput. This happens for regular TextInputs as well. That's because the text is edited immediately on the UI thread but input height needs to be calculcated by Yoga and it happens asynchronously which causes the delay. So basically the contents is updated quicker than the height, hence the jump.

edit: see #48281 (comment)

@j-piasecki
Copy link
Contributor

Hey! I was helping @tomekzaw investigate the issue and what he said in the above message turned out not to be entirely true. The conflict between commit hooks from react-native-reanimated and react-native-live-markdown isn't impacting the App (it's dependent on the ordering of imports, if Live Markdown is imported after Reanimated, the problem doesn't occur) but visually it produces really similar results which is what led us off.

As was already stated in multiple proposals above, the root of the issue here is autoGrow implementation which uses Text components to calculate the height of the TextInput. Since it doesn't use any markdown styling, it returns wrong values when markdown is used.

Regarding the issues from the video, the little jump when creating a heading:

Android iOS
Screen.Recording.2024-09-19.at.13.09.41.mov
Screen.Recording.2024-09-19.at.13.07.55.mov

is expected - until a letter is written after # it's not considered a heading so typing anything here results in changing styles for that paragraph. Since the font size for headings is different from the normal text on Android it also changes the height. One thing that could be improved here is updating the parser so that a space after # is enough for it to be considered a heading.

The other jump

Screen.Recording.2024-09-19.at.13.30.08.mov

as @tomekzaw explained above is coming from React Native itself. It's also happening on pure TextInput components and is caused by the layout being calculated on a different thread than the text is being updated.

I'd argue that both of these are outside the scope of this issue.

@tomekzaw
Copy link
Contributor

As for the other jump mentioned by @j-piasecki, here's a video recording that compares MarkdownTextInput and RN's TextInput side-by-side: Expensify/react-native-live-markdown#36 (comment)

Copy link

melvin-bot bot commented Sep 19, 2024

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

@alitoshmatov
Copy link
Contributor

@tomekzaw @j-piasecki Thank you both for these helpful insights.

@alitoshmatov
Copy link
Contributor

So I think we can conclude that this issue is outside of our scope and we should go on with our existing solution to fix height calculation problem. @tsa321 Since we are only using your solution, will you be able to work on this yourself?

@tsa321
Copy link
Contributor

tsa321 commented Sep 23, 2024

@alitoshmatov yes, if my proposal gets accepted, I can work on this.

@alitoshmatov
Copy link
Contributor

We can go with @tsa321 's proposal

C+ reviewed 🎀 👀 🎀

The issue in android where input height changes and jittery jump is known react-native-live-markdown issues

Copy link

melvin-bot bot commented Sep 25, 2024

Triggered auto assignment to @justinpersaud, see https://stackoverflow.com/c/expensify/questions/7972 for more details.

@justinpersaud
Copy link
Contributor

Sounds good to me!

@melvin-bot melvin-bot bot removed the Help Wanted Apply this label when an issue is open to proposals by contributors label Sep 26, 2024
Copy link

melvin-bot bot commented Sep 26, 2024

📣 @alitoshmatov 🎉 An offer has been automatically sent to your Upwork account for the Reviewer role 🎉 Thanks for contributing to the Expensify app!

Offer link
Upwork job

Copy link

melvin-bot bot commented Sep 26, 2024

📣 @tsa321 🎉 An offer has been automatically sent to your Upwork account for the Contributor role 🎉 Thanks for contributing to the Expensify app!

Offer link
Upwork job
Please accept the offer and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻
Keep in mind: Code of Conduct | Contributing 📖

Copy link

melvin-bot bot commented Sep 26, 2024

@justinpersaud @trjExpensify @alitoshmatov @tsa321 this issue is now 4 weeks old, please consider:

  • Finding a contributor to fix the bug
  • Closing the issue if BZ has been unable to add the issue to a VIP or Wave project
  • If you have any questions, don't hesitate to start a discussion in #expensify-open-source

Thanks!

@melvin-bot melvin-bot bot added Reviewing Has a PR in review Weekly KSv2 and removed Daily KSv2 labels Sep 29, 2024
@tsa321
Copy link
Contributor

tsa321 commented Sep 29, 2024

@alitoshmatov PR is ready

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something is broken. Auto assigns a BugZero manager. External Added to denote the issue can be worked on by a contributor Reviewing Has a PR in review Weekly KSv2
Projects
Status: No status
Development

No branches or pull requests

10 participants