[web] Fix image gap due to svg element without position attribute (flutter#21939)

ferhatb · web-flow · commit 79879802e05e · 2020-10-19T22:15:06.000-07:00
diff --git a/lib/web_ui/lib/src/engine/html/color_filter.dart b/lib/web_ui/lib/src/engine/html/color_filter.dart
@@ -208,7 +208,9 @@ String? svgFilterFromBlendMode(
     case ui.BlendMode.dstOver:
     case ui.BlendMode.clear:
     case ui.BlendMode.srcOver:
-      assert(false, 'Invalid svg filter request for blend-mode '
+      assert(
+          false,
+          'Invalid svg filter request for blend-mode '
           '$colorFilterBlendMode');
       break;
   }
@@ -232,7 +234,7 @@ int _filterIdCounter = 0;
 // A' = a1*R + a2*G + a3*B + a4*A + a5
 String _srcInColorFilterToSvg(ui.Color? color) {
   _filterIdCounter += 1;
-  return '<svg width="0" height="0">'
+  return '$kSvgResourceHeader'
       '<filter id="_fcf$_filterIdCounter" '
       'filterUnits="objectBoundingBox" x="0%" y="0%" width="100%" height="100%">'
       '<feColorMatrix values="0 0 0 0 1 ' // Ignore input, set it to absolute.
@@ -249,7 +251,7 @@ String _srcInColorFilterToSvg(ui.Color? color) {
 
 String _srcOutColorFilterToSvg(ui.Color? color) {
   _filterIdCounter += 1;
-  return '<svg width="0" height="0">'
+  return '$kSvgResourceHeader'
       '<filter id="_fcf$_filterIdCounter" '
       'filterUnits="objectBoundingBox" x="0%" y="0%" width="100%" height="100%">'
       '<feFlood flood-color="${colorToCssString(color)}" flood-opacity="1" result="flood">'
@@ -261,7 +263,7 @@ String _srcOutColorFilterToSvg(ui.Color? color) {
 
 String _xorColorFilterToSvg(ui.Color? color) {
   _filterIdCounter += 1;
-  return '<svg width="0" height="0">'
+  return '$kSvgResourceHeader'
       '<filter id="_fcf$_filterIdCounter" '
       'filterUnits="objectBoundingBox" x="0%" y="0%" width="100%" height="100%">'
       '<feFlood flood-color="${colorToCssString(color)}" flood-opacity="1" result="flood">'
@@ -276,7 +278,7 @@ String _xorColorFilterToSvg(ui.Color? color) {
 String _compositeColorFilterToSvg(
     ui.Color? color, double k1, double k2, double k3, double k4) {
   _filterIdCounter += 1;
-  return '<svg width="0" height="0">'
+  return '$kSvgResourceHeader'
       '<filter id="_fcf$_filterIdCounter" '
       'filterUnits="objectBoundingBox" x="0%" y="0%" width="100%" height="100%">'
       '<feFlood flood-color="${colorToCssString(color)}" flood-opacity="1" result="flood">'
@@ -295,7 +297,7 @@ String _modulateColorFilterToSvg(ui.Color color) {
   final double r = color.red / 255.0;
   final double b = color.blue / 255.0;
   final double g = color.green / 255.0;
-  return '<svg width="0" height="0">'
+  return '$kSvgResourceHeader'
       '<filter id="_fcf$_filterIdCounter" '
       'filterUnits="objectBoundingBox" x="0%" y="0%" width="100%" height="100%">'
       '<feColorMatrix values="0 0 0 0 $r ' // Ignore input, set it to absolute.
@@ -312,7 +314,7 @@ String _modulateColorFilterToSvg(ui.Color color) {
 String _blendColorFilterToSvg(ui.Color? color, String? feBlend,
     {bool swapLayers = false}) {
   _filterIdCounter += 1;
-  return '<svg width="0" height="0">'
+  return '$kSvgResourceHeader'
           '<filter id="_fcf$_filterIdCounter" filterUnits="objectBoundingBox" '
           'x="0%" y="0%" width="100%" height="100%">'
           '<feFlood flood-color="${colorToCssString(color)}" flood-opacity="1" result="flood">'
diff --git a/lib/web_ui/lib/src/engine/util.dart b/lib/web_ui/lib/src/engine/util.dart
@@ -111,8 +111,8 @@ TransformKind transformKindOf(Float32List matrix) {
 
   // If matrix contains scaling, rotation, z translation or
   // perspective transform, it is not considered simple.
-  final bool isSimple2dTransform =
-      m[15] == 1.0 &&  // start reading from the last element to eliminate range checks in subsequent reads.
+  final bool isSimple2dTransform = m[15] ==
+          1.0 && // start reading from the last element to eliminate range checks in subsequent reads.
       m[14] == 0.0 && // z translation is NOT simple
       // m[13] - y translation is simple
       // m[12] - x translation is simple
@@ -126,8 +126,8 @@ TransformKind transformKindOf(Float32List matrix) {
       // m[4] - 2D rotation is simple
       m[3] == 0.0 &&
       m[2] == 0.0;
-      // m[1] - 2D rotation is simple
-      // m[0] - scale x is simple
+  // m[1] - 2D rotation is simple
+  // m[0] - scale x is simple
 
   if (!isSimple2dTransform) {
     return TransformKind.complex;
@@ -136,8 +136,7 @@ TransformKind transformKindOf(Float32List matrix) {
   // From this point on we're sure the transform is 2D, but we don't know if
   // it's identity or not. To check, we need to look at the remaining elements
   // that were not checked above.
-  final bool isIdentityTransform =
-      m[0] == 1.0 &&
+  final bool isIdentityTransform = m[0] == 1.0 &&
       m[1] == 0.0 &&
       m[4] == 0.0 &&
       m[5] == 1.0 &&
@@ -165,7 +164,7 @@ bool isIdentityFloat32ListTransform(Float32List matrix) {
 /// transform. Consider removing the CSS `transform` property from elements
 /// that apply identity transform.
 String float64ListToCssTransform2d(Float32List matrix) {
-  assert (transformKindOf(matrix) != TransformKind.complex);
+  assert(transformKindOf(matrix) != TransformKind.complex);
   return 'matrix(${matrix[0]},${matrix[1]},${matrix[4]},${matrix[5]},${matrix[12]},${matrix[13]})';
 }
 
@@ -279,10 +278,22 @@ void transformLTRB(Matrix4 transform, Float32List ltrb) {
 
   _tempPointMatrix.multiplyTranspose(transform);
 
-  ltrb[0] = math.min(math.min(math.min(_tempPointData[0], _tempPointData[1]), _tempPointData[2]), _tempPointData[3]);
-  ltrb[1] = math.min(math.min(math.min(_tempPointData[4], _tempPointData[5]), _tempPointData[6]), _tempPointData[7]);
-  ltrb[2] = math.max(math.max(math.max(_tempPointData[0], _tempPointData[1]), _tempPointData[2]), _tempPointData[3]);
-  ltrb[3] = math.max(math.max(math.max(_tempPointData[4], _tempPointData[5]), _tempPointData[6]), _tempPointData[7]);
+  ltrb[0] = math.min(
+      math.min(
+          math.min(_tempPointData[0], _tempPointData[1]), _tempPointData[2]),
+      _tempPointData[3]);
+  ltrb[1] = math.min(
+      math.min(
+          math.min(_tempPointData[4], _tempPointData[5]), _tempPointData[6]),
+      _tempPointData[7]);
+  ltrb[2] = math.max(
+      math.max(
+          math.max(_tempPointData[0], _tempPointData[1]), _tempPointData[2]),
+      _tempPointData[3]);
+  ltrb[3] = math.max(
+      math.max(
+          math.max(_tempPointData[4], _tempPointData[5]), _tempPointData[6]),
+      _tempPointData[7]);
 }
 
 /// Returns true if [rect] contains every point that is also contained by the
@@ -300,6 +311,13 @@ bool rectContainsOther(ui.Rect rect, ui.Rect other) {
 /// Counter used for generating clip path id inside an svg <defs> tag.
 int _clipIdCounter = 0;
 
+/// Used for clipping and filter svg resources.
+///
+/// Position needs to be absolute since these svgs are sandwiched between
+/// canvas elements and can cause layout shifts otherwise.
+const String kSvgResourceHeader = '<svg width="0" height="0" '
+    'style="position:absolute">';
+
 /// Converts Path to svg element that contains a clip-path definition.
 ///
 /// Calling this method updates [_clipIdCounter]. The HTML id of the generated
@@ -311,8 +329,7 @@ String _pathToSvgClipPath(ui.Path path,
     double scaleY = 1.0}) {
   _clipIdCounter += 1;
   final StringBuffer sb = StringBuffer();
-  sb.write('<svg width="0" height="0" '
-      'style="position:absolute">');
+  sb.write(kSvgResourceHeader);
   sb.write('<defs>');
 
   final String clipId = 'svgClip$_clipIdCounter';
@@ -420,10 +437,11 @@ const Set<String> _genericFontFamilies = <String>{
 ///
 /// For iOS, default to -apple-system, where it should be available, otherwise
 /// default to Arial. BlinkMacSystemFont is used for Chrome on iOS.
-final String _fallbackFontFamily = _isMacOrIOS ?
-    '-apple-system, BlinkMacSystemFont' : 'Arial';
+final String _fallbackFontFamily =
+    _isMacOrIOS ? '-apple-system, BlinkMacSystemFont' : 'Arial';
 
-bool get _isMacOrIOS => operatingSystem == OperatingSystem.iOs ||
+bool get _isMacOrIOS =>
+    operatingSystem == OperatingSystem.iOs ||
     operatingSystem == OperatingSystem.macOs;
 
 /// Create a font-family string appropriate for CSS.
@@ -439,8 +457,10 @@ String? canonicalizeFontFamily(String? fontFamily) {
     // on sans-serif.
     // Map to San Francisco Text/Display fonts, use -apple-system,
     // BlinkMacSystemFont.
-    if (fontFamily == '.SF Pro Text' || fontFamily == '.SF Pro Display' ||
-        fontFamily == '.SF UI Text' || fontFamily == '.SF UI Display') {
+    if (fontFamily == '.SF Pro Text' ||
+        fontFamily == '.SF Pro Display' ||
+        fontFamily == '.SF UI Text' ||
+        fontFamily == '.SF UI Display') {
       return _fallbackFontFamily;
     }
   }
@@ -474,27 +494,27 @@ void applyWebkitClipFix(html.Element? containerElement) {
   }
 }
 
-final ByteData? _fontChangeMessage = JSONMessageCodec().encodeMessage(<String, dynamic>{'type': 'fontsChange'});
+final ByteData? _fontChangeMessage =
+    JSONMessageCodec().encodeMessage(<String, dynamic>{'type': 'fontsChange'});
 
 // Font load callbacks will typically arrive in sequence, we want to prevent
 // sendFontChangeMessage of causing multiple synchronous rebuilds.
 // This flag ensures we properly schedule a single call to framework.
 bool _fontChangeScheduled = false;
 
 FutureOr<void> sendFontChangeMessage() async {
-  if (window._onPlatformMessage != null)
-    if (!_fontChangeScheduled) {
-      _fontChangeScheduled = true;
-      // Batch updates into next animationframe.
-      html.window.requestAnimationFrame((num _) {
-        _fontChangeScheduled = false;
-        window.invokeOnPlatformMessage(
-          'flutter/system',
-          _fontChangeMessage,
-              (_) {},
-        );
-      });
-    }
+  if (window._onPlatformMessage != null && !_fontChangeScheduled) {
+    _fontChangeScheduled = true;
+    // Batch updates into next animationframe.
+    html.window.requestAnimationFrame((num _) {
+      _fontChangeScheduled = false;
+      window.invokeOnPlatformMessage(
+        'flutter/system',
+        _fontChangeMessage,
+        (_) {},
+      );
+    });
+  }
 }
 
 // Stores matrix in a form that allows zero allocation transforms.
@@ -535,14 +555,12 @@ bool isUnsoundNull(dynamic object) {
 }
 
 bool _offsetIsValid(ui.Offset offset) {
-  assert(offset != null, 'Offset argument was null.'); // ignore: unnecessary_null_comparison
   assert(!offset.dx.isNaN && !offset.dy.isNaN,
       'Offset argument contained a NaN value.');
   return true;
 }
 
 bool _matrix4IsValid(Float32List matrix4) {
-  assert(matrix4 != null, 'Matrix4 argument was null.'); // ignore: unnecessary_null_comparison
   assert(matrix4.length == 16, 'Matrix4 must have 16 entries.');
   return true;
 }
diff --git a/lib/web_ui/test/golden_tests/engine/color_filter_golden_test.dart b/lib/web_ui/test/golden_tests/engine/color_filter_golden_test.dart
@@ -2,8 +2,9 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-// @dart = 2.6
+// @dart = 2.10
 import 'dart:html' as html;
+import 'dart:js_util' as js_util;
 
 import 'package:test/bootstrap/browser.dart';
 import 'package:test/test.dart';
@@ -35,55 +36,118 @@ void testMain() async {
     final SurfaceSceneBuilder builder = SurfaceSceneBuilder();
     final Picture backgroundPicture = _drawBackground();
     builder.addPicture(Offset.zero, backgroundPicture);
-    builder.pushColorFilter(EngineColorFilter.mode(Color(0xF0000080),
-        BlendMode.color));
+    builder.pushColorFilter(
+        EngineColorFilter.mode(Color(0xF0000080), BlendMode.color));
     final Picture circles1 = _drawTestPictureWithCircles(30, 30);
     builder.addPicture(Offset.zero, circles1);
     builder.pop();
-    html.document.body.append(builder
-        .build()
-        .webOnlyRootElement);
+    html.document.body!.append(builder.build().webOnlyRootElement!);
 
     await matchGoldenFile('color_filter_blendMode_color.png', region: region);
   });
+
+  /// Regression test for https://github.com/flutter/flutter/issues/59451.
+  ///
+  /// Picture with overlay blend inside a physical shape. Should show image
+  /// at 0,0. In the filed issue it was leaving a gap on top.
+  test('Should render image with color filter without gap', () async {
+    final SurfaceSceneBuilder builder = SurfaceSceneBuilder();
+    final Path path = Path();
+    path.addRRect(RRect.fromRectAndRadius(
+        Rect.fromLTRB(0, 0, 400, 400), Radius.circular(2)));
+    PhysicalShapeEngineLayer oldLayer = builder.pushPhysicalShape(
+        path: path, color: Color(0xFFFFFFFF), elevation: 0);
+    final Picture circles1 = _drawTestPictureWithImage(
+        ColorFilter.mode(Color(0x3C4043), BlendMode.overlay));
+    builder.addPicture(Offset(10, 0), circles1);
+    builder.pop();
+    builder.build();
+
+    final SurfaceSceneBuilder builder2 = SurfaceSceneBuilder();
+    builder2.pushPhysicalShape(
+        path: path, color: Color(0xFFFFFFFF), elevation: 0, oldLayer: oldLayer);
+    builder2.addPicture(Offset(10, 0), circles1);
+    builder2.pop();
+
+    html.document.body!.append(builder2.build().webOnlyRootElement!);
+
+    await matchGoldenFile('color_filter_blendMode_overlay.png',
+        region: region);
+  });
 }
 
 Picture _drawTestPictureWithCircles(double offsetX, double offsetY) {
-  final EnginePictureRecorder recorder = PictureRecorder();
+  final EnginePictureRecorder recorder =
+      PictureRecorder() as EnginePictureRecorder;
   final RecordingCanvas canvas =
-  recorder.beginRecording(const Rect.fromLTRB(0, 0, 400, 400));
-  canvas.drawCircle(
-      Offset(offsetX + 10, offsetY + 10), 10, Paint()..style = PaintingStyle.fill);
+      recorder.beginRecording(const Rect.fromLTRB(0, 0, 400, 400));
+  canvas.drawCircle(Offset(offsetX + 10, offsetY + 10), 10,
+      (Paint()..style = PaintingStyle.fill) as SurfacePaint);
   canvas.drawCircle(
       Offset(offsetX + 60, offsetY + 10),
       10,
-      Paint()
+      (Paint()
         ..style = PaintingStyle.fill
-        ..color = const Color.fromRGBO(255, 0, 0, 1));
+        ..color = const Color.fromRGBO(255, 0, 0, 1)) as SurfacePaint);
   canvas.drawCircle(
       Offset(offsetX + 10, offsetY + 60),
       10,
-      Paint()
+      (Paint()
         ..style = PaintingStyle.fill
-        ..color = const Color.fromRGBO(0, 255, 0, 1));
+        ..color = const Color.fromRGBO(0, 255, 0, 1)) as SurfacePaint);
   canvas.drawCircle(
       Offset(offsetX + 60, offsetY + 60),
       10,
-      Paint()
+      (Paint()
         ..style = PaintingStyle.fill
-        ..color = const Color.fromRGBO(0, 0, 255, 1));
+        ..color = const Color.fromRGBO(0, 0, 255, 1)) as SurfacePaint);
+  return recorder.endRecording();
+}
+
+Picture _drawTestPictureWithImage(ColorFilter filter) {
+  final EnginePictureRecorder recorder =
+      PictureRecorder() as EnginePictureRecorder;
+  final RecordingCanvas canvas =
+      recorder.beginRecording(const Rect.fromLTRB(0, 0, 400, 400));
+  final Image testImage = createTestImage();
+  canvas.drawImageRect(
+      testImage,
+      Rect.fromLTWH(0, 0, 200, 150),
+      Rect.fromLTWH(0, 0, 300, 300),
+      (Paint()
+        ..style = PaintingStyle.fill
+        ..colorFilter = filter
+        ..color = const Color.fromRGBO(0, 0, 255, 1)) as SurfacePaint);
   return recorder.endRecording();
 }
 
 Picture _drawBackground() {
-  final EnginePictureRecorder recorder = PictureRecorder();
+  final EnginePictureRecorder recorder =
+      PictureRecorder() as EnginePictureRecorder;
   final RecordingCanvas canvas =
-  recorder.beginRecording(const Rect.fromLTRB(0, 0, 400, 400));
+      recorder.beginRecording(const Rect.fromLTRB(0, 0, 400, 400));
   canvas.drawRect(
       Rect.fromLTWH(8, 8, 400.0 - 16, 400.0 - 16),
-      Paint()
+      (Paint()
         ..style = PaintingStyle.fill
-        ..color = Color(0xFFE0FFE0)
-  );
+        ..color = Color(0xFFE0FFE0)) as SurfacePaint);
   return recorder.endRecording();
 }
+
+HtmlImage createTestImage({int width = 200, int height = 150}) {
+  html.CanvasElement canvas =
+      new html.CanvasElement(width: width, height: height);
+  html.CanvasRenderingContext2D ctx = canvas.context2D;
+  ctx.fillStyle = '#E04040';
+  ctx.fillRect(0, 0, width / 3, height);
+  ctx.fill();
+  ctx.fillStyle = '#40E080';
+  ctx.fillRect(width / 3, 0, width / 3, height);
+  ctx.fill();
+  ctx.fillStyle = '#2040E0';
+  ctx.fillRect(2 * width / 3, 0, width / 3, height);
+  ctx.fill();
+  html.ImageElement imageElement = html.ImageElement();
+  imageElement.src = js_util.callMethod(canvas, 'toDataURL', <dynamic>[]);
+  return HtmlImage(imageElement, width, height);
+}
