[scroll-animations-1] Use CSSTypedOM instead of strings (#5213) by majido · Pull Request #5300 · w3c/csswg-drafts

majido · 2020-07-08T19:14:51Z

Use appropriate CSSTypedOM instead of DOMStrings in the API. Fixes #5213.

Changes:

For offsets use CSSNumericValue or CSSKeywordish instead of DOMString
When setting the offset, the grammars for these values is check to ensure only the valid subset can be set.
When computing effective scroll offset check the offset actual type and interpret it accordingly
Update examples to showcase the API and add noted to clarify the usage.

andruud · 2020-07-09T11:10:16Z


-    :   <var>offset</var> is a <<length-percentage>>
+    :   If <var>offset</var> is a {{CSSNumberish}} and matches <<number>>:
+    ::  The distance indicated by the value in <a value>pixel unit</a> along


I was under the impression that treating plain numbers as px was a pattern we didn't want? Maybe get someone else's opinion on this. (@tabatkins ?)

I personally felt allowing numbers to be interpreted as px would make the API easier to use in Javascript. Specially as it matches all existing scroll related APIs where numbers are treated as pixels

But I can also see at least two possible concerns:

Automatic type conversion can be a footgun but perhaps this is limited enough to be safe

Having every downstream spec convert <numbers> differently would be bad. So perhaps have a common way to do this i.e., <number> -> <length> with pixel for all JS API's that accept length but want to accept number too.

BTW, does TypedOM have a special handling for 0 length for which in CSS the unit is optional?

In any case, this is a trade-off and @tabatkins expert's opinion is appreciated. :)

We really don't want to interpret bare numbers as px. The definition of CSSNumberish interprets numeric values as a CSS "number" not a length. offset should simply be a CSSNumericValue or a CSSStyleValue.

Done. I removed CSSNumberish and just use CSSNumericValue which I think is the right type.

Also added a procedure to validate the incoming values when they are being set.

andruud · 2020-07-09T11:10:36Z

+        {{source}}'s scroll range in {{orientation}}.
+
+
+    :   If <var>offset</var> is a {{CSSNumberish}} and matches


Can we link "matches" to https://drafts.css-houdini.org/css-typed-om-1/#cssstylevalue-match-a-grammar ?

andruud · 2020-07-09T11:13:44Z

 <pre class="idl">
 enum ScrollTimelineAutoKeyword { "auto" };

+typedef (CSSNumberish or ScrollTimelineAutoKeyword) ContainerBasedOffset;


We should IMO also accept CSSKeywordValue matching auto.

Would you want to use CSSKeywordish here (if it existed)?

My bad, CSSKeywordish already exists: https://drafts.css-houdini.org/css-typed-om-1/#typedefdef-csskeywordish

Do we want to use that somehow, then?

Done. As part of this I now have a procedure that rectifies strings into CSSKeyword and rejects invalid values at the time of setting the value.

andruud

👍

The spec changed a while ago to use CSSNumericValue/CSSKeywordish for specifying container-based offsets [1]. This CL makes that switch. [1] w3c/csswg-drafts#5300 Bug: 1109769 Change-Id: I1d14d68008098e39035aecba940c0e02645f9d99

The spec changed a while ago to use CSSNumericValue/CSSKeywordish for specifying container-based offsets [1]. This CL makes that switch. [1] w3c/csswg-drafts#5300 Bug: 1109769 Change-Id: I1d14d68008098e39035aecba940c0e02645f9d99 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2390740 Commit-Queue: Anders Hartvoll Ruud <andruud@chromium.org> Reviewed-by: Yuki Shiino <yukishiino@chromium.org> Reviewed-by: Kevin Ellis <kevers@chromium.org> Cr-Commit-Position: refs/heads/master@{#806346}

…o specify offsets, a=testonly Automatic update from web-platform-tests [scroll-animations] Use CSSStyleValues to specify offsets The spec changed a while ago to use CSSNumericValue/CSSKeywordish for specifying container-based offsets [1]. This CL makes that switch. [1] w3c/csswg-drafts#5300 Bug: 1109769 Change-Id: I1d14d68008098e39035aecba940c0e02645f9d99 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2390740 Commit-Queue: Anders Hartvoll Ruud <andruud@chromium.org> Reviewed-by: Yuki Shiino <yukishiino@chromium.org> Reviewed-by: Kevin Ellis <kevers@chromium.org> Cr-Commit-Position: refs/heads/master@{#806346} -- wpt-commits: c6ac22678d95c1a8414656822ff46d05ff0d1fed wpt-pr: 25366

…o specify offsets, a=testonly Automatic update from web-platform-tests [scroll-animations] Use CSSStyleValues to specify offsets The spec changed a while ago to use CSSNumericValue/CSSKeywordish for specifying container-based offsets [1]. This CL makes that switch. [1] w3c/csswg-drafts#5300 Bug: 1109769 Change-Id: I1d14d68008098e39035aecba940c0e02645f9d99 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2390740 Commit-Queue: Anders Hartvoll Ruud <andruudchromium.org> Reviewed-by: Yuki Shiino <yukishiinochromium.org> Reviewed-by: Kevin Ellis <keverschromium.org> Cr-Commit-Position: refs/heads/master{#806346} -- wpt-commits: c6ac22678d95c1a8414656822ff46d05ff0d1fed wpt-pr: 25366 UltraBlame original commit: ae84065cb5cf0390e0c328afcc132056e3943776

…o specify offsets, a=testonly Automatic update from web-platform-tests [scroll-animations] Use CSSStyleValues to specify offsets The spec changed a while ago to use CSSNumericValue/CSSKeywordish for specifying container-based offsets [1]. This CL makes that switch. [1] w3c/csswg-drafts#5300 Bug: 1109769 Change-Id: I1d14d68008098e39035aecba940c0e02645f9d99 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2390740 Commit-Queue: Anders Hartvoll Ruud <andruud@chromium.org> Reviewed-by: Yuki Shiino <yukishiino@chromium.org> Reviewed-by: Kevin Ellis <kevers@chromium.org> Cr-Commit-Position: refs/heads/master@{#806346} -- wpt-commits: c6ac22678d95c1a8414656822ff46d05ff0d1fed wpt-pr: 25366

[scroll-animations] Use CSSTypedOM instead of strings (w3c#5213)

d6fe7b0

Fixes w3c#5213

majido requested a review from andruud July 8, 2020 19:14

Add case where offset is in the worng form

2651aba

majido force-pushed the use-typed-om branch from 813e931 to 2651aba Compare July 8, 2020 19:18

andruud reviewed Jul 9, 2020

View reviewed changes

Use CSSNumeric and don't convert <number> to pixel

9eccba5

andruud approved these changes Jul 9, 2020

View reviewed changes

majido merged commit 44bca0a into w3c:master Jul 9, 2020

chromium-wpt-export-bot mentioned this pull request Sep 2, 2020

[scroll-animations] Use CSSStyleValues to specify offsets web-platform-tests/wpt#25366

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[scroll-animations-1] Use CSSTypedOM instead of strings (#5213)#5300

[scroll-animations-1] Use CSSTypedOM instead of strings (#5213)#5300
majido merged 3 commits into
w3c:masterfrom
majido:use-typed-om

majido commented Jul 8, 2020 •

edited

Loading

Uh oh!

andruud Jul 9, 2020

Uh oh!

majido Jul 9, 2020 •

edited

Loading

Uh oh!

plinss Jul 9, 2020

Uh oh!

majido Jul 9, 2020

Uh oh!

andruud Jul 9, 2020

Uh oh!

majido Jul 9, 2020

Uh oh!

andruud Jul 9, 2020

Uh oh!

andruud Jul 9, 2020

Uh oh!

majido Jul 9, 2020

Uh oh!

andruud left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

		{{source}}'s scroll range in {{orientation}}.


		: If <var>offset</var> is a {{CSSNumberish}} and matches

Conversation

majido commented Jul 8, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

majido Jul 9, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andruud left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

majido commented Jul 8, 2020 •

edited

Loading

majido Jul 9, 2020 •

edited

Loading