Support closest-corner/farthest-corner in clip-path circle()/ellipse() [chromium/src : main]
Hyowon Kim (Gerrit)
New activity on the change![]( "Open in Gerrit")
Related details
- Daniil Sakhapov
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Daniil Sakhapov (Gerrit)
Daniil Sakhapov added 2 comments![]( "Open in Gerrit")
I think we need a feature flag for this one
float CornerDistance(const gfx::PointF& center,
const gfx::SizeF& box_size,
bool closest) {
const gfx::RectF rect(box_size);
const std::array<gfx::PointF, 4> corners = {
rect.origin(), rect.top_right(), rect.bottom_right(), rect.bottom_left()};
float distance = (center - corners[0]).Length();
for (unsigned i = 1; i < std::size(corners); ++i) {
float new_distance = (center - corners[i]).Length();
if (closest ? new_distance < distance : new_distance > distance) {
distance = new_distance;
}
}
return distance;
}looking at - https://drafts.csswg.org/css-images-3/#valdef-radial-extent-closest-corner - it seems to be wrong for the circle()/ellipse() case?
Related details
- Hyowon Kim
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Hyowon Kim (Gerrit)
Hyowon Kim added 1 comment![]( "Open in Gerrit")
float CornerDistance(const gfx::PointF& center,
const gfx::SizeF& box_size,
bool closest) {
const gfx::RectF rect(box_size);
const std::array<gfx::PointF, 4> corners = {
rect.origin(), rect.top_right(), rect.bottom_right(), rect.bottom_left()};
float distance = (center - corners[0]).Length();
for (unsigned i = 1; i < std::size(corners); ++i) {
float new_distance = (center - corners[i]).Length();
if (closest ? new_distance < distance : new_distance > distance) {
distance = new_distance;
}
}
return distance;
}looking at - https://drafts.csswg.org/css-images-3/#valdef-radial-extent-closest-corner - it seems to be wrong for the circle()/ellipse() case?
Good point. The spec says ellipse closest-corner/farthest-corner should
preserve the aspect ratio from closest-side/farthest-side.
But Blink's parser for ellipse() accepts <shape-radius>{2} (two independent radius values), not a single <radial-size>. The WPT test also uses this form: [ellipse(farthest-corner closest-corner at ...)](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/web_tests/external/wpt/css/css-masking/clip-path/clip-path-ellipse-closest-farthest-corner.html;l=19).
With two independent radii, each resolves to the euclidean corner distance, and the WPT reference (rx=247.487, ry=125) matches this.
On wpt.fyi, these three tests are red for Chrome, Edge, and Firefox, and green only for Safari.
What do you think?
Related details
- Daniil Sakhapov
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Daniil Sakhapov (Gerrit)
Daniil Sakhapov added 1 comment![]( "Open in Gerrit")
float CornerDistance(const gfx::PointF& center,
const gfx::SizeF& box_size,
bool closest) {
const gfx::RectF rect(box_size);
const std::array<gfx::PointF, 4> corners = {
rect.origin(), rect.top_right(), rect.bottom_right(), rect.bottom_left()};
float distance = (center - corners[0]).Length();
for (unsigned i = 1; i < std::size(corners); ++i) {
float new_distance = (center - corners[i]).Length();
if (closest ? new_distance < distance : new_distance > distance) {
distance = new_distance;
}
}
return distance;
}Hyowon Kimlooking at - https://drafts.csswg.org/css-images-3/#valdef-radial-extent-closest-corner - it seems to be wrong for the circle()/ellipse() case?
Good point. The spec says ellipse closest-corner/farthest-corner should
preserve the aspect ratio from closest-side/farthest-side.But Blink's parser for ellipse() accepts <shape-radius>{2} (two independent radius values), not a single <radial-size>. The WPT test also uses this form: [ellipse(farthest-corner closest-corner at ...)](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/web_tests/external/wpt/css/css-masking/clip-path/clip-path-ellipse-closest-farthest-corner.html;l=19).
With two independent radii, each resolves to the euclidean corner distance, and the WPT reference (rx=247.487, ry=125) matches this.On wpt.fyi, these three tests are red for Chrome, Edge, and Firefox, and green only for Safari.
What do you think?
Hm, I can't find a spec saying that we should accept two <shape-radius> though...
Maybe there is a bug in current spec and it should be `[ <radial-extent> | <length [0,∞]> | <length-percentage [0,∞]> ]{2}` instead? (added `[` and `]`).
fs@, do you know anything about it?
Related details
- Hyowon Kim
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Fredrik Söderquist (Gerrit)
Fredrik Söderquist added 3 comments![]( "Open in Gerrit")
float CornerDistance(const gfx::PointF& center,Yay! Another copy of this code. Perhaps consider naming it similar to the one I assuem you copied this from (`RadiusToCorner` in `css_gradient_value.cc`) to make it a bit more discoverable in case someone decides to consolidate?
Yes, this ought to behave like radial gradients (which is where `<radial-size>` is defined, c.f `ConsumeRadialGradient`) - i.e accept only one `<radial-extent>` but one or two `<length-percentage>` - but it looks like this has been inconsistently implemented everywhere (always fun!). Should probably raise an issue for this, because the spec is out of sync with implementations (and one or the other would need to change).
auto ResolveRadius = [&](const BasicShapeRadius& radius, float center_coord,
float box_dimension) -> float {
if (radius.GetType() == BasicShapeRadius::kClosestCorner ||
radius.GetType() == BasicShapeRadius::kFarthestCorner) {
return CornerDistance(
center, bounding_box.size(),
radius.GetType() == BasicShapeRadius::kClosestCorner);
}
return FloatValueForRadiusInBox(radius, center_coord, box_dimension);
};This probably ought to go in `FloatValueForRadiusInBox()`, because that function is also called by the `shape-outside` code. (Maybe the result can be shared between circle and ellipse by moving it to the shared base class?)
Hyowon Kim (Gerrit)
Hyowon Kim added 1 comment![]( "Open in Gerrit")
float CornerDistance(const gfx::PointF& center,I filed a spec issue for this: https://github.com/w3c/csswg-drafts/issues/13814
It would be great if you could comment on the issue about which direction it should go.
I'll keep this CL as-is for now until the spec discussion is resolved.
Related details
- Daniil Sakhapov
- Fredrik Söderquist
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Fredrik Söderquist (Gerrit)
Fredrik Söderquist added 1 comment![]( "Open in Gerrit")
So, it's been a known issue for 1.5y... Maybe someone has the power to get on an agenda? Using our current "interpretation" seems fine (changing it would come with risks).
Related details
- Daniil Sakhapov
- Hyowon Kim
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Fredrik Söderquist (Gerrit)
Fredrik Söderquist added 1 comment![]( "Open in Gerrit")
You should probably have assigned the bug to you (it looks like someone else assigned it now).