-
Notifications
You must be signed in to change notification settings - Fork 1.9k
[All] Basic Right-To-Left Support #1222
Conversation
In UWP, Unlike entry behavior, Editor text is right-aligned if text already have rtl language value regardless of the selected language/locale, Also if keyboard changed to rtl language while editor text is empty, Editor will align text to right as you typing. |
Needs docs update |
@maamountki Yes, sorry, what I meant to say is that I am unable to force the text alignment, but it does properly align itself with the device language/locale. Thank you! |
Xamarin.Forms.Core/FlowDirection.cs
Outdated
public enum FlowDirection | ||
{ | ||
MatchParent = 0, | ||
LeftToRight = 1, |
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.
Indentation screwy (my fault)
|
||
_effectiveFlowDirection = value; | ||
InvalidateMeasureInternal(InvalidationTrigger.Undefined); | ||
OnPropertyChanged(FlowDirectionProperty.PropertyName); |
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.
This may have some negative impacts on anyone binding this property. I cant imagine why anyone would be TwoWay binding this, but ya know.
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.
this one isn't bindable, @jassmith. I don't get the comment
if (VisualElementController == null || NativeView == null) | ||
return; | ||
|
||
if (VisualElementController.EffectiveFlowDirection.HasFlag(EffectiveFlowDirection.RightToLeft)) |
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.
HasFlag is super evil and slow. Like really REALLY slow. Don't use HasFlag.
Approved pending tests and fixing HasFlag |
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 reviewed the Core
part, and wow, this is impressive! I left a few comments/questions but I like this.
|
||
public class FlowDirectionGalleryLandingPage : ContentPage | ||
{ | ||
FlowDirection DeviceDirection => Device.Info.CurrentFlowDirection; |
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.
is Device.Info.CurrentFlowDirection
the recommended way to access the FlowDirection?
if so, it causes 2 problems:
- Device.Info is attributed with
[EditorBrowsable(EditorBrowsableState.Never)]
- it makes it hard to access from a
XAML
page. the FlowDirection should probably be exposed at theDevice
level, so one can do<ContentPage FlowDirection="{x:Static Device.FlowDirection}" ... />
(x:Static
syntax isType.Member
and doesn't supportType.Member.Member
)
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 like it. Trying it out now...
Xamarin.Forms.Core/BindableObject.cs
Outdated
@@ -605,7 +604,8 @@ enum BindableContextAttributes | |||
IsBeingSet = 1 << 1, | |||
IsDynamicResource = 1 << 2, | |||
IsSetFromStyle = 1 << 3, | |||
IsDefaultValue = 1 << 4 | |||
IsDefaultValue = 1 << 4, | |||
IsInherited = 1 << 5, |
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.
do we support Inherited BPs ? I don't see this flag used anywhere
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.
sorry thats leftover from me when I was doing something else with this
Xamarin.Forms.Core/Cells/Cell.cs
Outdated
@@ -18,6 +19,24 @@ public abstract class Cell : Element, ICellController | |||
|
|||
bool _nextCallToForceUpdateSizeQueued; | |||
|
|||
EffectiveFlowDirection _effectiveFlowDirection = EffectiveFlowDirection.LeftToRight | EffectiveFlowDirection.Implicit; | |||
EffectiveFlowDirection IFlowDirectionController.EffectiveFlowDirection |
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.
as you're implementing this explicitly, I guess this is not meant to be set directly by the user, so there's no point in making this property bindable ?
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.
Correct
Xamarin.Forms.Core/Cells/Cell.cs
Outdated
OnPropertyChanged(VisualElement.FlowDirectionProperty.PropertyName); | ||
} | ||
} | ||
IFlowDirectionController FlowController => this; |
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.
Style Police here. You're missing a white line. Please pull over !
(well, fix it only if you have other things to push)
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.
👮
Xamarin.Forms.Core/Cells/Cell.cs
Outdated
OnPropertyChanged(VisualElement.FlowDirectionProperty.PropertyName); | ||
} | ||
} | ||
IFlowDirectionController FlowController => this; |
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 I understand this right, you added this property so you don't have to cast to IFlowDirectionController
everytime you want to invoke an explicitly implemented interface method ? If so, make it a field
IFlowDirectionController _flowController = this;
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.
Can't set a field to this
.
return EffectiveFlowDirection.RightToLeft | mode; | ||
} | ||
|
||
throw new InvalidOperationException($"Cannot convert {self} to {nameof(EffectiveFlowDirection)}."); |
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.
you mean default:
[EditorBrowsable(EditorBrowsableState.Never)] | ||
public static bool IsRightToLeft(this EffectiveFlowDirection self) | ||
{ | ||
return (self & EffectiveFlowDirection.RightToLeft) != 0; |
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.
the canonical way to check a flag is (self & EffectiveFlowDirection.RightToLeft) == EffectiveFlowDirection.RightToLeft
it's not important when the flag has only a single bit set, but otherwise, it is
Xamarin.Forms.Core/Layout.cs
Outdated
{ | ||
isRightToLeft = parent.EffectiveFlowDirection.IsRightToLeft(); | ||
if (isRightToLeft) | ||
{ |
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'd reduce nesting
if (parent != null && (isRightToLeft = parent.EffectiveFlowDirection.IsRightToLeft()))
region = new Rectangle(parent.Width - region.Right, region.Y, region.Width, region.Height);
|
||
_effectiveFlowDirection = value; | ||
InvalidateMeasureInternal(InvalidationTrigger.Undefined); | ||
OnPropertyChanged(FlowDirectionProperty.PropertyName); |
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.
this one isn't bindable, @jassmith. I don't get the comment
namespace Xamarin.Forms | ||
{ | ||
[Flags] | ||
public enum EffectiveFlowDirection |
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.
most of the implementation hide EffectiveFlowDirection
through explicit interface implementation, so I guess this isn't mean to be used by our users at all ? if so, attribute the type or move it to the Internals namespace
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 should only be set by Core
code (users set FlowDirection
), but it should be exposed via IVisualElementController
so renderers can use it.
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.
API only used by renderers is usually attributed, or in the internals namespace, or both
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.
Right, but our users will want to access this for their custom renderers, so it feels like it should be exposed.
@@ -1,6 +1,6 @@ | |||
<?xml version="1.0" encoding="utf-8"?> | |||
<manifest xmlns:android="http://schemas.android.com/apk/res/android" android:versionCode="1" android:versionName="1.0" package="AndroidControlGallery.AndroidControlGallery" android:installLocation="auto"> | |||
<uses-sdk android:minSdkVersion="15" /> | |||
<uses-sdk android:minSdkVersion="17" /> |
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.
Doesn't this prevent us from testing things for API 15/16 in the ControlGallery?
@@ -58,6 +63,17 @@ void UpdateHeight() | |||
_view.SetRenderHeight(Cell.RenderHeight); | |||
} | |||
|
|||
void UpdateFlowDirection() |
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.
This same UpdateFlowDirection code is repeated several times - seems like a candidate for an extension method.
@@ -27,6 +27,7 @@ public static class Device | |||
public static void SetIdiom(TargetIdiom value) => Idiom = value; | |||
public static TargetIdiom Idiom { get; internal set; } | |||
|
|||
//TODO: Why are there two of these? This is never used...? |
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.
🤦♂️
public enum EffectiveFlowDirection | ||
{ | ||
LeftToRight = 1 << 0, | ||
RightToLeft = 1 << 1, |
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's never used by users" seriously contradicts the "needs to be public for custom renderers". Please fix as this is public API
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.
Impressive! Most impressive!
Control.FlowDirection = WFlowDirection.RightToLeft; | ||
else if (VisualElementController.EffectiveFlowDirection.IsLeftToRight()) | ||
Control.FlowDirection = WFlowDirection.LeftToRight; | ||
} |
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's not ur fault but it's a shame our conventions lead us to repeat this code in so many places...
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.
The conventions don't force the repetition of the stuff inside UpdateFlowDirection
- all of that could be moved into an extension method.
Need to set minSdkVersion to 17 to test
@Encrypt0r Your post is much more descriptive than mine. Could you please create a new issue with the same content? |
@samhouts |
That's not good! Would you mind opening an issue? Thanks! |
Was anyone able to bind the FlowDirection using ViewModels? |
@samhouts
|
@ali-h2010 Very sorry to have missed your comment! Please open new issues for any problems you encounter so we don't lose them in this thread. Thank you! |
@ali-h2010 check my notes here as well to see if that resolves the issue for you |
* Restart RTL work * Remove IsInherited flag as it never got used * [Core] Unit tests * [Core] FlowDirection * Add FlowDirectionGallery * Android gallery supports RTL Need to set minSdkVersion to 17 to test * iOS gallery supports RTL * UWP gallery supports RTL * [Android] Implement FlowDirection * [iOS] Implement FlowDirection * [macOS] Implement FlowDirection * [UWP] Implement FlowDirection * Update docs * [Core] Simplify EffectiveFlowDirection enum & expose helper extensions Also, TEST TEST TEST * Update docs
In the newest version I download today. I confirm the problem is very different. The Label flow direction totally based on the content. Which mean it will totally based on the font, and Android or Xamarin.Foms auto decide that, |
Description of Change
Adds support for right-to-left layouts.
As with UWP, respecting the device's layout direction based on the selected language/locale is the developer's explicit choice and does not happen automatically. Set the
FlowDirection
of the root View/Page to theDevice.FlowDirection
value. All other child views withMatchParent
will inherit this value.All Views default to the
MatchParent
FlowDirection
. AView
without aParent
will default toLeftToRight
, and this value will be recursively passed down to theView
's children, unless they have an explicitly setFlowDirection
of their own.Renderers can access the
IVisualElementController.EffectiveFlowDirection
to get the actual specified direction for a View/Page and whether the value was inherited (implicit) or explicitly set.Note that you should set the
FlowDirection
once on initial layout. Changing this value causes an expensive relayout and will affect performance.Known Limitations
General
Android
iOS
UWP
macOS
Testing
Android
Users will need to set
android:supportsRtl="true"
and set Android'sMinSDKVersion
to 17 or higher.To test device-specified RTL on Android, either select a language that is RTL or enable
Force RTL layout direction
in the Developer Options. You will also need to add your language to the list of languages your app supports in the Info.plist. See https://developer.xamarin.com/guides/ios/advanced_topics/localization_and_internationalization/ for more information. If you do change your language and region, you may encounter an exception withDatePicker
s. See Bug 59077 for more information and a workaround.iOS
There's no easy way to test device-specified RTL on Xamarin.iOS. The
pseudolanguage
options are Xcode only. You will need to change the language and region to an RTL locale in order to test it. Please note that once you do so,DatePicker
s will throw an exception if you do not include the resources required for alternate calendars. For example, if you're testing in Arabic, be sure to selectmideast
in theiOSBuild
Internationalization
section.The ability to force a control to flip directions was added in iOS 9, so iOS 8 will not be fully supported.
UWP
Similar to iOS, you will need to change the language and region to an RTL locale in order to test device-specified RTL. You will also need to add the language resource to your
Package.appxmanifest
for your app to support it.Visual comparisons (you asked for it, get ready)
Android
iOS
UWP
Bugs Fixed
API Changes
Added:
enum EffectiveFlowDirection
enum FlowDirection
IVisualElementController.EffectiveFlowDirection
VisualElement.FlowDirection
Device.FlowDirection
Behavioral Changes
Android
TextAlignment
instead of theGravity
of theEditText
when we change theHorizontalTextAlignment
property.PR Checklist