From 021cf56fc97f96e0571bbb13d2c7a6d66de3beff Mon Sep 17 00:00:00 2001 From: Rami <2364772+rami-a@users.noreply.github.com> Date: Wed, 19 Aug 2020 09:30:28 -0400 Subject: [PATCH] [Material] Relanding fix to ensure time picker input mode lays out correctly in RTL (#64097) --- .../flutter/lib/src/material/time_picker.dart | 164 ++++++++++++------ .../test/material/time_picker_test.dart | 10 ++ .../test/material/time_picker_test.dart | 76 +++++++- 3 files changed, 200 insertions(+), 50 deletions(-) diff --git a/packages/flutter/lib/src/material/time_picker.dart b/packages/flutter/lib/src/material/time_picker.dart index 3b5d093d796..98d8835751c 100644 --- a/packages/flutter/lib/src/material/time_picker.dart +++ b/packages/flutter/lib/src/material/time_picker.dart @@ -1414,58 +1414,69 @@ class _TimePickerInputState extends State<_TimePickerInput> { ), const SizedBox(width: 12.0), ], - Expanded(child: Column( - crossAxisAlignment: CrossAxisAlignment.start, - children: [ - const SizedBox(height: 8.0), - _HourMinuteTextField( - selectedTime: _selectedTime, - isHour: true, - style: hourMinuteStyle, - validator: _validateHour, - onSavedSubmitted: _handleHourSavedSubmitted, - onChanged: _handleHourChanged, - ), - const SizedBox(height: 8.0), - if (!hourHasError && !minuteHasError) - ExcludeSemantics( - child: Text( - MaterialLocalizations.of(context).timePickerHourLabel, - style: theme.textTheme.caption, - maxLines: 1, - overflow: TextOverflow.ellipsis, + Expanded( + child: Row( + crossAxisAlignment: CrossAxisAlignment.start, + // Hour/minutes should not change positions in RTL locales. + textDirection: TextDirection.ltr, + children: [ + Expanded( + child: Column( + crossAxisAlignment: CrossAxisAlignment.start, + children: [ + const SizedBox(height: 8.0), + _HourTextField( + selectedTime: _selectedTime, + style: hourMinuteStyle, + validator: _validateHour, + onSavedSubmitted: _handleHourSavedSubmitted, + onChanged: _handleHourChanged, + ), + const SizedBox(height: 8.0), + if (!hourHasError && !minuteHasError) + ExcludeSemantics( + child: Text( + MaterialLocalizations.of(context).timePickerHourLabel, + style: theme.textTheme.caption, + maxLines: 1, + overflow: TextOverflow.ellipsis, + ), + ), + ], ), ), - ], - )), - Container( - margin: const EdgeInsets.only(top: 8.0), - height: _kTimePickerHeaderControlHeight, - child: _StringFragment(timeOfDayFormat: timeOfDayFormat), + Container( + margin: const EdgeInsets.only(top: 8.0), + height: _kTimePickerHeaderControlHeight, + child: _StringFragment(timeOfDayFormat: timeOfDayFormat), + ), + Expanded( + child: Column( + crossAxisAlignment: CrossAxisAlignment.start, + children: [ + const SizedBox(height: 8.0), + _MinuteTextField( + selectedTime: _selectedTime, + style: hourMinuteStyle, + validator: _validateMinute, + onSavedSubmitted: _handleMinuteSavedSubmitted, + ), + const SizedBox(height: 8.0), + if (!hourHasError && !minuteHasError) + ExcludeSemantics( + child: Text( + MaterialLocalizations.of(context).timePickerMinuteLabel, + style: theme.textTheme.caption, + maxLines: 1, + overflow: TextOverflow.ellipsis, + ), + ), + ], + ), + ), + ], + ), ), - Expanded(child: Column( - crossAxisAlignment: CrossAxisAlignment.start, - children: [ - const SizedBox(height: 8.0), - _HourMinuteTextField( - selectedTime: _selectedTime, - isHour: false, - style: hourMinuteStyle, - validator: _validateMinute, - onSavedSubmitted: _handleMinuteSavedSubmitted, - ), - const SizedBox(height: 8.0), - if (!hourHasError && !minuteHasError) - ExcludeSemantics( - child: Text( - MaterialLocalizations.of(context).timePickerMinuteLabel, - style: theme.textTheme.caption, - maxLines: 1, - overflow: TextOverflow.ellipsis, - ), - ), - ], - )), if (!use24HourDials && timeOfDayFormat != TimeOfDayFormat.a_space_h_colon_mm) ...[ const SizedBox(width: 12.0), _DayPeriodControl( @@ -1489,6 +1500,61 @@ class _TimePickerInputState extends State<_TimePickerInput> { } } +class _HourTextField extends StatelessWidget { + const _HourTextField({ + Key key, + @required this.selectedTime, + @required this.style, + @required this.validator, + @required this.onSavedSubmitted, + @required this.onChanged, + }) : super(key: key); + + final TimeOfDay selectedTime; + final TextStyle style; + final FormFieldValidator validator; + final ValueChanged onSavedSubmitted; + final ValueChanged onChanged; + + @override + Widget build(BuildContext context) { + return _HourMinuteTextField( + selectedTime: selectedTime, + isHour: true, + style: style, + validator: validator, + onSavedSubmitted: onSavedSubmitted, + onChanged: onChanged, + ); + } +} + +class _MinuteTextField extends StatelessWidget { + const _MinuteTextField({ + Key key, + @required this.selectedTime, + @required this.style, + @required this.validator, + @required this.onSavedSubmitted, + }) : super(key: key); + + final TimeOfDay selectedTime; + final TextStyle style; + final FormFieldValidator validator; + final ValueChanged onSavedSubmitted; + + @override + Widget build(BuildContext context) { + return _HourMinuteTextField( + selectedTime: selectedTime, + isHour: false, + style: style, + validator: validator, + onSavedSubmitted: onSavedSubmitted, + ); + } +} + class _HourMinuteTextField extends StatefulWidget { const _HourMinuteTextField({ Key key, diff --git a/packages/flutter/test/material/time_picker_test.dart b/packages/flutter/test/material/time_picker_test.dart index 74e02113f76..945067f9eb4 100644 --- a/packages/flutter/test/material/time_picker_test.dart +++ b/packages/flutter/test/material/time_picker_test.dart @@ -806,6 +806,16 @@ void _testsInput() { await finishPicker(tester); expect(result, equals(const TimeOfDay(hour: 8, minute: 15))); }); + + // Fixes regression that was reverted in https://github.com/flutter/flutter/pull/64094#pullrequestreview-469836378. + testWidgets('Ensure hour/minute fields are top-aligned with the separator', (WidgetTester tester) async { + await startPicker(tester, (TimeOfDay time) { }, entryMode: TimePickerEntryMode.input); + final double hourFieldTop = tester.getTopLeft(find.byWidgetPredicate((Widget w) => '${w.runtimeType}' == '_HourTextField')).dy; + final double minuteFieldTop = tester.getTopLeft(find.byWidgetPredicate((Widget w) => '${w.runtimeType}' == '_MinuteTextField')).dy; + final double separatorTop = tester.getTopLeft(find.byWidgetPredicate((Widget w) => '${w.runtimeType}' == '_StringFragment')).dy; + expect(hourFieldTop, separatorTop); + expect(minuteFieldTop, separatorTop); + }); } final Finder findDialPaint = find.descendant( diff --git a/packages/flutter_localizations/test/material/time_picker_test.dart b/packages/flutter_localizations/test/material/time_picker_test.dart index 870d06c9fbf..2d079941594 100644 --- a/packages/flutter_localizations/test/material/time_picker_test.dart +++ b/packages/flutter_localizations/test/material/time_picker_test.dart @@ -8,10 +8,16 @@ import 'package:flutter_localizations/flutter_localizations.dart'; import 'package:flutter_test/flutter_test.dart'; class _TimePickerLauncher extends StatelessWidget { - const _TimePickerLauncher({ Key key, this.onChanged, this.locale }) : super(key: key); + const _TimePickerLauncher({ + Key key, + this.onChanged, + this.locale, + this.entryMode = TimePickerEntryMode.dial, + }) : super(key: key); final ValueChanged onChanged; final Locale locale; + final TimePickerEntryMode entryMode; @override Widget build(BuildContext context) { @@ -28,6 +34,7 @@ class _TimePickerLauncher extends StatelessWidget { onPressed: () async { onChanged(await showTimePicker( context: context, + initialEntryMode: entryMode, initialTime: const TimeOfDay(hour: 7, minute: 0), )); }, @@ -207,6 +214,73 @@ void main() { tester.binding.window.devicePixelRatioTestValue = null; }); + testWidgets('can localize input mode in all known formats', (WidgetTester tester) async { + final Finder stringFragmentTextFinder = find.descendant( + of: find.byWidgetPredicate((Widget w) => '${w.runtimeType}' == '_StringFragment'), + matching: find.byType(Text), + ).first; + final Finder hourControlFinder = find.byWidgetPredicate((Widget w) => '${w.runtimeType}' == '_HourTextField'); + final Finder minuteControlFinder = find.byWidgetPredicate((Widget w) => '${w.runtimeType}' == '_MinuteTextField'); + final Finder dayPeriodControlFinder = find.byWidgetPredicate((Widget w) => '${w.runtimeType}' == '_DayPeriodControl'); + + // TODO(yjbanov): also test `HH.mm` (in_ID), `a h:mm` (ko_KR) and `HH:mm น.` (th_TH) when we have .arb files for them + final List locales = [ + const Locale('en', 'US'), //'h:mm a' + const Locale('en', 'GB'), //'HH:mm' + const Locale('es', 'ES'), //'H:mm' + const Locale('fr', 'CA'), //'HH \'h\' mm' + const Locale('zh', 'ZH'), //'ah:mm' + const Locale('fa', 'IR'), //'H:mm' but RTL + ]; + + for (final Locale locale in locales) { + await tester.pumpWidget(_TimePickerLauncher(onChanged: (TimeOfDay time) { }, locale: locale, entryMode: TimePickerEntryMode.input)); + await tester.tap(find.text('X')); + await tester.pumpAndSettle(const Duration(seconds: 1)); + + final Text stringFragmentText = tester.widget(stringFragmentTextFinder); + final double hourLeftOffset = tester.getTopLeft(hourControlFinder).dx; + final double minuteLeftOffset = tester.getTopLeft(minuteControlFinder).dx; + final double stringFragmentLeftOffset = tester.getTopLeft(stringFragmentTextFinder).dx; + + if (locale == const Locale('en', 'US')) { + final double dayPeriodLeftOffset = tester.getTopLeft(dayPeriodControlFinder).dx; + expect(stringFragmentText.data, ':'); + expect(hourLeftOffset, lessThan(stringFragmentLeftOffset)); + expect(stringFragmentLeftOffset, lessThan(minuteLeftOffset)); + expect(minuteLeftOffset, lessThan(dayPeriodLeftOffset)); + } else if (locale == const Locale('en', 'GB')) { + expect(stringFragmentText.data, ':'); + expect(hourLeftOffset, lessThan(stringFragmentLeftOffset)); + expect(stringFragmentLeftOffset, lessThan(minuteLeftOffset)); + expect(dayPeriodControlFinder, findsNothing); + } else if (locale == const Locale('es', 'ES')) { + expect(stringFragmentText.data, ':'); + expect(hourLeftOffset, lessThan(stringFragmentLeftOffset)); + expect(stringFragmentLeftOffset, lessThan(minuteLeftOffset)); + expect(dayPeriodControlFinder, findsNothing); + } else if (locale == const Locale('fr', 'CA')) { + expect(stringFragmentText.data, 'h'); + expect(hourLeftOffset, lessThan(stringFragmentLeftOffset)); + expect(stringFragmentLeftOffset, lessThan(minuteLeftOffset)); + expect(dayPeriodControlFinder, findsNothing); + } else if (locale == const Locale('zh', 'ZH')) { + final double dayPeriodLeftOffset = tester.getTopLeft(dayPeriodControlFinder).dx; + expect(stringFragmentText.data, ':'); + expect(dayPeriodLeftOffset, lessThan(hourLeftOffset)); + expect(hourLeftOffset, lessThan(stringFragmentLeftOffset)); + expect(stringFragmentLeftOffset, lessThan(minuteLeftOffset)); + } else if (locale == const Locale('fa', 'IR')) { + // Even though this is an RTL locale, the hours and minutes positions should remain the same. + expect(stringFragmentText.data, ':'); + expect(hourLeftOffset, lessThan(stringFragmentLeftOffset)); + expect(stringFragmentLeftOffset, lessThan(minuteLeftOffset)); + expect(dayPeriodControlFinder, findsNothing); + } + await finishPicker(tester); + } + }); + testWidgets('uses single-ring 24-hour dial for all formats', (WidgetTester tester) async { const List locales = [ Locale('en', 'US'), // h