From a365c41cb5bfcf31ff76cf2a840a1fd01fc1159d Mon Sep 17 00:00:00 2001 From: Greg Spencer Date: Thu, 10 May 2018 14:37:14 -0700 Subject: [PATCH] Fix handling of null body2 text style for chip and slider (#17311) Before this change, if you specified a non-null textTheme, but the theme you specified didn't have a body2 defined, then creating a ChipTheme would assert (which means creating a ThemeData would fail). This adds handling for this corner case to default to reasonable values in that case. The slider had the same problem, but for accentTextTheme, so I fixed that too. While I had the patient open, Hans and I noticed that TextTheme.merge wasn't doing the right thing in the case where some members were null either, so I fixed that, and added some examples, since merge/copyWith are common operations that are not always well understood. Fixes #17251 --- .../flutter/lib/src/material/theme_data.dart | 9 +- .../flutter/lib/src/material/typography.dart | 100 +++++++++++++++--- .../test/material/theme_data_test.dart | 20 ++++ .../test/material/typography_test.dart | 23 ++++ 4 files changed, 136 insertions(+), 16 deletions(-) diff --git a/packages/flutter/lib/src/material/theme_data.dart b/packages/flutter/lib/src/material/theme_data.dart index dc565bf5ed..6e6e80f767 100644 --- a/packages/flutter/lib/src/material/theme_data.dart +++ b/packages/flutter/lib/src/material/theme_data.dart @@ -144,9 +144,12 @@ class ThemeData extends Diagnosticable { accentIconTheme ??= accentIsDark ? const IconThemeData(color: Colors.white) : const IconThemeData(color: Colors.black); platform ??= defaultTargetPlatform; final Typography typography = new Typography(platform: platform); - textTheme ??= isDark ? typography.white : typography.black; - primaryTextTheme ??= primaryIsDark ? typography.white : typography.black; - accentTextTheme ??= accentIsDark ? typography.white : typography.black; + final TextTheme defaultTextTheme = isDark ? typography.white : typography.black; + textTheme = defaultTextTheme.merge(textTheme); + final TextTheme defaultPrimaryTextTheme = primaryIsDark ? typography.white : typography.black; + primaryTextTheme = defaultPrimaryTextTheme.merge(primaryTextTheme); + final TextTheme defaultAccentTextTheme = accentIsDark ? typography.white : typography.black; + accentTextTheme = defaultAccentTextTheme.merge(accentTextTheme); if (fontFamily != null) { textTheme = textTheme.apply(fontFamily: fontFamily); primaryTextTheme = primaryTextTheme.apply(fontFamily: fontFamily); diff --git a/packages/flutter/lib/src/material/typography.dart b/packages/flutter/lib/src/material/typography.dart index 2f215c1b15..e332854c0d 100644 --- a/packages/flutter/lib/src/material/typography.dart +++ b/packages/flutter/lib/src/material/typography.dart @@ -7,6 +7,8 @@ import 'package:flutter/painting.dart'; import 'colors.dart'; + + /// Material design text theme. /// /// Definitions for the various typographical styles found in material design @@ -103,6 +105,39 @@ class TextTheme extends Diagnosticable { /// Consider using [Typography.black] or [Typography.white], which implement /// the typography styles in the material design specification, as a starting /// point. + /// + /// ## Sample code + /// + /// ```dart + /// /// A Widget that sets the ambient theme's title text color for its + /// /// descendants, while leaving other ambient theme attributes alone. + /// class TitleColorThemeCopy extends StatelessWidget { + /// TitleColorThemeCopy({Key key, this.child, this.titleColor}) : super(key: key); + /// + /// final Color titleColor; + /// final Widget child; + /// + /// @override + /// Widget build(BuildContext context) { + /// final ThemeData theme = Theme.of(context); + /// return new Theme( + /// data: theme.copyWith( + /// textTheme: theme.textTheme.copyWith( + /// title: theme.textTheme.title.copyWith( + /// color: titleColor, + /// ), + /// ), + /// ), + /// child: child, + /// ); + /// } + /// } + /// ``` + /// + /// See also: + /// + /// * [merge] is used instead of [copyWith] when you want to merge all + /// of the fields of a TextTheme instead of individual fields. TextTheme copyWith({ TextStyle display4, TextStyle display3, @@ -139,24 +174,63 @@ class TextTheme extends Diagnosticable { /// the value of [TextStyle.inherit] flag. For more details, see the /// documentation on [TextStyle.merge] and [TextStyle.inherit]. /// + /// If this theme, or the `other` theme has members that are null, then the + /// non-null one (if any) is used. If the `other` theme is itself null, then + /// this [TextTheme] is returned unchanged. If values in both are set, then + /// the values are merged using [TextStyle.merge]. + /// /// This is particularly useful if one [TextTheme] defines one set of - /// properties and another defines a different set, e.g. having colors defined - /// in one text theme and font sizes in another. + /// properties and another defines a different set, e.g. having colors + /// defined in one text theme and font sizes in another, or when one + /// [TextTheme] has only some fields defined, and you want to define the rest + /// by merging it with a default theme. + /// + /// ## Sample code + /// + /// ```dart + /// /// A Widget that sets the ambient theme's title text color for its + /// /// descendants, while leaving other ambient theme attributes alone. + /// class TitleColorTheme extends StatelessWidget { + /// TitleColorTheme({Key key, this.child, this.titleColor}) : super(key: key); + /// + /// final Color titleColor; + /// final Widget child; + /// + /// @override + /// Widget build(BuildContext context) { + /// ThemeData theme = Theme.of(context); + /// // This partialTheme is incomplete: it only has the title style + /// // defined. Just replacing theme.textTheme with partialTheme would + /// // set the title, but everything else would be null. This isn't very + /// // useful, so merge it with the existing theme to keep all of the + /// // preexisting definitions for the other styles. + /// TextTheme partialTheme = new TextTheme(title: new TextStyle(color: titleColor)); + /// theme = theme.copyWith(textTheme: theme.textTheme.merge(partialTheme)); + /// return new Theme(data: theme, child: child); + /// } + /// } + /// ``` + /// + /// See also: + /// + /// * [copyWith] is used instead of [merge] when you wish to override + /// individual fields in the [TextTheme] instead of merging all of the + /// fields of two [TextTheme]s. TextTheme merge(TextTheme other) { if (other == null) return this; return copyWith( - display4: display4.merge(other.display4), - display3: display3.merge(other.display3), - display2: display2.merge(other.display2), - display1: display1.merge(other.display1), - headline: headline.merge(other.headline), - title: title.merge(other.title), - subhead: subhead.merge(other.subhead), - body2: body2.merge(other.body2), - body1: body1.merge(other.body1), - caption: caption.merge(other.caption), - button: button.merge(other.button), + display4: display4?.merge(other.display4) ?? other.display4, + display3: display3?.merge(other.display3) ?? other.display3, + display2: display2?.merge(other.display2) ?? other.display2, + display1: display1?.merge(other.display1) ?? other.display1, + headline: headline?.merge(other.headline) ?? other.headline, + title: title?.merge(other.title) ?? other.title, + subhead: subhead?.merge(other.subhead) ?? other.subhead, + body2: body2?.merge(other.body2) ?? other.body2, + body1: body1?.merge(other.body1) ?? other.body1, + caption: caption?.merge(other.caption) ?? other.caption, + button: button?.merge(other.button) ?? other.button, ); } diff --git a/packages/flutter/test/material/theme_data_test.dart b/packages/flutter/test/material/theme_data_test.dart index c45581771b..37d05e1e95 100644 --- a/packages/flutter/test/material/theme_data_test.dart +++ b/packages/flutter/test/material/theme_data_test.dart @@ -56,6 +56,26 @@ void main() { expect(darkTheme.accentTextTheme.title.color, typography.white.title.color); }); + test('Default slider indicator style gets a default body2 if accentTextTheme.body2 is null', () { + const TextTheme noBody2TextTheme = const TextTheme(body2: null); + final ThemeData lightTheme = new ThemeData(brightness: Brightness.light, accentTextTheme: noBody2TextTheme); + final ThemeData darkTheme = new ThemeData(brightness: Brightness.dark, accentTextTheme: noBody2TextTheme); + final Typography typography = new Typography(platform: lightTheme.platform); + + expect(lightTheme.sliderTheme.valueIndicatorTextStyle, equals(typography.white.body2)); + expect(darkTheme.sliderTheme.valueIndicatorTextStyle, equals(typography.black.body2)); + }); + + test('Default chip label style gets a default body2 if textTheme.body2 is null', () { + const TextTheme noBody2TextTheme = const TextTheme(body2: null); + final ThemeData lightTheme = new ThemeData(brightness: Brightness.light, textTheme: noBody2TextTheme); + final ThemeData darkTheme = new ThemeData(brightness: Brightness.dark, textTheme: noBody2TextTheme); + final Typography typography = new Typography(platform: lightTheme.platform); + + expect(lightTheme.chipTheme.labelStyle.color, equals(typography.black.body2.color.withAlpha(0xde))); + expect(darkTheme.chipTheme.labelStyle.color, equals(typography.white.body2.color.withAlpha(0xde))); + }); + test('Default icon theme contrasts with brightness', () { final ThemeData lightTheme = new ThemeData(brightness: Brightness.light); final ThemeData darkTheme = new ThemeData(brightness: Brightness.dark); diff --git a/packages/flutter/test/material/typography_test.dart b/packages/flutter/test/material/typography_test.dart index 216b1053be..8a773fab10 100644 --- a/packages/flutter/test/material/typography_test.dart +++ b/packages/flutter/test/material/typography_test.dart @@ -23,6 +23,29 @@ void main() { } }); + test('TextTheme merges properly in the presence of null fields.', () { + const TextTheme partialTheme = const TextTheme(title: const TextStyle(color: const Color(0xcafefeed))); + final TextTheme fullTheme = ThemeData.fallback().textTheme.merge(partialTheme); + expect(fullTheme.title.color, equals(partialTheme.title.color)); + + const TextTheme onlyHeadlineAndTitle = const TextTheme( + headline: const TextStyle(color: const Color(0xcafefeed)), + title: const TextStyle(color: const Color(0xbeefcafe)), + ); + const TextTheme onlyBody1AndTitle = const TextTheme( + body1: const TextStyle(color: const Color(0xfeedfeed)), + title: const TextStyle(color: const Color(0xdeadcafe)), + ); + TextTheme merged = onlyHeadlineAndTitle.merge(onlyBody1AndTitle); + expect(merged.body2, isNull); + expect(merged.body1.color, equals(onlyBody1AndTitle.body1.color)); + expect(merged.headline.color, equals(onlyHeadlineAndTitle.headline.color)); + expect(merged.title.color, equals(onlyBody1AndTitle.title.color)); + + merged = onlyHeadlineAndTitle.merge(null); + expect(merged, equals(onlyHeadlineAndTitle)); + }); + test('Typography on Android, Fuchsia defaults to Roboto', () { expect(new Typography(platform: TargetPlatform.android).black.title.fontFamily, 'Roboto'); expect(new Typography(platform: TargetPlatform.fuchsia).black.title.fontFamily, 'Roboto');