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.34k stars 2.77k forks source link

[$2000] Android - Title is not rendered fully in the available space. #21219

Open kbecciv opened 1 year ago

kbecciv commented 1 year 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!


Action Performed:

  1. Open the app on Android.
  2. See the chats in the LHN.
  3. Notice a few reports will longer title.

Expected Result:

Title is clipped when reached the right edge of the screen.

Actual Result:

Title is clipped with ellipsis in the middle of screen and there is more available room.

Workaround:

Unknown

Platforms:

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

Version Number: 1.3.27-2

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

Notes/Photos/Videos: Any additional supporting documentation

Screenshot 2023-06-14 at 6 54 44 PM (1)

Screenshot_20230620_203809_New Expensify

Expensify/Expensify Issue URL:

Issue reported by: @parasharrajat

Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1686749244980549

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~01ef0ae782c96792ac
  • Upwork Job ID: 1671576465794899968
  • Last Price Increase: 2023-11-06
  • Automatic offers:
    • aimane-chnaif | Reviewer | 27596163
    • fabriziobertoglio1987 | Contributor | 27981486
gzqyl commented 10 months ago

@aimane-chnaif the root cause is from Android Native. I could reproduce the issue on Android emulator, check the follow two images: 截屏2023-10-27 11 28 27 截屏2023-10-27 11 28 59

the key to solve this issue is using the new added string util function, check the function codes follow: 截屏2023-10-27 11 34 17

As I had already stated on the another similar issue, we should prevent the unexpected break to new line on Android when there's special char in string, the special char is not only white space, but also the '+...+', sometimes if there's only one special char '+' or '.', it will not go to unexpected word break behavior, however, we could use the special unicode char \u200D, it will tell the os to join char while not break to new line.

I have specified the platform in the function, as the issue only happened on Android, so I think we specify the platform will make the regression test more reliable, and just do not make the things overwhelm.

The good news is that if my proposal selected, another issue will be also solved, just wrap the string with this new function, the issue will disappear.

aimane-chnaif commented 10 months ago

I am aware of replacement approach. Let's see if we can have better solutions.

gzqyl commented 10 months ago

ok, but from my experience, there's only two ways to solve this issue, one is changing view layout, one is this replacement. I think the replacement would be better as the layout is hard to uniform when the text component wrapped in various place.

If someone customized a Text component which could replace react native Text component and fixes the bug with changing android native codes, it is sure better.

Though I had reached at native codes, I found it is a little hard to fix by native codes, so my proposal kept as the unicode solution, it is a trick to fix the root cause, as the unicode let the os know how to do when there's special chars, if without the unicode, the os will break to new line when deal with special chars, and the numberOfLines = 1 will just keep the first line, so we see the result 'aim...' as after the 'aiman' there is a special char '.', the first line width finally calculated as 'aiman' size, then ellipsis the whole string with that size, so the result is 'aim...'.

ospfranco commented 10 months ago

Hi, I'm from Margelo an external agency hired by Expensify and I've been tasked with taking a look into this. I read through the proposals. I was just wondering why is the flexGrow solution, not the accepted one? I think I've dealt with this in the past with the same solution.

Here is a screenshot of the change working on both iOS and Android:

Oscar Franco Screen 000270

@aimane-chnaif @pradeepmdk ?

aimane-chnaif commented 10 months ago

@ospfranco thanks for jumping in here. Please test in #focus mode. Also we recently added status badge.

ospfranco commented 10 months ago

Is this how it is supposed to look like?

aimane-chnaif commented 10 months ago

@ospfranco this time, test with short email or display name. And then last message should show next to user.

gzqyl commented 10 months ago

@ospfranco this issue truly has two ways to get solved, one is layout change, just you are wondering, and another is my proposal using unicode escape string.

At first, we should know the issue only happened on Android, if we accept the layout solution, when the same root cause issue happened on other place, like https://github.com/Expensify/App/issues/24339, we need to change the layout again, and this layout change is not very easy to find the right way to do.

From another angle, if we change the layout, though we solved the issue, we indeed make a overwhelm work, and hide the root cause.

I think the unicode solution does get it solved from root cause place, and if we use the unicode solution, we do not need to change the layout, here and another issue, we just need to escape the string with special unicode, it will work on both Android and iOS.

In the future, we could just use the new added unicode function to handle with this one line issue on Android.

ospfranco commented 10 months ago

@gzqyl No, I don't like that, messing with unicode characters is brittle.

The root cause is flex. Usually when there is this type of error is because the flex containers are not correctly implemented. The flex specification is a weird beast, but basically, one of the parent containers does not have a correct width (via flex itself or an actual width value) bound to it, then the flex calculation for children is messed up. Here is just a quick test, by applying a fixed width to the top container and then subsequently flexing 1 to the children, the children are correctly laid out.

Oscar Franco Screen 000273

I will create a proper PR in a bit.

gzqyl commented 10 months ago

@ospfranco how will you explain why the issue does not happen on the other platforms?

The layout only has issues on Android, of course, the layout itself is a little complex, we do not know why and when the same root cause issue will happen by what layout we used.

If we use the layout solution to solve this issue, we need to use another layout changes on another issue, in the future, when the same issue happen again, we will need to find the new layout changes to solve the new issue.

However, all of these issues, we could just use the unicode function to solve.

The unicode solution does solve this issue at root cause place, so we do not need to struggle with the complex layout issue, and the unicode solution only changes the Android, so the regression test will be much easier.

I think the unicode solution will be helpful when the next time same issue happened, the developer could just wrap string using the function while not struggle to find the new right way to layout view.

pradeepmdk commented 10 months ago

@ospfranco I agree flexGrow will be work as a said in proposal https://github.com/Expensify/App/issues/21219#issuecomment-1676364888 and

The root cause is flex. Usually when there is this type of error is because the flex containers are not correctly implemented. The flex specification is a weird beast, but basically, one of the parent containers does not have a correct width bound to it, then the flex (1) calculation is messed up. Here is just a quick test, by applying a fixed width to the top container and then subsequently flexing 1 to the children, the children are correctly laid out.

we don't need to fixed width with whenever parent component have flex row Text component will wrap the text middle due to textbreakstrategy https://github.com/Expensify/App/issues/21219#issuecomment-1721022715 so at level we need to set flex1 in the text component

you can try this code in new project

 <SafeAreaView style={{ flex: 1, backgroundColor: 'black' }}>
                <View style={{ flexDirection: 'row', flex:1 }}>
                    <Text numberOfLines={1} style={{ }} >From vincenzoddragon+five@gmail.com From vincenzoddragon+five@gmail.com  From vincenzoddragon+five@gmail.com  </Text>
                </View>
        </SafeAreaView>
gzqyl commented 10 months ago

the layout solution just avoid the root cause to appear, the root cause is still there.

the problem is not using the right layout, but the same layout works on the platforms, it is only not working on the Android, so if the similar issue happened next time, we will need to find a good layout to avoid the root cause again.

More the right layout maybe need us to try and try, it will waste of our time.

Why not just unicode function? Let's make the things easier, and get rid of the layout issue which only happened on Android.

Bypass the real root cause is not a good idea.

aimane-chnaif commented 10 months ago

I don't 100% confirm that unicode approach won't cause any regressions.

ospfranco commented 10 months ago

@pradeepmdk it was just a test. Applying flex: 1 to the parent container will fix it. In your proposal, if I'm not mistaken, you did not mention applying it to the parent, but only to the children. Maybe I'm understanding it wrong.

@gzqyl it is actually your solution that is not addressing the root issue. The root issue is that the Views are not correctly laid out and have calculated widths so that the OS can layout the elements properly. Why it happens on Android and not on iOS it's just an implementation detail. Converting unicode characters is slow and brittle, it will have to be applied everywhere.

Neither of these solutions will prevent it from happening in the future, but laying out the Views correctly is necessary anyways.

aimane-chnaif commented 10 months ago

If this is native android behavior, then this is not "bug" and no one is willing to fix this. So we're fine here to apply "workaround" of adjusting layout, given that there's no perfect solution. Btw, adding flex:1 doesn't simply solve the issue as it will cause regression when status badge shows, along with #focus mode. We should add some more conditional styles.

gzqyl commented 10 months ago

@aimane-chnaif first, we only need to test Android, as other platforms we indeed kept it as before, so I think the regression test should not be hard to confirm.

When the parent component does not explicit width size, the text view one line will have this issue on Android, as the native behavior will calculate the wrong width for the one line available width when there's special chars like space or '++...'.

I think the unicode function should be reliable as its special feature for the unicode meaning, the os does respect the unicode meaning, and make sure the text will be one line while not break at the space or '+..+' chars, with this function, the Android will be same as other platforms, so the issue will disappear automatically then.

I tested it at various situation, for now, I have not found regression issue.

gzqyl commented 10 months ago

@ospfranco the function does not need to be applied everywhere, just when such issue appears, we use this function to fix it is ok.

If no issue happened, we do not need to use this function.

pradeepmdk commented 10 months ago

@ospfranco

it was just a test. Applying flex: 1 to the parent container will fix it. In your proposal, if I'm not mistaken, you did not mention applying it to the parent, but only to the children. Maybe I'm understanding it wrong.

<SafeAreaView style={{ flex: 1, backgroundColor: 'black' }}>
<View style={{ flexDirection: 'row', flex:1 }}>
<Text numberOfLines={1} style={{ }} >From vincenzoddragon+five@gmail.com From vincenzoddragon+five@gmail.com  From vincenzoddragon+five@gmail.com  </Text>
</View>
</SafeAreaView>

In this case won't work As I said when parent component have flexrow, textcomponent won't Take flex1...here we need apply force flex1 in text component

ospfranco commented 10 months ago

@aimane-chnaif, take a look:

https://github.com/Expensify/App/compare/main...margelo:expensify-app-fork:osp/fix-title-not-displayed

Here is how it behaves.

Mobiles:

Web:

wildan-m commented 10 months ago

@ospfranco We have a similar on-hold issue https://github.com/Expensify/App/issues/24339 that couldn't be solved by stretching the container, otherwise it will cause this regression https://github.com/Expensify/App/issues/21215.

aimane-chnaif commented 10 months ago

@ospfranco We have a similar on-hold issue #24339 that couldn't be solved by stretching the container, otherwise it will cause this regression #21215.

Nope, this isn't related to #21215. We can fix #24339 separately. I was also expecting css only solution with adjusting various flex properties, unless fixable in native.

melvin-bot[bot] commented 10 months ago

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

ospfranco commented 10 months ago

Just dumping some info in here for future reference.

Here is where text width is measured and layouted basically works by creating a StaticLayout which takes care of measuring text to be drawn on the screen. This StaticLayout takes all the parameters necessary to calculate the position and width of the text, including the break strategy. After the base text is laid out, react-native then does some checks regarding how Yoga has laid out the container views for the text and then tries to fit the text as best as possible.

The issue is in this specific line, where RN asks the StaticLayout to give the width of the line (layoutWidth is the final width that will be rendered, width is the yoga calculated value, lineWidth is the width calculated by Android/StaticLayout). This internally already applies the ellipsis based on the selected text strategy. You can see on the screenshot which values are returned. RN/Yoga has calculated an ideal width of 250, but StaticLayout returns 63 (because it has already cut "aiman.chnaif+12345678@gmail.com" to "Am..."). Basically, the break strategy dictates what width this will take. The Unicode solution attacks the problem at this point, by messing with the ellipsis calculation, it forces it to break the text differently.

By refactoring the layout and giving the children flex/width, we force the other code path, the widthMode is turned to EXACT and then the text takes the available space that RN/Yoga has calculated.

From what I see it is not possible to change the text break strategy on the StaticLayout itself.

I don't think the current behavior is a bug, however, I've been moved to a different task. @janicduplessis will take over, or I might return to this once I have time.

Oscar Franco Screen 000291

melvin-bot[bot] commented 10 months ago

@adelekennedy, @aimane-chnaif Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

aimane-chnaif commented 10 months ago

@ospfranco I tested your branch. Title is truncated in this case: #focus mode, short title, long description

Before:

Screenshot 2023-11-05 at 10 26 27 AM

After:

Screenshot 2023-11-05 at 10 26 14 AM
melvin-bot[bot] commented 10 months ago

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

aimane-chnaif commented 10 months ago

Just wanna confirm who is currently working on this cc: @ospfranco @janicduplessis

ospfranco commented 10 months ago

Not working on this. Janic might get to it at some point.

wildan-m commented 10 months ago

Proposal

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

Option Row Title is not rendered fully in the available space

What is the root cause of that problem?

I agree with this explanation, the issue might be related to how to react native calculate the text width for some specific text and layout.

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

The root cause might not treated as a bug on RN side, so my solution is another workaround.

My workaround also involves Unicode characters, zero-width space. We can insert zero-width space between characters to make the words breakable at any place

    const insertZeroWidthSpace = (text) => {
        const result = [];
        const zeroWidthSpace = '\u200B';

        for (let i = 0; i < text.length; i++) {
            result.push(text[i]);
            if (i < text.length - 1) {
                result.push(zeroWidthSpace);
            }
        }

        return result.join('');
    };

implement it in DisplayName

                                    <DisplayNames
                                            accessibilityLabel={translate('accessibilityHints.chatUserDisplayNames')}
                                            fullTitle={insertZeroWidthSpace(fullTitle)}
                                            displayNamesWithTooltips={optionItem.displayNamesWithTooltips}
   ....

we can also add isSmallScreen check to prevent unexpected behavior of browser/desktop version

What alternative solutions did you explore? (Optional)

ospfranco commented 10 months ago

@aimane-chnaif I've updated the branch, and here are the results I got:

Only with small titles the text is shoved to the right. But with larger titles/emails, it is displayed better.

This is as good as it is going to get without using Unicode characters. If still not good enough then the only possible fix would be to write a custom Layout interpolator for Android, which I assume will never be merged to RN because of compatibility reasons (also more complex and easy to break and will need to be maintained separately from upstream, from looking at the code of StaticLayout)

aimane-chnaif commented 10 months ago

I think we can go with that css solution as it's almost impossible to fix in RN as it's not actually "bug". @ospfranco can you create formal proposal with all discussions summarized? So I can approve and then assigned engineer can get full context without reading 130 comments.

aimane-chnaif commented 10 months ago

Also, please consider that there's user status icon (which is in beta for now) next to title.

ospfranco commented 10 months ago

Proposal

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

On Android Option Row Title is not rendered fully in the available space

What is the root cause of that problem?

Here is where text width is measured and layouted basically works by creating a StaticLayout which takes care of measuring text to be drawn on the screen. This StaticLayout takes all the parameters necessary to calculate the position and width of the text, including the text break strategy. After the base text is laid out, react-native then does some checks regarding how Yoga has laid out the container views for the text and then tries to fit the text as best as possible.

The issue is in this specific line, where RN asks the StaticLayout to give the width of the line (layoutWidth is the final width that will be rendered, width is the yoga calculated value, lineWidth is the width calculated by Android/StaticLayout). This internally already applies the break and ellipsis based on the selected text strategy. You can see on the screenshot which values are returned. RN/Yoga has calculated an ideal width of 250, but StaticLayout returns 63 (because it has already cut aiman.chnaif+12345678@gmail.com to Am...). Basically, the break strategy dictates what width this will take.

Unicode solution

Some of the proposed solutions suggested changing some of the characters to Unicode white-space equivalents, these characters attack the problem at this point, by messing with the ellipsis calculation and forcing StaticLayout to break the text differently. Although this solution works, it is somewhat brittle since it relies on replacing certain characters which might need tweaking in the future.

Custom Android StaticLayout

It is not possible to change the text-breaking strategy on StaticLayout itself. The only possible solution at this level would be creating a custom Layout interpolator that allows for a different text-breaking strategy. This is however brittle as Android might change the internal implementation of this class and it will have to be maintained separately. It is also unlikely to be merged into React-Native upstream as it might cause a regression.

Layout solution

The other solution By refactoring the layout and giving the children flex/widths, we force the other code path, the widthMode is turned to EXACT and then the text takes the available space that RN/Yoga has calculated.

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

The proposed solution is implemented here. Basically it modifies the layout so that the title can take up to 80% of the available space, causing the text break to happen at that point and not earlier. One side effect is that with smaller titles the subtitle will be pushed to the right (when in focus, read: compact mode). Here are the results of this change:

@aimane-chnaif I can open a PR with the changes, just let me know.

aimane-chnaif commented 10 months ago

I suggest to go with @ospfranco's solution

🎀 👀 🎀 C+ reviewed

melvin-bot[bot] commented 10 months ago

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

rlinoz commented 10 months ago

I like the suggested solution, but I think the message going to the same line as the chat title can be considered a bug, and we shouldn't implement it that way.

Although I tried @ospfranco's solution with the new LHN and I didn't get this behavior, do you want to update your branch and see if it is still happening for you?

aimane-chnaif commented 10 months ago

I like the suggested solution, but I think the message going to the same line as the chat title can be considered a bug, and we shouldn't implement it that way.

Huh, please go to Settings > Preferences > Priority mode > #focus

rlinoz commented 10 months ago

Oh I am sorry, I got that wrong, you're totally right.

I agree, let's go with @ospfranco's solution, hired!

Thanks @aimane-chnaif

melvin-bot[bot] commented 10 months ago

📣 @aimane-chnaif 🎉 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

melvin-bot[bot] commented 10 months ago

📣 @parasharrajat Please request via NewDot manual requests for the Reporter role ($50)

ospfranco commented 10 months ago

I'm a third-party contractor, I don't require payment. @mountiny

mountiny commented 10 months ago

Oscar is from Margelo, payment is handled outside of Upwork

rlinoz commented 10 months ago

Hey @ospfranco if you could share a rough ETA of the PR :)

ospfranco commented 10 months ago

Ah, sorry, I was waiting for confirmation. One sec.

ospfranco commented 10 months ago

Here it is https://github.com/Expensify/App/pull/31261

rlinoz commented 10 months ago

Apparently from the PR we will have to go with the unicode solution, and since @ospfranco is not going to work on this anymore I added the Help Wanted label again.

@aimane-chnaif would you mind selecting a new proposal, please?

aimane-chnaif commented 10 months ago

I still have to verify that unicode solution works perfect in all possible cases. Will update asap

gzqyl commented 10 months ago

@aimane-chnaif the root cause is from Android Native. I could reproduce the issue on Android emulator, check the follow two images: 截屏2023-10-27 11 28 27 截屏2023-10-27 11 28 59

the key to solve this issue is using the new added string util function, check the function codes follow: 截屏2023-10-27 11 34 17

As I had already stated on the another similar issue, we should prevent the unexpected break to new line on Android when there's special char in string, the special char is not only white space, but also the '+...+', sometimes if there's only one special char '+' or '.', it will not go to unexpected word break behavior, however, we could use the special unicode char \u200D, it will tell the os to join char while not break to new line.

I have specified the platform in the function, as the issue only happened on Android, so I think we specify the platform will make the regression test more reliable, and just do not make the things overwhelm.

The good news is that if my proposal selected, another issue will be also solved, just wrap the string with this new function, the issue will disappear.

@aimane-chnaif If there's any issues on my function, kindly let me know, I would be gladly to revise the function though I think it is already nearly perfect in most cases.

aimane-chnaif commented 10 months ago

yes, kindly share your branches (merged from latest main) so I can test thoroughly.

gzqyl commented 10 months ago

okay, I will make a PR in later.