[HOLD for payment 2023-03-06] [$1000] Emoji tops are being cut off in the personal message field on invite members page #14674

kavimuru · 2023-01-30T22:04:10Z

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

Actions Performed:

Go to settings
Open any workspace
Go to manage members
Tap invite
Clear the personal message field
Add some emojis in the personal message field

Expected Result:

Emojis are entirely visible

Actual Result:

The tops of the emojis are cut off

Workaround:

unknown

Platforms:

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

Android / native
Android / Chrome
iOS / native
iOS / Safari

Version Number: 1.2.61-0
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:

23-01-30-15-43-52.mp4

Expensify/Expensify Issue URL:
Issue reported by: @adeel0202
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1675075667380219

View all open jobs on GitHub

Upwork Automation - Do Not Edit

Upwork Job URL: https://www.upwork.com/jobs/~013ed57e2a5ae4cce2
Upwork Job ID: 1620424216952336384
Last Price Increase: 2023-01-31

melvin-bot · 2023-01-31T14:10:17Z

Job added to Upwork: https://www.upwork.com/jobs/~013ed57e2a5ae4cce2

melvin-bot · 2023-01-31T14:10:19Z

Current assignee @garrettmknight is eligible for the External assigner, not assigning anyone new.

melvin-bot · 2023-01-31T14:10:27Z

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

melvin-bot · 2023-01-31T14:10:33Z

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

Pujan92 · 2023-01-31T14:24:36Z

Proposal

It seems the height of the label needs to provide as paddingTop to avoid overlap issue.

diff --git a/src/styles/styles.js b/src/styles/styles.js
index 4b1d1982ae..1835e7742d 100644
--- a/src/styles/styles.js
+++ b/src/styles/styles.js
@@ -772,11 +772,11 @@ const styles = {
     },
 
     textInputMultiline: {
-        scrollPadding: '23px 0 0 0',
+        scrollPadding: '25px 0 0 0',
     },
 
     textInputMultilineContainer: {
-        paddingTop: 23,
+        paddingTop: 25,
     },

Issue

After correct padding

Other way is to remove the z-index: -1 from the textInputAndIconContainer which shows the text top of the label but IMO not a good solution.

Note: Same issue exist for close account screen input also which will be resolved with this fix

Prince-Mendiratta · 2023-01-31T14:44:16Z

Proposal

Impact

The issue is reproducible on all platforms. The components affected by this issue are those that make use of the multiline prop in combination with a label. Namely,

CloseAccountPage
WorkspaceInvitePage

RCA

To separate the text from the label, we set a paddingTop: 23px. The height of the label itself is 25px. Thus, when an emoji is inserted, the label cuts off part of the emoji. This issue is that the component is using multiline prop. This further applies the textInputMultilineContainer and textInputMultiline (for non android) and textInputLabelBackground styles.

App/src/styles/styles.js

Lines 778 to 780 in 3691c81

    
           textInputMultilineContainer: { 
        
               paddingTop: 23, 
        
           },

App/src/styles/styles.js

Lines 774 to 776 in 3691c81

    
           textInputMultiline: { 
        
               scrollPadding: '23px 0 0 0', 
        
           },

Solution 1

We can set the height of the label to be 23px. This is a relatively better solution because the style for paddingTop in baseTextInput is of height 23px as well. All other styles dealing with multiline make use of 23px value as well, namely textInputMultilineContainer and textInputMultiline. This would be nice for consistency.

diff --git a/src/styles/styles.js b/src/styles/styles.js
index 4b1d1982ae..fa36e795cc 100644
--- a/src/styles/styles.js
+++ b/src/styles/styles.js
@@ -744,7 +744,7 @@ const styles = {
         position: 'absolute',
         top: 0,
         width: '100%',
-        height: 25,
+        height: 23,
         backgroundColor: themeColors.componentBG,
     },

Solution 2

We can set the paddingTop for the text container to be 25px.

diff --git a/src/styles/styles.js b/src/styles/styles.js
index 4b1d1982ae..c372011754 100644
--- a/src/styles/styles.js
+++ b/src/styles/styles.js
 
@@ -776,7 +776,7 @@ const styles = {
     },
 
     textInputMultilineContainer: {
-        paddingTop: 23,
+        paddingTop: 25,
     },
 
     textInputAndIconContainer: {

Results

2023-01-31.20-31-27.mp4

varshamb · 2023-01-31T14:53:00Z

Proposal

We need to change paddingTop: 23, to marginTop: 25,

App/src/styles/styles.js

Lines 778 to 780 in 3691c81

    
           textInputMultilineContainer: { 
        
               paddingTop: 23, 
        
           },

Result:

bernhardoj · 2023-01-31T15:00:56Z

Proposal

As the others already explained, the height of the label is set to 25. We need to set the padding top of the text input container also to 25. But we also need to change the line height to 20, the same value we set to the composer. (the text is still being cut on mac with line height 16)

diff --git a/src/styles/styles.js b/src/styles/styles.js
index 4b1d1982ae..04d643d8d4 100644
--- a/src/styles/styles.js
+++ b/src/styles/styles.js
@@ -763,7 +763,7 @@ const styles = {
     baseTextInput: {
         fontFamily: fontFamily.EXP_NEUE,
         fontSize: variables.fontSizeNormal,
-        lineHeight: variables.lineHeightNormal,
+        lineHeight: variables.lineHeightXLarge,
         color: themeColors.text,
         paddingTop: 23,
         paddingBottom: 8,
@@ -776,7 +776,7 @@ const styles = {
     },
 
     textInputMultilineContainer: {
-        paddingTop: 23,
+        paddingTop: 25,
     },

By changing the line height to the baseTextInput style, it will also solve text shift issue when we type a normal text then add an emoji (reference).

situchan · 2023-01-31T15:12:19Z

Proposal

The root cause was explained clearly in previous proposal.

And Part 1 of solution is also same.
Either (I prefer the 1st one because 2nd one requires changes in various parts: prefix label, picker, scroll, etc. Also 2nd one does visual change while 1st one not)
1)

diff --git a/src/styles/styles.js b/src/styles/styles.js
index 4b1d1982ae..fa36e795cc 100644
--- a/src/styles/styles.js
+++ b/src/styles/styles.js
@@ -744,7 +744,7 @@ const styles = {
         position: 'absolute',
         top: 0,
         width: '100%',
-        height: 25,
+        height: 23,
         backgroundColor: themeColors.componentBG,
     },

or
2)
Update all these values to 25:

App/src/styles/styles.js

Line 30 in 703b5c4

paddingTop: 23,

App/src/styles/styles.js

Line 768 in 703b5c4

paddingTop: 23,

App/src/styles/styles.js

Line 779 in 703b5c4

paddingTop: 23,

App/src/styles/styles.js

Line 823 in 703b5c4

paddingTop: 23,

App/src/styles/styles.js

Line 775 in 703b5c4

scrollPadding: '23px 0 0 0',

Part 2
But above solution still doesn't fix the issue completely because lineHeight is not large enough for specified fontSize.
i.e. on my android:

{"fontFamily": "ExpensifyNeue-Regular", "fontSize": 16.50000035762787, "lineHeight": 17.600000381469727, "paddingBottom": 8, "paddingTop": 23}

for multiline input style.

Solution: either remove lineHeight even when multiline or increase lineHeight a bit to match fontSize (same as composer)

App/src/styles/styles.js

Lines 763 to 772 in 3691c81

    
           baseTextInput: { 
        
               fontFamily: fontFamily.EXP_NEUE, 
        
               fontSize: variables.fontSizeNormal, 
        
               lineHeight: variables.lineHeightNormal, 
        
               color: themeColors.text, 
        
               paddingTop: 23, 
        
               paddingBottom: 8, 
        
               paddingLeft: 0, 
        
               borderWidth: 0, 
        
           },

Either (I prefer the 2nd one)
1)

        lineHeight: variables.lineHeightNormal,
-       lineHeight: variables.lineHeightNormal,

This will also change BaseTextInput on which lineHeight is not necessary:

App/src/components/TextInput/BaseTextInput.js

Line 291 in 3691c81

!this.props.multiline && {height: this.state.height, lineHeight: undefined},

-                                           !this.props.multiline && {height: this.state.height, lineHeight: undefined},
+                                           !this.props.multiline && {height: this.state.height},

or
2)

-       lineHeight: variables.lineHeightNormal,
+       lineHeight: variables.lineHeightXLarge,

Pujan92 · 2023-01-31T17:34:55Z

Proposal 2

I think we can get rid of the extra element for multiline input which seems not required and with this, it will create/render the same structure it does for other inputs.

diff --git a/src/components/TextInput/BaseTextInput.js b/src/components/TextInput/BaseTextInput.js
index 87f02ba528..504c86b464 100644
--- a/src/components/TextInput/BaseTextInput.js
+++ b/src/components/TextInput/BaseTextInput.js
@@ -238,7 +238,6 @@ class BaseTextInput extends Component {
                                     <>
                                         {/* Adding this background to the label only for multiline text input,
                                     to prevent text overlapping with label when scrolling */}
-                                        {this.props.multiline && <View style={styles.textInputLabelBackground} pointerEvents="
none" />}
                                         <TextInputLabel
                                             label={this.props.label}
diff --git a/src/styles/styles.js b/src/styles/styles.js
index 4b1d1982ae..22f5c0d9a8 100644
--- a/src/styles/styles.js
+++ b/src/styles/styles.js
@@ -740,14 +740,6 @@ const styles = {
         width: '100%',
     },
 
-    textInputLabelBackground: {
-        position: 'absolute',
-        top: 0,
-        width: '100%',
-        height: 25,
-        backgroundColor: themeColors.componentBG,
-    },
-

thuyle04 · 2023-02-01T08:42:11Z

Proposal
This error occurs because text input's pointer in the first line is overlapped by background of label.

File is fixed

Result

garrettmknight · 2023-02-02T18:47:11Z

Hey @rushatgabhane, can you review these proposals when you get a chance? Thanks!

rushatgabhane · 2023-02-03T20:00:09Z

I like @Pujan92's proposal but it also needs line height as mentioned by @bernhardoj (#14674 (comment) to fix text shift issue).

rushatgabhane · 2023-02-03T20:01:57Z

@tylerkaraszewski let's go with @Pujan92's proposal because the text shift issue is kinda unrelated.

C+ reviewed 🎀 👀 🎀

tylerkaraszewski · 2023-02-03T20:14:11Z

👍

situchan · 2023-02-03T21:52:32Z

@rushatgabhane If we wanna increase input paddingTop 2px more, I think this should be applied to all types of inputs as well as multiline input for consistency as I stated in Part 1- 2) of my proposal.

Pujan92 · 2023-02-04T04:34:04Z

@rushatgabhane @tylerkaraszewski shall I wait until I get assigned?

garrettmknight · 2023-02-06T11:56:26Z

@Pujan92 Just hired you. Go for it.

tylerkaraszewski · 2023-03-08T21:58:13Z

Is anyone working on the bugzero checkllist for this?

situchan · 2023-03-09T03:17:57Z

Regression Test Proposal

Login with any account
Go to Settings and open any workspace
Go to Manage members
Tap Invite button
Clear the personal message field
Add some emojis in the personal message field
Verify that emojis are not cut off

Do we agree 👍 or 👎

situchan · 2023-03-09T03:25:29Z

Is this eligible for timeline bonus?
I was assigned on Feb 6 and PR was approved by @rushatgabhane on Feb 8.
@tylerkaraszewski was OOO and back on Feb 22 and merged that day.

garrettmknight · 2023-03-09T16:21:33Z

@rushatgabhane / @tylerkaraszewski can one of you get the BZ steps started?

tylerkaraszewski · 2023-03-13T18:45:12Z

@rushatgabhane - have we ID'ed where this was introduced and gone through those parts of the checklist?

tylerkaraszewski · 2023-03-15T21:27:56Z

Waiting on a reply from @rushatgabhane

rushatgabhane · 2023-03-17T12:25:58Z

will get to this today

MelvinBot · 2023-03-20T18:22:04Z

@tylerkaraszewski, @garrettmknight, @shawnborton, @rushatgabhane, @situchan Whoops! This issue is 2 days overdue. Let's get this updated quick!

rushatgabhane · 2023-03-21T00:53:10Z

The PR that introduced the bug has been identified. Link to the PR:

#5805

The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment:

https://github.com/Expensify/App/pull/5805/files#r1142809457

A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion:

https://expensify.slack.com/archives/C049HHMV9SM/p1679361004650639

rushatgabhane · 2023-03-21T01:11:31Z

@tylerkaraszewski could you please update the checklist with this data because i can't edit comments.
Thanks!

garrettmknight · 2023-03-22T14:36:50Z

Got it - looks like everything is moving.

garrettmknight · 2023-03-22T14:43:23Z

It looks like this would have gone through quickly, but our internal reviewer was on vacay. No harm, no foul. Paying out - 50% bonus for C and C+.

garrettmknight · 2023-03-22T14:55:47Z

@situchan Looks like we didn't get you hired on this job. Can you accept the offer for this one so I can pay you out?

situchan · 2023-03-22T14:58:33Z

@garrettmknight I just accepted offer. However, there's already contract started on Feb 6.
Can you use the one for $1000 and another one for $500 please?

rushatgabhane · 2023-03-22T15:09:01Z

@garrettmknight appreciate it, thanks!

garrettmknight · 2023-03-22T16:50:12Z

@situchan Paid it all out in the other contract. We're all set here!

kavimuru added Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Jan 30, 2023

melvin-bot bot assigned garrettmknight Jan 30, 2023

melvin-bot bot locked and limited conversation to collaborators Jan 30, 2023

garrettmknight changed the title ~~Emojis are being cut in the personal message field on invite members page~~ Emoji tops are being cut off in the personal message field on invite members page Jan 31, 2023

garrettmknight added the External Added to denote the issue can be worked on by a contributor label Jan 31, 2023

melvin-bot bot unlocked this conversation Jan 31, 2023

melvin-bot bot changed the title ~~Emoji tops are being cut off in the personal message field on invite members page~~ [$1000] Emoji tops are being cut off in the personal message field on invite members page Jan 31, 2023

melvin-bot bot assigned rushatgabhane Jan 31, 2023

melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Jan 31, 2023

melvin-bot bot assigned tylerkaraszewski Jan 31, 2023

melvin-bot bot added the Overdue label Feb 6, 2023

melvin-bot bot added Daily KSv2 Overdue and removed Weekly KSv2 Daily KSv2 labels Mar 6, 2023

melvin-bot bot removed the Overdue label Mar 8, 2023

melvin-bot bot added the Overdue label Mar 13, 2023

melvin-bot bot added Overdue and removed Overdue labels Mar 13, 2023

melvin-bot bot removed the Overdue label Mar 15, 2023

melvin-bot bot added the Overdue label Mar 20, 2023

melvin-bot bot removed the Overdue label Mar 21, 2023

rushatgabhane mentioned this issue Mar 21, 2023

Fix: input alignment & error border for input #5805

Merged

5 tasks

garrettmknight closed this as completed Mar 22, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[HOLD for payment 2023-03-06] [$1000] Emoji tops are being cut off in the personal message field on invite members page #14674

[HOLD for payment 2023-03-06] [$1000] Emoji tops are being cut off in the personal message field on invite members page #14674

kavimuru commented Jan 30, 2023 •

edited by melvin-bot bot

Loading

melvin-bot bot commented Jan 31, 2023

melvin-bot bot commented Jan 31, 2023

melvin-bot bot commented Jan 31, 2023

melvin-bot bot commented Jan 31, 2023

Pujan92 commented Jan 31, 2023 •

edited

Loading

Prince-Mendiratta commented Jan 31, 2023 •

edited

Loading

varshamb commented Jan 31, 2023 •

edited

Loading

bernhardoj commented Jan 31, 2023 •

edited

Loading

situchan commented Jan 31, 2023 •

edited

Loading

Pujan92 commented Jan 31, 2023

thuyle04 commented Feb 1, 2023 •

edited

Loading

garrettmknight commented Feb 2, 2023

rushatgabhane commented Feb 3, 2023

rushatgabhane commented Feb 3, 2023 •

edited

Loading

tylerkaraszewski commented Feb 3, 2023

situchan commented Feb 3, 2023

Pujan92 commented Feb 4, 2023

garrettmknight commented Feb 6, 2023

tylerkaraszewski commented Mar 8, 2023

situchan commented Mar 9, 2023

situchan commented Mar 9, 2023

garrettmknight commented Mar 9, 2023

tylerkaraszewski commented Mar 13, 2023

tylerkaraszewski commented Mar 15, 2023

rushatgabhane commented Mar 17, 2023

MelvinBot commented Mar 20, 2023

rushatgabhane commented Mar 21, 2023 •

edited

Loading

rushatgabhane commented Mar 21, 2023 •

edited

Loading

garrettmknight commented Mar 22, 2023

garrettmknight commented Mar 22, 2023

garrettmknight commented Mar 22, 2023

situchan commented Mar 22, 2023

rushatgabhane commented Mar 22, 2023

garrettmknight commented Mar 22, 2023

[HOLD for payment 2023-03-06] [$1000] Emoji tops are being cut off in the personal message field on invite members page #14674

[HOLD for payment 2023-03-06] [$1000] Emoji tops are being cut off in the personal message field on invite members page #14674

Comments

kavimuru commented Jan 30, 2023 • edited by melvin-bot bot Loading

Actions Performed:

Expected Result:

Actual Result:

Workaround:

Platforms:

melvin-bot bot commented Jan 31, 2023

melvin-bot bot commented Jan 31, 2023

melvin-bot bot commented Jan 31, 2023

melvin-bot bot commented Jan 31, 2023

Pujan92 commented Jan 31, 2023 • edited Loading

Prince-Mendiratta commented Jan 31, 2023 • edited Loading

Proposal

Impact

RCA

Solution 1

Solution 2

Results

varshamb commented Jan 31, 2023 • edited Loading

Proposal

Result:

bernhardoj commented Jan 31, 2023 • edited Loading

Proposal

situchan commented Jan 31, 2023 • edited Loading

Proposal

Pujan92 commented Jan 31, 2023

thuyle04 commented Feb 1, 2023 • edited Loading

garrettmknight commented Feb 2, 2023

rushatgabhane commented Feb 3, 2023

rushatgabhane commented Feb 3, 2023 • edited Loading

tylerkaraszewski commented Feb 3, 2023

situchan commented Feb 3, 2023

Pujan92 commented Feb 4, 2023

garrettmknight commented Feb 6, 2023

tylerkaraszewski commented Mar 8, 2023

situchan commented Mar 9, 2023

situchan commented Mar 9, 2023

garrettmknight commented Mar 9, 2023

tylerkaraszewski commented Mar 13, 2023

tylerkaraszewski commented Mar 15, 2023

rushatgabhane commented Mar 17, 2023

MelvinBot commented Mar 20, 2023

rushatgabhane commented Mar 21, 2023 • edited Loading

rushatgabhane commented Mar 21, 2023 • edited Loading

garrettmknight commented Mar 22, 2023

garrettmknight commented Mar 22, 2023

garrettmknight commented Mar 22, 2023

situchan commented Mar 22, 2023

rushatgabhane commented Mar 22, 2023

garrettmknight commented Mar 22, 2023

kavimuru commented Jan 30, 2023 •

edited by melvin-bot bot

Loading

Pujan92 commented Jan 31, 2023 •

edited

Loading

Prince-Mendiratta commented Jan 31, 2023 •

edited

Loading

varshamb commented Jan 31, 2023 •

edited

Loading

bernhardoj commented Jan 31, 2023 •

edited

Loading

situchan commented Jan 31, 2023 •

edited

Loading

thuyle04 commented Feb 1, 2023 •

edited

Loading

rushatgabhane commented Feb 3, 2023 •

edited

Loading

rushatgabhane commented Mar 21, 2023 •

edited

Loading

rushatgabhane commented Mar 21, 2023 •

edited

Loading