-
Notifications
You must be signed in to change notification settings - Fork 3.5k
fix: Android & iOS - Text is cut off in the search field when device font size is minimum. #65071
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
fix: Android & iOS - Text is cut off in the search field when device font size is minimum. #65071
Conversation
…font size is minimum. Signed-off-by: krishna2323 <belivethatkg@gmail.com>
|
@dominictb Please 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] |
|
The loading indicator will be fixed in this PR. |
|
Ah okay, thanks for confirming. Would still love a quick before/after of the two mobile platforms just for posterity. Thanks! |
|
That's good for the indicator. But I mean before/after of what you are fixing in this specific PR. |
Before (The letter 'g' is half-visible):before.mp4After:after.mp4 |
|
Perfect, that looks pretty good to me 👍 I say we get this into final review then. |
|
@dominictb, please review this when you get a chance. Thanks! |
Signed-off-by: krishna2323 <belivethatkg@gmail.com>
Signed-off-by: krishna2323 <belivethatkg@gmail.com>
Signed-off-by: krishna2323 <belivethatkg@gmail.com>
|
@dominictb, all the above issues have been fixed. Note: All the issues were also reproducible without the original PR changes. FixedMonosnap.screencast.2025-07-01.06-42-18.mp4Reproducible without input update PRMonosnap.screencast.2025-06-30.22-27-00.mp4 |
| tabIndex={-1} | ||
| style={[styles.textInputPrefix, !hasLabel && styles.pv0, styles.pointerEventsNone, prefixStyle]} | ||
| dataSet={{[CONST.SELECTION_SCRAPER_HIDDEN_ELEMENT]: true}} | ||
| shouldUseDefaultLineHeight={!Object.keys(StyleSheet.flatten(prefixStyle)).includes('lineHeight') && !shouldApplyHeight} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is something I asked to fix earlier https://github.com/Expensify/App/pull/63034/files#r2143503208. We should not assume it this way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for missing that. I think this is the right way to handle it because if we introduce a prop for this, we’d need to pass both the prop and lineHeight: undefined to make it work. But with this approach, we can simply pass lineHeight: undefined without any extra steps. Do you still think we should add a prop?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes I think we still need the prop. There might be cases where we intentionally set the lineHeight to undefined just to reset a previously-set lineHeight value but still want to use the default lineHeight in Text component.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated
Signed-off-by: krishna2323 <belivethatkg@gmail.com>
|
Testing again |
|
@dominictb friendly bump ^ |
|
This is not fixed for me #65071 (comment). I THINK it has the same root cause with this one #63034 (comment) which I could but you couldn't reproduce. It was fixed in this commit 05f020d. It would be much much better if you could find out consistent reliable reproducible steps on your device too so we can early spot similar issues. |
|
I'll try to find reliable reproduction steps. |
|
@Krishna2323 Any update? |
|
@dominictb, sorry for delay, I'll try to finish this within 24h. Still trying to reproduce the issue on my end. |
Signed-off-by: krishna2323 <belivethatkg@gmail.com>
|
@dominictb, could you please check this again? The issue occurred because we were using
|
Signed-off-by: krishna2323 <belivethatkg@gmail.com>
Signed-off-by: krishna2323 <belivethatkg@gmail.com>
@dominictb fixed. |
Reviewer Checklist
Screenshots/Videos |
src/styles/utils/index.ts
Outdated
| marginTop: -(inputPaddingTop / 2), | ||
| height: '100%', | ||
| justifyContent: 'center', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you elaborate these changes?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I’ve updated the logic to dynamically center the icon instead of using a fixed marginTop.
Let’s say the input container is 52px tall with:
paddingTop = 8px
paddingBottom = 0pxNormally, the icon container takes the full height after top padding, so its height becomes:
icon height = 52px - 8px = 44pxBut visually, we want the icon to look centered within the full 52px height, not just the 44px.
To achieve that, we calculate the vertical padding difference:
paddingTop - paddingBottom = 8px - 0px = 8pxWe then divide this difference by 2 to get the offset needed for centering:
8px / 2 = 4px
Now, we apply a negative marginTop of -4px to the icon container:
marginTop: -(verticalPaddingDiff / 2)This pulls the icon upward just enough to make it visually centered in the full height, regardless of uneven padding.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But from the implementation I see that you're summing up the vertical paddings, right? Also the param name should be verticalPaddingDiff or something else, inputPaddingTop is not correct.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oops, sorry I forgot to push the local commit.
Signed-off-by: krishna2323 <belivethatkg@gmail.com>
Signed-off-by: krishna2323 <belivethatkg@gmail.com>
dominictb
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
![]()
|
@dominictb The checklist isn’t checked off. 😅 |
|
🚀 Deployed to staging by https://github.com/techievivek in version: 9.1.88-0 🚀
|
|
🚀 Deployed to production by https://github.com/grgia in version: 9.1.88-3 🚀
|















Explanation of Change
Fixed Issues
$ #62907
PROPOSAL:
Tests
Precondition:
4 Verify the text is shown without any cutoff
Offline tests
Precondition:
4 Verify the text is shown without any cutoff
QA Steps
Precondition:
4 Verify the text is shown without any cutoff
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.ScrollViewcomponent to make it scrollable when more elements are added to the page.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Android: Native
android_native.mp4
Android: mWeb Chrome
android_chrome.mp4
iOS: Native
ios_natvie.mp4
iOS: mWeb Safari
ios_safari.mp4
MacOS: Chrome / Safari
web_chrome.mp4
MacOS: Desktop
desktop_app.mp4