Parse full animation-range syntax #251
Conversation
johannesodland
force-pushed
the
parse-full-animation-range-syntax
branch
10 times, most recently
from
81c36cb to
7afa913
Compare
| @@ -40,11 +40,11 @@ const unitGroups = { | |||
| }, | |||
| // https://www.w3.org/TR/css-values-4/#absolute-lengths | |||
| absoluteLengths: { | |||
| units: new Set(["cm", "mm", "Q", "in", "pt", "pc", "px"]), | |||
| units: new Set(["cm", "mm", "q", "in", "pt", "pc", "px"]), | |||
There was a problem hiding this comment.
I see you lowercased this here, yet in test/unit/cssom/css-numeric-value-type.test.js the unit is uppercase
There was a problem hiding this comment.
I lower-cased the units here, as the units should be case insensitive. (Both new CSSUnitValue(5, 'Q').type() and new CSSUnitValue(5, 'q').type() should return {length: 1}.)
Q was the odd unit out, and I think its better to keep the representation of the units in lower case, in stead of changing the case om the fly when checking units.
The upper-case Q in the test is copy paste from before the unit was lower cased.
Come to think of it, the unit tests should probably test the units with both upper and lower case 🤔.
Talking about units, we have some case sensitivity issues in the cssom polyfill if you create a CSSUnitValue() or run CSSNumericValue.parse() with upper-case units. Units are then stored and serialized in uppercase, while Safari and Chrome will return them in lowercase. It should be simple to fix, but I'll save it for another PR :)
| @@ -1126,7 +1134,7 @@ export class ProxyAnimation { | |||
| effect: null, | |||
| // The animation attachment range, restricting the animation’s | |||
| // active interval to that range of a timeline | |||
| animationRange: isScrollAnimation ? parseAnimationRange(timeline, animOptions['animation-range']) : null, | |||
| animationRange: isScrollAnimation ? parseAnimationRange(animOptions['animation-range']) : null, | |||
There was a problem hiding this comment.
One of the things that was present in the previous implementation is that it checked whether the passed in range made sense for the type of timeline or not. E.g. passing incover 20% does not make sense for a regular ScrollTimeline.
With this functionality removed here now, do we check it elsewhere before it’s actually used?
There was a problem hiding this comment.
As I understand it, the syntax allows for both <length-percentage> and <timeline-range-name> <length-percentage>? which makes sense since animation-range is a property of the animation and not the timeline.
The values are checked when used.
When <length-percentage> is defined on a view-timeline, the offset corresponds to a specified point on the view-timeline. animation-range: 0% 100% is then treated similarly to cover 0% cover 100% and normal.
<length-percentage>
The animation attachment range starts at the specified point on the timeline measuring from the start of the timeline.
When <timeline-range-name> <length-percentage>? is used on a scroll-timeline, only the offsets are used. entry 0% entry 100% is treated similarly to 0% 100% and normal.
This seem to correspond well with Chrome, as far as I have tested. Specify animation-range: cover 20% on an animation with a scroll-timeline, and rangeStart will return {rangeName: 'cover', offset: CSS.px(20)}.
I don't see how we can validate a specified cover 20% for a ScrollTimeline, as the timeline might be changed to a ViewTimeline afterwards.
| @@ -940,7 +940,7 @@ function getNormalStartRange(timeline) { | |||
| } | |||
|
|
|||
| if (timeline instanceof ScrollTimeline) { | |||
| return CSS.percent(0); | |||
| return { rangeName: 'none', offset: CSS.percent(0) }; | |||
There was a problem hiding this comment.
Note that none is not mentioned in the spec and that it is something that Chrome uses to indicate if the range only has an offset.
There is some discussion to introduce scroll as a range name in w3c/csswg-drafts#9367, which would allow renaming none to scroll as I mentioned in w3c/csswg-drafts#9367 (comment).
Would leave it at how it is right now, but might need a comment linking to the WG Issue.
There was a problem hiding this comment.
Good point. I'll leave a comment linking to the issue.
There was a problem hiding this comment.
Documented the use of 'none' as range name in stead of null in 96c567c.
There was a problem hiding this comment.
I have filed https://issues.chromium.org/issues/371926668 on the Blink side.
|
Edit: Nevermind, our atomic css parser split the rules out so it couldn't parse it correctly.
animation-duration: 1ms;
animation-fill-mode: forwards;
animation-name: overflow-visible;
animation-range-start: entry 70%;
animation-range-end: entry 100%;
animation-timeline: --carousel-scroller;
view-timeline-name: --carousel-scroller;
@keyframes overflow-visible {
from {
opacity: 1;
}
to {
opacity: 0;
}
} |
|
@bramus @johannesodland Any chance this might still land? I'm trying to use syntax like the following for an effect to hide items going underneath a sidebar header but the current polyfill doesn't work with it. animation-timeline: view();
animation-range: exit calc(0% - 160px) exit calc(0% - 145px);
/* ^ The 160px is the height of my sticky header */Example of desired effect working in Chrome: Screen.Recording.2024-10-10.at.2.12.02.PM.mov |
johannesodland
force-pushed
the
parse-full-animation-range-syntax
branch
from
0586aaa to
400d661
Compare
johannesodland
force-pushed
the
parse-full-animation-range-syntax
branch
from
400d661 to
82f8ac3
Compare
|
Was stoked to see support soon to land in Safari (TP has basic and more fully supported in their main branch). Would this patch still be good to land? |
Implements parsing the full animation-range syntax to fix #236
Changes:
parseAnimationRange()consumeRange()add two typesfrom css-typed-om (needed for validating length-percentage)create a typecase insensitive<length-percentage>in rangeStart/End is stored and returned:{ rangeName: 'none', offset: CSS.percent(0)}vitestfor unit-testsanimation-rangeThe tests for
animation-rangeare not strictly unit tests and tests the implementation through the internalparseAnimationRange()function. The functionality is currently only exposed through css parsing which makes it difficult to test. We could expose the parsing partly throughCSSStyleValue.parse('animation-range', value)but then we would have to proxy that method in all browsers.