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

[$2000] Cursor is overlapping with amount digit on amount page - Reported by @aneequeahmad #10366

Closed
mvtglobally opened this issue Aug 11, 2022 · 48 comments
Assignees
Labels
Daily KSv2 Engineering External Added to denote the issue can be worked on by a contributor Help Wanted Apply this label when an issue is open to proposals by contributors

Comments

@mvtglobally
Copy link

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. Launch app on android or mweb
  2. Press + icon on bottom right
  3. Select send money
  4. see the cursor overlapping with digit

Expected Result:

Cursor shouldn't overlap with digit

Actual Result:

Cursor is overlapping with digit.

Workaround:

unknown

Platform:

Where is this issue occurring?

  • iOS
  • Mobile Web

Version Number: 1.1.88-0
Reproducible in staging?: Y
Reproducible in production?: Y
Email or phone of affected tester (no customers):
Logs: https://stackoverflow.com/c/expensify/questions/4856
Notes/Photos/Videos: Any additional supporting documentation

WhatsApp.Video.2022-07-28.at.3.09.44.PM.mp4

WhatsApp Image 2022-07-28 at 4 54 42 PM

Expensify/Expensify Issue URL:
Issue reported by: @aneequeahmad
Slack conversation: https://expensify.slack.com/archives/C01GTK53T8Q/p1659004018311559

View all open jobs on GitHub

@mvtglobally mvtglobally added AutoAssignerTriage Auto assign issues for triage to an available triage team member Daily KSv2 labels Aug 11, 2022
@melvin-bot
Copy link

melvin-bot bot commented Aug 11, 2022

Triggered auto assignment to @bfitzexpensify (AutoAssignerTriage), see https://stackoverflow.com/c/expensify/questions/4749 for more details.

@melvin-bot melvin-bot bot removed the AutoAssignerTriage Auto assign issues for triage to an available triage team member label Aug 11, 2022
@melvin-bot
Copy link

melvin-bot bot commented Aug 11, 2022

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

@bfitzexpensify
Copy link
Contributor

This is pretty minor, but now that I see it, I can't unsee it. Guessing it will be a pretty simple external fix.

@melvin-bot melvin-bot bot added the Overdue label Aug 15, 2022
@tylerkaraszewski
Copy link
Contributor

I agree, should likely be straightforward and external.

@melvin-bot melvin-bot bot removed the Overdue label Aug 15, 2022
@tylerkaraszewski tylerkaraszewski removed their assignment Aug 15, 2022
@tylerkaraszewski tylerkaraszewski added the External Added to denote the issue can be worked on by a contributor label Aug 15, 2022
@melvin-bot
Copy link

melvin-bot bot commented Aug 15, 2022

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

@bfitzexpensify
Copy link
Contributor

Upwork job here
Reporting job here - please apply to this @aneequeahmad

@melvin-bot
Copy link

melvin-bot bot commented Aug 17, 2022

Triggered auto assignment to Contributor-plus team member for initial proposal review - @Santhosh-Sellavel (Exported)

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

melvin-bot bot commented Aug 17, 2022

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

@melvin-bot melvin-bot bot changed the title Cursor is overlapping with amount digit on amount page - Reported by @aneequeahmad [$250] Cursor is overlapping with amount digit on amount page - Reported by @aneequeahmad Aug 17, 2022
@aneequeahmad
Copy link
Contributor

@bfitzexpensify applied to this job. Thanks

@elon-fask
Copy link

Sounds Interesting !!!

@Luke9389
Copy link
Contributor

I'm open to seeing proposals

@ghost
Copy link

ghost commented Aug 21, 2022

Proposal
issue:
Screenshot 2022-08-21 at 2 01 25 PM

we can solve this issue by adding latter spacing to 1 in  inputStyle={[styles.iouAmountTextInput
result:
1

@melvin-bot melvin-bot bot removed the Weekly KSv2 label Aug 21, 2022
@Santhosh-Sellavel
Copy link
Collaborator

@dimasna I really like the idea of decreasing cursor width. Can you share the changes you are proposing (2) to decrease cursor width:? Also, why should we remove the placeholderTextColor?

@shawnborton What are your thoughts above options?

@dimasna
Copy link

dimasna commented Sep 14, 2022

@dimasna I really like the idea of decreasing cursor width. Can you share the changes you are proposing (2) to decrease cursor width:? Also, why should we remove the placeholderTextColor?

@shawnborton What are your thoughts above options?

thanks, for the cursor width, I add new style in styles.xml (android/app/src/main/res/values) :

<!-- Base application theme. -->
<style name="AppTheme" parent="Theme.AppCompat.Light.NoActionBar">
    <!-- Customize your theme here. -->
    <item name="android:textColor">@color/dark</item>

    <!-- Customize width cursor -->
     <item name="android:textCursorDrawable">@drawable/editext_cursor</item> 

    <item name="android:colorEdgeEffect">@color/gray4</item>
    <item name="colorAccent">@color/accent</item>
    <item name="android:statusBarColor">@color/white</item>
    <item name="android:windowLightStatusBar" tools:ignore="NewApi">true</item>
    <item name="android:editTextBackground">@drawable/rn_edit_text_material</item>
</style>

<style name="BootTheme" parent="AppTheme">
    <!-- set the generated bootsplash.xml drawable as activity background -->
    <item name="android:background">@drawable/bootsplash</item>
</style>

with value :

    <shape xmlns:android="http://schemas.android.com/apk/res/android"
        android:shape="rectangle">
        <size android:width="1dip" />
        <solid android:color="#1e90ff" />
    </shape>

I think we should remove placeholderTextColor because overlapping cursor behind placeholder regarding #10604 issue, and actually the default color of placeholder (without placeholderTextColor )is looks similar with the existing placeholderText color

@shawnborton
Copy link
Contributor

I don't think we should remove the placeholder color, because then we end up using a non-brand color. Even when we remove it, it does look like there is still overlap between placeholder and cursor.

We could reduce the width but my fear there is that we then override the native cursors and that might feel a bit strange.

@Santhosh-Sellavel
Copy link
Collaborator

Okay @shawnborton, let's wait for other options!

@dimasna
Copy link

dimasna commented Sep 14, 2022

sorry, I mean we should remove placeholderTextColor if we not change the cursor width. I check cursor width on the mWeb android have the same width
WhatsApp Image 2022-09-14 at 11 11 08 PM

@Santhosh-Sellavel
Copy link
Collaborator

Native cursors not mWeb!

@bfitzexpensify bfitzexpensify changed the title [$1000] Cursor is overlapping with amount digit on amount page - Reported by @aneequeahmad [$2000] Cursor is overlapping with amount digit on amount page - Reported by @aneequeahmad Sep 16, 2022
@bfitzexpensify
Copy link
Contributor

Doubled to $2000

@aneequeahmad
Copy link
Contributor

aneequeahmad commented Sep 17, 2022

Proposal

Problem:

Cursor is overlapping with amount digit due to the following reasons.

  • The cursor width is noticeable on mWeb which can be decreased for mWeb only for this specific amount-input but not needed because the font-family: GTAmericaExp-Regular of amount digit is causing its size to increase and overlap with cursor.

Solution:

  • font-family if set to fontFamily.SYSTEM or some other font-family, it reduces the size of amount digit text and fixes the overlap issue.
font-family-change-effect-f.mov

After fix Mweb:

mWeb-with-bold.MP4

After fix IOS:

fix-ios-with-bold.mov

NOTE:

  • font-weight is not changed as it doesn't reduce the size from outsize of digit.

cc: @shawnborton

@melvin-bot melvin-bot bot added Daily KSv2 and removed Weekly KSv2 labels Sep 17, 2022
@shawnborton
Copy link
Contributor

That is not a valid solution as we do not want to change our brand font.

@aneequeahmad
Copy link
Contributor

@shawnborton

  • we can then reduce the font-weight from bold to normal this will fix the issue too. It will reduce the thickness of text and hence space will be added between cursor and digit.

Video:

fix-font-weight-bug-f.mov

@shawnborton
Copy link
Contributor

We don't want to change the font weight either, as that would be a branding change.

@bfitzexpensify bfitzexpensify removed their assignment Sep 19, 2022
@bfitzexpensify bfitzexpensify added External Added to denote the issue can be worked on by a contributor and removed External Added to denote the issue can be worked on by a contributor labels Sep 19, 2022
@melvin-bot
Copy link

melvin-bot bot commented Sep 19, 2022

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

@bfitzexpensify
Copy link
Contributor

reassigning - ooo for 2 weeks. @puneetlath we're still looking for proposals, last doubled on Friday

@melvin-bot melvin-bot bot added the Overdue label Sep 20, 2022
@ntdiary
Copy link
Contributor

ntdiary commented Sep 20, 2022

Proposal

Hi, I'm also interested in this issue and have found some information these days, hoping it can be helpful. 🙂

related concepts from fontforge:

The EM Square: Also called the ‘em size’ or ‘UPM’. Each character is fitted into its own space container. In an OpenType font, the UPM — or em size is usually set at 1000 units. When the font is used to set type, the em is scaled to the desired point size. This means that for 10 pt type, the 1000 units for instance get scaled to 10 pt.

Kerning: is the adjustment of the spacing between specific character pairs. Common examples of character pairs where kerning is often needed to improve spacing would be ‘WA’, ‘Wa’, ‘To’, and more...

side bearings: each glyph has a left side bearing and a right side bearing, like this image

Platform differences

Android (app) vs IOS (app / mWeb)

Their cursor width is 2, but Android draws the cursor first and then draws the text, IOS is the opposite

Android app vs Android chrome

Cursor width is 1 and color is black in chrome.

Cause

some glyph metrics for GTAmericaExp-Bold:

some glyph metrics for GTAmericaExp-Regular:

LBearing means the left side bearing.
The em size of GTAmericaExp is 1000.
There are also some kerning rules in this font family.
(For example, the kerning of To is -60, the spacing between them is smaller)

Cursor is acutally a movable rectangle at the insertion point.(sometimes called caret)
We can use this formula to estimate if the cursor will overlap with the first letter (ignore letter space, its default value is 0):
(LBearing * fontSize / 1000) < cursorWidth

For example, for AmountTextInput component:
placeholder is 0, its LBearing is 34, its fontSize is 40,
(34 * 40 / 1000) = 1.36, 1.36 < 2, so overlap will occur.

In different fonts, each glyph may have a different LBearing,
so there may not be a simple and perfect solution to avoid this overlap.
Even if we can just move the placeholder to the right for a short distance, then we can't guarantee that the first letter is just next to the cursor, there will always be a gap between them.
And then the overlap will still occur when we move the cursor between characters:

android-app.mp4
android-chrome.mp4

In summary

I think this overlap may be normal. And this is a screen recording of WhatsApp (also overlaps):

android-WhatsApp.mp4

If we still want to move the placeholder letters to the right for a short distance:
Could add a hair-space prefix to placeholder. (\u200a, in traditional typography, the thinnest space available)

diff --git a/src/components/TextInput/BaseTextInput.js b/src/components/TextInput/BaseTextInput.js
index eec708d91..151a09f83 100644
--- a/src/components/TextInput/BaseTextInput.js
+++ b/src/components/TextInput/BaseTextInput.js
@@ -18,6 +18,7 @@ import variables from '../../styles/variables';
 import Checkbox from '../Checkbox';
 import getSecureEntryKeyboardType from '../../libs/getSecureEntryKeyboardType';
 import CONST from '../../CONST';
+import prefixSpace from './prefixSpace';

 class BaseTextInput extends Component {
     constructor(props) {
@@ -263,7 +264,7 @@ class BaseTextInput extends Component {
                                         }}
                                         // eslint-disable-next-line
                                         {...inputProps}
-                                        placeholder={placeholder}
+                                        placeholder={prefixSpace(placeholder)}
                                         placeholderTextColor={themeColors.placeholderText}
                                         underlineColorAndroid="transparent"
                                         style={[

Finally, We couldn't just move the placeholder in the native component while keeping the cursor position unchanged.
Although we can also try to use text component instead of placeholder, or customize our own native component,
I think they are too complicated for this issue.(I have tried and then gave up 😅)

In addition.
it is easy to change the width and color on Android app.
changing width on IOS requires customize react-native native component.
-webkit-input-placeholder selector can move the placeholder letters to the right on the web, but it doesn't work for app.

@shawnborton
Copy link
Contributor

This is such a great and comprehensive proposal @ntdiary - thanks for putting it together. Ultimately after some discussion in Slack (cc @Julesssss), we agree that it's not worth solving this one and we should just close this out.

That being said, we think you should be rewarded for the diagnostic work that went into your proposal, so we'd like to pay you 25% of the bug amount - cc @puneetlath to process that.

Let me know how that sounds, thanks!

@melvin-bot melvin-bot bot removed the Overdue label Sep 20, 2022
@puneetlath
Copy link
Contributor

@ntdiary - please apply to the Upwork job here, thanks! https://www.upwork.com/jobs/~01017a317b4168596f

@ntdiary
Copy link
Contributor

ntdiary commented Sep 20, 2022

we agree that it's not worth solving this one and we should just close this out.

@shawnborton, I'm glad we have the same opinion, and thank you very much for the 25% reward. I will also pick up more issues in the repo, believe we will meet again, pleasant experience. 😄

@puneetlath Thanks, I've applied. 🙂

@shawnborton
Copy link
Contributor

@ntdiary glad to hear it! Excited to see more of your wonderful proposals, and hope to work together soon!

@puneetlath
Copy link
Contributor

Paid, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Daily KSv2 Engineering External Added to denote the issue can be worked on by a contributor Help Wanted Apply this label when an issue is open to proposals by contributors
Projects
None yet
Development

No branches or pull requests