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

Fixup direction == 'rtl' #4683

Merged
merged 5 commits into from
Apr 23, 2020
Merged

Fixup direction == 'rtl' #4683

merged 5 commits into from
Apr 23, 2020

Conversation

kmelmon
Copy link
Contributor

@kmelmon kmelmon commented Apr 22, 2020

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:

  1. Yoga would see the direction == 'rtl' and do layout in an RTL fashion. This results in absolute positions in a "flipped" coordinate space, which we would then push into XAML.
  2. We also mapped the direction property to the XAML FlowDirection property and set this on the backing XAML element. When FlowDirection == RightToLeft, XAML flips everything.

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:

  1. Do not let yoga layout anything in RTL. Always run yoga layout in LTR.
  2. Push FlowDirection into XAML, and let XAML be the one and only place we flip into RTL. This will do the correct thing for flexbox layout, and also allow native controls like ComboBox to pick up the correct FlowDirection as yoga doesn't do the layout of these controls, XAML does.

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

@kmelmon kmelmon requested a review from a team as a code owner April 22, 2020 23:53
Copy link
Contributor

@kaiguo kaiguo left a 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

@kmelmon
Copy link
Contributor Author

kmelmon commented Apr 23, 2020

@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.

Copy link
Collaborator

@NickGerleman NickGerleman left a 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.

"baseFile": "RNTester\\js\\examples\\RTL\\RTLExample.js",
"baseVersion": "0.62.2",
"baseHash": "70a65dc896406e7ad971f0a9a770b7ea30f43b9e",
"issue": 678
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wrong issue number?

Copy link
Contributor Author

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.

Copy link
Collaborator

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 @@
/**
Copy link
Collaborator

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?

Copy link
Contributor Author

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]

@kmelmon kmelmon merged commit b797774 into microsoft:master Apr 23, 2020
@kmelmon kmelmon deleted the direction-rtl branch April 23, 2020 21:16
facebook-github-bot pushed a commit to facebook/react-native that referenced this pull request Apr 24, 2020
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants