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

Fix tests for clamping to 0 with keyframes with calc() values #37457

Merged

Conversation

graouts
Copy link
Contributor

@graouts graouts commented Dec 12, 2022

Addresses issue #37053.

@graouts graouts requested review from andruud, emilio, anttijk and nt1m and removed request for flackr and birtles December 12, 2022 15:54
@nt1m nt1m requested a review from birtles December 12, 2022 17:47
webkit-commit-queue pushed a commit to graouts/WebKit that referenced this pull request Dec 13, 2022
https://bugs.webkit.org/show_bug.cgi?id=249151

Reviewed by Simon Fraser and Dean Jackson.

The "perspective" property is specified to only allow non-negative values,
see https://www.w3.org/TR/css-transforms-2/#perspective-property. However,
we don't clip values that are computed to less than 0, for instance using a
calc() expression. Instead, we deem them invalid internally and thus consider
as if "none" was provided, which has implications when blending. Indeed, when
interpolating between a valid value and "none", we use discrete interpolation.

We now correctly clip values below 0.

Note that the relevant WPT test still fails, because the test itself is not
valid. It is being addressed in a dedicated WPT PR and will be imported back
into WebKit yielding a PASS result. See web-platform-tests/wpt#37457.

* LayoutTests/imported/w3c/web-platform-tests/css/css-typed-om/the-stylepropertymap/properties/perspective-expected.txt:
* LayoutTests/imported/w3c/web-platform-tests/web-animations/responsive/perspective-expected.txt:
* Source/WebCore/css/parser/CSSPropertyParserHelpers.cpp:
(WebCore::CSSPropertyParserHelpers::consumePerspective):

Canonical link: https://commits.webkit.org/257779@main
@flackr
Copy link
Contributor

flackr commented Dec 13, 2022

Long term I would like to see chrome's behavior spec'd since i think it supports more use cases, but I can see this is what the spec currently says to do, so it seems reasonable to test.

Can you also add tests showing the expectations for an animation of both relative size affecting property (e.g. font-size or height) and a clamped property depending on that property?

@flackr
Copy link
Contributor

flackr commented Dec 13, 2022

+@kevers-google FYI

@graouts
Copy link
Contributor Author

graouts commented Dec 13, 2022

Long term I would like to see chrome's behavior spec'd since i think it supports more use cases, but I can see this is what the spec currently says to do, so it seems reasonable to test.

Yes, this may make sense, I agree. But at it stands better to have the tests reflect the specs than wait for the specs to catch up.

Can you also add tests showing the expectations for an animation of both relative size affecting property (e.g. font-size or height) and a clamped property depending on that property?

I'm sorry but I don't get what you mean. Can you write an example? I'm happy to then expand it to the various tests.

@graouts
Copy link
Contributor Author

graouts commented Dec 14, 2022

I'm merging now because there is a clear scope and @birtles reviewed, but when @flackr gets back to me to help me understand his request, I'll make sure to make a followup pull request.

@graouts graouts merged commit 9565baf into web-platform-tests:master Dec 14, 2022
@graouts graouts deleted the web-animations-clamped-keyframes branch December 14, 2022 07:40
webkit-commit-queue pushed a commit to graouts/WebKit that referenced this pull request Dec 14, 2022
https://bugs.webkit.org/show_bug.cgi?id=249292

Reviewed by Dean Jackson.

Import changes made in web-platform-tests/wpt#37457 which yield new
PASS results in WebKit.

* LayoutTests/imported/w3c/web-platform-tests/web-animations/responsive/columnGap-expected.txt:
* LayoutTests/imported/w3c/web-platform-tests/web-animations/responsive/columnGap.html:
* LayoutTests/imported/w3c/web-platform-tests/web-animations/responsive/lineHeight-expected.txt:
* LayoutTests/imported/w3c/web-platform-tests/web-animations/responsive/lineHeight.html:
* LayoutTests/imported/w3c/web-platform-tests/web-animations/responsive/minHeight-expected.txt:
* LayoutTests/imported/w3c/web-platform-tests/web-animations/responsive/minHeight.html:
* LayoutTests/imported/w3c/web-platform-tests/web-animations/responsive/perspective-expected.txt:
* LayoutTests/imported/w3c/web-platform-tests/web-animations/responsive/perspective.html:
* LayoutTests/imported/w3c/web-platform-tests/web-animations/responsive/rowGap-expected.txt:
* LayoutTests/imported/w3c/web-platform-tests/web-animations/responsive/rowGap.html:

Canonical link: https://commits.webkit.org/257849@main
@flackr
Copy link
Contributor

flackr commented Dec 14, 2022

I'm sorry but I don't get what you mean. Can you write an example? I'm happy to then expand it to the various tests.

In particular, I was thinking we should have testing for the interpolation of some cases like these:

element.animate([{minHeight: '30px', fontSize: '10px'}, {minHeight: 'calc(30px - 2em)', fontSize: '40px'}], 1000);

Or

element.animate([{minHeight: '30px'}, {minHeight: 'calc(30px - 2em)'}], 1000);
element.animate([{fontSize: '10px'}, {fontSize: '40px'}], 1000);
// Or ancestor.animate(...)

Or

window.CSS.registerProperty({
  name: '--min-height',
  syntax: '<length>',
  inherits: false,
  initialValue: '0px',
});
element.style.minHeight = 'var(--min-height)';
element.style.fontSize = '40px';
element.animate([{'--min-height': '30px'}, {'--min-height': 'calc(30px - 2em)'}], 1000);

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants