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

Soften shadows #17231

Merged
yjbanov merged 2 commits into from
Mar 21, 2020
Merged

Soften shadows #17231

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
4386745
Soften shadows
yjbanov Mar 20, 2020
1344049
update goldens
yjbanov Mar 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
2 changes: 1 addition & 1 deletion lib/web_ui/dev/goldens_lock.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,2 +1,2 @@
repository: https://github.com/flutter/goldens.git
revision: 8f692819e8881b7d2131dbd61d965c21d5e3e345
revision: ae6003206eb721137c20cd56d8d1d8e2a76d6dd1
7 changes: 6 additions & 1 deletion lib/web_ui/lib/src/engine/canvas_pool.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -613,7 +613,12 @@ class _CanvasPool extends _SaveStackTracking {
context.save();
context.filter = 'none';
context.strokeStyle = '';
context.fillStyle = colorToCssString(color);
final int red = color.red;
final int green = color.green;
final int blue = color.blue;
// Multiply by 0.4 to make shadows less aggressive (https://github.com/flutter/flutter/issues/52734)
final int alpha = (0.4 * color.alpha).round();
context.fillStyle = colorComponentsToCssString(red, green, blue, alpha);
context.shadowBlur = shadow.blurWidth;
context.shadowColor = colorToCssString(color.withAlpha(0xff));
Copy link
Member

@ditman ditman Mar 20, 2020

Choose a reason for hiding this comment

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

Why is this overriding the alpha channel of the shadow color to 0xff?

Can we do this instead?

context.shadowColor = colorToCssString(color);

(Assuming that color has the right alpha value set?)

Copy link
Contributor Author

@yjbanov yjbanov Mar 21, 2020

Choose a reason for hiding this comment

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

@hterkelsen, do you recall why this was necessary? FWIW, the shadows look OK to me: https://github.com/flutter/goldens/blob/ae6003206eb721137c20cd56d8d1d8e2a76d6dd1/engine/web/shadows.png

context.shadowOffsetX = shadow.offset.dx;
Expand Down
4 changes: 3 additions & 1 deletion lib/web_ui/lib/src/engine/shadow.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -133,7 +133,9 @@ void applyCssShadow(
if (shadow == null) {
element.style.boxShadow = 'none';
} else {
// Multiply by 0.4 to make shadows less aggressive (https://github.com/flutter/flutter/issues/52734)
final double alpha = 0.4 * color.alpha / 255;
element.style.boxShadow = '${shadow.offset.dx}px ${shadow.offset.dy}px '
'${shadow.blurWidth}px 0px rgb(${color.red}, ${color.green}, ${color.blue})';
'${shadow.blurWidth}px 0px rgba(${color.red}, ${color.green}, ${color.blue}, $alpha)';
}
}
10 changes: 10 additions & 0 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 @@ -349,6 +349,16 @@ String _colorToCssStringRgbOnly(ui.Color color) {
return '#${paddedValue.substring(paddedValue.length - 6)}';
}

/// Converts color components to a CSS compatible attribute value.
String colorComponentsToCssString(int r, int g, int b, int a) {
if (a == 255) {
return 'rgb($r,$g,$b)';
} else {
final double alphaRatio = a / 255;
return 'rgba($r,$g,$b,${alphaRatio.toStringAsFixed(2)})';
}
}

/// Determines if the (dynamic) exception passed in is a NS_ERROR_FAILURE
/// (from Firefox).
///
Expand Down
2 changes: 1 addition & 1 deletion lib/web_ui/test/golden_tests/engine/shadow_golden_test.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ import 'package:web_engine_tester/golden_tester.dart';

import 'scuba.dart';

const Color _kShadowColor = Color.fromARGB(255, 255, 0, 0);
Copy link
Contributor

@ferhatb ferhatb Mar 20, 2020

Choose a reason for hiding this comment

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

why the change in test?

Copy link
Contributor Author

@yjbanov yjbanov Mar 20, 2020

Choose a reason for hiding this comment

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

Making the shadows softer made the red color too subtle in the golden test, so I decided to switch the test to pitch black. Also, black shadows are the most commonly used ones.

const Color _kShadowColor = Color.fromARGB(255, 0, 0, 0);

void main() async {
final Rect region = Rect.fromLTWH(0, 0, 550, 300);
Expand Down