Account Switcher fix emojis cut-off#61111
Conversation
|
Hmm I thought we needed the line height to be 20px here, otherwise the combo of name + login ends up being taller than the avatar to the left. As it stands now, I don't think we should merge this. I think we should keep the line height at 20px, but just remove the overflow: hidden that is cutting the emoji off. |
|
Can you show me a side-by-side of when a display name has an emoji vs when it doesn't? You should easily be able to see the difference in height that we are trying to avoid. Thanks! |
|
@ChavdaSachin friendly bump on this 🙏 |
|
@shawnborton Here you go: |
|
This is what I see, notice how the layout jumps when we remove lineHeight 20? CleanShot.2025-05-05.at.10.14.38.mp4We need to prevent the shift in layout here... curious what other ideas we have. |
|
Sorry I totally missed to update this issue. |
|
I tried by applying both Since this is only happening with IOS native - there could be chances than it is an upstream bug. |
|
@jjcoffee do you have anything on mind, what could be done here? |
|
@ChavdaSachin Like you say, it's strange that it only happens on iOS (and for me it seems to only happen on certain iOS devices - as it didn't happen when I tested the original PR). Can you check what @shawnborton posted here? I'm curious if toggling an emoji on and off causes any layout shift on its own, or if it's simply the enabling/disabling of the line-height that does it. When I test on Chrome (Linux), I don't see any layout shifting at all. desktop-chrome-lineheight-toggle-2025-05-07_10.32.22.mp4 |
|
@shawnborton based on my observations I am suspecting emojis are not actually being rendered in 17px size even though font size is set to 17px they are being upscaled somehow. Could you maybe confirm if emojis being rendered are really 17px ? What I tried to reach above conclusion.
|
|
It doesn't seem like we're moving any closer towards a solution here, unfortunately. I'm thinking that since having emojis in your name is quite an edge case anyway, and that it isn't reproducible on all versions of iOS (I didn't see it during testing of the original PR, for example), maybe we can just add an iOS native-specific fix where we lose the line-height (and accept the minor layout adjustment)? Or I suppose we could just accept that it's a bit buggy on some versions of iOS 😄 Thoughts @shawnborton @ChavdaSachin? |
|
Maybe we should go back to the issue and find different/better proposals that actually solve this then. |
|
Sorry just remembering that the alignment issue was already present on the app before the original PR (see my comment here), so I think we should either:
|
|
We might even want to explicitly add |
|
@shawnborton Unfortunately, that doesn't work on iOS native (where this issue stems from), see @ChavdaSachin's comment. |
|
How about we just bump the line-height slightly? The difference is minuscule, but we could apply it to iOS native only if needed. @ChavdaSachin would need to test with a few different emojis to make sure it covers all bases. As a reminder, this issue was present before the original PR (line height was never set on this element), so it's a bit of scope-creep to fix it here. I think this works fine as an initial solution, though. With 22px line-height: Original line-height: |
|
I don't think we should change the line height to 22px, we want to use our standard text line height of 20px here. |
|
Alrighty! There was no line height set on this element before the previous PR, so I think we should proceed with this PR as-is to fix the core issue (emojis getting cropped), and create a separate issue to handle the alignment when emojis are in the display name. Since the fix is non-trivial I think it's unfair to expect the contributor to fix it here. Does that sound good? |
|
Hmm I'm not sure if I think we should merge this PR, it seems like it is going to make things worse by:
|
|
Just for a bit of extra context, the reason why the cropping happens here but not in the profile settings page (under
|
|
If that's the case, I would suggest we try something that only styles the emoji itself but maintains the same vertical alignment within the 20px box we expect for the name. Again, that area should only be 20px tall. |
|
The underlying issue here is that iOS clips emojis when their bounding box exceeds the line height, so you can't style the emoji separately whilst also maintaining a fixed line height on the parent element. In other words I don't think what you're asking is possible here. I suspect that the reason the emojis were rendered at 15px originally here was to work around this exact issue, as the smaller font size combined with the 1.3x larger line height gives the emoji glyph sufficient breathing room to render without clipping. |
|
I am sorry I wasn't able to participate in the conversation, but I am OOO today. Tomorrow I would try to come up with something concrete which should help us conclude this issue. What ideas I have on my mind as of now
|
|
@jjcoffee this seems to be a more widespread problem. I could notice - emojis are being cutoff in composer and other input fields too. Checkout these attachments.
|
|
This issue has been around for really long time. We could make use of the same component for this issue. - <Text
- numberOfLines={1}
+ <TextWithEmojiFragment
+ message={currentUserPersonalDetails?.displayName}
style={[styles.textBold, styles.textLarge, styles.flexShrink1
, styles.lineHeightXLarge]}
- >
- {currentUserPersonalDetails?.displayName}
- </Text>
+ />
does this look good enough @shawnborton ? |
|
That does look better, yes. |
|
@ChavdaSachin Do you want to go ahead and implement the change you suggested? |
d894c76 to
95b68fb
Compare
|
Changes are up. Ready for review, Screenshots are updated. cc. @jjcoffee |
Reviewer Checklist
Screenshots/Videos |
|
I'm having problems building iOS, I'll have to retry on Monday. @ChavdaSachin Are you able to provide a screenshot on iPhone 13? Just so we're sure this is fixed on the tester device from the original issue. |
|
Alright I would test it on iPhone 13. I also faced a build issue on Android, but locally merging with the main branch and reinstall modules and pods worked. |
|
@ChavdaSachin Bump on this. Also could you merge main? I'm still unable to test iOS but only on this PR now. |
|
would get it done within 24 hrs. |
…itcher-emojis-cut-off
|
@jjcoffee do let me know if you're facing any issue. And you could trigger an adHoc build and test(if you have a physical device). |
|
Thanks! And sorry, I had more urgent tasks to take care of. |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚀 Deployed to staging by https://github.com/MariaHCD in version: 9.1.60-1 🚀
|
|
🚀 Deployed to production by https://github.com/roryabraham in version: 9.1.61-0 🚀
|
|
🚀 Deployed to staging by https://github.com/MariaHCD in version: 9.1.62-0 🚀
|
|
🚀 Deployed to production by https://github.com/Julesssss in version: 9.1.62-0 🚀
|

















Explanation of Change
Fixed Issues
$ #61030
PROPOSAL: #61030
Tests
Same as QA
Offline tests
Same as QA
QA Steps
// TODO: These must be filled out, or the issue title must include "[No QA]."
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectioncanBeMissingparam foruseOnyxtoggleReportand 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: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari
MacOS: Desktop