From 5e166f2e445189a4f99972fcc8821b766a04b6fb Mon Sep 17 00:00:00 2001 From: Yegor Jbanov Date: Tue, 19 Jan 2021 09:43:09 -0800 Subject: [PATCH 1/5] [canvaskit] fix text background, foreground, familyFallback; add style tests --- lib/web_ui/dev/test_runner.dart | 16 +- lib/web_ui/lib/src/engine/assets.dart | 11 +- .../src/engine/canvaskit/font_fallbacks.dart | 50 +- lib/web_ui/lib/src/engine/canvaskit/text.dart | 295 +++++++----- .../lib/src/engine/text/font_collection.dart | 4 +- .../test/canvaskit/canvas_golden_test.dart | 442 ++++++++++++++++++ 6 files changed, 683 insertions(+), 135 deletions(-) diff --git a/lib/web_ui/dev/test_runner.dart b/lib/web_ui/dev/test_runner.dart index 842a5318b26b6..f5d9ed67433ec 100644 --- a/lib/web_ui/dev/test_runner.dart +++ b/lib/web_ui/dev/test_runner.dart @@ -88,6 +88,15 @@ class TestCommand extends Command with ArgUtils { '.dart_tool/goldens. Use this option to bulk-update all screenshots, ' 'for example, when a new browser version affects pixels.', ) + ..addFlag( + 'fetch-goldens-repo', + defaultsTo: true, + negatable: true, + help: + 'Whether to fetch the goldens repo. Set this to false to iterate ' + 'on golden tests without fearing that the fetcher will overwrite ' + 'your local changes.', + ) ..addOption( 'browser', defaultsTo: 'chrome', @@ -272,9 +281,9 @@ class TestCommand extends Command with ArgUtils { environment.webUiTestResultsDirectory.createSync(recursive: true); // If screenshot tests are available, fetch the screenshot goldens. - if (isScreenshotTestsAvailable) { + if (isScreenshotTestsAvailable && doFetchGoldensRepo) { if (isVerboseLoggingEnabled) { - print('INFO: Screenshot tests available'); + print('INFO: Fetching goldens repo'); } final GoldensRepoFetcher goldensRepoFetcher = GoldensRepoFetcher( environment.webUiGoldensRepositoryDirectory, @@ -483,6 +492,9 @@ class TestCommand extends Command with ArgUtils { /// ".dart_tool/goldens". bool get doUpdateScreenshotGoldens => boolArg('update-screenshot-goldens'); + /// Whether to fetch the goldens repo prior to running tests. + bool get doFetchGoldensRepo => boolArg('fetch-goldens-repo'); + /// Runs all tests specified in [targets]. /// /// Unlike [_runAllTestsForCurrentPlatform], this does not filter targets diff --git a/lib/web_ui/lib/src/engine/assets.dart b/lib/web_ui/lib/src/engine/assets.dart index 61c056f3f6763..4f0fab752c03f 100644 --- a/lib/web_ui/lib/src/engine/assets.dart +++ b/lib/web_ui/lib/src/engine/assets.dart @@ -88,7 +88,16 @@ class AssetManagerException implements Exception { class WebOnlyMockAssetManager implements AssetManager { String defaultAssetsDir = ''; String defaultAssetManifest = '{}'; - String defaultFontManifest = '[]'; + String defaultFontManifest = '''[ + { + "family":"$_robotoFontFamily", + "fonts":[{"asset":"$_robotoTestFontUrl"}] + }, + { + "family":"$_ahemFontFamily", + "fonts":[{"asset":"$_ahemFontUrl"}] + } + ]'''; @override String get assetsDir => defaultAssetsDir; diff --git a/lib/web_ui/lib/src/engine/canvaskit/font_fallbacks.dart b/lib/web_ui/lib/src/engine/canvaskit/font_fallbacks.dart index 5c6ad00aab42b..b395ec706af73 100644 --- a/lib/web_ui/lib/src/engine/canvaskit/font_fallbacks.dart +++ b/lib/web_ui/lib/src/engine/canvaskit/font_fallbacks.dart @@ -388,6 +388,9 @@ class _ResolvedNotoSubset { final List ranges; _ResolvedNotoSubset(this.url, this.family, this.ranges); + + @override + String toString() => '_ResolvedNotoSubset($family, $url)'; } _NotoFont _notoSansSC = _NotoFont('Noto Sans SC', [ @@ -660,21 +663,64 @@ class FallbackFontDownloadQueue { } class NotoDownloader { + int _debugActiveDownloadCount = 0; + + /// Returns a future that resolves when there are no pending downloads. + /// + /// Useful in tests to make sure that fonts are loaded before working with + /// text. + Future debugWhenIdle() async { + if (assertionsEnabled) { + // Some downloads begin asynchronously in a microtask or in a Timer.run. + // Let those run before waiting for downloads to finish. + await Future.delayed(Duration.zero); + while (_debugActiveDownloadCount > 0) { + await Future.delayed(const Duration(milliseconds: 100)); + // If we started with a non-zero count and hit zero while waiting, wait a + // little more to make sure another download doesn't get chained after + // the last one (e.g. font file download after font CSS download). + if (_debugActiveDownloadCount == 0) { + await Future.delayed(const Duration(milliseconds: 100)); + } + } + } else { + throw UnimplementedError(); + } + } + /// Downloads the [url] and returns it as a [ByteBuffer]. /// /// Override this for testing. Future downloadAsBytes(String url) { - return html.window.fetch(url).then((dynamic fetchResult) => fetchResult + if (assertionsEnabled) { + _debugActiveDownloadCount += 1; + } + final Future result = html.window.fetch(url).then((dynamic fetchResult) => fetchResult .arrayBuffer() .then((dynamic x) => x as ByteBuffer)); + if (assertionsEnabled) { + result.whenComplete(() { + _debugActiveDownloadCount -= 1; + }); + } + return result; } /// Downloads the [url] and returns is as a [String]. /// /// Override this for testing. Future downloadAsString(String url) { - return html.window.fetch(url).then((dynamic response) => + if (assertionsEnabled) { + _debugActiveDownloadCount += 1; + } + final Future result = html.window.fetch(url).then((dynamic response) => response.text().then((dynamic x) => x as String)); + if (assertionsEnabled) { + result.whenComplete(() { + _debugActiveDownloadCount -= 1; + }); + } + return result; } } diff --git a/lib/web_ui/lib/src/engine/canvaskit/text.dart b/lib/web_ui/lib/src/engine/canvaskit/text.dart index 3b7735e284086..1e2337e7c6740 100644 --- a/lib/web_ui/lib/src/engine/canvaskit/text.dart +++ b/lib/web_ui/lib/src/engine/canvaskit/text.dart @@ -5,6 +5,7 @@ // @dart = 2.12 part of engine; +@immutable class CkParagraphStyle implements ui.ParagraphStyle { CkParagraphStyle({ ui.TextAlign? textAlign, @@ -32,20 +33,19 @@ class CkParagraphStyle implements ui.ParagraphStyle { strutStyle, ellipsis, locale, - ) { - _textDirection = textDirection ?? ui.TextDirection.ltr; - _fontFamily = fontFamily; - _fontSize = fontSize; - _fontWeight = fontWeight; - _fontStyle = fontStyle; - } - - SkParagraphStyle skParagraphStyle; - ui.TextDirection? _textDirection; - String? _fontFamily; - double? _fontSize; - ui.FontWeight? _fontWeight; - ui.FontStyle? _fontStyle; + ), + _textDirection = textDirection ?? ui.TextDirection.ltr, + _fontFamily = fontFamily, + _fontSize = fontSize, + _fontWeight = fontWeight, + _fontStyle = fontStyle; + + final SkParagraphStyle skParagraphStyle; + final ui.TextDirection? _textDirection; + final String? _fontFamily; + final double? _fontSize; + final ui.FontWeight? _fontWeight; + final ui.FontStyle? _fontStyle; static SkTextStyleProperties toSkTextStyleProperties( String? fontFamily, @@ -159,29 +159,8 @@ class CkParagraphStyle implements ui.ParagraphStyle { } } +@immutable class CkTextStyle implements ui.TextStyle { - SkTextStyle skTextStyle; - - ui.Color? color; - ui.TextDecoration? decoration; - ui.Color? decorationColor; - ui.TextDecorationStyle? decorationStyle; - double? decorationThickness; - ui.FontWeight? fontWeight; - ui.FontStyle? fontStyle; - ui.TextBaseline? textBaseline; - String? fontFamily; - List? fontFamilyFallback; - double? fontSize; - double? letterSpacing; - double? wordSpacing; - double? height; - ui.Locale? locale; - CkPaint? background; - CkPaint? foreground; - List? shadows; - List? fontFeatures; - factory CkTextStyle({ ui.Color? color, ui.TextDecoration? decoration, @@ -203,6 +182,126 @@ class CkTextStyle implements ui.TextStyle { List? shadows, List? fontFeatures, }) { + return CkTextStyle._( + color, + decoration, + decorationColor, + decorationStyle, + decorationThickness, + fontWeight, + fontStyle, + textBaseline, + fontFamily, + fontFamilyFallback, + fontSize, + letterSpacing, + wordSpacing, + height, + locale, + background, + foreground, + shadows, + fontFeatures, + ); + } + + CkTextStyle._( + this.color, + this.decoration, + this.decorationColor, + this.decorationStyle, + this.decorationThickness, + this.fontWeight, + this.fontStyle, + this.textBaseline, + this.fontFamily, + this.fontFamilyFallback, + this.fontSize, + this.letterSpacing, + this.wordSpacing, + this.height, + this.locale, + this.background, + this.foreground, + this.shadows, + this.fontFeatures, + ); + + final ui.Color? color; + final ui.TextDecoration? decoration; + final ui.Color? decorationColor; + final ui.TextDecorationStyle? decorationStyle; + final double? decorationThickness; + final ui.FontWeight? fontWeight; + final ui.FontStyle? fontStyle; + final ui.TextBaseline? textBaseline; + final String? fontFamily; + final List? fontFamilyFallback; + final double? fontSize; + final double? letterSpacing; + final double? wordSpacing; + final double? height; + final ui.Locale? locale; + final CkPaint? background; + final CkPaint? foreground; + final List? shadows; + final List? fontFeatures; + + /// Merges this text style with [other] and returns the new text style. + /// + /// The values in this text style are used unless [other] specifically + /// overrides it. + CkTextStyle mergeWith(CkTextStyle other) { + return CkTextStyle( + color: other.color ?? color, + decoration: other.decoration ?? decoration, + decorationColor: other.decorationColor ?? decorationColor, + decorationStyle: other.decorationStyle ?? decorationStyle, + decorationThickness: other.decorationThickness ?? decorationThickness, + fontWeight: other.fontWeight ?? fontWeight, + fontStyle: other.fontStyle ?? fontStyle, + textBaseline: other.textBaseline ?? textBaseline, + fontFamily: other.fontFamily ?? fontFamily, + fontFamilyFallback: other.fontFamilyFallback ?? fontFamilyFallback, + fontSize: other.fontSize ?? fontSize, + letterSpacing: other.letterSpacing ?? letterSpacing, + wordSpacing: other.wordSpacing ?? wordSpacing, + height: other.height ?? height, + locale: other.locale ?? locale, + background: other.background ?? background, + foreground: other.foreground ?? foreground, + shadows: other.shadows ?? shadows, + fontFeatures: other.fontFeatures ?? fontFeatures, + ); + } + + /// Lazy-initialized list of font families sent to Skia. + late final List effectiveFontFamilies = _getEffectiveFontFamilies(fontFamily, fontFamilyFallback); + + /// Lazy-initialized Skia style used to pass the style to Skia. + /// + /// This is lazy because not every style ends up being passed to Skia, so the + /// conversion would be wasteful. + late final SkTextStyle skTextStyle = () { + // Write field values to locals so null checks promote types to non-null. + final ui.Color? color = this.color; + final ui.TextDecoration? decoration = this.decoration; + final ui.Color? decorationColor = this.decorationColor; + final ui.TextDecorationStyle? decorationStyle = this.decorationStyle; + final double? decorationThickness = this.decorationThickness; + final ui.FontWeight? fontWeight = this.fontWeight; + final ui.FontStyle? fontStyle = this.fontStyle; + final ui.TextBaseline? textBaseline = this.textBaseline; + final double? fontSize = this.fontSize; + final double? letterSpacing = this.letterSpacing; + final double? wordSpacing = this.wordSpacing; + final double? height = this.height; + final ui.Locale? locale = this.locale; + final CkPaint? background = this.background; + final CkPaint? foreground = this.foreground; + final List? shadows = this.shadows; + final List? fontFeatures = this.fontFeatures; + final SkTextStyleProperties properties = SkTextStyleProperties(); if (background != null) { @@ -263,8 +362,7 @@ class CkTextStyle implements ui.TextStyle { properties.locale = locale.toLanguageTag(); } - properties.fontFamilies = - _getEffectiveFontFamilies(fontFamily, fontFamilyFallback); + properties.fontFamilies = effectiveFontFamilies; if (fontWeight != null || fontStyle != null) { properties.fontStyle = toSkFontStyle(fontWeight, fontStyle); @@ -287,90 +385,18 @@ class CkTextStyle implements ui.TextStyle { } if (fontFeatures != null) { - List ckFontFeatures = []; + List skFontFeatures = []; for (ui.FontFeature fontFeature in fontFeatures) { - SkFontFeature ckFontFeature = SkFontFeature(); - ckFontFeature.name = fontFeature.feature; - ckFontFeature.value = fontFeature.value; - ckFontFeatures.add(ckFontFeature); + SkFontFeature skFontFeature = SkFontFeature(); + skFontFeature.name = fontFeature.feature; + skFontFeature.value = fontFeature.value; + skFontFeatures.add(skFontFeature); } - properties.fontFeatures = ckFontFeatures; + properties.fontFeatures = skFontFeatures; } - return CkTextStyle._( - canvasKit.TextStyle(properties), - color, - decoration, - decorationColor, - decorationStyle, - decorationThickness, - fontWeight, - fontStyle, - textBaseline, - fontFamily, - fontFamilyFallback, - fontSize, - letterSpacing, - wordSpacing, - height, - locale, - background, - foreground, - shadows, - fontFeatures, - ); - } - - /// Merges this text style with [other] and returns the new text style. - /// - /// The values in this text style are used unless [other] specifically - /// overrides it. - CkTextStyle mergeWith(CkTextStyle other) { - return CkTextStyle( - color: other.color ?? color, - decoration: other.decoration ?? decoration, - decorationColor: other.decorationColor ?? decorationColor, - decorationStyle: other.decorationStyle ?? decorationStyle, - decorationThickness: other.decorationThickness ?? decorationThickness, - fontWeight: other.fontWeight ?? fontWeight, - fontStyle: other.fontStyle ?? fontStyle, - textBaseline: other.textBaseline ?? textBaseline, - fontFamily: other.fontFamily ?? fontFamily, - fontFamilyFallback: other.fontFamilyFallback ?? fontFamilyFallback, - fontSize: other.fontSize ?? fontSize, - letterSpacing: other.letterSpacing ?? letterSpacing, - wordSpacing: other.wordSpacing ?? wordSpacing, - height: other.height ?? height, - locale: other.locale ?? locale, - background: other.background ?? background, - foreground: other.foreground ?? foreground, - shadows: other.shadows ?? shadows, - fontFeatures: other.fontFeatures ?? fontFeatures, - ); - } - - CkTextStyle._( - this.skTextStyle, - this.color, - this.decoration, - this.decorationColor, - this.decorationStyle, - this.decorationThickness, - this.fontWeight, - this.fontStyle, - this.textBaseline, - this.fontFamily, - this.fontFamilyFallback, - this.fontSize, - this.letterSpacing, - this.wordSpacing, - this.height, - this.locale, - this.background, - this.foreground, - this.shadows, - this.fontFeatures, - ); + return canvasKit.TextStyle(properties); + }(); } SkFontStyle toSkFontStyle(ui.FontWeight? fontWeight, ui.FontStyle? fontStyle) { @@ -582,7 +608,9 @@ class CkParagraphBuilder implements ui.ParagraphBuilder { _paragraphBuilder = canvasKit.ParagraphBuilder.MakeFromFontProvider( style.skParagraphStyle, skiaFontCollection.fontProvider, - ); + ) { + _styleStack.add(_style.getTextStyle()); + } @override void addPlaceholder( @@ -658,8 +686,7 @@ class CkParagraphBuilder implements ui.ParagraphBuilder { return; } CkTextStyle style = _peekStyle(); - List fontFamilies = - _getEffectiveFontFamilies(style.fontFamily, style.fontFamilyFallback); + List fontFamilies = style.effectiveFontFamilies; List typefaces = []; for (var font in fontFamilies) { List? typefacesForFamily = @@ -723,13 +750,19 @@ class CkParagraphBuilder implements ui.ParagraphBuilder { @override void pop() { + if (_styleStack.length <= 1) { + // The top-level text style is paragraph-level. We don't pop it off. + return; + } _commands.add(const _ParagraphCommand.pop()); _styleStack.removeLast(); _paragraphBuilder.pop(); } - CkTextStyle _peekStyle() => - _styleStack.isEmpty ? _style.getTextStyle() : _styleStack.last; + CkTextStyle _peekStyle() { + assert(_styleStack.isNotEmpty); + return _styleStack.last; + } // Used as the paint for background or foreground in the text style when // the other one is not specified. CanvasKit either both background and @@ -738,7 +771,8 @@ class CkParagraphBuilder implements ui.ParagraphBuilder { // // This object is never deleted. It is effectively a static global constant. // Therefore it doesn't need to be wrapped in CkPaint. - static final SkPaint _defaultTextStylePaint = SkPaint(); + static final SkPaint _defaultTextForeground = SkPaint(); + static final SkPaint _defaultTextBackground = SkPaint()..setColorInt(0x00000000); @override void pushStyle(ui.TextStyle style) { @@ -748,10 +782,15 @@ class CkParagraphBuilder implements ui.ParagraphBuilder { _styleStack.add(skStyle); _commands.add(_ParagraphCommand.pushStyle(ckStyle)); if (skStyle.foreground != null || skStyle.background != null) { - final SkPaint foreground = - skStyle.foreground?.skiaObject ?? _defaultTextStylePaint; - final SkPaint background = - skStyle.background?.skiaObject ?? _defaultTextStylePaint; + SkPaint? foreground = skStyle.foreground?.skiaObject; + if (foreground == null) { + _defaultTextForeground.setColorInt( + skStyle.color?.value ?? 0xFF000000, + ); + foreground = _defaultTextForeground; + } + + final SkPaint background = skStyle.background?.skiaObject ?? _defaultTextBackground; _paragraphBuilder.pushPaintStyle( skStyle.skTextStyle, foreground, background); } else { diff --git a/lib/web_ui/lib/src/engine/text/font_collection.dart b/lib/web_ui/lib/src/engine/text/font_collection.dart index 3d09d8d393ac5..8587c9713c6e5 100644 --- a/lib/web_ui/lib/src/engine/text/font_collection.dart +++ b/lib/web_ui/lib/src/engine/text/font_collection.dart @@ -8,7 +8,7 @@ part of engine; const String _ahemFontFamily = 'Ahem'; const String _ahemFontUrl = 'packages/ui/assets/ahem.ttf'; const String _robotoFontFamily = 'Roboto'; -const String _robotoFontUrl = 'packages/ui/assets/Roboto-Regular.ttf'; +const String _robotoTestFontUrl = 'packages/ui/assets/Roboto-Regular.ttf'; /// This class is responsible for registering and loading fonts. /// @@ -79,7 +79,7 @@ class FontCollection { _testFontManager!.registerAsset( _ahemFontFamily, 'url($_ahemFontUrl)', const {}); _testFontManager!.registerAsset( - _robotoFontFamily, 'url($_robotoFontUrl)', const {}); + _robotoFontFamily, 'url($_robotoTestFontUrl)', const {}); } /// Returns a [Future] that completes when the registered fonts are loaded diff --git a/lib/web_ui/test/canvaskit/canvas_golden_test.dart b/lib/web_ui/test/canvaskit/canvas_golden_test.dart index f0f56cccefded..df714c80bb699 100644 --- a/lib/web_ui/test/canvaskit/canvas_golden_test.dart +++ b/lib/web_ui/test/canvaskit/canvas_golden_test.dart @@ -212,6 +212,286 @@ void testMain() { dispatcher.rasterizer!.draw(layerTree); await matchGoldenFile('canvaskit_shadow_bounds.png', region: region); }); + + test('text styles - default', () async { + await testTextStyle('default'); + }); + + test('text styles - center aligned', () async { + await testTextStyle('center aligned', paragraphTextAlign: ui.TextAlign.center); + }); + + test('text styles - right aligned', () async { + await testTextStyle('right aligned', paragraphTextAlign: ui.TextAlign.right); + }); + + test('text styles - rtl', () async { + await testTextStyle('rtl', paragraphTextDirection: ui.TextDirection.rtl); + }); + + test('text styles - multiline', () async { + await testTextStyle('multiline', layoutWidth: 50); + }); + + test('text styles - max lines', () async { + await testTextStyle('max lines', paragraphMaxLines: 1, layoutWidth: 50); + }); + + test('text styles - ellipsis', () async { + await testTextStyle('ellipsis', paragraphMaxLines: 1, paragraphEllipsis: '...', layoutWidth: 60); + }); + + test('text styles - paragraph font family', () async { + await testTextStyle('paragraph font family', paragraphFontFamily: 'Ahem'); + }); + + test('text styles - paragraph font size', () async { + await testTextStyle('paragraph font size', paragraphFontSize: 22); + }); + + // TODO(yjbanov): paragraphHeight seems to have no effect, but maybe I'm using it wrong. + // https://github.com/flutter/flutter/issues/74337 + test('text styles - paragraph height', () async { + await testTextStyle('paragraph height', layoutWidth: 50, paragraphHeight: 1.5); + }); + + // TODO(yjbanov): paragraphTextHeightBehavior seems to have no effect. Unsure how to use it. + // https://github.com/flutter/flutter/issues/74337 + test('text styles - paragraph text height behavior', () async { + await testTextStyle('paragraph text height behavior', layoutWidth: 50, paragraphHeight: 1.5, paragraphTextHeightBehavior: ui.TextHeightBehavior( + applyHeightToFirstAscent: false, + applyHeightToLastDescent: false, + )); + }); + + // TODO(yjbanov): paragraph fontWeight doesn't seem to work. + // https://github.com/flutter/flutter/issues/74338 + test('text styles - paragraph weight', () async { + await testTextStyle('paragraph weight', paragraphFontWeight: ui.FontWeight.w900); + }); + + // TODO(yjbanov): paragraph fontStyle doesn't seem to work. + // https://github.com/flutter/flutter/issues/74338 + test('text style - paragraph font style', () async { + await testTextStyle( + 'paragraph font style', + paragraphFontStyle: ui.FontStyle.italic, + ); + }); + + // TODO(yjbanov): locales specified in paragraph styles don't work: + // https://github.com/flutter/flutter/issues/74687 + // TODO(yjbanov): spaces are not rendered correctly: + // https://github.com/flutter/flutter/issues/74742 + test('text styles - paragraph locale zh_CN', () async { + await testTextStyle('paragraph locale zh_CN', outerText: '次 化 刃 直 入 令', innerText: '', paragraphLocale: const ui.Locale('zh', 'CN')); + }); + + test('text styles - paragraph locale zh_TW', () async { + await testTextStyle('paragraph locale zh_TW', outerText: '次 化 刃 直 入 令', innerText: '', paragraphLocale: const ui.Locale('zh', 'TW')); + }); + + test('text styles - paragraph locale ja', () async { + await testTextStyle('paragraph locale ja', outerText: '次 化 刃 直 入 令', innerText: '', paragraphLocale: const ui.Locale('ja')); + }); + + test('text styles - paragraph locale ko', () async { + await testTextStyle('paragraph locale ko', outerText: '次 化 刃 直 入 令', innerText: '', paragraphLocale: const ui.Locale('ko')); + }); + + test('text styles - color', () async { + await testTextStyle('color', color: const ui.Color(0xFF009900)); + }); + + test('text styles - decoration', () async { + await testTextStyle('decoration', decoration: ui.TextDecoration.underline); + }); + + test('text styles - decoration style', () async { + await testTextStyle('decoration style', decoration: ui.TextDecoration.underline, decorationStyle: ui.TextDecorationStyle.dashed); + }); + + test('text styles - decoration thickness', () async { + await testTextStyle('decoration thickness', decoration: ui.TextDecoration.underline, decorationThickness: 5.0); + }); + + test('text styles - font weight', () async { + await testTextStyle('font weight', fontWeight: ui.FontWeight.w900); + }); + + test('text styles - font style', () async { + await testTextStyle('font style', fontStyle: ui.FontStyle.italic); + }); + + // TODO(yjbanov): not sure how to test this. + test('text styles - baseline', () async { + await testTextStyle('baseline', textBaseline: ui.TextBaseline.ideographic); + }); + + test('text styles - font family', () async { + await testTextStyle('font family', fontFamily: 'Ahem'); + }); + + test('text styles - non-existent font family', () async { + await testTextStyle('non-existent font family', fontFamily: 'DoesNotExist'); + }); + + test('text styles - family fallback', () async { + await testTextStyle('family fallback', fontFamily: 'DoesNotExist', fontFamilyFallback: ['Ahem']); + }); + + test('text styles - font size', () async { + await testTextStyle('font size', fontSize: 24); + }); + + test('text styles - letter spacing', () async { + await testTextStyle('letter spacing', letterSpacing: 5); + }); + + test('text styles - word spacing', () async { + await testTextStyle('word spacing', innerText: 'Beautiful World!', wordSpacing: 25); + }); + + test('text styles - height', () async { + await testTextStyle('height', height: 2); + }); + + // TODO(yjbanov): locales specified in text styles don't work: + // https://github.com/flutter/flutter/issues/74687 + // TODO(yjbanov): spaces are not rendered correctly: + // https://github.com/flutter/flutter/issues/74742 + test('text styles - locale zh_CN', () async { + await testTextStyle('locale zh_CN', innerText: '次 化 刃 直 入 令', outerText: '', locale: const ui.Locale('zh', 'CN')); + }); + + test('text styles - locale zh_TW', () async { + await testTextStyle('locale zh_TW', innerText: '次 化 刃 直 入 令', outerText: '', locale: const ui.Locale('zh', 'TW')); + }); + + test('text styles - locale ja', () async { + await testTextStyle('locale ja', innerText: '次 化 刃 直 入 令', outerText: '', locale: const ui.Locale('ja')); + }); + + test('text styles - locale ko', () async { + await testTextStyle('locale ko', innerText: '次 化 刃 直 入 令', outerText: '', locale: const ui.Locale('ko')); + }); + + test('text styles - background', () async { + await testTextStyle('background', background: CkPaint()..color = const ui.Color(0xFF00FF00)); + }); + + test('text styles - foreground', () async { + await testTextStyle('foreground', foreground: CkPaint()..color = const ui.Color(0xFF0000FF)); + }); + + test('text styles - foreground and background', () async { + await testTextStyle( + 'foreground and background', + foreground: CkPaint()..color = const ui.Color(0xFFFF5555), + background: CkPaint()..color = const ui.Color(0xFF007700), + ); + }); + + test('text styles - background and color', () async { + await testTextStyle( + 'background and color', + color: const ui.Color(0xFFFFFF00), + background: CkPaint()..color = const ui.Color(0xFF007700), + ); + }); + + test('text styles - shadows', () async { + await testTextStyle('shadows', shadows: [ + ui.Shadow( + color: const ui.Color(0xFF999900), + offset: const ui.Offset(10, 10), + blurRadius: 5, + ), + ui.Shadow( + color: const ui.Color(0xFF009999), + offset: const ui.Offset(-10, -10), + blurRadius: 10, + ), + ]); + }); + + test('text styles - old style figures', () async { + // TODO(yjbanov): we should not need to reset the fallbacks, see + // https://github.com/flutter/flutter/issues/74741 + skiaFontCollection.debugResetFallbackFonts(); + await testTextStyle( + 'old style figures', + paragraphFontFamily: 'Roboto', + paragraphFontSize: 24, + outerText: '0 1 2 3 4 5 ', + innerText: '0 1 2 3 4 5', + fontFeatures: [const ui.FontFeature.oldstyleFigures()], + ); + }); + + test('text styles - stylistic set 1', () async { + // TODO(yjbanov): we should not need to reset the fallbacks, see + // https://github.com/flutter/flutter/issues/74741 + skiaFontCollection.debugResetFallbackFonts(); + await testTextStyle( + 'stylistic set 1', + paragraphFontFamily: 'Roboto', + paragraphFontSize: 24, + outerText: 'g', + innerText: 'g', + fontFeatures: [ui.FontFeature.stylisticSet(1)], + ); + }); + + test('text styles - stylistic set 2', () async { + // TODO(yjbanov): we should not need to reset the fallbacks, see + // https://github.com/flutter/flutter/issues/74741 + skiaFontCollection.debugResetFallbackFonts(); + await testTextStyle( + 'stylistic set 2', + paragraphFontFamily: 'Roboto', + paragraphFontSize: 24, + outerText: 'α', + innerText: 'α', + fontFeatures: [ui.FontFeature.stylisticSet(2)], + ); + }); + + test('text styles - override font family', () async { + await testTextStyle( + 'override font family', + paragraphFontFamily: 'Ahem', + fontFamily: 'Roboto', + ); + }); + + test('text styles - override font size', () async { + await testTextStyle( + 'override font size', + paragraphFontSize: 36, + fontSize: 18, + ); + }); + + // TODO(yjbanov): paragraph fontWeight doesn't seem to work. + // https://github.com/flutter/flutter/issues/74338 + test('text style - override font weight', () async { + await testTextStyle( + 'override font weight', + paragraphFontWeight: ui.FontWeight.w900, + fontWeight: ui.FontWeight.normal, + ); + }); + + // TODO(yjbanov): paragraph fontStyle doesn't seem to work. + // https://github.com/flutter/flutter/issues/74338 + test('text style - override font style', () async { + await testTextStyle( + 'override font style', + paragraphFontStyle: ui.FontStyle.italic, + fontStyle: ui.FontStyle.normal, + ); + }); // TODO: https://github.com/flutter/flutter/issues/60040 // TODO: https://github.com/flutter/flutter/issues/71520 }, skip: isIosSafari || isFirefox); @@ -547,3 +827,165 @@ CkImage generateTestImage() { 4 * 20); return CkImage(skImage); } + +/// A convenience function for testing paragraph and text styles. +/// +/// Renders a paragraph with two pieces of text, [outerText] and [innerText]. +/// [outerText] is added to the root of the paragraph where only paragraph +/// style applies. [innerText] is added under a text style with properties +/// set from the arguments to this method. Parameters with prefix "paragraph" +/// are applied to the paragraph style. Others are applied to the text style. +/// +/// [name] is the name of the test used as the description on the golden as +/// well as in the golden file name. Avoid special characters. Spaces are OK; +/// they are replaced by "_" in the file name. +/// +/// Set [write] to true to overwrite the golden file. +/// +/// Use [layoutWidth] to customize the width of the paragraph constraints. +Future testTextStyle( + // Test properties + String name, { + bool write = false, + double? layoutWidth, + // Top-level text where only paragraph style applies + String outerText = 'Hello ', + // Second-level text where paragraph and text styles both apply. + String innerText = 'World!', + + // ParagraphStyle properties + ui.TextAlign? paragraphTextAlign, + ui.TextDirection? paragraphTextDirection, + int? paragraphMaxLines, + String? paragraphFontFamily, + double? paragraphFontSize, + double? paragraphHeight, + ui.TextHeightBehavior? paragraphTextHeightBehavior, + ui.FontWeight? paragraphFontWeight, + ui.FontStyle? paragraphFontStyle, + ui.StrutStyle? paragraphStrutStyle, + String? paragraphEllipsis, + ui.Locale? paragraphLocale, + + // TextStyle properties + ui.Color? color, + ui.TextDecoration? decoration, + ui.Color? decorationColor, + ui.TextDecorationStyle? decorationStyle, + double? decorationThickness, + ui.FontWeight? fontWeight, + ui.FontStyle? fontStyle, + ui.TextBaseline? textBaseline, + String? fontFamily, + List? fontFamilyFallback, + double? fontSize, + double? letterSpacing, + double? wordSpacing, + double? height, + ui.Locale? locale, + CkPaint? background, + CkPaint? foreground, + List? shadows, + List? fontFeatures, +}) async { + late ui.Rect region; + CkPicture renderPicture() { + const double testWidth = 512; + final CkPictureRecorder recorder = CkPictureRecorder(); + final CkCanvas canvas = recorder.beginRecording(ui.Rect.largest); + canvas.translate(30, 10); + final CkParagraphBuilder descriptionBuilder = CkParagraphBuilder(CkParagraphStyle()); + descriptionBuilder.addText(name); + final CkParagraph descriptionParagraph = descriptionBuilder.build(); + descriptionParagraph.layout(ui.ParagraphConstraints(width: testWidth / 2 - 70)); + final ui.Offset descriptionOffset = ui.Offset(testWidth / 2 + 30, 0); + canvas.drawParagraph(descriptionParagraph, descriptionOffset); + + final CkParagraphBuilder pb = CkParagraphBuilder(CkParagraphStyle( + textAlign: paragraphTextAlign, + textDirection: paragraphTextDirection, + maxLines: paragraphMaxLines, + fontFamily: paragraphFontFamily, + fontSize: paragraphFontSize, + height: paragraphHeight, + textHeightBehavior: paragraphTextHeightBehavior, + fontWeight: ui.FontWeight.normal, + fontStyle: ui.FontStyle.normal, + strutStyle: paragraphStrutStyle, + ellipsis: paragraphEllipsis, + locale: paragraphLocale, + )); + + pb.addText(outerText); + + pb.pushStyle(CkTextStyle( + color: color, + decoration: decoration, + decorationColor: decorationColor, + decorationStyle: decorationStyle, + decorationThickness: decorationThickness, + fontWeight: fontWeight, + fontStyle: fontStyle, + textBaseline: textBaseline, + fontFamily: fontFamily, + fontFamilyFallback: fontFamilyFallback, + fontSize: fontSize, + letterSpacing: letterSpacing, + wordSpacing: wordSpacing, + height: height, + locale: locale, + background: background, + foreground: foreground, + shadows: shadows, + fontFeatures: fontFeatures, + )); + pb.addText(innerText); + pb.pop(); + final CkParagraph p = pb.build(); + p.layout(ui.ParagraphConstraints(width: layoutWidth ?? testWidth / 2)); + canvas.drawParagraph(p, ui.Offset.zero); + + canvas.drawPath( + CkPath() + ..moveTo(-10, 0) + ..lineTo(-20, 0) + ..lineTo(-20, p.height) + ..lineTo(-10, p.height), + CkPaint() + ..style = ui.PaintingStyle.stroke + ..strokeWidth = 1.0, + ); + canvas.drawPath( + CkPath() + ..moveTo(testWidth / 2 + 10, 0) + ..lineTo(testWidth / 2 + 20, 0) + ..lineTo(testWidth / 2 + 20, p.height) + ..lineTo(testWidth / 2 + 10, p.height), + CkPaint() + ..style = ui.PaintingStyle.stroke + ..strokeWidth = 1.0, + ); + const double padding = 20; + region = ui.Rect.fromLTRB( + 0, 0, testWidth, + math.max( + descriptionOffset.dy + descriptionParagraph.height + padding, + p.height + padding, + ), + ); + return recorder.endRecording(); + } + + // Render once to trigger font downloads. + renderPicture(); + // Wait for fonts to finish loading. + await notoDownloadQueue.downloader.debugWhenIdle(); + // Render again for actual screenshotting. + final CkPicture picture = renderPicture(); + await matchPictureGolden( + 'canvaskit_text_styles_${name.replaceAll(' ', '_')}.png', + picture, + region: region, + write: write, + ); +} From 235db8d18efa327e88e7a038effed10d3b5e7339 Mon Sep 17 00:00:00 2001 From: Yegor Jbanov Date: Tue, 26 Jan 2021 12:00:01 -0800 Subject: [PATCH 2/5] add leak test --- .../test/canvaskit/canvas_golden_test.dart | 91 +++++++++++++++++++ 1 file changed, 91 insertions(+) diff --git a/lib/web_ui/test/canvaskit/canvas_golden_test.dart b/lib/web_ui/test/canvaskit/canvas_golden_test.dart index df714c80bb699..5b35bcbd6a995 100644 --- a/lib/web_ui/test/canvaskit/canvas_golden_test.dart +++ b/lib/web_ui/test/canvaskit/canvas_golden_test.dart @@ -492,11 +492,102 @@ void testMain() { fontStyle: ui.FontStyle.normal, ); }); + + test('text style - foreground/background/color do not leak across paragraphs', () async { + const double testWidth = 440; + const double middle = testWidth / 2; + CkParagraph createTestParagraph({ + ui.Color? color, + CkPaint? foreground, + CkPaint? background + }) { + final CkParagraphBuilder builder = CkParagraphBuilder(CkParagraphStyle()); + builder.pushStyle(CkTextStyle( + fontSize: 16, + color: color, + foreground: foreground, + background: background, + )); + final StringBuffer text = StringBuffer(); + if (color == null && foreground == null && background == null) { + text.write('Default'); + } else { + if (color != null) { + text.write('Color'); + } + if (foreground != null) { + if (text.isNotEmpty) { + text.write('+'); + } + text.write('Foreground'); + } + if (background != null) { + if (text.isNotEmpty) { + text.write('+'); + } + text.write('Background'); + } + } + builder.addText(text.toString()); + final CkParagraph paragraph = builder.build(); + paragraph.layout(ui.ParagraphConstraints(width: testWidth)); + return paragraph; + } + + final List variations = [ + () => createTestParagraph(), + () => createTestParagraph(color: ui.Color(0xFF009900)), + () => createTestParagraph(foreground: CkPaint()..color = ui.Color(0xFF990000)), + () => createTestParagraph(background: CkPaint()..color = ui.Color(0xFF7777FF)), + () => createTestParagraph( + color: ui.Color(0xFFFF00FF), + background: CkPaint()..color = ui.Color(0xFF0000FF), + ), + () => createTestParagraph( + foreground: CkPaint()..color = ui.Color(0xFF00FFFF), + background: CkPaint()..color = ui.Color(0xFF0000FF), + ), + ]; + + final CkPictureRecorder recorder = CkPictureRecorder(); + final CkCanvas canvas = recorder.beginRecording(ui.Rect.largest); + canvas.translate(10, 10); + + for (ParagraphFactory from in variations) { + for (ParagraphFactory to in variations) { + canvas.save(); + final CkParagraph fromParagraph = from(); + canvas.drawParagraph(fromParagraph, ui.Offset.zero); + + final ui.Offset leftEnd = ui.Offset(fromParagraph.maxIntrinsicWidth + 10, fromParagraph.height / 2); + final ui.Offset rightEnd = ui.Offset(middle - 10, leftEnd.dy); + final ui.Offset tipOffset = ui.Offset(-5, -5); + canvas.drawLine(leftEnd, rightEnd, CkPaint()); + canvas.drawLine(rightEnd, rightEnd + tipOffset, CkPaint()); + canvas.drawLine(rightEnd, rightEnd + tipOffset.scale(1, -1), CkPaint()); + + canvas.translate(middle, 0); + canvas.drawParagraph(to(), ui.Offset.zero); + canvas.restore(); + canvas.translate(0, 22); + } + } + + final CkPicture picture = recorder.endRecording(); + await matchPictureGolden( + 'canvaskit_text_styles_do_not_leak.png', + picture, + region: ui.Rect.fromLTRB(0, 0, testWidth, 850), + write: true, + ); + }, solo: true); // TODO: https://github.com/flutter/flutter/issues/60040 // TODO: https://github.com/flutter/flutter/issues/71520 }, skip: isIosSafari || isFirefox); } +typedef ParagraphFactory = CkParagraph Function(); + void drawTestPicture(CkCanvas canvas) { canvas.clear(ui.Color(0xFFFFFFF)); From 75e1c68071e41337ea4344d5a643ecb1a220f8b5 Mon Sep 17 00:00:00 2001 From: Yegor Jbanov Date: Tue, 26 Jan 2021 13:17:33 -0800 Subject: [PATCH 3/5] update goldens_lock.yaml --- lib/web_ui/dev/goldens_lock.yaml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/web_ui/dev/goldens_lock.yaml b/lib/web_ui/dev/goldens_lock.yaml index 310fd510023c1..af98c8b3c60dc 100644 --- a/lib/web_ui/dev/goldens_lock.yaml +++ b/lib/web_ui/dev/goldens_lock.yaml @@ -1,2 +1,2 @@ repository: https://github.com/flutter/goldens.git -revision: 4c3d34f19045a1df10757e5b3cb6c9ace9a6038c +revision: f331294a781874e213a01f093f8113a2206d3e59 \ No newline at end of file From a3f185da967895236cf9a07979a1eb2066e02c13 Mon Sep 17 00:00:00 2001 From: Yegor Jbanov Date: Tue, 26 Jan 2021 13:18:23 -0800 Subject: [PATCH 4/5] remove solo --- lib/web_ui/test/canvaskit/canvas_golden_test.dart | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/web_ui/test/canvaskit/canvas_golden_test.dart b/lib/web_ui/test/canvaskit/canvas_golden_test.dart index 5b35bcbd6a995..9026d65761fcf 100644 --- a/lib/web_ui/test/canvaskit/canvas_golden_test.dart +++ b/lib/web_ui/test/canvaskit/canvas_golden_test.dart @@ -580,7 +580,7 @@ void testMain() { region: ui.Rect.fromLTRB(0, 0, testWidth, 850), write: true, ); - }, solo: true); + }); // TODO: https://github.com/flutter/flutter/issues/60040 // TODO: https://github.com/flutter/flutter/issues/71520 }, skip: isIosSafari || isFirefox); From cb2460f3ea097672447dc5b049f4c9ea3b857c6b Mon Sep 17 00:00:00 2001 From: Yegor Jbanov Date: Tue, 26 Jan 2021 16:48:14 -0800 Subject: [PATCH 5/5] Warn when popping out of empty text style stack --- lib/web_ui/lib/src/engine/canvaskit/text.dart | 6 ++++++ lib/web_ui/test/canvaskit/canvas_golden_test.dart | 1 - 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/lib/web_ui/lib/src/engine/canvaskit/text.dart b/lib/web_ui/lib/src/engine/canvaskit/text.dart index 1e2337e7c6740..ed79932ff999f 100644 --- a/lib/web_ui/lib/src/engine/canvaskit/text.dart +++ b/lib/web_ui/lib/src/engine/canvaskit/text.dart @@ -752,6 +752,12 @@ class CkParagraphBuilder implements ui.ParagraphBuilder { void pop() { if (_styleStack.length <= 1) { // The top-level text style is paragraph-level. We don't pop it off. + if (assertionsEnabled) { + html.window.console.warn( + 'Cannot pop text style in ParagraphBuilder. ' + 'Already popped all text styles from the style stack.', + ); + } return; } _commands.add(const _ParagraphCommand.pop()); diff --git a/lib/web_ui/test/canvaskit/canvas_golden_test.dart b/lib/web_ui/test/canvaskit/canvas_golden_test.dart index 9026d65761fcf..3688f67e0171b 100644 --- a/lib/web_ui/test/canvaskit/canvas_golden_test.dart +++ b/lib/web_ui/test/canvaskit/canvas_golden_test.dart @@ -578,7 +578,6 @@ void testMain() { 'canvaskit_text_styles_do_not_leak.png', picture, region: ui.Rect.fromLTRB(0, 0, testWidth, 850), - write: true, ); }); // TODO: https://github.com/flutter/flutter/issues/60040