-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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
RTL Text gets renderd properly #29434
RTL Text gets renderd properly #29434
Conversation
CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅ |
I have read the CLA Document and I hereby sign the CLA |
@burczu I have made the changes as per the discussion.
But on android native the convertToLTR funciton does not works properly. It breaks the input box functionality.This is because Android does not properly support bidirectional text for mixed content due to missing support for writingDirection, this is what i came to know when I searched on the internet. Thats why I have added an extra check to know whether the platform is android or not |
@burczu I have resolved the merge conflicts and also added changes to ensure proper rendering on Safari as well. |
src/pages/home/report/ReportActionCompose/ComposerWithSuggestions.js
Outdated
Show resolved
Hide resolved
Reviewer Checklist
Screenshots/VideosWebScreen.Recording.2023-10-17.at.10.57.04.movMobile Web - ChromeScreen.Recording.2023-10-19.at.10.00.18.movMobile Web - SafariUnable to test... DesktopScreen.Recording.2023-10-17.at.10.54.36.moviOSScreen.Recording.2023-10-17.at.10.50.05.movAndroidScreen.Recording.2023-10-17.at.10.48.54.mov |
@HardikChoudhary24 I'm testing on all the devices and it looks like something is broken on iOS mWeb - please take a look below: Screen.Recording.2023-10-17.at.11.02.43.movWorks well on other devices (except native Android, but you've explained why). Can you confirm? |
I think you are using laptop keyboard can you try with the inbuilt ios keyboard Screen.Recording.2023-10-17.at.2.45.13.PM.movIts working as expected on my device |
@HardikChoudhary24 I've just tested with native keyboard and the same effect. To be sure I have nothing cached etc, I've also used private tab in Safari... |
@burczu I have tested and checked all possibilities but I am not able to reproduce it. I have also tried on a private tab, but I am getting the expected result. Is there anything you would like to suggest? |
Hey @burczu is the above issue resolved which you were facing during testing or is there anything that I can do on my end as I am not able to reproduce it? |
@HardikChoudhary24 I'm sorry, I didn't have time yet to get back to it... I'll do my best to do it soon. |
@HardikChoudhary24 Checked one more time, but first erased all the content and settings on my iPhone simulator and restarted it. Unfortunately with no luck, it just not working for iOS mWeb... Are you sure you have no uncommitted changes or smth? P.S. You have now merge conflicts. |
I will just resolve the merge conflicts and push the code one more time. After that please check once |
@burczu after deleting and erasing all the content on my current simulator I have checked it again but its still showing me the expected result. Screen.Recording.2023-10-18.at.5.13.00.PM.mov |
Hey @burczu I asked one of my friends to pull the changes and try this once on his physical ios device and he is also getting the expected result. WhatsApp.Video.2023-10-18.at.6.32.43.PM.mp4 |
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.
LGTM, but it must be tested on iOS mWeb, because it didn't work for me.
@neil-marcellini Please check if it works for you on iOS mWeb, because it doesn't for me, but it does for @HardikChoudhary24. Thank you. |
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.
Looking pretty good but I have some questions. I can test on iOS Safari next time
@@ -226,7 +227,7 @@ function ComposerWithSuggestions({ | |||
} | |||
emojisPresentBefore.current = emojis; | |||
setIsCommentEmpty(!!newComment.match(/^(\s)*$/)); | |||
setValue(newComment); | |||
setValue(Platform.OS === 'android' ? newComment : convertToLTR(newComment)); |
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 don't get this. Before we only converted on android, now we are not. Why? Should the logic be flipped?
Also we prefer platform specific files vs using Platform.OS.
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.
Previously we used this only for android to get the desired result for components that are using convertToLTR function but previously we were not using the function for input box. Now we are doing it for those components as well as for the input box also. But in case of android we don't want the text inside input box to get converted by the converToLTR function because it breaks the input box functionality. This is because Android does not properly support bidirectional text for mixed content due to missing support for writingDirection. So now we want to use convertToLTR function for all platforms including android but we don't want it in case of input box of android. So just for the input box of android we need a check to see whether the platform is android or not.
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.
If we make specific file for android in which we return plane text in that case it will apply to all components of android as well for the input box of android also. But we require the function to return plane text just in case of input box of android, we want it to work normally when its used inside other components of android. So to do this I think we can just check the platform when using it inside the input box.
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.
Great thank you for the explanation. I think I understand now. However, we are really strict about not using Platform.OS. You said "now we want to use convertToLTR function for all platforms including android but we don't want it in case of input box of android". So let's just use a custom convertToLTR function in an android specific file for ComposerWithSuggestions. Or we could create a separate module convertToLTRForComposer which has an android specific implementation. That's probably best. Also please include a comment about why we need to do this.
…-text-cursor-position
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.
Ok, thanks for the updates. Looking good to me! @burczu would you please test again before merging?
@neil-marcellini Re-tested and it still does not work as expected on iOS mWeb - could you please check on your side, because it may be just something with my environment. |
Hey @neil-marcellini are you getting the expected result on iOS mWeb or is there anything that I can do on my end because its working as expected on my local. |
Oh right, I'll test today |
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.
It works well for me on iOS Safari
Simulator.Screen.Recording.-.iPhone.15.-.2023-10-26.at.12.52.06.mp4
Simulator.Screen.Recording.-.iPhone.15.-.2023-10-26.at.12.50.23.mp4
@neil-marcellini Thanks for your feedback. Is there anything else that needs to be addressed at my end? |
I wish github had a button for "auto merge if all check pass". |
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
This PR caused regression. Emoji/Mention suggestions not showing at all Screen.Recording.2023-10-27.at.7.08.07.PM.mov |
@situchan , I've identified the root cause of this regression and fixed it. This was happening because of |
@HardikChoudhary24 yes please. I can help review if needed. |
Thanks @situchan. I will raise the PR with the fix. |
@situchan I have raised the PR with the fix. Please review it. |
🚀 Deployed to staging by https://github.com/neil-marcellini in version: 1.3.93-0 🚀
|
🚀 Deployed to production by https://github.com/Beamanator in version: 1.3.93-1 🚀
|
🚀 Deployed to staging by https://github.com/neil-marcellini in version: 1.3.94-0 🚀
|
1 similar comment
🚀 Deployed to staging by https://github.com/neil-marcellini in version: 1.3.94-0 🚀
|
🚀 Deployed to production by https://github.com/Beamanator in version: 1.3.94-2 🚀
|
Details
Chat - Compose box - Wrong cursor position for RTL texts and inverted arrow keys functionality
Fixed Issues
$ #28149
PROPOSAL: #28149 (comment)
Tests
Offline tests
QA Steps
PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)myBool && <MyComponent />
.src/languages/*
files and using the translation methodWaiting for Copy
label for a copy review on the original GH to get the correct copy.STYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)/** comment above it */
this
properly so there are no scoping issues (i.e. foronClick={this.submit}
the methodthis.submit
should be bound tothis
in the constructor)this
are necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);
ifthis.submit
is never passed to a component event handler likeonClick
)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)ScrollView
component to make it scrollable when more elements are added to the page.main
branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTest
steps.Screenshots/Videos
MacOS: Chrome
chrome.mp4
MacOS: Desktop
desktop.mp4
Android: mWeb Chrome
chromeMobile.mp4
iOS: Native
ios.mp4
iOS: mWeb Safari
newMSafari.mp4
MacOS: Safari
NewSafari.mp4
Android: Native
android.mp4