From a25bbc7bfca05f0e217358ffa3e11900f54fb609 Mon Sep 17 00:00:00 2001 From: Greg Spencer Date: Mon, 14 May 2018 15:51:12 -0700 Subject: [PATCH] Fixing list tile size in the presence of a large subtitle (#17580) Take into account the subtitle height when adjusting the height of the widget. Added a test. --- .../flutter/lib/src/material/list_tile.dart | 35 +++++++++------- .../flutter/test/material/list_tile_test.dart | 40 ++++++++++++------- 2 files changed, 45 insertions(+), 30 deletions(-) diff --git a/packages/flutter/lib/src/material/list_tile.dart b/packages/flutter/lib/src/material/list_tile.dart index 0bfa596c19..7e0a42e45c 100644 --- a/packages/flutter/lib/src/material/list_tile.dart +++ b/packages/flutter/lib/src/material/list_tile.dart @@ -443,11 +443,11 @@ class ListTile extends StatelessWidget { ); } - const EdgeInsets _kDefaultContentPadding = const EdgeInsets.symmetric(horizontal: 16.0); + const EdgeInsets _defaultContentPadding = const EdgeInsets.symmetric(horizontal: 16.0); final TextDirection textDirection = Directionality.of(context); final EdgeInsets resolvedContentPadding = contentPadding?.resolve(textDirection) ?? tileTheme?.contentPadding?.resolve(textDirection) - ?? _kDefaultContentPadding; + ?? _defaultContentPadding; return new InkWell( onTap: enabled ? onTap : null, @@ -529,8 +529,11 @@ class _RenderListTile extends RenderBox { _isThreeLine = isThreeLine, _textDirection = textDirection; - static const double _kMinLeadingWidth = 40.0; - static const double _kTitleGap = 16.0; // between the titles and the leading/trailing widgets + static const double _minLeadingWidth = 40.0; + // The horizontal gap between the titles and the leading/trailing widgets + static const double _horizontalTitleGap = 16.0; + // The minimum padding on the top and bottom of the title and subtitle widgets. + static const double _minVerticalPadding = 4.0; final Map<_ListTileSlot, RenderBox> slotToChild = <_ListTileSlot, RenderBox>{}; final Map childToSlot = {}; @@ -664,7 +667,7 @@ class _RenderListTile extends RenderBox { @override double computeMinIntrinsicWidth(double height) { final double leadingWidth = leading != null - ? math.max(leading.getMinIntrinsicWidth(height), _kMinLeadingWidth) + _kTitleGap + ? math.max(leading.getMinIntrinsicWidth(height), _minLeadingWidth) + _horizontalTitleGap : 0.0; return leadingWidth + math.max(_minWidth(title, height), _minWidth(subtitle, height)) @@ -674,7 +677,7 @@ class _RenderListTile extends RenderBox { @override double computeMaxIntrinsicWidth(double height) { final double leadingWidth = leading != null - ? math.max(leading.getMaxIntrinsicWidth(height), _kMinLeadingWidth) + _kTitleGap + ? math.max(leading.getMaxIntrinsicWidth(height), _minLeadingWidth) + _horizontalTitleGap : 0.0; return leadingWidth + math.max(_maxWidth(title, height), _maxWidth(subtitle, height)) @@ -746,10 +749,10 @@ class _RenderListTile extends RenderBox { final Size trailingSize = _layoutBox(trailing, looseConstraints); final double titleStart = hasLeading - ? math.max(_kMinLeadingWidth, leadingSize.width) + _kTitleGap + ? math.max(_minLeadingWidth, leadingSize.width) + _horizontalTitleGap : 0.0; final BoxConstraints textConstraints = looseConstraints.tighten( - width: tileWidth - titleStart - (hasTrailing ? trailingSize.width + _kTitleGap : 0.0), + width: tileWidth - titleStart - (hasTrailing ? trailingSize.width + _horizontalTitleGap : 0.0), ); final Size titleSize = _layoutBox(title, textConstraints); final Size subtitleSize = _layoutBox(subtitle, textConstraints); @@ -770,7 +773,7 @@ class _RenderListTile extends RenderBox { double titleY; double subtitleY; if (!hasSubtitle) { - tileHeight = math.max(_defaultTileHeight, titleSize.height); + tileHeight = math.max(_defaultTileHeight, titleSize.height + 2.0 * _minVerticalPadding); titleY = (tileHeight - titleSize.height) / 2.0; } else { titleY = titleBaseline - _boxBaseline(title); @@ -787,11 +790,13 @@ class _RenderListTile extends RenderBox { } // If the title or subtitle overflow tileHeight then punt: title - // and subtitle are arranged in a column, tileHeight = column height. - if (titleY < 0.0 || subtitleY > tileHeight) { - tileHeight = titleSize.height + subtitleSize.height; - titleY = 0.0; - subtitleY = titleSize.height; + // and subtitle are arranged in a column, tileHeight = column height plus + // _minVerticalPadding on top and bottom. + if (titleY < _minVerticalPadding || + (subtitleY + subtitleSize.height + _minVerticalPadding) > tileHeight) { + tileHeight = titleSize.height + subtitleSize.height + 2.0 * _minVerticalPadding; + titleY = _minVerticalPadding; + subtitleY = titleSize.height + _minVerticalPadding; } } @@ -802,7 +807,7 @@ class _RenderListTile extends RenderBox { case TextDirection.rtl: { if (hasLeading) _positionBox(leading, new Offset(tileWidth - leadingSize.width, leadingY)); - final double titleX = hasTrailing ? trailingSize.width + _kTitleGap : 0.0; + final double titleX = hasTrailing ? trailingSize.width + _horizontalTitleGap : 0.0; _positionBox(title, new Offset(titleX, titleY)); if (hasSubtitle) _positionBox(subtitle, new Offset(titleX, subtitleY)); diff --git a/packages/flutter/test/material/list_tile_test.dart b/packages/flutter/test/material/list_tile_test.dart index a2b9e4f49c..886e672352 100644 --- a/packages/flutter/test/material/list_tile_test.dart +++ b/packages/flutter/test/material/list_tile_test.dart @@ -56,8 +56,9 @@ void main() { const double leftPadding = 10.0; const double rightPadding = 20.0; - Widget buildFrame({ bool dense: false, bool isTwoLine: false, bool isThreeLine: false, double textScaleFactor: 1.0 }) { + Widget buildFrame({ bool dense: false, bool isTwoLine: false, bool isThreeLine: false, double textScaleFactor: 1.0, double subtitleScaleFactor }) { hasSubtitle = isTwoLine || isThreeLine; + subtitleScaleFactor ??= textScaleFactor; return new MaterialApp( home: new MediaQuery( data: new MediaQueryData( @@ -69,7 +70,7 @@ void main() { child: new ListTile( leading: new Container(key: leadingKey, width: 24.0, height: 24.0), title: const Text('title'), - subtitle: hasSubtitle ? const Text('subtitle') : null, + subtitle: hasSubtitle ? new Text('subtitle', textScaleFactor: subtitleScaleFactor) : null, trailing: new Container(key: trailingKey, width: 24.0, height: 24.0), dense: dense, isThreeLine: isThreeLine, @@ -91,6 +92,7 @@ void main() { double left(String text) => tester.getTopLeft(find.text(text)).dx; double top(String text) => tester.getTopLeft(find.text(text)).dy; double bottom(String text) => tester.getBottomLeft(find.text(text)).dy; + double height(String text) => tester.getRect(find.text(text)).height; double leftKey(Key key) => tester.getTopLeft(find.byKey(key)).dx; double rightKey(Key key) => tester.getTopRight(find.byKey(key)).dx; @@ -98,7 +100,7 @@ void main() { double heightKey(Key key) => tester.getSize(find.byKey(key)).height; // ListTiles are contained by a SafeArea defined like this: - // SafeArea(top: false, bottom: false, minimim: contentPadding) + // SafeArea(top: false, bottom: false, minimum: contentPadding) // The default contentPadding is 16.0 on the left and right. void testHorizontalGeometry() { expect(leftKey(leadingKey), math.max(16.0, leftPadding)); @@ -111,9 +113,15 @@ void main() { } void testVerticalGeometry(double expectedHeight) { - expect(tester.getSize(find.byType(ListTile)), new Size(800.0, expectedHeight)); - if (hasSubtitle) + final Rect tileRect = tester.getRect(find.byType(ListTile)); + expect(tileRect.size, new Size(800.0, expectedHeight)); + expect(top('title'), greaterThanOrEqualTo(tileRect.top)); + if (hasSubtitle) { expect(top('subtitle'), greaterThanOrEqualTo(bottom('title'))); + expect(bottom('subtitle'), lessThan(tileRect.bottom)); + } else { + expect(top('title'), equals(tileRect.top + (tileRect.height - height('title')) / 2.0)); + } expect(heightKey(trailingKey), 24.0); } @@ -150,35 +158,40 @@ void main() { await tester.pumpWidget(buildFrame(textScaleFactor: 4.0)); testChildren(); testHorizontalGeometry(); - testVerticalGeometry(64.0); + testVerticalGeometry(72.0); await tester.pumpWidget(buildFrame(dense: true, textScaleFactor: 4.0)); testChildren(); testHorizontalGeometry(); - testVerticalGeometry(64.0); + testVerticalGeometry(72.0); await tester.pumpWidget(buildFrame(isTwoLine: true, textScaleFactor: 4.0)); testChildren(); testHorizontalGeometry(); - testVerticalGeometry(120.0); + testVerticalGeometry(128.0); + + // Make sure that the height of a large subtitle is taken into account. + await tester.pumpWidget(buildFrame(isTwoLine: true, textScaleFactor: 0.5, subtitleScaleFactor: 4.0)); + testChildren(); + testHorizontalGeometry(); + testVerticalGeometry(72.0); await tester.pumpWidget(buildFrame(isTwoLine: true, dense: true, textScaleFactor: 4.0)); testChildren(); testHorizontalGeometry(); - testVerticalGeometry(120.0); + testVerticalGeometry(128.0); await tester.pumpWidget(buildFrame(isThreeLine: true, textScaleFactor: 4.0)); testChildren(); testHorizontalGeometry(); - testVerticalGeometry(120.0); + testVerticalGeometry(128.0); await tester.pumpWidget(buildFrame(isThreeLine: true, dense: true, textScaleFactor: 4.0)); testChildren(); testHorizontalGeometry(); - testVerticalGeometry(120.0); + testVerticalGeometry(128.0); }); - testWidgets('ListTile geometry (RTL)', (WidgetTester tester) async { const double leftPadding = 10.0; const double rightPadding = 20.0; @@ -477,7 +490,6 @@ void main() { expect(right('L'), 790.0); // 800 - contentPadding.start }); - testWidgets('ListTileTheme wide leading Widget', (WidgetTester tester) async { const Key leadingKey = const ValueKey('L'); @@ -544,7 +556,5 @@ void main() { expect(tester.getBottomLeft(find.byKey(leadingKey)), const Offset(800.0 - 56.0, 52.0)); expect(right('title'), 800.0 - 72.0); expect(right('subtitle'), 800.0 - 72.0); - }); - }