From 9cdfb95b1ffa8a2ecc9803340a1d1a92bd917c20 Mon Sep 17 00:00:00 2001 From: Adam Barth Date: Fri, 1 Sep 2017 12:55:31 -0700 Subject: [PATCH] Add RTL support to AppBar (#11834) Fixes #11381 --- .../flutter/lib/src/material/app_bar.dart | 2 +- .../lib/src/widgets/navigation_toolbar.dart | 69 ++++++++--- .../flutter/test/material/app_bar_test.dart | 113 ++++++++++++++++-- .../test/widgets/nested_scroll_view_test.dart | 40 ++++--- .../test/widgets/sliver_semantics_test.dart | 41 ++++--- 5 files changed, 204 insertions(+), 61 deletions(-) diff --git a/packages/flutter/lib/src/material/app_bar.dart b/packages/flutter/lib/src/material/app_bar.dart index f4c72d0399..8cb764c336 100644 --- a/packages/flutter/lib/src/material/app_bar.dart +++ b/packages/flutter/lib/src/material/app_bar.dart @@ -387,7 +387,7 @@ class _AppBarState extends State { } final Widget toolbar = new Padding( - padding: const EdgeInsets.only(right: 4.0), + padding: const EdgeInsetsDirectional.only(end: 4.0), child: new NavigationToolbar( leading: leading, middle: title, diff --git a/packages/flutter/lib/src/widgets/navigation_toolbar.dart b/packages/flutter/lib/src/widgets/navigation_toolbar.dart index 37391f6c3d..f73f70ab95 100644 --- a/packages/flutter/lib/src/widgets/navigation_toolbar.dart +++ b/packages/flutter/lib/src/widgets/navigation_toolbar.dart @@ -4,6 +4,7 @@ import 'dart:math' as math; +import 'package:flutter/foundation.dart'; import 'package:flutter/rendering.dart'; import 'basic.dart'; @@ -59,9 +60,12 @@ class NavigationToolbar extends StatelessWidget { if (trailing != null) children.add(new LayoutId(id: _ToolbarSlot.trailing, child: trailing)); + final TextDirection textDirection = Directionality.of(context); + assert(textDirection != null); return new CustomMultiChildLayout( delegate: new _ToolbarLayout( centerMiddle: centerMiddle, + textDirection: textDirection, ), children: children, ); @@ -76,17 +80,20 @@ enum _ToolbarSlot { const double _kMiddleMargin = 16.0; -// TODO(xster): support RTL. class _ToolbarLayout extends MultiChildLayoutDelegate { - _ToolbarLayout({ this.centerMiddle }); + _ToolbarLayout({ + this.centerMiddle, + @required this.textDirection, + }) : assert(textDirection != null); - // If false the middle widget should be left justified within the space + // If false the middle widget should be start-justified within the space // between the leading and trailing widgets. // If true the middle widget is centered within the toolbar (not within the horizontal // space between the leading and trailing widgets). - // TODO(xster): document RTL once supported. final bool centerMiddle; + final TextDirection textDirection; + @override void performLayout(Size size) { double leadingWidth = 0.0; @@ -100,16 +107,33 @@ class _ToolbarLayout extends MultiChildLayoutDelegate { maxHeight: size.height, ); leadingWidth = layoutChild(_ToolbarSlot.leading, constraints).width; - positionChild(_ToolbarSlot.leading, Offset.zero); + double leadingX; + switch (textDirection) { + case TextDirection.rtl: + leadingX = size.width - leadingWidth; + break; + case TextDirection.ltr: + leadingX = 0.0; + break; + } + positionChild(_ToolbarSlot.leading, new Offset(leadingX, 0.0)); } if (hasChild(_ToolbarSlot.trailing)) { final BoxConstraints constraints = new BoxConstraints.loose(size); final Size trailingSize = layoutChild(_ToolbarSlot.trailing, constraints); - final double trailingLeft = size.width - trailingSize.width; - final double trailingTop = (size.height - trailingSize.height) / 2.0; + double trailingX; + switch (textDirection) { + case TextDirection.rtl: + trailingX = 0.0; + break; + case TextDirection.ltr: + trailingX = size.width - trailingSize.width; + break; + } + final double trailingY = (size.height - trailingSize.height) / 2.0; trailingWidth = trailingSize.width; - positionChild(_ToolbarSlot.trailing, new Offset(trailingLeft, trailingTop)); + positionChild(_ToolbarSlot.trailing, new Offset(trailingX, trailingY)); } if (hasChild(_ToolbarSlot.middle)) { @@ -117,17 +141,27 @@ class _ToolbarLayout extends MultiChildLayoutDelegate { final BoxConstraints constraints = new BoxConstraints.loose(size).copyWith(maxWidth: maxWidth); final Size middleSize = layoutChild(_ToolbarSlot.middle, constraints); - final double middleLeftMargin = leadingWidth + _kMiddleMargin; - double middleX = middleLeftMargin; + final double middleStartMargin = leadingWidth + _kMiddleMargin; + double middleStart = middleStartMargin; final double middleY = (size.height - middleSize.height) / 2.0; // If the centered middle will not fit between the leading and trailing // widgets, then align its left or right edge with the adjacent boundary. if (centerMiddle) { - middleX = (size.width - middleSize.width) / 2.0; - if (middleX + middleSize.width > size.width - trailingWidth) - middleX = size.width - trailingWidth - middleSize.width; - else if (middleX < middleLeftMargin) - middleX = middleLeftMargin; + middleStart = (size.width - middleSize.width) / 2.0; + if (middleStart + middleSize.width > size.width - trailingWidth) + middleStart = size.width - trailingWidth - middleSize.width; + else if (middleStart < middleStartMargin) + middleStart = middleStartMargin; + } + + double middleX; + switch (textDirection) { + case TextDirection.rtl: + middleX = size.width - middleSize.width - middleStart; + break; + case TextDirection.ltr: + middleX = middleStart; + break; } positionChild(_ToolbarSlot.middle, new Offset(middleX, middleY)); @@ -135,5 +169,8 @@ class _ToolbarLayout extends MultiChildLayoutDelegate { } @override - bool shouldRelayout(_ToolbarLayout oldDelegate) => centerMiddle != oldDelegate.centerMiddle; + bool shouldRelayout(_ToolbarLayout oldDelegate) { + return oldDelegate.centerMiddle != centerMiddle + || oldDelegate.textDirection != textDirection; + } } diff --git a/packages/flutter/test/material/app_bar_test.dart b/packages/flutter/test/material/app_bar_test.dart index 8ad2c3ebdf..c0b4df2577 100644 --- a/packages/flutter/test/material/app_bar_test.dart +++ b/packages/flutter/test/material/app_bar_test.dart @@ -157,7 +157,7 @@ void main() { expect(center.dx, lessThan(400 + size.width / 2.0)); }); - testWidgets('AppBar centerTitle:false title left edge is 16.0 ', (WidgetTester tester) async { + testWidgets('AppBar centerTitle:false title start edge is 16.0 (LTR)', (WidgetTester tester) async { await tester.pumpWidget( new MaterialApp( home: new Scaffold( @@ -172,8 +172,26 @@ void main() { expect(tester.getTopLeft(find.text('X')).dx, 16.0); }); + testWidgets('AppBar centerTitle:false title start edge is 16.0 (RTL)', (WidgetTester tester) async { + await tester.pumpWidget( + new MaterialApp( + home: new Directionality( + textDirection: TextDirection.rtl, + child: new Scaffold( + appBar: new AppBar( + centerTitle: false, + title: const Text('X'), + ), + ), + ), + ), + ); + + expect(tester.getTopRight(find.text('X')).dx, 800.0 - 16.0); + }); + testWidgets( - 'AppBar centerTitle:false leading button title left edge is 72.0 ', + 'AppBar centerTitle:false leading button title left edge is 72.0 (LTR)', (WidgetTester tester) async { await tester.pumpWidget( new MaterialApp( @@ -191,6 +209,28 @@ void main() { expect(tester.getTopLeft(find.text('X')).dx, 72.0); }); + testWidgets( + 'AppBar centerTitle:false leading button title left edge is 72.0 (RTL)', + (WidgetTester tester) async { + await tester.pumpWidget( + new MaterialApp( + home: new Directionality( + textDirection: TextDirection.rtl, + child: new Scaffold( + appBar: new AppBar( + centerTitle: false, + title: const Text('X'), + ), + // A drawer causes a leading hamburger. + drawer: const Drawer(), + ), + ), + ), + ); + + expect(tester.getTopRight(find.text('X')).dx, 800.0 - 72.0); + }); + testWidgets('AppBar centerTitle:false title overflow OK ', (WidgetTester tester) async { // The app bar's title should be constrained to fit within the available space // between the leading and actions widgets. @@ -249,10 +289,10 @@ void main() { expect(tester.getSize(title).width, equals(800.0 - 4.0 - 56.0 - 16.0 - 16.0 - 200.0)); }); - testWidgets('AppBar centerTitle:true title overflow OK ', (WidgetTester tester) async { + testWidgets('AppBar centerTitle:true title overflow OK (LTR)', (WidgetTester tester) async { // The app bar's title should be constrained to fit within the available space // between the leading and actions widgets. When it's also centered it may - // also be left or right justified if it doesn't fit in the overall center. + // also be start or end justified if it doesn't fit in the overall center. final Key titleKey = new UniqueKey(); double titleWidth = 700.0; @@ -276,8 +316,8 @@ void main() { } // Centering a title with width 700 within the 800 pixel wide test widget - // would mean that its left edge would have to be 50. The material spec says - // that the left edge of the title must be atleast 72. + // would mean that its start edge would have to be 50. The material spec says + // that the start edge of the title must be atleast 72. await tester.pumpWidget(buildApp()); final Finder title = find.byKey(titleKey); @@ -285,9 +325,9 @@ void main() { expect(tester.getSize(title).width, equals(700.0)); // Centering a title with width 620 within the 800 pixel wide test widget - // would mean that its left edge would have to be 90. We reserve 72 - // on the left and the padded actions occupy 96 + 4 on the right. That - // leaves 628, so the title is right justified but its width isn't changed. + // would mean that its start edge would have to be 90. We reserve 72 + // on the start and the padded actions occupy 96 + 4 on the end. That + // leaves 628, so the title is end justified but its width isn't changed. await tester.pumpWidget(buildApp()); leading = null; @@ -301,6 +341,61 @@ void main() { expect(tester.getSize(title).width, equals(620.0)); }); + testWidgets('AppBar centerTitle:true title overflow OK (RTL)', (WidgetTester tester) async { + // The app bar's title should be constrained to fit within the available space + // between the leading and actions widgets. When it's also centered it may + // also be start or end justified if it doesn't fit in the overall center. + + final Key titleKey = new UniqueKey(); + double titleWidth = 700.0; + Widget leading = new Container(); + List actions; + + Widget buildApp() { + return new MaterialApp( + home: new Directionality( + textDirection: TextDirection.rtl, + child: new Scaffold( + appBar: new AppBar( + leading: leading, + centerTitle: true, + title: new Container( + key: titleKey, + constraints: new BoxConstraints.loose(new Size(titleWidth, 1000.0)), + ), + actions: actions, + ), + ), + ), + ); + } + + // Centering a title with width 700 within the 800 pixel wide test widget + // would mean that its start edge would have to be 50. The material spec says + // that the start edge of the title must be atleast 72. + await tester.pumpWidget(buildApp()); + + final Finder title = find.byKey(titleKey); + expect(tester.getTopRight(title).dx, 800.0 - 72.0); + expect(tester.getSize(title).width, equals(700.0)); + + // Centering a title with width 620 within the 800 pixel wide test widget + // would mean that its start edge would have to be 90. We reserve 72 + // on the start and the padded actions occupy 96 + 4 on the end. That + // leaves 628, so the title is end justified but its width isn't changed. + + await tester.pumpWidget(buildApp()); + leading = null; + titleWidth = 620.0; + actions = [ + const SizedBox(width: 48.0), + const SizedBox(width: 48.0) + ]; + await tester.pumpWidget(buildApp()); + expect(tester.getTopRight(title).dx, 620 + 48 + 48 + 4); + expect(tester.getSize(title).width, equals(620.0)); + }); + testWidgets('AppBar with no Scaffold', (WidgetTester tester) async { await tester.pumpWidget( new MaterialApp( diff --git a/packages/flutter/test/widgets/nested_scroll_view_test.dart b/packages/flutter/test/widgets/nested_scroll_view_test.dart index 19bce038ee..0d528fe18d 100644 --- a/packages/flutter/test/widgets/nested_scroll_view_test.dart +++ b/packages/flutter/test/widgets/nested_scroll_view_test.dart @@ -21,10 +21,10 @@ class _CustomPhysics extends ClampingScrollPhysics { } Widget buildTest({ ScrollController controller, String title:'TTTTTTTT' }) { - return new MediaQuery( - data: const MediaQueryData(), - child: new Directionality( - textDirection: TextDirection.ltr, + return new Directionality( + textDirection: TextDirection.ltr, + child: new MediaQuery( + data: const MediaQueryData(), child: new Scaffold( body: new DefaultTabController( length: 4, @@ -307,20 +307,24 @@ void main() { }); testWidgets('NestedScrollViews with custom physics', (WidgetTester tester) async { - await tester.pumpWidget(new MediaQuery( - data: const MediaQueryData(), - child: new NestedScrollView( - physics: const _CustomPhysics(), - headerSliverBuilder: (BuildContext context, bool innerBoxIsScrolled) { - return [ - const SliverAppBar( - floating: true, - title: const Text('AA'), - ), - ]; - }, - body: new Container(), - ))); + await tester.pumpWidget(new Directionality( + textDirection: TextDirection.ltr, + child: new MediaQuery( + data: const MediaQueryData(), + child: new NestedScrollView( + physics: const _CustomPhysics(), + headerSliverBuilder: (BuildContext context, bool innerBoxIsScrolled) { + return [ + const SliverAppBar( + floating: true, + title: const Text('AA'), + ), + ]; + }, + body: new Container(), + ), + ), + )); expect(find.text('AA'), findsOneWidget); await tester.pump(const Duration(milliseconds: 500)); final Offset point1 = tester.getCenter(find.text('AA')); diff --git a/packages/flutter/test/widgets/sliver_semantics_test.dart b/packages/flutter/test/widgets/sliver_semantics_test.dart index e9f28dab8f..56ca149d35 100644 --- a/packages/flutter/test/widgets/sliver_semantics_test.dart +++ b/packages/flutter/test/widgets/sliver_semantics_test.dart @@ -23,8 +23,9 @@ void main() { child: new Text('Item $i'), ); }); - await tester.pumpWidget( - new MediaQuery( + await tester.pumpWidget(new Directionality( + textDirection: TextDirection.ltr, + child: new MediaQuery( data: const MediaQueryData(), child: new CustomScrollView( controller: scrollController, @@ -39,6 +40,7 @@ void main() { ), ], ), + ), )); // AppBar is child of node with semantic scroll actions. @@ -293,8 +295,9 @@ void main() { ); }); final ScrollController controller = new ScrollController(initialScrollOffset: 280.0); - await tester.pumpWidget( - new MediaQuery( + await tester.pumpWidget(new Directionality( + textDirection: TextDirection.ltr, + child: new MediaQuery( data: const MediaQueryData(), child: new CustomScrollView( slivers: [ @@ -309,7 +312,7 @@ void main() { controller: controller, ), ), - ); + )); // 'Item 0' is covered by app bar. expect(semantics, isNot(includesNodeWith(label: 'Item 0'))); @@ -375,8 +378,9 @@ void main() { ), ); }); - await tester.pumpWidget( - new MediaQuery( + await tester.pumpWidget(new Directionality( + textDirection: TextDirection.ltr, + child: new MediaQuery( data: const MediaQueryData(), child: new CustomScrollView( controller: controller, @@ -388,7 +392,7 @@ void main() { ]..addAll(slivers), ), ), - ); + )); // 'Item 0' is covered by app bar. expect(semantics, isNot(includesNodeWith(label: 'Item 0'))); @@ -452,8 +456,9 @@ void main() { ); }); final ScrollController controller = new ScrollController(initialScrollOffset: 280.0); - await tester.pumpWidget( - new MediaQuery( + await tester.pumpWidget(new Directionality( + textDirection: TextDirection.ltr, + child: new MediaQuery( data: const MediaQueryData(), child: new CustomScrollView( reverse: true, // This is the important setting for this test. @@ -469,7 +474,7 @@ void main() { controller: controller, ), ), - ); + )); // 'Item 0' is covered by app bar. expect(semantics, isNot(includesNodeWith(label: 'Item 0'))); @@ -536,8 +541,9 @@ void main() { ), ); }); - await tester.pumpWidget( - new MediaQuery( + await tester.pumpWidget(new Directionality( + textDirection: TextDirection.ltr, + child: new MediaQuery( data: const MediaQueryData(), child: new CustomScrollView( reverse: true, // This is the important setting for this test. @@ -550,7 +556,7 @@ void main() { ]..addAll(slivers), ), ), - ); + )); // 'Item 0' is covered by app bar. expect(semantics, isNot(includesNodeWith(label: 'Item 0'))); @@ -622,8 +628,9 @@ void main() { child: new Text('Backward Item $i'), ); }); - await tester.pumpWidget( - new MediaQuery( + await tester.pumpWidget(new Directionality( + textDirection: TextDirection.ltr, + child: new MediaQuery( data: const MediaQueryData(), child: new Scrollable( controller: controller, @@ -658,7 +665,7 @@ void main() { }, ), ), - ); + )); // 'Forward Item 0' is covered by app bar. expect(semantics, isNot(includesNodeWith(label: 'Forward Item 0')));