Bolded and Italics code does not render properly on mobile #2491

NicMendonca · 2021-04-20T15:20:18Z

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!

Expected Result:

Bold and italics are applied to the code block

Actual Result:

Bold and italics are not applied to the code block

Action Performed:

Send the following message on mobile

*`test`*
_`test`_

See how italics and bold are not applied to the code section
View the message on desktop / web
See how italics and bold are applied

Workaround:

Can the user still use Expensify without this being fixed? Have you informed them of the workaround?

Platform:

Where is this issue occurring?

Mobile, reproduced on both -
iOS
Android

Version Number: 1.0.2-74
Notes/Photos/Videos:

Bold / Strikethrough on mobile

Bold/ Strikethrough on desktop

Video: https://recordit.co/QtKls2EXDi

Expensify/Expensify Issue URL: https://github.com/Expensify/Expensify/issues/158825

View all open jobs on Upwork

Upwork job: https://www.upwork.com/jobs/~016d9851436c63f135

The text was updated successfully, but these errors were encountered:

parasharrajat · 2021-04-20T18:30:47Z

~~@NicMendonca This is intended behavior and defined in the code.~~

Sorry. Misunderstood it. Please ignore the above.

aliabbasmalik8 · 2021-04-21T02:31:11Z

@NicMendonca We can fix this by updating RenderHTML component like this. tested on IOS and WEB and it's working accordingly

and here is classesStyles that placed in webViewStyles object.

classesStyles: {
        em: {
            fontFamily: fontFamily.GTA_ITALIC,
            fontStyle: italic,
        },

        strong: {
            fontFamily: fontFamily.GTA_BOLD,
            fontWeight: fontWeightBold,
        },

    },

0xthierry · 2021-04-21T17:14:47Z

@NicMendonca

I spend a time debugging here and I found that in android we are not using the correct values.

When text must be italic, it is changed to normal.
When text must be 700 (bold), it is changed 600 (semibold).

That is why it is not showing correctly, you could see it below.

https://github.com/Expensify/Expensify.cash/blob/9efcc63e4eb9dd27fac35d7bfc92b3400554be0c/src/styles/fontWeight/bold/index.android.js#L3
https://github.com/Expensify/Expensify.cash/blob/9efcc63e4eb9dd27fac35d7bfc92b3400554be0c/src/styles/italic/index.android.js#L1

In the begging of the file, the @marcaaron explain why

Android has GTAmericaExp-Bold, but fontWeight: '700' will result in an incorrect font displaying on Android.

I changed to use bold and italic.

_

_

parasharrajat · 2021-04-21T17:22:37Z

@Thierrysantos LOL. This issue literally killed me. I graved out the react-native-render-html's code. What a absolutely waste of time.
Why italic = normal?????

Nice found. Great work. 🎉

marcaaron · 2021-04-21T17:23:53Z

In the screenshots the type is no longer monospaced ?

0xthierry · 2021-04-21T17:24:47Z

@marcaaron yeah, when using bold or italic the text isn't monospaced

parasharrajat · 2021-04-21T17:25:22Z

So we should load a monospaced italic font for fontstyle italic.

marcaaron · 2021-04-21T17:26:03Z

I think we want them to be the same on all platforms cc @shawnborton have you got the right font to use on hand ?

parasharrajat · 2021-04-21T17:26:33Z

GTAmericaExpMono-Rg.otf should have the italic and bold variant.

0xthierry · 2021-04-21T17:29:53Z

@Thierrysantos LOL. This issue literally killed me. I graved out the react-native-render-html's code. What a absolutely waste of time.

Yes, if I didn't find that, looking at the react-native-render-html code would be the next step 😂

0xthierry · 2021-04-21T17:32:33Z

GTAmericaExpMono-Rg.otf should have the italic and bold variant.

I try with that and have the same result 🤔

parasharrajat · 2021-04-21T17:34:44Z

I mean it should have the italic and bold-variant on the internet which we can use something line GTAmericaExpMono-Italic.otf.

0xthierry · 2021-04-21T17:36:48Z

Yeah, I found some variants here https://www.grillitype.com/typeface/gt-america

parasharrajat · 2021-04-21T17:39:30Z

@shawnborton would be a good person to get the assets.

0xthierry · 2021-04-21T17:44:29Z

nice, I'm waiting to the next steps 🤘🏽

aliabbasmalik8 · 2021-04-21T17:55:12Z

Hi @Thierrysantos
this solution did not work on IOS for me. Please let me know if it's working for you. Thanks,
#2491 (comment)

Luke9389 · 2021-04-21T18:50:21Z

Hello @Thierrysantos! I'll be reviewing your proposal on this issue. The description of the issue does include both Android and iOS, and I see that you've cracked part of the problem on Android. Can you account for why this is happening on iOS, and what you'd change to fix it?

Luke9389 · 2021-04-21T18:59:55Z

@aliabbasmalik8 I'll also review your proposal. Can you be more specific about the exact file you'll be changing? Also I'm not sure that this addresses the root cause of the issue. It should not be necessary to apply styles in this case, considering that the following HTML all works fine. (you can try it here: https://www.w3schools.com/html/tryit.asp?filename=tryhtml_intro)

<strong><code>Hello</code></strong><br/>
<em><code>Hello</code></em><br>
<code>Hello</code><br/>
Hello

aliabbasmalik8 · 2021-04-21T19:14:24Z

@Luke9389
these changes made in these files. RenderHTML.js && src/styles/styles.js and classesStyles added in webViewStyles style object
#2491 (comment)

Luke9389 · 2021-04-21T19:21:55Z

@aliabbasmalik8
Thanks for that! But OK, that really just addresses my surface level concern about the level of detail in the proposal. I'm still not convinced that it's a good idea to add styles in this case. It seems like we're fixing this downstream of the actual problem.

aliabbasmalik8 · 2021-04-21T19:29:51Z

@Luke9389
<strong>, <code>, <em> etc, these tags achieved in react native by adding styling in the <Text> component like you can see in the code. so when we have code like this <strong><code> then inner tag style applying so I updated inner tag styling.

Luke9389 · 2021-04-21T19:33:54Z

@aliabbasmalik8
Ah I see. So you're saying that the fontFamily style is being over written because we are applying it in two places (e.g. code and em)?

aliabbasmalik8 · 2021-04-21T19:36:48Z

@aliabbasmalik8
Ah I see. So you're saying that the fontFamily style is being over written because we are applying it in two places (e.g. code and em)?

yes

Luke9389 · 2021-04-21T19:55:24Z

@aliabbasmalik8
Ah, ok gotcha. Thanks for helping me understand here.
Have you tested this on Android? I don't think it'll work because Android doesn't like fontStyle: italic .
Here is a link to the original change that @Thierrysantos found above: #549 (comment)

Seems like the fontStyle will need to remain platform dependent, but your change will be overwriting that. Correct?
Maybe it'd be good for @sketchydroide to weigh in here.

0xthierry · 2021-04-21T20:03:09Z

I'm thinking here and probably the problem is related to font variant.

-> Without font monospace

When we are using monospace(regular), when the text is rendered as bold it doesn't occurs.

aliabbasmalik8 · 2021-04-21T20:04:40Z

@Luke9389
for android, we already have a separate file as you can see italic/index.android.js

const italic = 'normal';
export default italic;

but I think we also need to add fontFamily.MONOSPACE Italic and bold version. right now I test with these styling and fonts family.

classesStyles: {
        em: {
            fontFamily: fontFamily.GTA_ITALIC,
            fontStyle: italic,
        },

        strong: {
            fontFamily: fontFamily.GTA_BOLD,
            fontWeight: fontWeightBold,
        },

    },

we can test with adding Italic and bold version of MONOSPACE fonts because I tested this way and it also works for me

0xthierry · 2021-04-21T20:06:29Z

Also we could see here some issues related to styling #2491 (comment)

shawnborton · 2021-04-21T20:41:01Z

Just to make sure I understand this correctly, it sounds like we need our monospaced font in regular italic, bold, and bold italic. Does that sound right? We can provide the font files to implement these natively, and I think we'll also have to make sure these fonts are added to our Expensify.com web application so we can serve them up for desktop/web.

Luke9389 · 2021-04-21T20:42:57Z

@shawnborton Yes I think that's correct.

@aliabbasmalik8 I'm liking the direction you're heading, but you'll need to verify that your proposed solution works for iOS AND Android (with screenshots). If you can do that, please resubmit a proposal with those included, and a summary of exactly what you're going to change, links to those files, and any resources you'll need (like a new font for example). Please use links to our actual codebase (example), and markdown code blocks rather than screenshots of your IDE. Generally speaking, I'm looking for a very clean proposal that has everything we need, all in one comment.

@Thierrysantos I'm not quite following your last two comments, but I ask that you please only comment when you have a solution ready and have a proposal to resubmit. Posting investigation steps is making this thread noisy and hard to follow.

0xthierry · 2021-04-21T20:59:34Z

Sorry, if I'm not being clear enough, but what I'm suggesting here (#2491 (comment) and #2491 (comment)) is to have other variants for monospace font, such as bold and italics @Luke9389

shawnborton · 2021-04-21T22:14:04Z

Here are the font files we need for native implementation: GTAMono.zip

As I mentioned, we'll need to implement these on Web-E as well as I am pretty sure desktop/web on Expensfiy.cash just grabs font URLs from the www.expensify.com web app. @Luke9389 is that something you would like to take care of? I can help you implement them as well.

aliabbasmalik8 · 2021-04-21T22:15:03Z

@Luke9389
we need to add GTAmericaExpMono font in regular italic, bold, and bold italic.
and need to delete fontWeight/bold/index.android.js && fontWeight/italic/index.android.js

NOTE:

I tested with OpenSans free fonts instead GTAmericaExpMono fonts and it worked for me.
below images are used OpenSans free fonts instead GTAmericaExpMono fonts for testing purpose

IOS

WEB

aliabbasmalik8 · 2021-04-21T22:24:58Z

@Luke9389
screenshots with provided fonts
IOS

Android

shawnborton · 2021-04-21T22:26:26Z

@aliabbasmalik8 for your testing, can you be sure to use words with a lowercase "g" in them? That makes it easy to tell if the correct font is being applied.

For Android in your screenshot above, the fonts look like they are incorrect.

aliabbasmalik8 · 2021-04-21T22:27:13Z

@aliabbasmalik8 for your testing, can you be sure to use words with a lowercase "g" in them? That makes it easy to tell if the correct font is being applied.

For Android in your screenshot above, the fonts look like they are incorrect.

Okay,

aliabbasmalik8 · 2021-04-21T22:49:24Z

@shawnborton You are right. fonts not added in android. I have no idea why it's happened but i linked all assets in app by running npx react-native link.

0xthierry · 2021-04-23T05:22:34Z

Hello @Luke9389 ,

I have a proposal right now, after a lot of time debugging we need to add this new fonts and rename it based on PostScript name.

We need to add this fonts here

https://github.com/Expensify/Expensify.cash/blob/d27cf29a5579cbaab20fd4c29ad92140f55580e8/src/styles/fontFamily/index.js#L3

and here

https://github.com/Expensify/Expensify.cash/blob/d27cf29a5579cbaab20fd4c29ad92140f55580e8/src/components/RenderHTML.js#L20

Also we need to change the strong and em to be:

        em: {
            fontFamily: fontFamily.GTA_ITALIC,
            fontStyle: 'italic',
        },
        
         strong: {
            fontFamily: fontFamily.GTA_BOLD,
            fontWeight: 'bold',
        },

And in the we need to load the font variant dynamically (only for android), because react-native in android don't work well with fontWeight and fontStyle.

https://github.com/Expensify/Expensify.cash/blob/d27cf29a5579cbaab20fd4c29ad92140f55580e8/src/components/RenderHTML.js#L62

The code below is not the final version, and it is just to show the solution, I will improve it

    if (Platform.OS === 'android') {
        const isItalic = textStyle.fontStyle === 'italic';
        const isBold = textStyle.fontWeight === 'bold' || Number(textStyle.fontWeight) >= 700;
        const isItalicAndBold = isItalic && isBold;
       
       // The better solution to fix that is use the correct font variant instead fontWeight or fontStyle
       
        const fontName = isItalicAndBold
            ? fontFamily.MONOSPACE_BOLD_ITALIC
            : isItalic
                ? fontFamily.MONOSPACE_ITALIC
                : isBold ? fontFamily.MONOSPACE_BOLD : fontFamily.MONOSPACE;
        customStyle = {
            ...customStyle,
            fontFamily: fontName,
            fontStyle: undefined, // We need to do it, because react-native-render-html will send it as 'italic' and will broken in android
            fontWeight: undefined, // We need to do it, because react-native-render-html will send it as 'bold' and will broken in android
    }

That is the final result:

aliabbasmalik8 · 2021-04-23T10:31:14Z

REVISED PROPOSAL
STEP 1: ADD FONTS IN APP AND LINKED
STEP 2: ADD FONT FAMILY CONSTANTS AT HERE
https://github.com/Expensify/Expensify.cash/blob/d27cf29a5579cbaab20fd4c29ad92140f55580e8/src/styles/fontFamily/index.js#L3

    MONOSPACE_BOLD: 'GTAmericaExpMono-Bold',
    MONOSPACE_ITALIC: 'GTAmericaExpMono-RgIt',
    MONOSPACE_BOLD_ITALIC: 'GTAmericaExpMono-BdIt',

STEP 4: DELETE THESE 2 files

STEP 5: ADD CODE HERE
https://github.com/Expensify/Expensify.cash/blob/d27cf29a5579cbaab20fd4c29ad92140f55580e8/src/components/RenderHTML.js#L156

    // added classed to add appropraite fontfamily
    let updatedHTML = html.replace(/<em><code>/g, '<em><code class="em">');
    updatedHTML = updatedHTML.replace(/<strong><code>/g, '<strong><code class="strong">');
    updatedHTML = updatedHTML.replace(/<code>/g, '<code class="code">');

STEP 6: ADD classesStyles PROPS IN HTML COMPONENT
https://github.com/Expensify/Expensify.cash/blob/d27cf29a5579cbaab20fd4c29ad92140f55580e8/src/components/RenderHTML.js#L158

            html={updatedHTML}
           classesStyles={webViewStyles.classesStyles}

STEP 7: UPDATE styles.js

REMOVE fontFamily: fontFamily.MONOSPACE, from codeobject at this point
https://github.com/Expensify/Expensify.cash/blob/d27cf29a5579cbaab20fd4c29ad92140f55580e8/src/styles/styles.js#L1424
ADD classesStyles object webViewStyles styles object
https://github.com/Expensify/Expensify.cash/blob/d27cf29a5579cbaab20fd4c29ad92140f55580e8/src/styles/styles.js#L1372

classesStyles: {
        em: {
            fontFamily: fontFamily.MONOSPACE_ITALIC,
        },
        strong: {
            fontFamily: fontFamily.MONOSPACE_BOLD,
        },
        code: {
            fontFamily: fontFamily.MONOSPACE,
        },
    },

SCREENSHOTS
IOS

ANDROID

Luke9389 · 2021-04-23T18:54:55Z

@Thierrysantos I see you've got platform-specific code in your proposal. Could you describe to me at a high level how you'd improve your code?

0xthierry · 2021-04-23T19:04:43Z

@Luke9389 that idea is just refactor the code to remove the nested ternary. Like this

const italic = textStyle.fontStyle === 'italic' && fontFamily.MONOSPACE_ITALIC
const bold = textStyle.fontWeight === 'bold'&& fontFamily.MONOSPACE_BOLD
const italicBold = italic && bold && fontFamily.MONOSPACE_BOLD_ITALIC

const fontFamily = italicBold || bold || italic || fontFamily.MONOSPACE

Luke9389 · 2021-04-23T19:07:58Z

@Thierrysantos I see. But what about Platform.OS === 'android'? In our contributing guidelines, we mention that we don't want platform-specific code. Is there a way to avoid having that line? Perhaps we could put this logic in a lib and import it to RenderHTML instead?

0xthierry · 2021-04-23T19:36:09Z

@Luke9389 I'm looking a way here and I give a feedback for you

0xthierry · 2021-04-23T20:47:25Z

@Luke9389 we don't need this Platform.OS === 'android', so is just that here https://github.com/Expensify/Expensify.cash/blob/d27cf29a5579cbaab20fd4c29ad92140f55580e8/src/components/RenderHTML.js#L62

const italic = textStyle.fontStyle === 'italic' && fontFamily.MONOSPACE_ITALIC
const bold = textStyle.fontWeight === 'bold'&& fontFamily.MONOSPACE_BOLD
const italicBold = italic && bold && fontFamily.MONOSPACE_BOLD_ITALIC

const fontFamily = italicBold || bold || italic || fontFamily.MONOSPACE

The result in the ios and android still the same.

Web:

Luke9389 · 2021-04-23T22:32:06Z

Ok, @Thierrysantos you can go ahead and start the PR. Thanks for all your patience here.

@aliabbasmalik8 I appreciate the effort you put into this thread, but I've decided not to move forward with your proposal. The main reason for this is that after I asked for a clear, complete, and well laid out proposal from both of you, Thierry was the one to provide that first. I do hope that you continue to make proposals with us, but in the future, I think you should focus less on speed, and more on making sure your proposal is clear and complete. The revised version you did was good, had you posted that much sooner this would have gone the other way. Thank you again very much for your time and efforts.

0xthierry · 2021-04-23T22:39:37Z

Ok, @Thierrysantos you can go ahead and start the PR. Thanks for all your patience here.

Okay, thanks and sorry if I made something wrong here ❤️ .

Luke9389 · 2021-04-23T23:24:12Z

No, you've been great! No worries at all.
The process needs all of us :)

0xthierry · 2021-04-24T00:02:23Z

Nice, I sent the proposal on UpWork

NicMendonca · 2021-04-26T12:38:04Z

@Thierrysantos just sent over the the offer in Upwork. Thanks!

0xthierry · 2021-04-30T11:51:09Z

@Luke9389 The PR is open 🎉 #2648

NicMendonca assigned Luke9389 Apr 20, 2021

NicMendonca added the Exported label Apr 20, 2021

NicMendonca assigned 0xthierry Apr 26, 2021

0xthierry mentioned this issue Apr 30, 2021

Font-family difference between platforms #2648

Merged

5 tasks

NicMendonca closed this as completed May 18, 2021

Bolded and Italics code does not render properly on mobile #2491

Bolded and Italics code does not render properly on mobile #2491

Comments

NicMendonca commented Apr 20, 2021 • edited Loading

Expected Result:

Actual Result:

Action Performed:

Workaround:

Platform:

parasharrajat commented Apr 20, 2021 • edited Loading

aliabbasmalik8 commented Apr 21, 2021

0xthierry commented Apr 21, 2021 • edited Loading

parasharrajat commented Apr 21, 2021

marcaaron commented Apr 21, 2021

0xthierry commented Apr 21, 2021

parasharrajat commented Apr 21, 2021

marcaaron commented Apr 21, 2021

parasharrajat commented Apr 21, 2021

0xthierry commented Apr 21, 2021

0xthierry commented Apr 21, 2021

parasharrajat commented Apr 21, 2021

0xthierry commented Apr 21, 2021

parasharrajat commented Apr 21, 2021

0xthierry commented Apr 21, 2021

aliabbasmalik8 commented Apr 21, 2021 • edited Loading

Luke9389 commented Apr 21, 2021

Luke9389 commented Apr 21, 2021 • edited Loading

aliabbasmalik8 commented Apr 21, 2021

Luke9389 commented Apr 21, 2021 • edited Loading

aliabbasmalik8 commented Apr 21, 2021

Luke9389 commented Apr 21, 2021

aliabbasmalik8 commented Apr 21, 2021

Luke9389 commented Apr 21, 2021

0xthierry commented Apr 21, 2021

aliabbasmalik8 commented Apr 21, 2021 • edited Loading

0xthierry commented Apr 21, 2021

shawnborton commented Apr 21, 2021

Luke9389 commented Apr 21, 2021

0xthierry commented Apr 21, 2021 • edited Loading

shawnborton commented Apr 21, 2021

aliabbasmalik8 commented Apr 21, 2021 • edited Loading

aliabbasmalik8 commented Apr 21, 2021

shawnborton commented Apr 21, 2021

aliabbasmalik8 commented Apr 21, 2021

aliabbasmalik8 commented Apr 21, 2021 • edited Loading

0xthierry commented Apr 23, 2021 • edited Loading

aliabbasmalik8 commented Apr 23, 2021

Luke9389 commented Apr 23, 2021

0xthierry commented Apr 23, 2021

Luke9389 commented Apr 23, 2021

0xthierry commented Apr 23, 2021

0xthierry commented Apr 23, 2021 • edited Loading

Luke9389 commented Apr 23, 2021

0xthierry commented Apr 23, 2021

Luke9389 commented Apr 23, 2021

0xthierry commented Apr 24, 2021

NicMendonca commented Apr 26, 2021

0xthierry commented Apr 30, 2021

NicMendonca commented Apr 20, 2021 •

edited

Loading

parasharrajat commented Apr 20, 2021 •

edited

Loading

0xthierry commented Apr 21, 2021 •

edited

Loading

aliabbasmalik8 commented Apr 21, 2021 •

edited

Loading

Luke9389 commented Apr 21, 2021 •

edited

Loading

Luke9389 commented Apr 21, 2021 •

edited

Loading

aliabbasmalik8 commented Apr 21, 2021 •

edited

Loading

0xthierry commented Apr 21, 2021 •

edited

Loading

aliabbasmalik8 commented Apr 21, 2021 •

edited

Loading

aliabbasmalik8 commented Apr 21, 2021 •

edited

Loading

0xthierry commented Apr 23, 2021 •

edited

Loading

0xthierry commented Apr 23, 2021 •

edited

Loading