Partially fix canvas vs text paint order when running on Blink/Webkit browsers (#16483)

fix canvas vs text paint order in Blink/Webkit

lib/web_ui/dev/goldens_lock.yaml

 repository: https://github.com/flutter/goldens.git
-revision: 1637835646ef187884ceeb59011d70c463429876
+revision: 956d4e1862b108b31afd06cbf0a767cefc72f4c5

lib/web_ui/lib/src/engine/bitmap_canvas.dart

@@ -371,8 +371,8 @@ class BitmapCanvas extends EngineCanvas {
         _children.add(clipElement);
       }
     } else {
-      final String cssTransform = matrix4ToCssTransform3d(
-          transformWithOffset(_canvasPool.currentTransform, p));
+      final String cssTransform = float64ListToCssTransform(
+          transformWithOffset(_canvasPool.currentTransform, p).storage);
       imgElement.style
         ..transformOrigin = '0 0 0'
         ..transform = cssTransform;
@@ -705,6 +705,7 @@ List<html.Element> _clipContent(List<_SaveClipEntry> clipStack,
     final _SaveClipEntry entry = clipStack[clipIndex];
     final html.HtmlElement newElement = html.DivElement();
     newElement.style.position = 'absolute';
+    applyWebkitClipFix(newElement);
     if (root == null) {
       root = newElement;
     } else {

lib/web_ui/lib/src/engine/canvas_pool.dart

@@ -8,8 +8,8 @@ part of engine;
 ///
 /// [BitmapCanvas] signals allocation of first canvas using allocateCanvas.
 /// When a painting command such as drawImage or drawParagraph requires
-/// multiple canvases for correct compositing, it calls allocateExtraCanvas and
-/// adds the canvas(s) to a [_pool] of active canvas(s).
+/// multiple canvases for correct compositing, it calls [allocateExtraCanvas]
+/// and adds the canvas(s) to a [_pool] of active canvas(s).
 ///
 /// To make sure transformations and clips are preserved correctly when a new
 /// canvas is allocated, [_CanvasPool] replays the current stack on the newly
@@ -29,6 +29,7 @@ class _CanvasPool extends _SaveStackTracking {
   List<html.CanvasElement> _reusablePool;
   // Current canvas element or null if marked for lazy allocation.
   html.CanvasElement _canvas;
+
   html.HtmlElement _rootElement;
   int _saveContextCount = 0;
 
@@ -98,6 +99,21 @@ class _CanvasPool extends _SaveStackTracking {
         ..width = '${cssWidth}px'
         ..height = '${cssHeight}px';
     }
+
+    // When the picture has a 90-degree transform and clip in its
+    // ancestor layers, it triggers a bug in Blink and Webkit browsers
+    // that results in canvas obscuring text that should be painted on
+    // top. Setting z-index to any negative value works around the bug.
+    // This workaround only works with the first canvas. If more than
+    // one element have negative z-index, the bug is triggered again.
+    //
+    // Possible Blink bugs that are causing this:
+    // * https://bugs.chromium.org/p/chromium/issues/detail?id=370604
+    // * https://bugs.chromium.org/p/chromium/issues/detail?id=586601
+    final bool isFirstChildElement = _rootElement.firstChild == null;
+    if (isFirstChildElement) {
+      _canvas.style.zIndex = '-1';
+    }
     _rootElement.append(_canvas);
     _context = _canvas.context2D;
     _contextHandle = ContextStateHandle(_context);

lib/web_ui/lib/src/engine/surface/surface.dart

@@ -638,6 +638,7 @@ abstract class PersistedSurface implements ui.EngineLayer {
     assert(rootElement == null);
     assert(isCreated);
     rootElement = createElement();
+    applyWebkitClipFix(rootElement);
     if (_debugExplainSurfaceStats) {
       _surfaceStatsFor(this).allocatedDomNodeCount++;
     }

lib/web_ui/lib/src/engine/util.dart

@@ -54,73 +54,52 @@ String matrix4ToCssTransform(Matrix4 matrix) {
   return float64ListToCssTransform(matrix.storage);
 }
 
-/// Converts [matrix] to CSS transform value.
-String matrix4ToCssTransform3d(Matrix4 matrix) {
-  return float64ListToCssTransform3d(matrix.storage);
+/// Applies a transform to the [element].
+///
+/// See [float64ListToCssTransform] for details on how the CSS value is chosen.
+void setElementTransform(html.Element element, Float64List matrix4) {
+  element.style
+    ..transformOrigin = '0 0 0'
+    ..transform = float64ListToCssTransform(matrix4);
 }
 
-/// Applies a transform to the [element].
+/// Converts [matrix] to CSS transform value.
 ///
-/// There are several ways to transform an element. This function chooses
-/// between CSS "transform", "left", "top" or no transform, depending on the
-/// [matrix4] and the current device's screen properties. This function
-/// attempts to avoid issues with text blurriness on low pixel density screens.
+/// To avoid blurry text on some screens this function uses a 2D CSS transform
+/// if it detects that [matrix] is a 2D transform. Otherwise, it uses a 3D CSS
+/// transform.
 ///
 /// See also:
 ///  * https://github.com/flutter/flutter/issues/32274
 ///  * https://bugs.chromium.org/p/chromium/issues/detail?id=1040222
-void setElementTransform(html.Element element, Float64List matrix4) {
-  final TransformKind transformKind = transformKindOf(matrix4);
-
-  // On low device-pixel ratio screens using CSS "transform" causes text blurriness
-  // at least on Blink browsers. We therefore prefer using CSS "left" and "top" instead.
-  final bool isHighDevicePixelRatioScreen =
-      EngineWindow.browserDevicePixelRatio > 1.0;
-
-  if (transformKind == TransformKind.scaleAndTranslate2d) {
-    final String cssTransform = float64ListToCssTransform2d(matrix4);
-    element.style
-      ..transformOrigin = '0 0 0'
-      ..transform = cssTransform
-      ..top = null
-      ..left = null;
-  } else if (transformKind == TransformKind.complex || isHighDevicePixelRatioScreen) {
-    final String cssTransform = float64ListToCssTransform3d(matrix4);
-    element.style
-      ..transformOrigin = '0 0 0'
-      ..transform = cssTransform
-      ..top = null
-      ..left = null;
-  } else if (transformKind == TransformKind.translation2d) {
-    final double ty = matrix4[13];
-    final double tx = matrix4[12];
-    element.style
-      ..transformOrigin = null
-      ..transform = null
-      ..left = '${tx}px'
-      ..top = '${ty}px';
+String float64ListToCssTransform(Float64List matrix) {
+  assert(matrix.length == 16);
+  final TransformKind transformKind = transformKindOf(matrix);
+  if (transformKind == TransformKind.transform2d) {
+    return float64ListToCssTransform2d(matrix);
+  } else if (transformKind == TransformKind.complex) {
+    return float64ListToCssTransform3d(matrix);
   } else {
     assert(transformKind == TransformKind.identity);
-    element.style
-      ..transformOrigin = null
-      ..transform = null
-      ..left = null
-      ..top = null;
+    return null;
   }
 }
 
 /// The kind of effect a transform matrix performs.
 enum TransformKind {
   /// No effect.
+  ///
+  /// We do not want to set any CSS properties in this case.
   identity,
 
-  /// A transform that contains only 2d scale and transform.
-  scaleAndTranslate2d,
-
-  /// A translation along either X or Y axes, or both.
-  translation2d,
+  /// A transform that contains only 2d scale, rotation, and translation.
+  ///
+  /// We prefer to use "matrix" instead of "matrix3d" in this case.
+  transform2d,
 
   /// All other kinds of transforms.
+  ///
+  /// In this case we will use "matrix3d".
   complex,
 }
 
@@ -128,41 +107,46 @@ enum TransformKind {
 TransformKind transformKindOf(Float64List matrix) {
   assert(matrix.length == 16);
   final Float64List m = matrix;
-  final double ty = m[13];
-  final double tx = m[12];
 
   // If matrix contains scaling, rotation, z translation or
   // perspective transform, it is not considered simple.
   final bool isSimple2dTransform =
-      // m[0] - scale x is simple
-      m[1] == 0.0 &&
-      m[2] == 0.0 &&
-      m[3] == 0.0 &&
-      m[4] == 0.0 &&
-      // m[5] - scale y is simple
-      m[6] == 0.0 &&
-      m[7] == 0.0 &&
-      m[8] == 0.0 &&
-      m[9] == 0.0 &&
-      m[10] == 1.0 &&
-      m[11] == 0.0 &&
-      // m[12] - x translation is simple
-      // m[13] - y translation is simple
+      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[15] == 1.0;
+      // m[13] - y translation is simple
+      // m[12] - x translation is simple
+      m[11] == 0.0 &&
+      m[10] == 1.0 &&
+      m[9] == 0.0 &&
+      m[8] == 0.0 &&
+      m[7] == 0.0 &&
+      m[6] == 0.0 &&
+      // m[5] - scale y is simple
+      // 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
 
   if (!isSimple2dTransform) {
     return TransformKind.complex;
   }
 
-  if (m[0] == 1.0 && m[5] == 1.0) {
-    if (ty != 0.0 || tx != 0.0) {
-      return TransformKind.translation2d;
-    } else {
+  // 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 &&
+      m[1] == 0.0 &&
+      m[4] == 0.0 &&
+      m[5] == 1.0 &&
+      m[12] == 0.0 &&
+      m[13] == 0.0;
+
+  if (isIdentityTransform) {
     return TransformKind.identity;
-    }
   } else {
-    return TransformKind.scaleAndTranslate2d;
+    return TransformKind.transform2d;
   }
 }
 
@@ -172,44 +156,19 @@ bool isIdentityFloat64ListTransform(Float64List matrix) {
   return transformKindOf(matrix) == TransformKind.identity;
 }
 
-/// Converts [matrix] to CSS transform value.
+/// Converts [matrix] to CSS transform 2D matrix value.
+///
+/// The [matrix] must not be a [TransformKind.complex] transform, because CSS
+/// `matrix` can only express 2D transforms. [TransformKind.identity] is
+/// permitted. However, it is inefficient to construct a matrix for an identity
+/// transform. Consider removing the CSS `transform` property from elements
+/// that apply identity transform.
 String float64ListToCssTransform2d(Float64List matrix) {
-  assert (transformKindOf(matrix) == TransformKind.scaleAndTranslate2d);
-  return 'matrix(${matrix[0]},0,0,${matrix[5]},${matrix[12]},${matrix[13]})';
-}
-
-/// Converts [matrix] to CSS transform value.
-String float64ListToCssTransform(Float64List matrix) {
-  assert(matrix.length == 16);
-  final Float64List m = matrix;
-  if (m[1] == 0.0 &&
-      m[2] == 0.0 &&
-      m[3] == 0.0 &&
-      m[4] == 0.0 &&
-      m[6] == 0.0 &&
-      m[7] == 0.0 &&
-      m[8] == 0.0 &&
-      m[9] == 0.0 &&
-      m[10] == 1.0 &&
-      m[11] == 0.0 &&
-      // 12 can be anything (translation)
-      // 13 can be anything (translation)
-      m[14] == 0.0 &&
-      m[15] == 1.0) {
-    final double tx = m[12];
-    final double ty = m[13];
-    if (m[0] == 1.0 &&
-          m[5] == 1.0) {
-        return 'translate(${tx}px, ${ty}px)';
-    } else {
-      return 'matrix(${m[0]},0,0,${m[5]},${tx},${ty})';
-    }
-  } else {
-    return 'matrix3d(${m[0]},${m[1]},${m[2]},${m[3]},${m[4]},${m[5]},${m[6]},${m[7]},${m[8]},${m[9]},${m[10]},${m[11]},${m[12]},${m[13]},${m[14]},${m[15]})';
-  }
+  assert (transformKindOf(matrix) != TransformKind.complex);
+  return 'matrix(${matrix[0]},${matrix[1]},${matrix[4]},${matrix[5]},${matrix[12]},${matrix[13]})';
 }
 
-/// Converts [matrix] to CSS transform value.
+/// Converts [matrix] to a 3D CSS transform value.
 String float64ListToCssTransform3d(Float64List matrix) {
   assert(matrix.length == 16);
   final Float64List m = matrix;
@@ -458,3 +417,19 @@ Float32List offsetListToFloat32List(List<ui.Offset> offsetList) {
   }
   return floatList;
 }
+
+/// Apply this function to container elements in the HTML render tree (this is
+/// not relevant to semantics tree).
+///
+/// On WebKit browsers this will apply `z-order: 0` to ensure that clips are
+/// applied correctly. Otherwise, the browser will refuse to clip its contents.
+///
+/// Other possible fixes that were rejected:
+///
+/// * Use 3D transform instead of 2D: this does not work because it causes text
+///   blurriness: https://github.com/flutter/flutter/issues/32274
+void applyWebkitClipFix(html.Element containerElement) {
+  if (browserEngine == BrowserEngine.webkit) {
+    containerElement.style.zIndex = '0';
+  }
+}

lib/web_ui/test/engine/semantics/semantics_test.dart

@@ -356,7 +356,7 @@ void _testContainer() {
     final html.Element container =
         html.document.querySelector('flt-semantics-container');
 
-    expect(parentElement.style.transform, 'translate(10px, 10px)');
+    expect(parentElement.style.transform, 'matrix(1, 0, 0, 1, 10, 10)');
     expect(parentElement.style.transformOrigin, '0px 0px 0px');
     expect(container.style.transform, 'translate(-10px, -10px)');
     expect(container.style.transformOrigin, '0px 0px 0px');

lib/web_ui/test/engine/util_test.dart

@@ -12,23 +12,27 @@ final Float64List identityTransform = Matrix4.identity().storage;
 final Float64List xTranslation = (Matrix4.identity()..translate(10)).storage;
 final Float64List yTranslation = (Matrix4.identity()..translate(0, 10)).storage;
 final Float64List zTranslation = (Matrix4.identity()..translate(0, 0, 10)).storage;
-final Float64List scaleAndTransform2d = (Matrix4.identity()..scale(2, 3, 1)..translate(4, 5, 0)).storage;
+final Float64List scaleAndTranslate2d = (Matrix4.identity()..scale(2, 3, 1)..translate(4, 5, 0)).storage;
+final Float64List rotation2d = (Matrix4.identity()..rotateZ(0.2)).storage;
 
 void main() {
   test('transformKindOf and isIdentityFloat64ListTransform identify matrix kind', () {
     expect(transformKindOf(identityTransform), TransformKind.identity);
     expect(isIdentityFloat64ListTransform(identityTransform), isTrue);
 
-    expect(transformKindOf(xTranslation), TransformKind.translation2d);
+    expect(transformKindOf(zTranslation), TransformKind.complex);
+    expect(isIdentityFloat64ListTransform(zTranslation), isFalse);
+
+    expect(transformKindOf(xTranslation), TransformKind.transform2d);
     expect(isIdentityFloat64ListTransform(xTranslation), isFalse);
 
-    expect(transformKindOf(yTranslation), TransformKind.translation2d);
+    expect(transformKindOf(yTranslation), TransformKind.transform2d);
     expect(isIdentityFloat64ListTransform(yTranslation), isFalse);
 
-    expect(transformKindOf(zTranslation), TransformKind.complex);
-    expect(isIdentityFloat64ListTransform(zTranslation), isFalse);
+    expect(transformKindOf(scaleAndTranslate2d), TransformKind.transform2d);
+    expect(isIdentityFloat64ListTransform(scaleAndTranslate2d), isFalse);
 
-    expect(transformKindOf(scaleAndTransform2d), TransformKind.scaleAndTranslate2d);
-    expect(isIdentityFloat64ListTransform(scaleAndTransform2d), isFalse);
+    expect(transformKindOf(rotation2d), TransformKind.transform2d);
+    expect(isIdentityFloat64ListTransform(rotation2d), isFalse);
   });
 }

lib/web_ui/test/golden_tests/engine/canvas_golden_test.dart

@@ -3,6 +3,7 @@
 // found in the LICENSE file.
 
 import 'dart:html' as html;
+import 'dart:math' as math;
 
 import 'package:ui/src/engine.dart';
 import 'package:ui/ui.dart';
@@ -170,4 +171,73 @@ void main() async {
       pixelComparison: PixelComparison.precise,
     );
   }, timeout: const Timeout(Duration(seconds: 10)), testOn: 'chrome');
+
+  // NOTE: Chrome in --headless mode does not reproduce the bug that this test
+  //       attempts to reproduce. However, it's still good to have this test
+  //       for potential future regressions related to paint order.
+  test('draws text on top of canvas when transformed and clipped', () async {
+    final ParagraphBuilder builder = ParagraphBuilder(ParagraphStyle(
+      fontFamily: 'Ahem',
+      fontSize: 18,
+    ));
+
+    const String text = 'This text is intentionally very long to make sure that it '
+      'breaks into multiple lines.';
+    builder.addText(text);
+
+    final Paragraph paragraph = builder.build();
+    paragraph.layout(const ParagraphConstraints(width: 100));
+
+    final Rect canvasSize = Offset.zero & Size(500, 500);
+
+    canvas = BitmapCanvas(canvasSize);
+    canvas.debugChildOverdraw = true;
+
+    final SurfacePaintData pathPaint = SurfacePaintData()
+      ..color = const Color(0xFF7F7F7F)
+      ..style = PaintingStyle.fill;
+
+    const double r = 200.0;
+    const double l = 50.0;
+
+    final Path path = (Path()
+      ..moveTo(-l, -l)
+      ..lineTo(0, -r)
+      ..lineTo(l, -l)
+      ..lineTo(r, 0)
+      ..lineTo(l, l)
+      ..lineTo(0, r)
+      ..lineTo(-l, l)
+      ..lineTo(-r, 0)
+      ..close()).shift(const Offset(250, 250));
+
+    canvas.drawPath(path, pathPaint);
+    canvas.drawParagraph(paragraph, const Offset(180, 50));
+
+    expect(
+      canvas.rootElement.querySelectorAll('p').map<String>((e) => e.innerText).toList(),
+      <String>[text],
+      reason: 'Expected to render text using HTML',
+    );
+
+    final SceneBuilder sb = SceneBuilder();
+    sb.pushTransform(Matrix4.rotationZ(math.pi / 2).storage);
+    sb.pushOffset(0, -500);
+    sb.pushClipRect(canvasSize);
+    sb.pop();
+    sb.pop();
+    sb.pop();
+    final SurfaceScene scene = sb.build();
+    final html.Element sceneElement = scene.webOnlyRootElement;
+
+    sceneElement.querySelector('flt-clip').append(canvas.rootElement);
+    html.document.querySelector('flt-scene-host').append(sceneElement);
+
+    await matchGoldenFile(
+      'bitmap_canvas_draws_text_on_top_of_canvas.png',
+      region: canvasSize,
+      maxDiffRatePercent: 0.0,
+      pixelComparison: PixelComparison.precise,
+    );
+  });
 }

lib/web_ui/test/text_editing_test.dart

@@ -356,7 +356,7 @@ void main() {
       ));
 
       // setEditableSizeAndTransform calls placeElement, so expecting geometry to be applied.
-      expect(editingElement.domElement.style.transform, 'translate(14px, 15px)');
+      expect(editingElement.domElement.style.transform, 'matrix(1, 0, 0, 1, 14, 15)');
       expect(editingElement.domElement.style.width, '13px');
       expect(editingElement.domElement.style.height, '12px');
     });