Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[css-shapes-2] Order of points and control points in shape() #10666

Closed
smfr opened this issue Aug 1, 2024 · 9 comments · Fixed by #11207 or #11464
Closed

[css-shapes-2] Order of points and control points in shape() #10666

smfr opened this issue Aug 1, 2024 · 9 comments · Fixed by #11207 or #11464

Comments

@smfr
Copy link
Contributor

smfr commented Aug 1, 2024

In #5841 we resolved that the points and the control points could be specified in any order, e.g. these are equivalent:

curve using 10px 30px by 20px 20px
curve by 20px 20px using 10px 30px

I'm having second thoughts. The by or to affect the interpretation of the control point coordinate pairs; they are either absolute (relative to the reference box), or relative to (the current point). So it's odd to specify the control points before you've given the by or to.

I think we should revert this part of the change.

@smfr
Copy link
Contributor Author

smfr commented Aug 1, 2024

@noamr

smfr added a commit to smfr/WebKit that referenced this issue Aug 2, 2024
https://bugs.webkit.org/show_bug.cgi?id=277347
rdar://132814728

Reviewed by NOBODY (OOPS!).

Implement parsing, property value and computed style support for the `shape()` function[1]. We support the syntax
as of the most recent edit[2] but without the flexible order of points and control points due to [3].

Added BasicShapesShapeSegmentConversion.h/cpp with helpers to convert between CSS values and the internal shapes.
CSSShapeValue::customCSSText() is implemented.

BasicShapeShape stores its segments as a vector of std::variant<>, much as we do for Paths, which avoids heap allocations
per segment.

Tentative WPT are included.

[1] https://drafts.csswg.org/css-shapes-2/#shape-function
[2] w3c/csswg-drafts@9770805c4e53
[3] w3c/csswg-drafts#10666

* LayoutTests/imported/w3c/web-platform-tests/css/css-shapes/shape-functions/shape-function-computed.tentative.html: Added.
* LayoutTests/imported/w3c/web-platform-tests/css/css-shapes/shape-functions/shape-function-invalid.tentative.html: Added.
* LayoutTests/imported/w3c/web-platform-tests/css/css-shapes/shape-functions/shape-function-valid.tentative.html: Added.
* Source/WebCore/Sources.txt:
* Source/WebCore/WebCore.xcodeproj/project.pbxproj:
* Source/WebCore/css/BasicShapeConversion.cpp:
(WebCore::valueForBasicShape):
(WebCore::convertToLengthPoint):
(WebCore::basicShapeForValue):
(WebCore::basicShapeShapeForValue):
* Source/WebCore/css/BasicShapeConversion.h:
* Source/WebCore/css/BasicShapesShapeSegmentConversion.cpp: Added.
(WebCore::lengthToCSSValue):
(WebCore::lengthPointToCSSValue):
(WebCore::lengthSizeToCSSValue):
(WebCore::toCSSShapeSegmentValue):
(WebCore::fromCSSShapeSegmentValue):
* Source/WebCore/css/BasicShapesShapeSegmentConversion.h: Copied from Source/WebCore/css/BasicShapeConversion.h.
* Source/WebCore/css/CSSBasicShapes.cpp:
(WebCore::CSSShapeValue::CSSShapeValue):
(WebCore::CSSShapeValue::customCSSText const):
(WebCore::CSSShapeValue::equals const):
* Source/WebCore/css/CSSShapeSegmentValue.cpp:
(WebCore::CSSShapeSegmentValue::customCSSText const):
(WebCore::CSSShapeSegmentValue::toShapeSegment const):
* Source/WebCore/css/CSSShapeSegmentValue.h:
* Source/WebCore/css/CSSValueKeywords.in:
* Source/WebCore/css/parser/CSSPropertyParserHelpers.cpp:
(WebCore::CSSPropertyParserHelpers::consumeCoordinatePair):
(WebCore::CSSPropertyParserHelpers::consumeShapeCommand):
(WebCore::CSSPropertyParserHelpers::consumeBasicShapeShape):
(WebCore::CSSPropertyParserHelpers::consumeBasicShape):
* Source/WebCore/rendering/style/BasicShapes.cpp:
(WebCore::operator<<):
* Source/WebCore/rendering/style/BasicShapes.h:
* Source/WebCore/rendering/style/BasicShapesShape.cpp: Added.
(WebCore::BasicShapeShape::create):
(WebCore::BasicShapeShape::BasicShapeShape):
(WebCore::BasicShapeShape::clone const):
(WebCore::BasicShapeShape::path const):
(WebCore::BasicShapeShape::canBlend const):
(WebCore::BasicShapeShape::blend const):
(WebCore::BasicShapeShape::operator== const):
(WebCore::BasicShapeShape::dump const):
(WebCore::operator<<):
* Source/WebCore/rendering/style/BasicShapesShape.h: Added.
(WebCore::ShapeSegmentBase::ShapeSegmentBase):
(WebCore::ShapeSegmentBase::affinity const):
smfr added a commit to smfr/WebKit that referenced this issue Aug 2, 2024
https://bugs.webkit.org/show_bug.cgi?id=277347
rdar://132814728

Reviewed by NOBODY (OOPS!).

Implement parsing, property value and computed style support for the `shape()` function[1]. We support the syntax
as of the most recent edit[2] but without the flexible order of points and control points due to [3].

Added BasicShapesShapeSegmentConversion.h/cpp with helpers to convert between CSS values and the internal shapes.
CSSShapeValue::customCSSText() is implemented.

BasicShapeShape stores its segments as a vector of std::variant<>, much as we do for Paths, which avoids heap allocations
per segment.

Plumb Style::BuilderState deeper into BasicShapeConversion functions.

Tentative WPT are included.

Based on code by Noam Rosenthal.

[1] https://drafts.csswg.org/css-shapes-2/#shape-function
[2] w3c/csswg-drafts@9770805c4e53
[3] w3c/csswg-drafts#10666

* LayoutTests/imported/w3c/web-platform-tests/css/css-shapes/shape-functions/shape-function-computed.tentative.html: Added.
* LayoutTests/imported/w3c/web-platform-tests/css/css-shapes/shape-functions/shape-function-invalid.tentative.html: Added.
* LayoutTests/imported/w3c/web-platform-tests/css/css-shapes/shape-functions/shape-function-valid.tentative.html: Added.
* Source/WebCore/Sources.txt:
* Source/WebCore/WebCore.xcodeproj/project.pbxproj:
* Source/WebCore/css/BasicShapeConversion.cpp:
(WebCore::valueForBasicShape):
(WebCore::convertToLengthPoint):
(WebCore::basicShapeForValue):
(WebCore::basicShapeShapeForValue):
* Source/WebCore/css/BasicShapeConversion.h:
* Source/WebCore/css/BasicShapesShapeSegmentConversion.cpp: Added.
(WebCore::lengthToCSSValue):
(WebCore::lengthPointToCSSValue):
(WebCore::lengthSizeToCSSValue):
(WebCore::toCSSShapeSegmentValue):
(WebCore::fromCSSShapeSegmentValue):
* Source/WebCore/css/BasicShapesShapeSegmentConversion.h: Copied from Source/WebCore/css/BasicShapeConversion.h.
* Source/WebCore/css/CSSBasicShapes.cpp:
(WebCore::CSSShapeValue::CSSShapeValue):
(WebCore::CSSShapeValue::customCSSText const):
(WebCore::CSSShapeValue::equals const):
* Source/WebCore/css/CSSShapeSegmentValue.cpp:
(WebCore::CSSShapeSegmentValue::customCSSText const):
(WebCore::CSSShapeSegmentValue::toShapeSegment const):
* Source/WebCore/css/CSSShapeSegmentValue.h:
* Source/WebCore/css/CSSValueKeywords.in:
* Source/WebCore/css/parser/CSSPropertyParserHelpers.cpp:
(WebCore::CSSPropertyParserHelpers::consumeCoordinatePair):
(WebCore::CSSPropertyParserHelpers::consumeShapeCommand):
(WebCore::CSSPropertyParserHelpers::consumeBasicShapeShape):
(WebCore::CSSPropertyParserHelpers::consumeBasicShape):
* Source/WebCore/rendering/style/BasicShapes.cpp:
(WebCore::operator<<):
* Source/WebCore/rendering/style/BasicShapes.h:
* Source/WebCore/rendering/style/BasicShapesShape.cpp: Added.
(WebCore::BasicShapeShape::create):
(WebCore::BasicShapeShape::BasicShapeShape):
(WebCore::BasicShapeShape::clone const):
(WebCore::BasicShapeShape::path const):
(WebCore::BasicShapeShape::canBlend const):
(WebCore::BasicShapeShape::blend const):
(WebCore::BasicShapeShape::operator== const):
(WebCore::BasicShapeShape::dump const):
(WebCore::operator<<):
* Source/WebCore/rendering/style/BasicShapesShape.h: Added.
(WebCore::ShapeSegmentBase::ShapeSegmentBase):
(WebCore::ShapeSegmentBase::affinity const):
smfr added a commit to smfr/WebKit that referenced this issue Aug 2, 2024
https://bugs.webkit.org/show_bug.cgi?id=277347
rdar://132814728

Reviewed by NOBODY (OOPS!).

Implement parsing, property value and computed style support for the `shape()` function[1]. We support the syntax
as of the most recent edit[2] but without the flexible order of points and control points due to [3].

Added BasicShapesShapeSegmentConversion.h/cpp with helpers to convert between CSS values and the internal shapes.
CSSShapeValue::customCSSText() is implemented.

BasicShapeShape stores its segments as a vector of std::variant<>, much as we do for Paths, which avoids heap allocations
per segment.

Plumb Style::BuilderState deeper into BasicShapeConversion functions.

Tentative WPT are included.

Based on code by Noam Rosenthal.

[1] https://drafts.csswg.org/css-shapes-2/#shape-function
[2] w3c/csswg-drafts@9770805c4e53
[3] w3c/csswg-drafts#10666

* LayoutTests/imported/w3c/web-platform-tests/css/css-shapes/shape-functions/shape-function-computed.tentative.html: Added.
* LayoutTests/imported/w3c/web-platform-tests/css/css-shapes/shape-functions/shape-function-invalid.tentative.html: Added.
* LayoutTests/imported/w3c/web-platform-tests/css/css-shapes/shape-functions/shape-function-valid.tentative.html: Added.
* Source/WebCore/Sources.txt:
* Source/WebCore/WebCore.xcodeproj/project.pbxproj:
* Source/WebCore/css/BasicShapeConversion.cpp:
(WebCore::valueForBasicShape):
(WebCore::convertToLengthPoint):
(WebCore::basicShapeForValue):
(WebCore::basicShapeShapeForValue):
* Source/WebCore/css/BasicShapeConversion.h:
* Source/WebCore/css/BasicShapesShapeSegmentConversion.cpp: Added.
(WebCore::lengthToCSSValue):
(WebCore::lengthPointToCSSValue):
(WebCore::lengthSizeToCSSValue):
(WebCore::toCSSShapeSegmentValue):
(WebCore::fromCSSShapeSegmentValue):
* Source/WebCore/css/BasicShapesShapeSegmentConversion.h: Copied from Source/WebCore/css/BasicShapeConversion.h.
* Source/WebCore/css/CSSBasicShapes.cpp:
(WebCore::CSSShapeValue::CSSShapeValue):
(WebCore::CSSShapeValue::customCSSText const):
(WebCore::CSSShapeValue::equals const):
* Source/WebCore/css/CSSShapeSegmentValue.cpp:
(WebCore::CSSShapeSegmentValue::customCSSText const):
(WebCore::CSSShapeSegmentValue::toShapeSegment const):
* Source/WebCore/css/CSSShapeSegmentValue.h:
* Source/WebCore/css/CSSValueKeywords.in:
* Source/WebCore/css/parser/CSSPropertyParserHelpers.cpp:
(WebCore::CSSPropertyParserHelpers::consumeCoordinatePair):
(WebCore::CSSPropertyParserHelpers::consumeShapeCommand):
(WebCore::CSSPropertyParserHelpers::consumeBasicShapeShape):
(WebCore::CSSPropertyParserHelpers::consumeBasicShape):
* Source/WebCore/rendering/style/BasicShapes.cpp:
(WebCore::operator<<):
* Source/WebCore/rendering/style/BasicShapes.h:
* Source/WebCore/rendering/style/BasicShapesShape.cpp: Added.
(WebCore::BasicShapeShape::create):
(WebCore::BasicShapeShape::BasicShapeShape):
(WebCore::BasicShapeShape::clone const):
(WebCore::BasicShapeShape::path const):
(WebCore::BasicShapeShape::canBlend const):
(WebCore::BasicShapeShape::blend const):
(WebCore::BasicShapeShape::operator== const):
(WebCore::BasicShapeShape::dump const):
(WebCore::operator<<):
* Source/WebCore/rendering/style/BasicShapesShape.h: Added.
(WebCore::ShapeSegmentBase::ShapeSegmentBase):
(WebCore::ShapeSegmentBase::affinity const):
smfr added a commit to smfr/WebKit that referenced this issue Aug 2, 2024
https://bugs.webkit.org/show_bug.cgi?id=277347
rdar://132814728

Reviewed by NOBODY (OOPS!).

Implement parsing, property value and computed style support for the `shape()` function[1]. We support the syntax
as of the most recent edit[2] but without the flexible order of points and control points due to [3].

Added BasicShapesShapeSegmentConversion.h/cpp with helpers to convert between CSS values and the internal shapes.
CSSShapeValue::customCSSText() is implemented.

BasicShapeShape stores its segments as a vector of std::variant<>, much as we do for Paths, which avoids heap allocations
per segment.

Plumb Style::BuilderState deeper into BasicShapeConversion functions.

Tentative WPT are included.

Based on code by Noam Rosenthal.

[1] https://drafts.csswg.org/css-shapes-2/#shape-function
[2] w3c/csswg-drafts@9770805c4e53
[3] w3c/csswg-drafts#10666

* LayoutTests/imported/w3c/web-platform-tests/css/css-shapes/shape-functions/shape-function-computed.tentative.html: Added.
* LayoutTests/imported/w3c/web-platform-tests/css/css-shapes/shape-functions/shape-function-invalid.tentative.html: Added.
* LayoutTests/imported/w3c/web-platform-tests/css/css-shapes/shape-functions/shape-function-valid.tentative.html: Added.
* Source/WebCore/Sources.txt:
* Source/WebCore/WebCore.xcodeproj/project.pbxproj:
* Source/WebCore/css/BasicShapeConversion.cpp:
(WebCore::valueForBasicShape):
(WebCore::convertToLengthPoint):
(WebCore::basicShapeForValue):
(WebCore::basicShapeShapeForValue):
* Source/WebCore/css/BasicShapeConversion.h:
* Source/WebCore/css/BasicShapesShapeSegmentConversion.cpp: Added.
(WebCore::lengthToCSSValue):
(WebCore::lengthPointToCSSValue):
(WebCore::lengthSizeToCSSValue):
(WebCore::toCSSShapeSegmentValue):
(WebCore::fromCSSShapeSegmentValue):
* Source/WebCore/css/BasicShapesShapeSegmentConversion.h: Copied from Source/WebCore/css/BasicShapeConversion.h.
* Source/WebCore/css/CSSBasicShapes.cpp:
(WebCore::CSSShapeValue::CSSShapeValue):
(WebCore::CSSShapeValue::customCSSText const):
(WebCore::CSSShapeValue::equals const):
* Source/WebCore/css/CSSShapeSegmentValue.cpp:
(WebCore::CSSShapeSegmentValue::customCSSText const):
(WebCore::CSSShapeSegmentValue::toShapeSegment const):
* Source/WebCore/css/CSSShapeSegmentValue.h:
* Source/WebCore/css/CSSValueKeywords.in:
* Source/WebCore/css/parser/CSSPropertyParserHelpers.cpp:
(WebCore::CSSPropertyParserHelpers::consumeCoordinatePair):
(WebCore::CSSPropertyParserHelpers::consumeShapeCommand):
(WebCore::CSSPropertyParserHelpers::consumeBasicShapeShape):
(WebCore::CSSPropertyParserHelpers::consumeBasicShape):
* Source/WebCore/rendering/style/BasicShapes.cpp:
(WebCore::operator<<):
* Source/WebCore/rendering/style/BasicShapes.h:
* Source/WebCore/rendering/style/BasicShapesShape.cpp: Added.
(WebCore::BasicShapeShape::create):
(WebCore::BasicShapeShape::BasicShapeShape):
(WebCore::BasicShapeShape::clone const):
(WebCore::BasicShapeShape::path const):
(WebCore::BasicShapeShape::canBlend const):
(WebCore::BasicShapeShape::blend const):
(WebCore::BasicShapeShape::operator== const):
(WebCore::BasicShapeShape::dump const):
(WebCore::operator<<):
* Source/WebCore/rendering/style/BasicShapesShape.h: Added.
(WebCore::ShapeSegmentBase::ShapeSegmentBase):
(WebCore::ShapeSegmentBase::affinity const):
@noamr
Copy link
Collaborator

noamr commented Aug 2, 2024

@smfr I think I've reached a similar but slightly different conclusion.
I think it's important to have by or to before the control points, but also it makes more sense to have the control points before the final point.

What do you think of #10644 (comment)?
Basically it's ordered, but the control points come first, and you can swap from by to to at any point.

clip-path: shape(from top left,
  hline by 100px,
  curve by 100px 50px / to right bottom,
  curve by 50px 50px / 10% 20% / var(--x) var(--y),
  curve by 50px 50px / 10% 20% / to bottom 50%,
  smooth to bottom left / 0 0,
)

We can play with the exact syntax/separators.
This also allows using position in control points without losing the flexibility of by.

smfr added a commit to smfr/WebKit that referenced this issue Aug 2, 2024
https://bugs.webkit.org/show_bug.cgi?id=277347
rdar://132814728

Reviewed by NOBODY (OOPS!).

Implement parsing, property value and computed style support for the `shape()` function[1]. We support the syntax
as of the most recent edit[2] but without the flexible order of points and control points due to [3].

Added BasicShapesShapeSegmentConversion.h/cpp with helpers to convert between CSS values and the internal shapes.
CSSShapeValue::customCSSText() is implemented.

BasicShapeShape stores its segments as a vector of std::variant<>, much as we do for Paths, which avoids heap allocations
per segment.

Plumb Style::BuilderState deeper into BasicShapeConversion functions. This required passing an optional zoom override,
used in some SVG cases (exercised by layout tests).

Tentative WPT are included.

Based on code by Noam Rosenthal.

[1] https://drafts.csswg.org/css-shapes-2/#shape-function
[2] w3c/csswg-drafts@9770805c4e53
[3] w3c/csswg-drafts#10666

* LayoutTests/imported/w3c/web-platform-tests/css/css-masking/animations/clip-path-interpolation-shape-expected.txt: Interpolation is not yet supported.
* LayoutTests/imported/w3c/web-platform-tests/css/css-masking/parsing/clip-path-shape-parsing-expected.txt:
* LayoutTests/imported/w3c/web-platform-tests/css/css-shapes/shape-functions/shape-function-computed.tentative-expected.txt: Added.
* LayoutTests/imported/w3c/web-platform-tests/css/css-shapes/shape-functions/shape-function-computed.tentative.html: Added.
* LayoutTests/imported/w3c/web-platform-tests/css/css-shapes/shape-functions/shape-function-invalid.tentative-expected.txt: Added.
* LayoutTests/imported/w3c/web-platform-tests/css/css-shapes/shape-functions/shape-function-invalid.tentative.html: Added.
* LayoutTests/imported/w3c/web-platform-tests/css/css-shapes/shape-functions/shape-function-valid.tentative-expected.txt: Added.
* LayoutTests/imported/w3c/web-platform-tests/css/css-shapes/shape-functions/shape-function-valid.tentative.html: Added.
* Source/WTF/Scripts/Preferences/UnifiedWebPreferences.yaml:
* Source/WebCore/Sources.txt:
* Source/WebCore/WebCore.xcodeproj/project.pbxproj:
* Source/WebCore/css/BasicShapeConversion.cpp:
(WebCore::valueForBasicShape):
(WebCore::convertToLengthPoint):
(WebCore::basicShapeForValue):
(WebCore::basicShapePathForValue):
(WebCore::basicShapeShapeForValue):
* Source/WebCore/css/BasicShapeConversion.h:
* Source/WebCore/css/BasicShapesShapeSegmentConversion.cpp: Added.
(WebCore::lengthToCSSValue):
(WebCore::lengthPointToCSSValue):
(WebCore::lengthSizeToCSSValue):
(WebCore::toCSSShapeSegmentValue):
(WebCore::fromCSSShapeSegmentValue):
* Source/WebCore/css/BasicShapesShapeSegmentConversion.h: Copied from Source/WebCore/css/BasicShapeConversion.h.
* Source/WebCore/css/CSSBasicShapes.cpp:
(WebCore::CSSShapeValue::CSSShapeValue):
(WebCore::CSSShapeValue::customCSSText const):
(WebCore::CSSShapeValue::equals const):
* Source/WebCore/css/CSSShapeSegmentValue.cpp:
(WebCore::CSSShapeSegmentValue::customCSSText const):
(WebCore::CSSShapeSegmentValue::toShapeSegment const):
* Source/WebCore/css/CSSShapeSegmentValue.h:
* Source/WebCore/css/CSSToStyleMap.cpp:
(WebCore::convertToLengthSize):
* Source/WebCore/css/CSSValueKeywords.in:
* Source/WebCore/css/parser/CSSPropertyParserHelpers.cpp:
(WebCore::CSSPropertyParserHelpers::consumeBasicShapePolygon):
(WebCore::CSSPropertyParserHelpers::consumeBasicShapePath):
(WebCore::CSSPropertyParserHelpers::consumeCoordinatePair):
(WebCore::CSSPropertyParserHelpers::consumeShapeCommand):
(WebCore::CSSPropertyParserHelpers::consumeBasicShapeShape):
(WebCore::CSSPropertyParserHelpers::consumeBasicShape):
* Source/WebCore/rendering/style/BasicShapes.cpp:
(WebCore::operator<<):
* Source/WebCore/rendering/style/BasicShapes.h:
* Source/WebCore/rendering/style/BasicShapesShape.cpp: Added.
(WebCore::BasicShapeShape::create):
(WebCore::BasicShapeShape::BasicShapeShape):
(WebCore::BasicShapeShape::clone const):
(WebCore::BasicShapeShape::path const):
(WebCore::BasicShapeShape::canBlend const):
(WebCore::BasicShapeShape::blend const):
(WebCore::BasicShapeShape::operator== const):
(WebCore::BasicShapeShape::dump const):
(WebCore::operator<<):
* Source/WebCore/rendering/style/BasicShapesShape.h: Added.
(WebCore::ShapeSegmentBase::ShapeSegmentBase):
(WebCore::ShapeSegmentBase::affinity const):
* Source/WebCore/style/StyleBuilderConverter.h:
(WebCore::Style::BuilderConverter::convertSVGPath):
(WebCore::Style::BuilderConverter::convertPathOperation):
(WebCore::Style::BuilderConverter::convertShapeValue):
* Source/WebCore/style/StyleBuilderState.h:
(WebCore::Style::BuilderState::style const):
smfr added a commit to smfr/WebKit that referenced this issue Aug 3, 2024
https://bugs.webkit.org/show_bug.cgi?id=277347
rdar://132814728

Reviewed by NOBODY (OOPS!).

Implement parsing, property value and computed style support for the `shape()` function[1]. We support the syntax
as of the most recent edit[2] but without the flexible order of points and control points due to [3].

Added BasicShapesShapeSegmentConversion.h/cpp with helpers to convert between CSS values and the internal shapes.
CSSShapeValue::customCSSText() is implemented.

BasicShapeShape stores its segments as a vector of std::variant<>, much as we do for Paths, which avoids heap allocations
per segment.

Plumb Style::BuilderState deeper into BasicShapeConversion functions. This required passing an optional zoom override,
used in some SVG cases (exercised by layout tests).

Tentative WPT are included.

Based on code by Noam Rosenthal.

[1] https://drafts.csswg.org/css-shapes-2/#shape-function
[2] w3c/csswg-drafts@9770805c4e53
[3] w3c/csswg-drafts#10666

* LayoutTests/imported/w3c/web-platform-tests/css/css-masking/animations/clip-path-interpolation-shape-expected.txt: Interpolation is not yet supported.
* LayoutTests/imported/w3c/web-platform-tests/css/css-masking/parsing/clip-path-shape-parsing-expected.txt:
* LayoutTests/imported/w3c/web-platform-tests/css/css-shapes/shape-functions/shape-function-computed.tentative-expected.txt: Added.
* LayoutTests/imported/w3c/web-platform-tests/css/css-shapes/shape-functions/shape-function-computed.tentative.html: Added.
* LayoutTests/imported/w3c/web-platform-tests/css/css-shapes/shape-functions/shape-function-invalid.tentative-expected.txt: Added.
* LayoutTests/imported/w3c/web-platform-tests/css/css-shapes/shape-functions/shape-function-invalid.tentative.html: Added.
* LayoutTests/imported/w3c/web-platform-tests/css/css-shapes/shape-functions/shape-function-valid.tentative-expected.txt: Added.
* LayoutTests/imported/w3c/web-platform-tests/css/css-shapes/shape-functions/shape-function-valid.tentative.html: Added.
* LayoutTests/imported/w3c/web-platform-tests/css/motion/animation/offset-path-interpolation-008-expected.txt:
* Source/WTF/Scripts/Preferences/UnifiedWebPreferences.yaml:
* Source/WebCore/Sources.txt:
* Source/WebCore/WebCore.xcodeproj/project.pbxproj:
* Source/WebCore/css/BasicShapeConversion.cpp:
(WebCore::valueForBasicShape):
(WebCore::convertToLengthPoint):
(WebCore::basicShapeForValue):
(WebCore::basicShapePathForValue):
(WebCore::basicShapeShapeForValue):
* Source/WebCore/css/BasicShapeConversion.h:
* Source/WebCore/css/BasicShapesShapeSegmentConversion.cpp: Added.
(WebCore::lengthToCSSValue):
(WebCore::lengthPointToCSSValue):
(WebCore::lengthSizeToCSSValue):
(WebCore::toCSSShapeSegmentValue):
(WebCore::fromCSSShapeSegmentValue):
* Source/WebCore/css/BasicShapesShapeSegmentConversion.h: Copied from Source/WebCore/css/BasicShapeConversion.h.
* Source/WebCore/css/CSSBasicShapes.cpp:
(WebCore::CSSShapeValue::CSSShapeValue):
(WebCore::CSSShapeValue::customCSSText const):
(WebCore::CSSShapeValue::equals const):
* Source/WebCore/css/CSSShapeSegmentValue.cpp:
(WebCore::CSSShapeSegmentValue::customCSSText const):
(WebCore::CSSShapeSegmentValue::toShapeSegment const):
* Source/WebCore/css/CSSShapeSegmentValue.h:
* Source/WebCore/css/CSSToStyleMap.cpp:
(WebCore::convertToLengthSize):
* Source/WebCore/css/CSSValueKeywords.in:
* Source/WebCore/css/parser/CSSPropertyParserHelpers.cpp:
(WebCore::CSSPropertyParserHelpers::consumeBasicShapePolygon):
(WebCore::CSSPropertyParserHelpers::consumeBasicShapePath):
(WebCore::CSSPropertyParserHelpers::consumeCoordinatePair):
(WebCore::CSSPropertyParserHelpers::consumeShapeCommand):
(WebCore::CSSPropertyParserHelpers::consumeBasicShapeShape):
(WebCore::CSSPropertyParserHelpers::consumeBasicShape):
* Source/WebCore/rendering/style/BasicShapes.cpp:
(WebCore::operator<<):
* Source/WebCore/rendering/style/BasicShapes.h:
* Source/WebCore/rendering/style/BasicShapesShape.cpp: Added.
(WebCore::BasicShapeShape::create):
(WebCore::BasicShapeShape::BasicShapeShape):
(WebCore::BasicShapeShape::clone const):
(WebCore::BasicShapeShape::path const):
(WebCore::BasicShapeShape::canBlend const):
(WebCore::BasicShapeShape::blend const):
(WebCore::BasicShapeShape::operator== const):
(WebCore::BasicShapeShape::dump const):
(WebCore::operator<<):
* Source/WebCore/rendering/style/BasicShapesShape.h: Added.
(WebCore::ShapeSegmentBase::ShapeSegmentBase):
(WebCore::ShapeSegmentBase::affinity const):
* Source/WebCore/style/StyleBuilderConverter.h:
(WebCore::Style::BuilderConverter::convertSVGPath):
(WebCore::Style::BuilderConverter::convertPathOperation):
(WebCore::Style::BuilderConverter::convertShapeValue):
* Source/WebCore/style/StyleBuilderState.h:
(WebCore::Style::BuilderState::style const):
webkit-commit-queue pushed a commit to smfr/WebKit that referenced this issue Aug 3, 2024
https://bugs.webkit.org/show_bug.cgi?id=277347
rdar://132814728

Reviewed by Tim Nguyen.

Implement parsing, property value and computed style support for the `shape()` function[1]. We support the syntax
as of the most recent edit[2] but without the flexible order of points and control points due to [3].

Added BasicShapesShapeSegmentConversion.h/cpp with helpers to convert between CSS values and the internal shapes.
CSSShapeValue::customCSSText() is implemented.

BasicShapeShape stores its segments as a vector of std::variant<>, much as we do for Paths, which avoids heap allocations
per segment.

Plumb Style::BuilderState deeper into BasicShapeConversion functions. This required passing an optional zoom override,
used in some SVG cases (exercised by layout tests).

Tentative WPT are included.

Based on code by Noam Rosenthal.

[1] https://drafts.csswg.org/css-shapes-2/#shape-function
[2] w3c/csswg-drafts@9770805c4e53
[3] w3c/csswg-drafts#10666

* LayoutTests/imported/w3c/web-platform-tests/css/css-masking/animations/clip-path-interpolation-shape-expected.txt: Interpolation is not yet supported.
* LayoutTests/imported/w3c/web-platform-tests/css/css-masking/parsing/clip-path-shape-parsing-expected.txt:
* LayoutTests/imported/w3c/web-platform-tests/css/css-shapes/shape-functions/shape-function-computed.tentative-expected.txt: Added.
* LayoutTests/imported/w3c/web-platform-tests/css/css-shapes/shape-functions/shape-function-computed.tentative.html: Added.
* LayoutTests/imported/w3c/web-platform-tests/css/css-shapes/shape-functions/shape-function-invalid.tentative-expected.txt: Added.
* LayoutTests/imported/w3c/web-platform-tests/css/css-shapes/shape-functions/shape-function-invalid.tentative.html: Added.
* LayoutTests/imported/w3c/web-platform-tests/css/css-shapes/shape-functions/shape-function-valid.tentative-expected.txt: Added.
* LayoutTests/imported/w3c/web-platform-tests/css/css-shapes/shape-functions/shape-function-valid.tentative.html: Added.
* LayoutTests/imported/w3c/web-platform-tests/css/motion/animation/offset-path-interpolation-008-expected.txt:
* Source/WTF/Scripts/Preferences/UnifiedWebPreferences.yaml:
* Source/WebCore/Sources.txt:
* Source/WebCore/WebCore.xcodeproj/project.pbxproj:
* Source/WebCore/css/BasicShapeConversion.cpp:
(WebCore::valueForBasicShape):
(WebCore::convertToLengthPoint):
(WebCore::basicShapeForValue):
(WebCore::basicShapePathForValue):
(WebCore::basicShapeShapeForValue):
* Source/WebCore/css/BasicShapeConversion.h:
* Source/WebCore/css/BasicShapesShapeSegmentConversion.cpp: Added.
(WebCore::lengthToCSSValue):
(WebCore::lengthPointToCSSValue):
(WebCore::lengthSizeToCSSValue):
(WebCore::toCSSShapeSegmentValue):
(WebCore::fromCSSShapeSegmentValue):
* Source/WebCore/css/BasicShapesShapeSegmentConversion.h: Copied from Source/WebCore/css/BasicShapeConversion.h.
* Source/WebCore/css/CSSBasicShapes.cpp:
(WebCore::CSSShapeValue::CSSShapeValue):
(WebCore::CSSShapeValue::customCSSText const):
(WebCore::CSSShapeValue::equals const):
* Source/WebCore/css/CSSShapeSegmentValue.cpp:
(WebCore::CSSShapeSegmentValue::customCSSText const):
(WebCore::CSSShapeSegmentValue::toShapeSegment const):
* Source/WebCore/css/CSSShapeSegmentValue.h:
* Source/WebCore/css/CSSToStyleMap.cpp:
(WebCore::convertToLengthSize):
* Source/WebCore/css/CSSValueKeywords.in:
* Source/WebCore/css/parser/CSSPropertyParserHelpers.cpp:
(WebCore::CSSPropertyParserHelpers::consumeBasicShapePolygon):
(WebCore::CSSPropertyParserHelpers::consumeBasicShapePath):
(WebCore::CSSPropertyParserHelpers::consumeCoordinatePair):
(WebCore::CSSPropertyParserHelpers::consumeShapeCommand):
(WebCore::CSSPropertyParserHelpers::consumeBasicShapeShape):
(WebCore::CSSPropertyParserHelpers::consumeBasicShape):
* Source/WebCore/rendering/style/BasicShapes.cpp:
(WebCore::operator<<):
* Source/WebCore/rendering/style/BasicShapes.h:
* Source/WebCore/rendering/style/BasicShapesShape.cpp: Added.
(WebCore::BasicShapeShape::create):
(WebCore::BasicShapeShape::BasicShapeShape):
(WebCore::BasicShapeShape::clone const):
(WebCore::BasicShapeShape::path const):
(WebCore::BasicShapeShape::canBlend const):
(WebCore::BasicShapeShape::blend const):
(WebCore::BasicShapeShape::operator== const):
(WebCore::BasicShapeShape::dump const):
(WebCore::operator<<):
* Source/WebCore/rendering/style/BasicShapesShape.h: Added.
(WebCore::ShapeSegmentBase::ShapeSegmentBase):
(WebCore::ShapeSegmentBase::affinity const):
* Source/WebCore/style/StyleBuilderConverter.h:
(WebCore::Style::BuilderConverter::convertSVGPath):
(WebCore::Style::BuilderConverter::convertPathOperation):
(WebCore::Style::BuilderConverter::convertShapeValue):
* Source/WebCore/style/StyleBuilderState.h:
(WebCore::Style::BuilderState::style const):

Canonical link: https://commits.webkit.org/281805@main
@noamr noamr moved this to Regular agenda items in CSSWG Agenda TPAC 2024 Sep 18, 2024
@css-meeting-bot
Copy link
Member

The CSS Working Group just discussed [css-shapes-2] Order of points and control points in `shape()` , and agreed to the following:

  • RESOLVED: Make the order strict: endpoints before control points.
The full IRC log of that discussion <dbaron> noamr: This is a very related issue.
<dbaron> noamr: We resolved on allowing any order -- either control points first or end points first.
<dbaron> noamr: With verbosity it seems like it's making things unnecessarily complex
<dbaron> noamr: Maybe endpoints should always go first and control points second?
<dbaron> Rossen9: any reason we can't do it? Implementations that need to change?
<dbaron> noamr: Not shipped yet anywhere.
<dbaron> Rossen9: any objections to making the order strict with the endpoints first?
<dbaron> RESOLVED: Make the order strict: endpoints before control points.
<dbaron> chrishtr: did you resolve to use the adjustment approach in the issue or what simon said in the beginning?
<dbaron> TabAtkins: I think the opposite.
<dbaron> noamr: yeah, what Simon was suggesting that the endpoints go firs.t
<TabAtkins> (noam's comment has the endpoints last; we resolved (correctly) on Simon's preference for endpoints first)

@dholbert
Copy link
Member

@smfr, question for you, just to be explicit about where things stand:

The resolution here was "Make the order strict: endpoints before control points", but if I understood the discussion in #10649 (comment) , we're relaxing that now and not requiring that strict order anymore. Is that correct?

@noamr
Copy link
Collaborator

noamr commented Nov 13, 2024

@smfr, question for you, just to be explicit about where things stand:

The resolution here was "Make the order strict: endpoints before control points", but if I understood the discussion in #10649 (comment) , we're relaxing that now and not requiring that strict order anymore. Is that correct?

That's correct. Now that we have the with keyword we don't need the strict order.

noamr added a commit to noamr/csswg-drafts that referenced this issue Nov 13, 2024
- <<position>> can be used instead of an absolute point.
- One of a <<position>>'s dimension can be used for hline/vline
- Since keywords define the different components, order of components is flexible.

Curves:
- `with` is used for control points
- Two control points are separated by /
- Relative control points can be relative to segment-start/segment-end/reference-box

Resolution: w3c#10649 (comment)

Closes w3c#10649
Closes w3c#10666
@smfr
Copy link
Contributor Author

smfr commented Dec 13, 2024

I want to re-open this for further discussion. The allowed control point values depend on the command affinity (the to/by), so if the control points come first, you have to parse the control points in a relaxed way, and then go back and validate them after you hit the to or by. This doesn't feel great implementation-wise, but it also feels odd as an author to write out control points before you've written the to or by that determines how those control points are interpreted.

I thus propose that we go back to strict ordering of points and control points in curve commands.

@smfr smfr reopened this Dec 13, 2024
@noamr
Copy link
Collaborator

noamr commented Dec 13, 2024

I want to re-open this for further discussion. The allowed control point values depend on the command affinity (the to/by), so if the control points come first, you have to parse the control points in a relaxed way, and then go back and validate them after you hit the to or by. This doesn't feel great implementation-wise, but it also feels odd as an author to write out control points before you've written the to or by that determines how those control points are interpreted.

I thus propose that we go back to strict ordering of points and control points in curve commands.

To be clear, you're proposing that the strict ordering would be to have the end point first, correct?
I'm OK with this

@smfr
Copy link
Contributor Author

smfr commented Dec 13, 2024

Yes, the to or by would have to come first. This would apply to curve, smooth and arc commands.

smfr added a commit to smfr/WebKit that referenced this issue Jan 2, 2025
https://bugs.webkit.org/show_bug.cgi?id=285303
rdar://142276582

Reviewed by NOBODY (OOPS!).

The CSS shape function [1] syntax is pretty much stable now.

There's one open resolution [2] which we may still have to make changes for, but we still have
some time to respond.

[1] https://drafts.csswg.org/css-shapes-2/#shape-function
[2] w3c/csswg-drafts#10666

* Source/WTF/Scripts/Preferences/UnifiedWebPreferences.yaml:
webkit-commit-queue pushed a commit to smfr/WebKit that referenced this issue Jan 2, 2025
https://bugs.webkit.org/show_bug.cgi?id=285303
rdar://142276582

Reviewed by Tim Nguyen.

The CSS shape function [1] syntax is pretty much stable now.

There's one open resolution [2] which we may still have to make changes for, but we still have
some time to respond.

[1] https://drafts.csswg.org/css-shapes-2/#shape-function
[2] w3c/csswg-drafts#10666

* Source/WTF/Scripts/Preferences/UnifiedWebPreferences.yaml:

Canonical link: https://commits.webkit.org/288378@main
@smfr smfr added the Agenda+ label Jan 7, 2025
@css-meeting-bot
Copy link
Member

The CSS Working Group just discussed [css-shapes-2] Order of points and control points in `shape()` , and agreed to the following:

  • RESOLVED: restrict ordering such that `to` or `by` would need to come first
The full IRC log of that discussion <kbabbitt> smfr: previously we decided you could specify dest point and control points in any order
<kbabbitt> ... later we added ability to specify control points in a way that depended on relative/absolute to/by on dest point
<kbabbitt> ... possible to write segments where control points come first but there's ambiguity on how to get to dest point
<kbabbitt> ... curve using top left by 10% 10%
<kbabbitt> ... which is invalid
<fantasai> s/using/with/
<kbabbitt> ... you cannot read left to right, you cannot parse left to right, have to maintain state to resolve `to` keyword
<TabAtkins> q+
<kbabbitt> ... do we have a desire in CSS to make things valid when read left to right?
<astearns> ack TabAtkins
<fantasai> curve with 10px 30px by 20px 20px
<fantasai> curve by 20px 20px with 10px 30px
<kbabbitt> TabAtkins: I'm fine with smfr's suggestoin
<kbabbitt> ... being able to know what coordinate space you're working in early seems reasonable
<kbabbitt> fantasai: prefer to keep it reorderable but not a big deal
<kbabbitt> Proposed: restrict ordering such that `to` or `by` would need to come first
<kbabbitt> fantasai: could decide later that it can be reorderable
<kbabbitt> RESOLVED: restrict ordering such that `to` or `by` would need to come first

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Friday morning
5 participants