Set default lineHeight for webViewStyles(HTML rendered messages)

[daraksha-dk]

Details

#13807 (comment)

Fixed Issues

$ #13807

PROPOSAL: #13807 (comment)

Tests

1. Open the app
2. Go to any chat
3. Write a message with 1 or 2 lines & send
4. Write the same message but use some formatting like bold in one of those lines & send
5. Verify that both the messages will have same height

• Verify that no errors appear in the JS console

Offline tests

• N/A

QA Steps

1. Open the app
2. Go to any chat
3. Write a message with 1 or 2 lines & send
4. Write the same message but use some formatting like bold in one of those lines & send
5. Verify that both the messages will have same height

• Verify that no errors appear in the JS console

PR Author Checklist

• I linked the correct issue in the ### Fixed Issues section above
• I wrote clear testing steps that cover the changes made in this PR
  • I added steps for local testing in the Tests section
  • I added steps for the expected offline behavior in the Offline steps section
  • I added steps for Staging and/or Production testing in the QA steps section
  • I added steps to cover failure scenarios (i.e. verify an input displays the correct error message if the entered data is not correct)
  • I turned off my network connection and tested it while offline to ensure it matches the expected behavior (i.e. verify the default avatar icon is displayed if app is offline)
  • I tested this PR with a High Traffic account against the staging or production API to ensure there are no regressions (e.g. long loading states that impact usability).
• I included screenshots or videos for tests on all platforms
• I ran the tests on all platforms & verified they passed on:
  • Android / native
  • Android / Chrome
  • iOS / native
  • iOS / Safari
  • MacOS / Chrome / Safari
  • MacOS / Desktop
• I verified there are no console errors (if there's a console error not related to the PR, report it or open an issue for it to be fixed)
• I followed proper code patterns (see Reviewing the code)
  • I verified that any callback methods that were added or modified are named for what the method does and never what callback they handle (i.e. toggleReport and not onIconClick)
  • I verified that comments were added to code that is not self explanatory
  • I verified that any new or modified comments were clear, correct English, and explained "why" the code was doing something instead of only explaining "what" the code was doing.
  • I verified any copy / text shown in the product is localized by adding it to src/languages/* files and using the translation method
    • If any non-english text was added/modified, I verified the translation was requested/reviewed in #expensify-open-source and it was approved by an internal Expensify engineer. Link to Slack message:
  • I verified all numbers, amounts, dates and phone numbers shown in the product are using the localization methods
  • I verified any copy / text that was added to the app is correct English and approved by marketing by adding the Waiting for Copy label for a copy review on the original GH to get the correct copy.
  • I verified proper file naming conventions were followed for any new files or renamed files. All non-platform specific files are named after what they export and are not named "index.js". All platform-specific files are named for the platform the code supports as outlined in the README.
  • I verified the JSDocs style guidelines (in STYLE.md) were followed
• If a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers
• I followed the guidelines as stated in the Review Guidelines
• I tested other components that can be impacted by my changes (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar are working as expected)
• I verified all code is DRY (the PR doesn't include any logic written more than once, with the exception of tests)
• I verified any variables that can be defined as constants (ie. in CONST.js or at the top of the file that uses the constant) are defined as such
• I verified that if a function's arguments changed that all usages have also been updated correctly
• If a new component is created I verified that:
  • A similar component doesn't exist in the codebase
  • All props are defined accurately and each prop has a /** comment above it */
  • The file is named correctly
  • The component has a clear name that is non-ambiguous and the purpose of the component can be inferred from the name alone
  • The only data being stored in the state is data necessary for rendering and nothing else
  • For Class Components, any internal methods passed to components event handlers are bound to this properly so there are no scoping issues (i.e. for onClick={this.submit} the method this.submit should be bound to this in the constructor)
  • Any internal methods bound to this are necessary to be bound (i.e. avoid this.submit = this.submit.bind(this); if this.submit is never passed to a component event handler like onClick)
  • All JSX used for rendering exists in the render method
  • The component has the minimum amount of code necessary for its purpose, and it is broken down into smaller components in order to separate concerns and functions
• If any new file was added I verified that:
  • The file has a description of what it does and/or why is needed at the top of the file if the code is not self explanatory
• If a new CSS style is added I verified that:
  • A similar style doesn't already exist
  • The style can't be created with an existing StyleUtils function (i.e. StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)
• If the PR modifies a generic component, I tested and verified that those changes do not break usages of that component in the rest of the App (i.e. if a shared library or component like Avatar is modified, I verified that Avatar is working as expected in all cases)
• If the PR modifies a component related to any of the existing Storybook stories, I tested and verified all stories for that component are still working as expected.
• If a new page is added, I verified it's using the ScrollView component to make it scrollable when more elements are added to the page.
• I have checked off every checkbox in the PR author checklist, including those that don't apply to this PR.

Screenshots/Videos

Web

Mobile Web - Chrome

Mobile Web - Safari

Desktop

iOS

Android

[melvin-bot]

@puneetlath @mananjadhav One of you needs to copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button]

[mananjadhav]

Reviewer Checklist

• I have verified the author checklist is complete (all boxes are checked off).
• I verified the correct issue is linked in the ### Fixed Issues section above
• I verified testing steps are clear and they cover the changes made in this PR
  • I verified the steps for local testing are in the Tests section
  • I verified the steps for Staging and/or Production testing are in the QA steps section
  • I verified the steps cover any possible failure scenarios (i.e. verify an input displays the correct error message if the entered data is not correct)
  • I turned off my network connection and tested it while offline to ensure it matches the expected behavior (i.e. verify the default avatar icon is displayed if app is offline)
• I checked that screenshots or videos are included for tests on all platforms
• I included screenshots or videos for tests on all platforms
• I verified tests pass on all platforms & I tested again on:
  • Android / native
  • Android / Chrome
  • iOS / native
  • iOS / Safari
  • MacOS / Chrome / Safari
  • MacOS / Desktop
• If there are any errors in the console that are unrelated to this PR, I either fixed them (preferred) or linked to where I reported them in Slack
• I verified proper code patterns were followed (see Reviewing the code)
  • I verified that any callback methods that were added or modified are named for what the method does and never what callback they handle (i.e. toggleReport and not onIconClick).
  • I verified that comments were added to code that is not self explanatory
  • I verified that any new or modified comments were clear, correct English, and explained "why" the code was doing something instead of only explaining "what" the code was doing.
  • I verified any copy / text shown in the product is localized by adding it to src/languages/* files and using the translation method
  • I verified all numbers, amounts, dates and phone numbers shown in the product are using the localization methods
  • I verified any copy / text that was added to the app is correct English and approved by marketing by adding the Waiting for Copy label for a copy review on the original GH to get the correct copy.
  • I verified proper file naming conventions were followed for any new files or renamed files. All non-platform specific files are named after what they export and are not named "index.js". All platform-specific files are named for the platform the code supports as outlined in the README.
  • I verified the JSDocs style guidelines (in STYLE.md) were followed
• If a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers
• I verified that this PR follows the guidelines as stated in the Review Guidelines
• I verified other components that can be impacted by these changes have been tested, and I retested again (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar have been tested & I retested again)
• I verified all code is DRY (the PR doesn't include any logic written more than once, with the exception of tests)
• I verified any variables that can be defined as constants (ie. in CONST.js or at the top of the file that uses the constant) are defined as such
• If a new component is created I verified that:
  • A similar component doesn't exist in the codebase
  • All props are defined accurately and each prop has a /** comment above it */
  • The file is named correctly
  • The component has a clear name that is non-ambiguous and the purpose of the component can be inferred from the name alone
  • The only data being stored in the state is data necessary for rendering and nothing else
  • For Class Components, any internal methods passed to components event handlers are bound to this properly so there are no scoping issues (i.e. for onClick={this.submit} the method this.submit should be bound to this in the constructor)
  • Any internal methods bound to this are necessary to be bound (i.e. avoid this.submit = this.submit.bind(this); if this.submit is never passed to a component event handler like onClick)
  • All JSX used for rendering exists in the render method
  • The component has the minimum amount of code necessary for its purpose, and it is broken down into smaller components in order to separate concerns and functions
• If any new file was added I verified that:
  • The file has a description of what it does and/or why is needed at the top of the file if the code is not self explanatory
• If a new CSS style is added I verified that:
  • A similar style doesn't already exist
  • The style can't be created with an existing StyleUtils function (i.e. StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)
• If the PR modifies a generic component, I tested and verified that those changes do not break usages of that component in the rest of the App (i.e. if a shared library or component like Avatar is modified, I verified that Avatar is working as expected in all cases)
• If the PR modifies a component related to any of the existing Storybook stories, I tested and verified all stories for that component are still working as expected.
• If a new page is added, I verified it's using the ScrollView component to make it scrollable when more elements are added to the page.
• I have checked off every checkbox in the PR reviewer checklist, including those that don't apply to this PR.

Screenshots/Videos

Web

web-line-height.mov

Mobile Web - Chrome

mweb-chrome-line-height.mov

Mobile Web - Safari

mweb-safari-line-height.mov

Desktop

desktop-line-height.mov

iOS

ios-line-height.mov

Android

android-line-height.mov

---

I am left with verifying the change in Android. I am having some difficulties in running the debugger on emulator. I'll check back again in an hour or so. Done with the Android too.

[mananjadhav]

@puneetlath All yours.

@shawnborton If you'd like to take a quick look here or anything specific you want me to test.

[shawnborton]

Oh interesting, so by adding the lineHeight to baseFontStyle that means we'll change the line height everywhere in the entire app? I could have sworn we said we'd start by just changing the line height for messages.

[mananjadhav]

baseFontStyle that means we'll change the line height everywhere in the entire app

I don't think so. Can you give me an example, where apart from the messages has it impacted. We're updating styles.webViewStyles.baseFontStyle, which is used for comment, email-comment, in the messages.

[shawnborton]

Got it, I wasn't sure if that would change the line height for things like this as well:

[mananjadhav]

It won't change @shawnborton. But just fyi, the example that you gave has the lineHeight of 20px anyway.

@puneetlath Did you get a chance to look at the PR?

[daraksha-dk]

bump @puneetlath

[Pujan92]

Just to point out message which has italic font style and includes code seems to take a height of 21px even though 20px has been given as per this PR change. Tried -> _`Test`_ (Not sure about our app allow this usecase or not)

Screen.Recording.2023-01-21.at.12.19.05.AM.mp4

[shawnborton]

Good catch! Let's try to clean that up to be 20px as well.

[mananjadhav]

@daraksha-dk Can you confirm the above comments?

[daraksha-dk]

@mananjadhav, the height is becoming 21px when one type of formatting like bold/italic is being mixed with ``(code) formatting.
I think we can take this up as a separate issue (where complex scenarios like this can be handled) since this PR is only dealing with one type of formatting

[OSBotify]

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

[github-actions]

Performance Comparison Report 📊

Significant Changes To Duration

There are no entries

Meaningless Changes To Duration

Show entries

Name	Duration
App start TTI	736.551 ms → 773.352 ms (+36.801 ms, +5.0%)
App start runJsBundle	207.156 ms → 215.355 ms (+8.199 ms, +4.0%)
Open Search Page TTI	611.665 ms → 618.060 ms (+6.395 ms, +1.0%)
App start nativeLaunch	20.333 ms → 20.667 ms (+0.333 ms, +1.6%)
App start regularAppStart	0.022 ms → 0.017 ms (-0.005 ms, -23.3%) 🟢

Show details

Name	Duration
App start TTI	Baseline Mean: 736.551 ms Stdev: 30.083 ms (4.1%) Runs: 680.6298380000517 686.6382489996031 701.1129289995879 705.0870660003275 706.2959519997239 708.9057930000126 710.5452990001068 710.8586229998618 711.7272300003096 713.1344510000199 719.5786399999633 727.3373630000278 731.8782360004261 732.2613869998604 733.0145460003987 735.382129999809 736.0799989998341 737.3172030001879 738.6865569995716 742.285560999997 743.7306770002469 743.9791729999706 745.9916260000318 749.8602120000869 750.9851660002023 755.1993570001796 765.8932980000973 777.2397199999541 778.7130720000714 778.9506219998002 802.5901819998398 807.7414539996535 Current Mean: 773.352 ms Stdev: 33.553 ms (4.3%) Runs: 715.4030309999362 724.4741820003837 726.8367659999058 733.0053340001032 734.1543629998341 739.1836259998381 740.5499799996614 748.6424569999799 752.3984080003574 753.2765359999612 756.0383230000734 761.6287759998813 764.1319979997352 767.4035069998354 768.5762550001964 769.7352790003642 770.7081110002473 772.7308289995417 773.4039880000055 777.5281790001318 777.6042729998007 780.5407899999991 788.473437000066 803.9860309995711 804.6206839997321 805.0967509998009 805.5313919996843 806.8170400001109 816.3386110002175 818.579617000185 821.3022360000759 868.5711529999971
App start runJsBundle	Baseline Mean: 207.156 ms Stdev: 14.729 ms (7.1%) Runs: 176 181 184 188 194 194 197 199 199 200 200 201 201 201 202 202 206 207 211 211 213 216 218 219 219 221 223 224 224 230 231 237 Current Mean: 215.355 ms Stdev: 13.642 ms (6.3%) Runs: 185 193 196 199 201 204 208 209 209 210 211 211 213 213 213 213 213 214 214 218 219 221 223 224 226 228 231 234 234 240 249
Open Search Page TTI	Baseline Mean: 611.665 ms Stdev: 16.224 ms (2.7%) Runs: 572.468302000314 587.3979489998892 591.7226980002597 595.0574960000813 595.7956960005686 596.5131839998066 598.1309009995311 600.0983069995418 602.0159100005403 603.7328290008008 604.6340330000967 605.4589849999174 605.8758540004492 607.5461019994691 607.8057059999555 609.5925300000235 610.6120600001886 612.0137940002605 612.1890869997442 613.8215750008821 614.2982989996672 616.011800000444 618.5229089995846 624.168335000053 624.7881269995123 626.5837809992954 627.7349849995226 628.5232750000432 633.6541340006515 635.9875490004197 643.342042000033 647.1701259994879 Current Mean: 618.060 ms Stdev: 20.183 ms (3.3%) Runs: 568.6083979997784 591.3820810001343 593.4137779995799 596.8843179997057 597.540974999778 597.6638590004295 599.4074299996719 604.5417480003089 605.868815000169 605.9459239998832 606.7627370003611 611.0431310003623 611.2413739999756 611.4150799997151 612.7647700002417 615.5728350002319 616.5475260000676 619.4733070004731 620.3178300000727 620.3652759995311 621.7142339991406 622.7408039998263 624.8603929998353 625.6441659992561 631.1925459997728 632.5127769997343 634.2205410003662 636.0896410001442 637.5430100001395 649.2095130002126 652.219076000154 659.53702800069 661.719319999218
App start nativeLaunch	Baseline Mean: 20.333 ms Stdev: 1.599 ms (7.9%) Runs: 17 18 18 18 19 19 19 19 19 19 20 20 20 20 20 20 21 21 21 21 21 21 22 22 22 22 22 23 23 23 Current Mean: 20.667 ms Stdev: 2.329 ms (11.3%) Runs: 18 19 19 19 19 19 19 19 19 19 19 19 19 19 20 20 20 20 20 21 21 22 22 23 23 23 23 24 24 29
App start regularAppStart	Baseline Mean: 0.022 ms Stdev: 0.001 ms (5.7%) Runs: 0.01985699962824583 0.020181999541819096 0.02034500055015087 0.021077999845147133 0.021443999372422695 0.021606000140309334 0.0217289999127388 0.0217289999127388 0.021769000217318535 0.02180900052189827 0.021971999667584896 0.022013000212609768 0.02217600028961897 0.02217600028961897 0.02217600028961897 0.02274599950760603 0.023029999807476997 0.023030000738799572 0.02307199966162443 0.023152999579906464 0.0231929998844862 0.0231929998844862 0.02323400042951107 0.0233559999614954 0.023886000737547874 0.023966999724507332 0.024942999705672264 0.025472000241279602 Current Mean: 0.017 ms Stdev: 0.001 ms (6.4%) Runs: 0.015056000091135502 0.015502999536693096 0.0157880000770092 0.015868999995291233 0.016032000072300434 0.016235999763011932 0.016397999599575996 0.01647899951785803 0.016600999981164932 0.01664199959486723 0.01664199959486723 0.016764000058174133 0.016764000058174133 0.016844999976456165 0.016927000135183334 0.01696799974888563 0.017009000293910503 0.017130999825894833 0.017211999744176865 0.017252999357879162 0.017456000670790672 0.017496999353170395 0.017700999975204468 0.018148000352084637 0.0183100001886487 0.018311000429093838 0.018391999416053295 0.018595000728964806 0.018799000419676304 0.018799000419676304 0.018880000337958336 0.019857000559568405

[OSBotify]

🚀 Deployed to staging by https://github.com/puneetlath in version: 1.2.59-0 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅

[OSBotify]

🚀 Deployed to production by https://github.com/luacmartins in version: 1.2.59-1 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	failure ❌
🕸 web 🕸	success ✅