From 3eb1b412e0d14f86265369215401ad8a4e85434e Mon Sep 17 00:00:00 2001 From: Ian Hickson Date: Fri, 12 Feb 2016 23:18:39 -0800 Subject: [PATCH] Fix color of icons in drawers in dark theme. This makes it match the material spec more. https://www.google.com/design/spec/style/icons.html Fixes https://github.com/flutter/flutter/issues/1357 --- examples/stocks/test/icon_color_test.dart | 109 +++++++++++++++++ examples/stocks/test/locale_test.dart | 6 +- .../flutter/lib/src/material/drawer_item.dart | 28 +++-- packages/flutter/lib/src/material/icon.dart | 8 +- .../flutter/lib/src/rendering/binding.dart | 2 +- packages/flutter/lib/src/rendering/image.dart | 22 +++- .../flutter/lib/src/rendering/object.dart | 17 +++ .../lib/src/services/asset_bundle.dart | 3 + .../lib/src/services/image_resource.dart | 13 ++ .../flutter/lib/src/widgets/asset_vendor.dart | 28 +++-- packages/flutter/lib/src/widgets/basic.dart | 111 +++++++++++++++++- .../flutter_test/lib/src/widget_tester.dart | 9 ++ 12 files changed, 329 insertions(+), 27 deletions(-) create mode 100644 examples/stocks/test/icon_color_test.dart diff --git a/examples/stocks/test/icon_color_test.dart b/examples/stocks/test/icon_color_test.dart new file mode 100644 index 0000000000..9afd3b1590 --- /dev/null +++ b/examples/stocks/test/icon_color_test.dart @@ -0,0 +1,109 @@ +// Copyright 2016 The Chromium Authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +import 'dart:ui' as ui show window; + +import 'package:test/test.dart'; +import 'package:flutter_test/flutter_test.dart'; +import 'package:flutter/material.dart'; +import 'package:flutter/rendering.dart'; +import 'package:stocks/main.dart' as stocks; +import 'package:stocks/stock_data.dart' as stock_data; + +Element findElementOfExactWidgetTypeGoingDown(Element node, Type targetType) { + void walker(Element child) { + if (child.widget.runtimeType == targetType) + throw child; + child.visitChildElements(walker); + } + try { + walker(node); + } on Element catch (result) { + return result; + } + return null; +} + +Element findElementOfExactWidgetTypeGoingUp(Element node, Type targetType) { + Element result; + bool walker(Element ancestor) { + if (ancestor.widget.runtimeType == targetType) + result = ancestor; + return result == null; + } + node.visitAncestorElements(walker); + return result; +} + +final RegExp materialIconAssetNameColorExtractor = new RegExp(r'[^/]+/ic_.+_(white|black)_[0-9]+dp\.png'); + +void checkIconColor(WidgetTester tester, String label, Color color) { + // The icon is going to be in the same merged semantics box as the text + // regardless of how the menu item is represented, so this is a good + // way to find the menu item. I hope. + Element semantics = findElementOfExactWidgetTypeGoingUp(tester.findText(label), MergeSemantics); + expect(semantics, isNotNull); + Element asset = findElementOfExactWidgetTypeGoingDown(semantics, AssetImage); + RenderImage imageBox = asset.findRenderObject(); + Match match = materialIconAssetNameColorExtractor.firstMatch(asset.widget.name); + expect(match, isNotNull); + if (color == const Color(0xFFFFFFFF)) { + expect(match[1], equals('white')); + expect(imageBox.color, isNull); + } else if (color == const Color(0xFF000000)) { + expect(match[1], equals('black')); + expect(imageBox.color, isNull); + } else { + expect(imageBox.color, equals(color)); + } +} + +void main() { + stock_data.StockDataFetcher.actuallyFetchData = false; + + test("Test icon colors", () { + testWidgets((WidgetTester tester) { + stocks.main(); // builds the app and schedules a frame but doesn't trigger one + tester.pump(); // see https://github.com/flutter/flutter/issues/1865 + tester.pump(); // triggers a frame + + // sanity check + expect(tester.findText('MARKET'), isNotNull); + expect(tester.findText('Help & Feedback'), isNull); + tester.pump(new Duration(seconds: 2)); + expect(tester.findText('MARKET'), isNotNull); + expect(tester.findText('Help & Feedback'), isNull); + + // drag the drawer out + TestPointer pointer = new TestPointer(1); + Point left = new Point(0.0, ui.window.size.height / 2.0); + Point right = new Point(ui.window.size.width, left.y); + tester.dispatchEvent(pointer.down(left), left); + tester.pump(); + tester.dispatchEvent(pointer.move(right), left); + tester.pump(); + tester.dispatchEvent(pointer.up(), left); + tester.pump(); + expect(tester.findText('MARKET'), isNotNull); + expect(tester.findText('Help & Feedback'), isNotNull); + + // check the colour of the icon - light mode + checkIconColor(tester, 'Stock List', Colors.purple[500]); // theme primary color + checkIconColor(tester, 'Account Balance', Colors.black45); // enabled + checkIconColor(tester, 'Help & Feedback', Colors.black26); // disabled + + // switch to dark mode + tester.tap(tester.findText('Pessimistic')); + tester.pump(); // get the tap and send the notification that the theme has changed + tester.pump(); // start the theme transition + tester.pump(const Duration(seconds: 5)); // end the transition + + // check the colour of the icon - dark mode + checkIconColor(tester, 'Stock List', Colors.redAccent[200]); // theme accent color + checkIconColor(tester, 'Account Balance', Colors.white); // enabled + checkIconColor(tester, 'Help & Feedback', Colors.white30); // disabled + + }); + }); +} diff --git a/examples/stocks/test/locale_test.dart b/examples/stocks/test/locale_test.dart index 6c09313414..98b31b8d0f 100644 --- a/examples/stocks/test/locale_test.dart +++ b/examples/stocks/test/locale_test.dart @@ -1,3 +1,6 @@ +// Copyright 2016 The Chromium Authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. import 'package:test/test.dart'; import 'package:flutter_test/flutter_test.dart'; @@ -11,7 +14,8 @@ void main() { test("Test changing locale", () { testWidgets((WidgetTester tester) { stocks.main(); - tester.pump(const Duration(seconds: 1)); // Unclear why duration is required. + tester.async.flushMicrotasks(); // see https://github.com/flutter/flutter/issues/1865 + tester.pump(); Element tab = tester.findText('MARKET'); expect(tab, isNotNull); diff --git a/packages/flutter/lib/src/material/drawer_item.dart b/packages/flutter/lib/src/material/drawer_item.dart index 67d7154fb1..035fc6db45 100644 --- a/packages/flutter/lib/src/material/drawer_item.dart +++ b/packages/flutter/lib/src/material/drawer_item.dart @@ -24,21 +24,31 @@ class DrawerItem extends StatelessComponent { final bool selected; Color _getIconColor(ThemeData themeData) { - if (selected) { - if (themeData.brightness == ThemeBrightness.dark) - return themeData.accentColor; - return themeData.primaryColor; + switch (themeData.brightness) { + case ThemeBrightness.light: + if (selected) + return themeData.primaryColor; + if (onPressed == null) + return Colors.black26; + return Colors.black45; + case ThemeBrightness.dark: + if (selected) + return themeData.accentColor; + if (onPressed == null) + return Colors.white30; + return null; // use default icon theme colour unmodified } - return Colors.black45; } TextStyle _getTextStyle(ThemeData themeData) { TextStyle result = themeData.text.body2; if (selected) { - if (themeData.brightness == ThemeBrightness.dark) - result = result.copyWith(color: themeData.accentColor); - else - result = result.copyWith(color: themeData.primaryColor); + switch (themeData.brightness) { + case ThemeBrightness.light: + return result.copyWith(color: themeData.primaryColor); + case ThemeBrightness.dark: + return result.copyWith(color: themeData.accentColor); + } } return result; } diff --git a/packages/flutter/lib/src/material/icon.dart b/packages/flutter/lib/src/material/icon.dart index 9cbcd3e657..c7d5f2522d 100644 --- a/packages/flutter/lib/src/material/icon.dart +++ b/packages/flutter/lib/src/material/icon.dart @@ -77,9 +77,9 @@ class Icon extends StatelessComponent { Color iconColor = color; final int iconAlpha = (255.0 * (IconTheme.of(context)?.clampedOpacity ?? 1.0)).round(); if (iconAlpha != 255) { - if (color != null) + if (color != null) { iconColor = color.withAlpha(iconAlpha); - else { + } else { switch(iconThemeColor) { case IconThemeColor.black: iconColor = Colors.black.withAlpha(iconAlpha); @@ -103,5 +103,9 @@ class Icon extends StatelessComponent { super.debugFillDescription(description); description.add('$icon'); description.add('size: $size'); + if (this.colorTheme != null) + description.add('colorTheme: $colorTheme'); + if (this.color != null) + description.add('color: $color'); } } diff --git a/packages/flutter/lib/src/rendering/binding.dart b/packages/flutter/lib/src/rendering/binding.dart index 8769ba85e9..52105fd602 100644 --- a/packages/flutter/lib/src/rendering/binding.dart +++ b/packages/flutter/lib/src/rendering/binding.dart @@ -111,7 +111,7 @@ void debugDumpLayerTree() { /// This will only work if there is a semantics client attached. /// Otherwise, the tree is empty and this will print "null". void debugDumpSemanticsTree() { - debugPrint(Renderer.instance?.renderView?.debugSemantics?.toStringDeep()); + debugPrint(Renderer.instance?.renderView?.debugSemantics?.toStringDeep() ?? 'Semantics not collected.'); } /// A concrete binding for applications that use the Rendering framework diff --git a/packages/flutter/lib/src/rendering/image.dart b/packages/flutter/lib/src/rendering/image.dart index 597dfa2ed7..d9f23b551f 100644 --- a/packages/flutter/lib/src/rendering/image.dart +++ b/packages/flutter/lib/src/rendering/image.dart @@ -251,7 +251,25 @@ class RenderImage extends RenderBox { void debugDescribeSettings(List settings) { super.debugDescribeSettings(settings); - settings.add('width: $width'); - settings.add('height: $height'); + if (image != null) + settings.add('image: [${image.width}\u00D7${image.height}]'); + else + settings.add('image: null'); + if (width != null) + settings.add('width: $width'); + if (height != null) + settings.add('height: $height'); + if (scale != 1.0) + settings.add('scale: $scale'); + if (color != null) + settings.add('color: $color'); + if (fit != null) + settings.add('fit: $fit'); + if (alignment != null) + settings.add('alignment: $alignment'); + if (repeat != ImageRepeat.noRepeat) + settings.add('repeat: $repeat'); + if (centerSlice != null) + settings.add('centerSlice: $centerSlice'); } } diff --git a/packages/flutter/lib/src/rendering/object.dart b/packages/flutter/lib/src/rendering/object.dart index 4a6eaf67e4..22d73bfb2a 100644 --- a/packages/flutter/lib/src/rendering/object.dart +++ b/packages/flutter/lib/src/rendering/object.dart @@ -1712,6 +1712,23 @@ abstract class RenderObject extends AbstractNode implements HitTestTarget { return result; } + /// Returns a one-line detailed description of the render object. + /// This description is often somewhat long. + /// + /// This includes the same information for this RenderObject as given by + /// [toStringDeep()], but does not recurse to any children. + String toStringShallow() { + RenderObject debugPreviousActiveLayout = _debugActiveLayout; + _debugActiveLayout = null; + StringBuffer result = new StringBuffer(); + result.write('$this; '); + List settings = []; + debugDescribeSettings(settings); + result.write(settings.join('; ')); + _debugActiveLayout = debugPreviousActiveLayout; + return result.toString(); + } + /// Returns a list of strings describing the current node's fields, one field /// per string. Subclasses should override this to have their information /// included in toStringDeep(). diff --git a/packages/flutter/lib/src/services/asset_bundle.dart b/packages/flutter/lib/src/services/asset_bundle.dart index 3b6ce49aa4..0569939ae8 100644 --- a/packages/flutter/lib/src/services/asset_bundle.dart +++ b/packages/flutter/lib/src/services/asset_bundle.dart @@ -21,6 +21,7 @@ abstract class AssetBundle { ImageResource loadImage(String key); Future loadString(String key); Future load(String key); + String toString() => '$runtimeType@$hashCode()'; } class NetworkAssetBundle extends AssetBundle { @@ -39,6 +40,8 @@ class NetworkAssetBundle extends AssetBundle { Future loadString(String key) async { return (await http.get(_urlFromKey(key))).body; } + + String toString() => '$runtimeType@$hashCode($_baseUrl)'; } abstract class CachingAssetBundle extends AssetBundle { diff --git a/packages/flutter/lib/src/services/image_resource.dart b/packages/flutter/lib/src/services/image_resource.dart index c6d0e9e2b4..4234b9ab42 100644 --- a/packages/flutter/lib/src/services/image_resource.dart +++ b/packages/flutter/lib/src/services/image_resource.dart @@ -11,6 +11,7 @@ class ImageInfo { ImageInfo({ this.image, this.scale: 1.0 }); final ui.Image image; final double scale; + String toString() => '[${image.width}\u00D7${image.height}] @ ${scale}x'; } /// A callback for when the image is available. @@ -82,4 +83,16 @@ class ImageResource { debugPrint('$stack'); debugPrint('------------------------------------------------------------------------'); } + + String toString() { + StringBuffer result = new StringBuffer(); + result.write('$runtimeType('); + if (!_resolved) + result.write('unresolved'); + else + result.write('$_image'); + result.write('; ${_listeners.length} listener${_listeners.length == 1 ? "" : "s" }'); + result.write(')'); + return result.toString(); + } } diff --git a/packages/flutter/lib/src/widgets/asset_vendor.dart b/packages/flutter/lib/src/widgets/asset_vendor.dart index 6f7f7e0065..9422070601 100644 --- a/packages/flutter/lib/src/widgets/asset_vendor.dart +++ b/packages/flutter/lib/src/widgets/asset_vendor.dart @@ -191,14 +191,20 @@ class AssetVendor extends StatefulComponent { final Widget child; _AssetVendorState createState() => new _AssetVendorState(); + + void debugFillDescription(List description) { + super.debugFillDescription(description); + description.add('bundle: $bundle'); + if (devicePixelRatio != null) + description.add('devicePixelRatio: $devicePixelRatio'); + } } class _AssetVendorState extends State { _ResolvingAssetBundle _bundle; - void initState() { - super.initState(); + void _initBundle() { _bundle = new _ResolutionAwareAssetBundle( bundle: config.bundle, resolver: new _ResolutionAwareAssetResolver( @@ -208,20 +214,24 @@ class _AssetVendorState extends State { ); } + void initState() { + super.initState(); + _initBundle(); + } + void didUpdateConfig(AssetVendor oldConfig) { if (config.bundle != oldConfig.bundle || config.devicePixelRatio != oldConfig.devicePixelRatio) { - _bundle = new _ResolutionAwareAssetBundle( - bundle: config.bundle, - resolver: new _ResolutionAwareAssetResolver( - bundle: config.bundle, - devicePixelRatio: config.devicePixelRatio - ) - ); + _initBundle(); } } Widget build(BuildContext context) { return new DefaultAssetBundle(bundle: _bundle, child: config.child); } + + void debugFillDescription(List description) { + super.debugFillDescription(description); + description.add('bundle: $_bundle'); + } } diff --git a/packages/flutter/lib/src/widgets/basic.dart b/packages/flutter/lib/src/widgets/basic.dart index 0db20a8a7c..e77500ea9a 100644 --- a/packages/flutter/lib/src/widgets/basic.dart +++ b/packages/flutter/lib/src/widgets/basic.dart @@ -1576,7 +1576,8 @@ class RawImage extends LeafRenderObjectWidget { fit: fit, alignment: alignment, repeat: repeat, - centerSlice: centerSlice); + centerSlice: centerSlice + ); void updateRenderObject(RenderImage renderObject, RawImage oldWidget) { renderObject.image = image; @@ -1589,6 +1590,30 @@ class RawImage extends LeafRenderObjectWidget { renderObject.repeat = repeat; renderObject.centerSlice = centerSlice; } + + void debugFillDescription(List description) { + super.debugFillDescription(description); + if (image != null) + description.add('image: [${image.width}\u00D7${image.height}]'); + else + description.add('image: null'); + if (width != null) + description.add('width: $width'); + if (height != null) + description.add('height: $height'); + if (scale != 1.0) + description.add('scale: $scale'); + if (color != null) + description.add('color: $color'); + if (fit != null) + description.add('fit: $fit'); + if (alignment != null) + description.add('alignment: $alignment'); + if (repeat != ImageRepeat.noRepeat) + description.add('repeat: $repeat'); + if (centerSlice != null) + description.add('centerSlice: $centerSlice'); + } } /// Displays an [ImageResource]. @@ -1655,10 +1680,29 @@ class RawImageResource extends StatefulComponent { /// the center slice will be stretched only vertically. final Rect centerSlice; - _ImageListenerState createState() => new _ImageListenerState(); + _RawImageResourceState createState() => new _RawImageResourceState(); + + void debugFillDescription(List description) { + super.debugFillDescription(description); + description.add('image: $image'); + if (width != null) + description.add('width: $width'); + if (height != null) + description.add('height: $height'); + if (color != null) + description.add('color: $color'); + if (fit != null) + description.add('fit: $fit'); + if (alignment != null) + description.add('alignment: $alignment'); + if (repeat != ImageRepeat.noRepeat) + description.add('repeat: $repeat'); + if (centerSlice != null) + description.add('centerSlice: $centerSlice'); + } } -class _ImageListenerState extends State { +class _RawImageResourceState extends State { void initState() { super.initState(); config.image.addListener(_handleImageChanged); @@ -1771,6 +1815,27 @@ class NetworkImage extends StatelessComponent { centerSlice: centerSlice ); } + + void debugFillDescription(List description) { + super.debugFillDescription(description); + description.add('src: $src'); + if (width != null) + description.add('width: $width'); + if (height != null) + description.add('height: $height'); + if (scale != 1.0) + description.add('scale: $scale'); + if (color != null) + description.add('color: $color'); + if (fit != null) + description.add('fit: $fit'); + if (alignment != null) + description.add('alignment: $alignment'); + if (repeat != ImageRepeat.noRepeat) + description.add('repeat: $repeat'); + if (centerSlice != null) + description.add('centerSlice: $centerSlice'); + } } /// Sets a default asset bundle for its descendants. @@ -1869,6 +1934,25 @@ class AsyncImage extends StatelessComponent { centerSlice: centerSlice ); } + + void debugFillDescription(List description) { + super.debugFillDescription(description); + description.add('provider: $provider'); + if (width != null) + description.add('width: $width'); + if (height != null) + description.add('height: $height'); + if (color != null) + description.add('color: $color'); + if (fit != null) + description.add('fit: $fit'); + if (alignment != null) + description.add('alignment: $alignment'); + if (repeat != ImageRepeat.noRepeat) + description.add('repeat: $repeat'); + if (centerSlice != null) + description.add('centerSlice: $centerSlice'); + } } /// Displays an image from an [AssetBundle]. @@ -1949,6 +2033,27 @@ class AssetImage extends StatelessComponent { centerSlice: centerSlice ); } + + void debugFillDescription(List description) { + super.debugFillDescription(description); + description.add('name: $name'); + if (width != null) + description.add('width: $width'); + if (height != null) + description.add('height: $height'); + if (color != null) + description.add('color: $color'); + if (fit != null) + description.add('fit: $fit'); + if (alignment != null) + description.add('alignment: $alignment'); + if (repeat != ImageRepeat.noRepeat) + description.add('repeat: $repeat'); + if (centerSlice != null) + description.add('centerSlice: $centerSlice'); + if (bundle != null) + description.add('bundle: $bundle'); + } } /// An adapter for placing a specific [RenderBox] in the widget tree. diff --git a/packages/flutter_test/lib/src/widget_tester.dart b/packages/flutter_test/lib/src/widget_tester.dart index 6c120f1e08..ba68ac9e8c 100644 --- a/packages/flutter_test/lib/src/widget_tester.dart +++ b/packages/flutter_test/lib/src/widget_tester.dart @@ -29,17 +29,26 @@ class WidgetTester extends Instrumentation { final FakeAsync async; final Clock clock; + /// Calls [runApp()] with the given widget, then triggers a frame sequent and + /// flushes microtasks, by calling [pump()] with the same duration (if any). void pumpWidget(Widget widget, [ Duration duration ]) { runApp(widget); pump(duration); } + /// Artificially calls dispatchLocaleChanged on the Widget binding, + /// then flushes microtasks. void setLocale(String languageCode, String countryCode) { Locale locale = new Locale(languageCode, countryCode); binding.dispatchLocaleChanged(locale); async.flushMicrotasks(); } + /// Triggers a frame sequence (build/layout/paint/etc), + /// then flushes microtasks. + /// + /// If duration is set, then advances the clock by that much first. + /// Doing this flushes microtasks. void pump([ Duration duration ]) { if (duration != null) async.elapse(duration);