[web] Workaround for compositing bug in chrome/webkit (#18128)

* Fix compositing error when multiple canvas(s) are reused
* address review comments
* track all element counts

Author: ferhatb

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

@@ -10,7 +10,7 @@ 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 [closeCurrentCanvas]
-/// and adds the canvas(s) to a [_pool] of active canvas(s).
+/// and adds the canvas(s) to [_activeCanvasList].
 ///
 /// To make sure transformations and clips are preserved correctly when a new
 /// canvas is allocated, [_CanvasPool] replays the current stack on the newly
@@ -25,14 +25,16 @@ class _CanvasPool extends _SaveStackTracking {
   ContextStateHandle _contextHandle;
   final int _widthInBitmapPixels, _heightInBitmapPixels;
   // List of canvases that have been allocated and used in this paint cycle.
-  List<html.CanvasElement> _pool;
+  List<html.CanvasElement> _activeCanvasList;
   // List of canvases available to reuse from prior paint cycle.
   List<html.CanvasElement> _reusablePool;
   // Current canvas element or null if marked for lazy allocation.
   html.CanvasElement _canvas;
 
   html.HtmlElement _rootElement;
   int _saveContextCount = 0;
+  // Number of elements that have been added to flt-canvas.
+  int _activeElementCount = 0;
 
   _CanvasPool(this._widthInBitmapPixels, this._heightInBitmapPixels);
 
@@ -66,12 +68,13 @@ class _CanvasPool extends _SaveStackTracking {
     if (_canvas != null) {
       _restoreContextSave();
       _contextHandle.reset();
-      _pool ??= [];
-      _pool.add(_canvas);
+      _activeCanvasList ??= [];
+      _activeCanvasList.add(_canvas);
       _canvas = null;
       _context = null;
       _contextHandle = null;
     }
+    _activeElementCount++;
   }
 
   void allocateCanvas(html.HtmlElement rootElement) {
@@ -83,8 +86,8 @@ class _CanvasPool extends _SaveStackTracking {
     bool reused = false;
     if (_reusablePool != null && _reusablePool.isNotEmpty) {
       _canvas = _reusablePool.removeAt(0);
-      reused = true;
       requiresClearRect = true;
+      reused = true;
     } else {
       // Compute the final CSS canvas size given the actual pixel count we
       // allocated. This is done for the following reasons:
@@ -120,24 +123,16 @@ class _CanvasPool extends _SaveStackTracking {
     if (_rootElement.lastChild != _canvas) {
       _rootElement.append(_canvas);
     }
-    // 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
-    if (_rootElement.firstChild == _canvas) {
+
+    if (_activeElementCount == 0) {
       _canvas.style.zIndex = '-1';
     } else if (reused) {
       // If a canvas is the first element we set z-index = -1 to workaround
       // blink compositing bug. To make sure this does not leak when reused
       // reset z-index.
       _canvas.style.removeProperty('z-index');
     }
+    ++_activeElementCount;
 
     _context = _canvas.context2D;
     _contextHandle = ContextStateHandle(_context);
@@ -250,16 +245,17 @@ class _CanvasPool extends _SaveStackTracking {
     if (_canvas != null) {
       _restoreContextSave();
       _contextHandle.reset();
-      _pool ??= [];
-      _pool.add(_canvas);
+      _activeCanvasList ??= [];
+      _activeCanvasList.add(_canvas);
       _context = null;
       _contextHandle = null;
     }
-    _reusablePool = _pool;
-    _pool = null;
+    _reusablePool = _activeCanvasList;
+    _activeCanvasList = null;
     _canvas = null;
     _context = null;
     _contextHandle = null;
+    _activeElementCount = 0;
   }
 
   void endOfPaint() {
@@ -666,19 +662,19 @@ class _CanvasPool extends _SaveStackTracking {
     if (browserEngine == BrowserEngine.webkit && _canvas != null) {
       _canvas.width = _canvas.height = 0;
     }
-    _clearPool();
+    _clearActiveCanvasList();
   }
 
-  void _clearPool() {
-    if (_pool != null) {
-      for (html.CanvasElement c in _pool) {
+  void _clearActiveCanvasList() {
+    if (_activeCanvasList != null) {
+      for (html.CanvasElement c in _activeCanvasList) {
         if (browserEngine == BrowserEngine.webkit) {
           c.width = c.height = 0;
         }
         c.remove();
       }
     }
-    _pool = null;
+    _activeCanvasList = null;
   }
 }
 

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

@@ -28,7 +28,9 @@ mixin _DomClip on PersistedContainerSurface {
     if (!debugShowClipLayers) {
       // Hide overflow in production mode. When debugging we want to see the
       // clipped picture in full.
-      element.style.overflow = 'hidden';
+      element.style
+        ..overflow = 'hidden'
+        ..zIndex = '0';
     } else {
       // Display the outline of the clipping region. When debugShowClipLayers is
       // `true` we don't hide clip overflow (see above). This outline helps
@@ -41,6 +43,7 @@ mixin _DomClip on PersistedContainerSurface {
       _surfaceStatsFor(this).allocatedDomNodeCount++;
     }
     _childContainer.style.position = 'absolute';
+
     element.append(_childContainer);
     return element;
   }

lib/web_ui/test/compositing_test.dart

@@ -4,7 +4,7 @@
 
 // @dart = 2.6
 import 'dart:html' as html;
-
+import 'dart:js_util' as js_util;
 import 'package:ui/src/engine.dart';
 import 'package:ui/ui.dart';
 
@@ -183,7 +183,7 @@ void main() {
     //
     // When BitmapCanvas uses multiple elements to paint, the very first
     // canvas needs to have a -1 zIndex so it can preserve compositing order.
-    test('First canvas element should retain -1 zIndex after update', () async {
+    test('Canvas element should retain -1 zIndex after update', () async {
       final SurfaceSceneBuilder builder = SurfaceSceneBuilder();
       final Picture picture1 = _drawPicture();
       EngineLayer oldLayer = builder.pushClipRect(
@@ -208,6 +208,35 @@ void main() {
       html.HtmlElement contentAfterReuse = builder2.build().webOnlyRootElement;
       expect(contentAfterReuse.querySelector('canvas').style.zIndex, '-1');
     });
+
+    test('Multiple canvas elements should retain zIndex after update', () async {
+      final SurfaceSceneBuilder builder = SurfaceSceneBuilder();
+      final Picture picture1 = _drawPathImagePath();
+      EngineLayer oldLayer = builder.pushClipRect(
+        const Rect.fromLTRB(10, 10, 300, 300),
+      );
+      builder.addPicture(Offset.zero, picture1);
+      builder.pop();
+
+      html.HtmlElement content = builder.build().webOnlyRootElement;
+      expect(content.querySelector('canvas').style.zIndex, '-1');
+
+      // Force update to scene which will utilize reuse code path.
+      final SurfaceSceneBuilder builder2 = SurfaceSceneBuilder();
+      builder2.pushClipRect(
+          const Rect.fromLTRB(5, 10, 300, 300),
+          oldLayer: oldLayer
+      );
+      final Picture picture2 = _drawPathImagePath();
+      builder2.addPicture(Offset.zero, picture2);
+      builder2.pop();
+
+      html.HtmlElement contentAfterReuse = builder2.build().webOnlyRootElement;
+      List<html.CanvasElement> list =
+          contentAfterReuse.querySelectorAll('canvas');
+      expect(list[0].style.zIndex, '-1');
+      expect(list[1].style.zIndex, '');
+    });
   });
 
 }
@@ -374,3 +403,51 @@ Picture _drawPicture() {
         ..color = const Color.fromRGBO(0, 0, 255, 1));
   return recorder.endRecording();
 }
+
+Picture _drawPathImagePath() {
+  const double offsetX = 50;
+  const double offsetY = 50;
+  final EnginePictureRecorder recorder = PictureRecorder();
+  final RecordingCanvas canvas =
+  recorder.beginRecording(const Rect.fromLTRB(0, 0, 400, 400));
+  canvas.drawCircle(
+      Offset(offsetX + 10, offsetY + 10), 10, Paint()..style = PaintingStyle.fill);
+  canvas.drawCircle(
+      Offset(offsetX + 60, offsetY + 10),
+      10,
+      Paint()
+        ..style = PaintingStyle.fill
+        ..color = const Color.fromRGBO(255, 0, 0, 1));
+  canvas.drawCircle(
+      Offset(offsetX + 10, offsetY + 60),
+      10,
+      Paint()
+        ..style = PaintingStyle.fill
+        ..color = const Color.fromRGBO(0, 255, 0, 1));
+  canvas.drawImage(createTestImage(), Offset(0, 0), Paint());
+  canvas.drawCircle(
+      Offset(offsetX + 60, offsetY + 60),
+      10,
+      Paint()
+        ..style = PaintingStyle.fill
+        ..color = const Color.fromRGBO(0, 0, 255, 1));
+  return recorder.endRecording();
+}
+
+HtmlImage createTestImage({int width = 100, int height = 50}) {
+  html.CanvasElement canvas =
+  new html.CanvasElement(width: width, height: height);
+  html.CanvasRenderingContext2D ctx = canvas.context2D;
+  ctx.fillStyle = '#E04040';
+  ctx.fillRect(0, 0, 33, 50);
+  ctx.fill();
+  ctx.fillStyle = '#40E080';
+  ctx.fillRect(33, 0, 33, 50);
+  ctx.fill();
+  ctx.fillStyle = '#2040E0';
+  ctx.fillRect(66, 0, 33, 50);
+  ctx.fill();
+  html.ImageElement imageElement = html.ImageElement();
+  imageElement.src = js_util.callMethod(canvas, 'toDataURL', <dynamic>[]);
+  return HtmlImage(imageElement, width, height);
+}