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

[web] Fix some clip and stroke calculations #36801

Merged
merged 7 commits into from
Oct 18, 2022

Conversation

mdebbar
Copy link
Contributor

@mdebbar mdebbar commented Oct 17, 2022

When an element is drawn with a stroke paint, we correctly bake stroke width into the transform calculations. But if the the element is clipped, the transform is recalculated without the stroke width math.

This PR makes sure that the stroke width adjustments are carried over to the clip calculations.

Fixes flutter/flutter#76879

I added new screenshot tests, here's what they look like before and after the fix:

Before After
Rect image image
RRect image image
Circle image image

@flutter-dashboard flutter-dashboard bot added the platform-web Code specifically for the web engine label Oct 17, 2022
@skia-gold
Copy link

Gold has detected about 1 new digest(s) on patchset 2.
View them at https://flutter-engine-gold.skia.org/cl/github/36801

double height = rect.height.abs();

final bool isStroke = paint.style == ui.PaintingStyle.stroke;
final double strokeWidth = paint.strokeWidth ?? 0.0;
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we remove both of the if blocks below, if we did the following ahead of time?

if (paint.style == ui.PaintingStyle.fill || strokeWidth == 0.0) {
  return rect;
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unfortunately, we can't.

Stroke-related adjustments aren't the only ones needed. We need to also adjust left and top to avoid flipped rectangles.

E.g. Rect.fromLTRB(200, 200, 100, 100) is completely fine in Flutter, but the DOM doesn't like it. If you do rect.width on a flipped rectangle, you get a negative value. Setting a negative width/height on a DOM element is equivalent to zero.

buildDrawRectElement and various draw operations used to handle it. See this:

final double left = math.min(rect.left, rect.right);
final double right = math.max(rect.left, rect.right);
final double top = math.min(rect.top, rect.bottom);
final double bottom = math.max(rect.top, rect.bottom);

and:
_drawElement(
element,
ui.Offset(
math.min(rect.left, rect.right), math.min(rect.top, rect.bottom)),
paint);

All of that can go away now because rects produced by the new adjustRectForDom are never flipped.

@skia-gold
Copy link

Gold has detected about 2 new digest(s) on patchset 2.
View them at https://flutter-engine-gold.skia.org/cl/github/36801

@skia-gold
Copy link

Gold has detected about 3 new digest(s) on patchset 3.
View them at https://flutter-engine-gold.skia.org/cl/github/36801

Copy link
Contributor

@harryterkelsen harryterkelsen left a comment

Choose a reason for hiding this comment

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

LGTM

@skia-gold
Copy link

Gold has detected about 3 new digest(s) on patchset 5.
View them at https://flutter-engine-gold.skia.org/cl/github/36801

@mdebbar mdebbar added the autosubmit Merge PR when tree becomes green via auto submit App label Oct 18, 2022
@skia-gold
Copy link

Gold has detected about 4 new digest(s) on patchset 7.
View them at https://flutter-engine-gold.skia.org/cl/github/36801

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
autosubmit Merge PR when tree becomes green via auto submit App platform-web Code specifically for the web engine
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[web: html] radio buttons look a little off
4 participants