Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

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

Merged
ferhatb merged 9 commits into from
Oct 20, 2020
Merged

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

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
a56bcf1
Fix image gap due to svg element without position attribute
ferhatb Oct 17, 2020
d8a5139
remove screenshot write
ferhatb Oct 17, 2020
bf7b8f4
Merge remote-tracking branch 'upstream/master' into filter_gap
ferhatb Oct 19, 2020
00752be
update golden lock
ferhatb Oct 19, 2020
ad7f334
address reviewer comments on style
ferhatb Oct 19, 2020
9f7e9fe
Merge remote-tracking branch 'upstream/master' into filter_gap
ferhatb Oct 19, 2020
8ee91f5
remove unnecessary null comparisons
ferhatb Oct 19, 2020
5da6afb
Merge remote-tracking branch 'upstream/master' into filter_gap
ferhatb Oct 19, 2020
f0dbaf6
Merge remote-tracking branch 'upstream/master' into filter_gap
ferhatb Oct 20, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 9 additions & 7 deletions lib/web_ui/lib/src/engine/html/color_filter.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
Expand All @@ -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.
Expand All @@ -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">'
Expand All @@ -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">'
Expand All @@ -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">'
Expand All @@ -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.
Expand All @@ -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">'
Expand Down
90 changes: 56 additions & 34 deletions lib/web_ui/lib/src/engine/util.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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;
Expand All @@ -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 &&
Expand Down Expand Up @@ -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]})';
}

Expand Down Expand Up @@ -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
Expand All @@ -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
Expand All @@ -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';
Expand Down Expand Up @@ -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.
Expand All @@ -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;
}
}
Expand Down Expand Up @@ -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) if (!_fontChangeScheduled) {
Copy link
Contributor

@nturgut nturgut Oct 19, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

very nit: curly braces around this if statement

Copy link
Contributor Author

@ferhatb ferhatb Oct 19, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

_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.
Expand Down Expand Up @@ -529,14 +549,16 @@ bool isUnsoundNull(dynamic object) {
}

bool _offsetIsValid(ui.Offset offset) {
assert(offset != null, 'Offset argument was null.'); // ignore: unnecessary_null_comparison
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 != null,
'Matrix4 argument was null.'); // ignore: unnecessary_null_comparison
assert(matrix4.length == 16, 'Matrix4 must have 16 entries.');
return true;
}
Expand Down
106 changes: 85 additions & 21 deletions lib/web_ui/test/golden_tests/engine/color_filter_golden_test.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down Expand Up @@ -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));
Image testImage = createTestImage();
Copy link
Contributor

@nturgut nturgut Oct 19, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: final

Copy link
Contributor Author

@ferhatb ferhatb Oct 19, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

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);
}