Skip to content

Compute values of basic shape rect() and xywh() as the equivalent inset()#21903

Merged
webkit-commit-queue merged 1 commit intoWebKit:mainfrom
darinadler:eng/Compute-values-of-basic-shape-rect-and-xywh-as-the-equivalent-inset
Dec 16, 2023
Merged

Compute values of basic shape rect() and xywh() as the equivalent inset()#21903
webkit-commit-queue merged 1 commit intoWebKit:mainfrom
darinadler:eng/Compute-values-of-basic-shape-rect-and-xywh-as-the-equivalent-inset

Conversation

@darinadler
Copy link
Copy Markdown
Member

@darinadler darinadler commented Dec 15, 2023

534a722

Compute values of basic shape rect() and xywh() as the equivalent inset()
https://bugs.webkit.org/show_bug.cgi?id=266513
rdar://problem/119739406

Reviewed by Tim Nguyen.

CSS Shapes called for this behavior but we did not implement it yet.

* LayoutTests/imported/w3c/web-platform-tests/css/css-masking/animations/clip-path-interpolation-xywh-rect-expected.txt:
* LayoutTests/imported/w3c/web-platform-tests/css/css-masking/parsing/clip-path-computed-expected.txt:
* LayoutTests/imported/w3c/web-platform-tests/css/motion/animation/offset-path-interpolation-006-expected.txt:
* LayoutTests/imported/w3c/web-platform-tests/css/motion/parsing/offset-path-computed-expected.txt:
More PASS, less FAIL.

* Source/WebCore/css/BasicShapeFunctions.cpp:
(WebCore::valueForBasicShape): Added code that computes the equivalent inset as specified.
Use the CSSCalcValue machinery as needed.

Canonical link: https://commits.webkit.org/272180@main

977dfbc

Misc iOS, tvOS & watchOS macOS Linux Windows
✅ 🧪 style ✅ 🛠 ios ✅ 🛠 mac ✅ 🛠 wpe ✅ 🛠 wincairo
✅ 🧪 bindings ✅ 🛠 ios-sim ✅ 🛠 mac-AS-debug ✅ 🧪 wpe-wk2
✅ 🧪 webkitperl ✅ 🧪 ios-wk2 ✅ 🧪 api-mac ✅ 🧪 api-wpe
✅ 🧪 ios-wk2-wpt ✅ 🧪 mac-wk1 ✅ 🛠 gtk
✅ 🧪 api-ios ✅ 🧪 mac-wk2 ✅ 🧪 gtk-wk2
✅ 🛠 tv ✅ 🧪 mac-AS-debug-wk2 ✅ 🧪 api-gtk
✅ 🛠 tv-sim
✅ 🛠 🧪 merge ✅ 🛠 watch
✅ 🛠 watch-sim

@darinadler darinadler self-assigned this Dec 15, 2023
@darinadler darinadler added the CSS Cascading Style Sheets implementation label Dec 15, 2023
@darinadler darinadler requested review from nmoucht and nt1m December 15, 2023 23:44
Copy link
Copy Markdown
Member

@nt1m nt1m left a comment

Choose a reason for hiding this comment

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

Looks good aside from one comment, thanks!

{
auto createValue = [&](const Length& length) {
return CSSPrimitiveValue::create(length, style);
return CSSPrimitiveValue::create(length.isAuto() ? Length(0, LengthType::Percent) : length, style);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

From https://drafts.csswg.org/css-shapes-1/#basic-shape-computed-values :

An auto value makes the edge of the box coincide with the corresponding edge of the reference box: it’s equivalent to 0% as the first (top) or fourth (left) value, and equivalent to 100% as the second (right) or third (bottom) value.

Is this conversion correct? Maybe converting to 100% in the relevant cases would fix the interpolation tests?

Copy link
Copy Markdown
Member Author

@darinadler darinadler Dec 16, 2023

Choose a reason for hiding this comment

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

It’s correct. There are no 100 cases, because all the auto are in rect and we are always converting to inset and so it’s always 0 once it’s a value in the inset. Earlier ways I wrote the code had some 100 cases because I wrote it based on the above text, but as I refined and refactored the code, it led to it just being this. I’ve looked at remaining failing interpolation tests and the issues are the interpolation not the values.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Oh, actually, I thought of another way to explain the refactoring. Rather than have a separate case of 100 in here, I put a special case for auto into createReflectedValue. If I had not done that, then I would have had to pass in a value of 0 or 100, and it would have computed 100% - 100% and ended up 0% anyway.

@darinadler darinadler added the safe-merge-queue Applied to automatically send a pull-request to merge-queue after passing EWS checks label Dec 16, 2023
@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Dec 16, 2023
@Ahmad-S792 Ahmad-S792 removed the merging-blocked Applied to prevent a change from being merged label Dec 16, 2023
@webkit-ews-buildbot webkit-ews-buildbot added merging-blocked Applied to prevent a change from being merged and removed safe-merge-queue Applied to automatically send a pull-request to merge-queue after passing EWS checks labels Dec 16, 2023
@darinadler darinadler added safe-merge-queue Applied to automatically send a pull-request to merge-queue after passing EWS checks and removed merging-blocked Applied to prevent a change from being merged labels Dec 16, 2023
@WebKit WebKit deleted a comment from webkit-ews-buildbot Dec 16, 2023
@WebKit WebKit deleted a comment from webkit-ews-buildbot Dec 16, 2023
@WebKit WebKit deleted a comment from webkit-ews-buildbot Dec 16, 2023
@WebKit WebKit deleted a comment from webkit-ews-buildbot Dec 16, 2023
@webkit-ews-buildbot webkit-ews-buildbot added merge-queue Applied to send a pull request to merge-queue and removed safe-merge-queue Applied to automatically send a pull-request to merge-queue after passing EWS checks labels Dec 16, 2023
@webkit-ews-buildbot
Copy link
Copy Markdown
Collaborator

Safe-Merge-Queue: Build #6396.

…et()

https://bugs.webkit.org/show_bug.cgi?id=266513
rdar://problem/119739406

Reviewed by Tim Nguyen.

CSS Shapes called for this behavior but we did not implement it yet.

* LayoutTests/imported/w3c/web-platform-tests/css/css-masking/animations/clip-path-interpolation-xywh-rect-expected.txt:
* LayoutTests/imported/w3c/web-platform-tests/css/css-masking/parsing/clip-path-computed-expected.txt:
* LayoutTests/imported/w3c/web-platform-tests/css/motion/animation/offset-path-interpolation-006-expected.txt:
* LayoutTests/imported/w3c/web-platform-tests/css/motion/parsing/offset-path-computed-expected.txt:
More PASS, less FAIL.

* Source/WebCore/css/BasicShapeFunctions.cpp:
(WebCore::valueForBasicShape): Added code that computes the equivalent inset as specified.
Use the CSSCalcValue machinery as needed.

Canonical link: https://commits.webkit.org/272180@main
@webkit-commit-queue webkit-commit-queue force-pushed the eng/Compute-values-of-basic-shape-rect-and-xywh-as-the-equivalent-inset branch from 977dfbc to 534a722 Compare December 16, 2023 15:18
@webkit-commit-queue
Copy link
Copy Markdown
Collaborator

Committed 272180@main (534a722): https://commits.webkit.org/272180@main

Reviewed commits have been landed. Closing PR #21903 and removing active labels.

@webkit-commit-queue webkit-commit-queue merged commit 534a722 into WebKit:main Dec 16, 2023
@webkit-commit-queue webkit-commit-queue removed the merge-queue Applied to send a pull request to merge-queue label Dec 16, 2023
@darinadler darinadler deleted the eng/Compute-values-of-basic-shape-rect-and-xywh-as-the-equivalent-inset branch December 16, 2023 17:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CSS Cascading Style Sheets implementation

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants