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

Border radius is not getting updated correctly in RTL #4499

Closed
JakovS opened this issue Apr 6, 2020 · 2 comments · Fixed by #4683
Closed

Border radius is not getting updated correctly in RTL #4499

JakovS opened this issue Apr 6, 2020 · 2 comments · Fixed by #4683
Assignees
Labels
Area: Borders and Brushes Area: Layout bug Partner: Xbox (label may be applied by bot based on author)
Milestone

Comments

@JakovS
Copy link
Member

JakovS commented Apr 6, 2020

Environment

If you are using latest version:

  1. react-native -v:
    react-native-cli: 2.0.1
    react-native: 0.60.6
  2. react-native run-windows --info:
    System:
    OS: Windows 10 10.0.18363
    CPU: (12) x64 Intel(R) Xeon(R) CPU E5-1650 v3 @ 3.50GHz
    Memory: 19.41 GB / 31.91 GB
    Binaries:
    Node: 10.15.0 - C:\Program Files\nodejs\node.EXE
    Yarn: 1.21.1 - C:\Program Files\nodejs\yarn.CMD
    npm: 6.4.1 - C:\Program Files\nodejs\npm.CMD
    npmPackages:
    react: 16.8.6 => 16.8.6
    react-native: 0.60.6 => 0.60.6
    react-native-windows: 0.60.0-vnext.155 => 0.60.0-vnext.155
    Installed UWP SDKs:
    10.0.16299.0
    10.0.17134.0
    10.0.17749.0
    10.0.17763.0
    10.0.18362.0

Then, specify:

  • Target Platform Version(s):
    10.0.18362.0
  • Target Device(s):
    Desktop
  • Visual Studio Version:
    Visual Studio 2017
  • Build Configuration:
    Debug

Steps to Reproduce

(Write your steps here:)

  1. Create a View and give it a border radius style using one of the: borderTopStartRadius, borderBottomStartRadius, borderTopEndRadius, borderBottomEndRadius
  2. Some time after the View has been mounted, provide a new style with updated values for the above.
  3. Observe in LTR this works as expected.
  4. Switch to RTL, relaunch the app
  5. Notice that on mount the border radius is applied as expected (start on he right, end on the left)
  6. After the style is updated, the radius gets applied as if it was LTR (start on the left, end on the right)

Expected Behavior

I would expect the style to behave the same way that it did on mount.

Actual Behavior

After the style is updated, the radius gets applied as if it was LTR (start on the left, end on the right)

LTR behaves as expected:
LTRradius
RTL after update style gets applied incorrectly (switches sides), on remounting it does applies correctly:
RTLradius

Reproducible Demo

https://snack.expo.io/SLis5ZWht
Put this component in an application somewhere, and launch with an RTL language.

@JakovS JakovS added the bug label Apr 6, 2020
@ghost ghost added the Needs: Triage 🔍 New issue that needs to be reviewed by the issue management team (label applied by bot) label Apr 6, 2020
@chrisglein chrisglein added Area: Borders and Brushes Area: Layout and removed Needs: Triage 🔍 New issue that needs to be reviewed by the issue management team (label applied by bot) labels Apr 6, 2020
@chrisglein chrisglein added this to the 0.62 (M5) milestone Apr 6, 2020
@jagorrin jagorrin added the Partner: Xbox (label may be applied by bot based on author) label Apr 6, 2020
@kmelmon
Copy link
Contributor

kmelmon commented Apr 18, 2020

I can grab this one, should be an easy fix.

@kmelmon
Copy link
Contributor

kmelmon commented Apr 21, 2020

Bug understood. This code is swapping the corner "left" and "right" radius properties before pushing them into XAML:

inline winrt::Windows::UI::Xaml::CornerRadius GetCornerRadius(

There are multiple problems with this:

  1. XAML already swaps "left" and "right" when FlowDirection == RightToLeft, which it is when direction == 'rtl'. This code is incorrectly swapping back.
  2. The incorrect swapping doesn't correctly detect FlowDirection == RightToLeft in all scenarios. It assumes that FlowDirection has already been pushed into the XAML tree, which isn't guaranteed at the time we're calling this function.

Problem 2 explains why we only see the bug sometimes.

The fix for most basic scenario is straightforward - we don't need to do this swapping. So the fix will be to remove the rtl swapping code.

Note that there is another scenario we'll eventually need to handle. If the app calls a special method, I18nManager.swapLeftAndRightInRTL(false), this is supposed to swap left and right back, like what this code is trying to do. We don't yet implement this API, filed #4662 to track.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: Borders and Brushes Area: Layout bug Partner: Xbox (label may be applied by bot based on author)
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants