[scroll-animations] ViewTimelineOptions.inset should use CSSKeywordish over CSSKeywordValue
#11477
Labels
scroll-animations-1
Current Work
Comments
|
Cc @flackr, @bramus, @kevers-google and @andruud. |
ViewTimelineOptions.inset should use CSSKeywordishViewTimelineOptions.inset should use CSSKeywordish over CSSKeywordValue
graouts
added a commit
to graouts/WebKit
that referenced
this issue
Jan 10, 2025
…meline-inset.html is a failure https://bugs.webkit.org/show_bug.cgi?id=285750 rdar://142690868 Reviewed by NOBODY (OOPS!). Our support for view timeline insets was very rudimentary. While we fixed the most egregious issue in 288684@main, we still had many issues left as shown in the failures in the relevant WPT test. The main task was to only convert inset values provided via the JS bindings to `CSSPrimitiveValue` such that we may resolve the insets when the view timeline's current time is being recomputed, thus correctly handling any relative value. Values provided via CSS are already fully resolved and are used as-is. We also implement the behavior where an `"auto"` values means that the values provided through the `scroll-padding` CSS property on the scroll container should be used. Additionally, we now raise exceptions for forbidden value types and units. We also switched the IDL type for `ViewTimelineInsets.inset` to use a `CSSKeywordish` rather than `CSSKeywordValue` alone so that strings would be allowed. This matches what Chrome implements and one of the cases tested in the test. The issue w3c/csswg-drafts#11477 was filed to track resolution of this discrepancy between the spec and WPT. * LayoutTests/imported/w3c/web-platform-tests/scroll-animations/view-timelines/view-timeline-inset-expected.txt: * Source/WebCore/animation/ViewTimeline.cpp: (WebCore::isValidInset): (WebCore::ViewTimeline::create): (WebCore::ViewTimeline::ViewTimeline): (WebCore::ViewTimeline::validateSpecifiedInsets): (WebCore::ViewTimeline::cacheCurrentTime): (WebCore::lengthForInset): Deleted. (WebCore::insetsFromOptions): Deleted. * Source/WebCore/animation/ViewTimeline.h: * Source/WebCore/animation/ViewTimeline.idl: * Source/WebCore/animation/ViewTimelineOptions.h: * Source/WebCore/animation/ViewTimelineOptions.idl:
graouts
added a commit
to graouts/WebKit
that referenced
this issue
Jan 13, 2025
…meline-inset.html is a failure https://bugs.webkit.org/show_bug.cgi?id=285750 rdar://142690868 Reviewed by NOBODY (OOPS!). Our support for view timeline insets was very rudimentary. While we fixed the most egregious issue in 288684@main, we still had many issues left as shown in the failures in the relevant WPT test. The main task was to only convert inset values provided via the JS bindings to `CSSPrimitiveValue` such that we may resolve the insets when the view timeline's current time is being recomputed, thus correctly handling any relative value. Values provided via CSS are already fully resolved and are used as-is. We also implement the behavior where an `"auto"` values means that the values provided through the `scroll-padding` CSS property on the scroll container should be used. Additionally, we now raise exceptions for forbidden value types and units. We also switched the IDL type for `ViewTimelineInsets.inset` to use a `CSSKeywordish` rather than `CSSKeywordValue` alone so that strings would be allowed. This matches what Chrome implements and one of the cases tested in the test. The issue w3c/csswg-drafts#11477 was filed to track resolution of this discrepancy between the spec and WPT. * LayoutTests/imported/w3c/web-platform-tests/scroll-animations/view-timelines/view-timeline-inset-expected.txt: * Source/WebCore/animation/ViewTimeline.cpp: (WebCore::isValidInset): (WebCore::ViewTimeline::create): (WebCore::ViewTimeline::ViewTimeline): (WebCore::ViewTimeline::validateSpecifiedInsets): (WebCore::ViewTimeline::cacheCurrentTime): (WebCore::lengthForInset): Deleted. (WebCore::insetsFromOptions): Deleted. * Source/WebCore/animation/ViewTimeline.h: * Source/WebCore/animation/ViewTimeline.idl: * Source/WebCore/animation/ViewTimelineOptions.h: * Source/WebCore/animation/ViewTimelineOptions.idl:
webkit-commit-queue
pushed a commit
to graouts/WebKit
that referenced
this issue
Jan 13, 2025
…meline-inset.html is a failure https://bugs.webkit.org/show_bug.cgi?id=285750 rdar://142690868 Reviewed by Anne van Kesteren. Our support for view timeline insets was very rudimentary. While we fixed the most egregious issue in 288684@main, we still had many issues left as shown in the failures in the relevant WPT test. The main task was to only convert inset values provided via the JS bindings to `CSSPrimitiveValue` such that we may resolve the insets when the view timeline's current time is being recomputed, thus correctly handling any relative value. Values provided via CSS are already fully resolved and are used as-is. We also implement the behavior where an `"auto"` values means that the values provided through the `scroll-padding` CSS property on the scroll container should be used. Additionally, we now raise exceptions for forbidden value types and units. We also switched the IDL type for `ViewTimelineInsets.inset` to use a `CSSKeywordish` rather than `CSSKeywordValue` alone so that strings would be allowed. This matches what Chrome implements and one of the cases tested in the test. The issue w3c/csswg-drafts#11477 was filed to track resolution of this discrepancy between the spec and WPT. * LayoutTests/imported/w3c/web-platform-tests/scroll-animations/view-timelines/view-timeline-inset-expected.txt: * Source/WebCore/animation/ViewTimeline.cpp: (WebCore::isValidInset): (WebCore::ViewTimeline::create): (WebCore::ViewTimeline::ViewTimeline): (WebCore::ViewTimeline::validateSpecifiedInsets): (WebCore::ViewTimeline::cacheCurrentTime): (WebCore::lengthForInset): Deleted. (WebCore::insetsFromOptions): Deleted. * Source/WebCore/animation/ViewTimeline.h: * Source/WebCore/animation/ViewTimeline.idl: * Source/WebCore/animation/ViewTimelineOptions.h: * Source/WebCore/animation/ViewTimelineOptions.idl: Canonical link: https://commits.webkit.org/288795@main
|
I remember from the discussion when we added support for strings the main reason was to allow passing CSS style strings directly for which I imagine you'd pass multiple values space separated: "auto auto". See #7748 (comment) However, I don't think there's much additional complexity to supporting parsing a string for each argument, so I think making this change is reasonable. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
The
insetproperty of theViewTimelineOptionsis currently defined as(DOMString or sequence<(CSSNumericValue or CSSKeywordValue)>) inset = "auto";. This means that setting that value to"auto"is valid but not setting it to["auto", "auto"].I suggest we use
CSSKeywordishinstead ofCSSKeywordValueso that strings can be used as well asCSSKeywordValue. As it turns out, this is already what is being tested in scroll-animations/view-timelines/view-timeline-inset.html and what Chromes implements.The text was updated successfully, but these errors were encountered: