-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Fixup direction == 'rtl' #4683
Fixup direction == 'rtl' #4683
Conversation
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 remember I did something special for Image RTL, could you check and see if this is still needed after your fix?
https://github.com/kaiguo/react-native-windows/blob/master/vnext/ReactUWP/Views/Image/ReactImage.cpp#L36-L38
@kaiguo thanks. Yes this fix is still needed. Since we're still pushing FlowDirection == RightToLeft into the panel it would mirror the image without your change. |
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.
Approving, but please update the manifest with a real issue tracking upstreaming before checking in.
vnext/src/overrides.json
Outdated
"baseFile": "RNTester\\js\\examples\\RTL\\RTLExample.js", | ||
"baseVersion": "0.62.2", | ||
"baseHash": "70a65dc896406e7ad971f0a9a770b7ea30f43b9e", | ||
"issue": 678 |
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.
Wrong issue number?
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 think there's a bug in the script. I typed in 4678 and it truncated or something.
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 think the "prompts" package behavior is to clear the number after a delay if you type something new. I've had the same thing pop up before. I can check if there's an option or new release that fixes the issue.
@@ -0,0 +1,749 @@ | |||
/** |
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.
Could you mark the Windows specific changes with "// [windows" etc?
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.
they already do have // [windows]
Summary: This change upstreams a small change we did in react-native-windows to allow the RTLExample RNTester page to function correctly on Windows. The change is part of this Pr: microsoft/react-native-windows#4683 Currently the direction property is gated behind a check for Platform == 'iOS', which means it only works on iOS. Windows supports direction = 'rtl' so I've chanced this check to Platform != 'android'. ## Changelog <!-- Help reviewers and the release process by writing your own changelog entry. For an example, see: https://github.com/facebook/react-native/wiki/Changelog --> [Internal] [Changed] - Changed RTLExample RNTester page to use direction = 'rtl' when Platform is not Android. Pull Request resolved: #28742 Test Plan: Confirmed this change works correctly in RNTester on Windows. Have not confirmed iOS as I don't have Mac hardware. Differential Revision: D21235579 Pulled By: shergin fbshipit-source-id: 47ab93c2bcd0dbc8347c6746081ae3c64f88faa5
Fixes #4668
Fixes #4646
Fixes #4645
Fixes #4499
This change fixes multiple RTL related issues. At the most basic level, our implementation of direction == 'rtl' was incorrect. The problem here was that we would do two things:
These two cancel each other out in the most basic scenario (a container with children stacked horizontally), so you'd end up with an LTR layout instead of RTL.
Also, there was code in place which tried to manually flip certain properties (padding, margin, and border), but this code was also incorrect. Most importantly, this code is unnecessary. It appears there was just a misunderstanding about how XAML applies these properties - it already flips these properties when in an RTL mode. I manually verified each of these properties is supposed to flip in RTL mode, and XAML does flip them in RTL. Also, after debugging some scenarios, I realized this code was buggy. It was manually querying the FlowDirection of the element having its properties set, which isn't reliable. The FlowDirection property inherits down the tree, so at the time a XAML element is being created, it hasn't inherited the property yet. But later, after it is in the tree, and XAML layout runs, it will. So the result of querying for FlowDirection varies depending on when we update the properties. Yikes! This caused the symptoms we saw in bug #4499, where the corner radius was sometimes flipped, depending on exactly when the property was applied.
This change fixes both sets of issues by doing the following:
As a result of the above, I was able to also remove the code that was trying to manually flip certain properties as it's totally unnecessary.
Also, I noticed the RTLExample RNTester page essentially does nothing on Windows, as the page only actually applies direction == 'rtl' if the Platform is iOS. This is because rtl isn't supported on Android. The fix was simple - just check for "not Android".
Microsoft Reviewers: Open in CodeFlow