Implement CSSNumericValue.parse() #206

johannesodland · 2024-01-21T15:51:01Z

Add proper tokenization and parsing for CSSNumericValue.parse().

test/expected.txt

bramus · 2024-01-26T18:19:25Z

I think we might have come at a point where it might be a good idea to split off the CSSOM polyfill into its own repo with its own tests? I mean, feels like this could become its own library.

johannesodland · 2024-01-26T18:45:21Z

I think we might have come at a point where it might be a good idea to split off the CSSOM polyfill into its own repo with its own tests? I mean, feels like this could become its own library.

It still limited to implementing the parts needed for this library.. But I’ve been thinking we should start by moving the implementation into its own folder to make it more explicit that the code is meant for polyfilling parts of the typed cssom.

As it is right now it could be tempting to use internal functions directly without going through the public api when implementing scroll driven animations. Then we would end up with too tight a coupling.

I can create a refactoring PR once the open PRs are merged.

bramus · 2024-01-26T23:33:12Z

I can create a refactoring PR once the open PRs are merged.

Yeah, sounds like a good plan.

Which PRs specifically – other than this one – definitely need to be merged?

johannesodland · 2024-01-27T09:41:59Z

Which PRs specifically – other than this one – definitely need to be merged?

I was wrong. I looked through the other PRs, and it looks like none of them are touching the cssom implementation, so we should be fine after this PR is in. We could do it as part of this PR as well, if that's preferable.

bramus · 2024-01-27T10:14:14Z

I think it’d be best to do this refactor as part of a separate PR.

bramus

Some minor nits to address (changes suggested).

src/simplify-calculation.js

src/numeric-values.js

johannesodland · 2024-02-02T16:51:27Z

@bramus Thank you for the review 🙏
I think I have addressed all your remarks, and have rebased onto master.

bramus · 2024-02-02T17:34:27Z

Thanks!

Pinging @flackr to take a look before merging. I think this one should go in before #221.

flackr

Wow, this is a lot! I'm a bit concerned about the size increase. We'll have to think about what parts of this could be removed for a reduced build. Also, do we skip this parsing code if the browser supports CSSStyleValue.parse?

src/numeric-values.js

src/tokenizer.js

flackr · 2024-02-02T19:30:37Z

Wow, this is a lot! I'm a bit concerned about the size increase. We'll have to think about what parts of this could be removed for a reduced build. Also, do we skip this parsing code if the browser supports CSSStyleValue.parse?

To be clear, I'm ecstatic to see such a precise implementation following the spec steps! I just think the extra kilobytes will be too much for some sites, and we'll want to have a light version of the polyfill where you have to construct the CSSOM value instead of parsing it from a string.

johannesodland · 2024-02-03T11:40:53Z

Wow, this is a lot! I'm a bit concerned about the size increase. We'll have to think about what parts of this could be removed for a reduced build. Also, do we skip this parsing code if the browser supports CSSStyleValue.parse?

To be clear, I'm ecstatic to see such a precise implementation following the spec steps! I just think the extra kilobytes will be too much for some sites, and we'll want to have a light version of the polyfill where you have to construct the CSSOM value instead of parsing it from a string.

Yeah, I know it's a lot 😅, and by all means we don't have to use it. It's been an interesting exercise :)
The parsing code should only be executed in browsers without CSSStyleValue.parse() support (Looking at you Firefox), but it does add 2.1kB to the compressed build.

If we had a build system that could build two targets, we could build one with a full implementation of CSSStyleValue.parse() and a light build with with a simple or no implementation of CSSStyleValue.parse(). That simple implementation could be limited to supporting CSSUnitValues.

The current solution though (that I'm at fault for), supports some math functions but can fail in more complex calculations. And there's no clear definition of what it will support or not. It also fails in browsers without regex lookahead support (Safari < 16.4).

flackr · 2024-02-03T15:10:52Z

src/numeric-values.js

+    return tokenizeString(input);
+  }
+  // Assert: Only the preceding types should be passed as input.
+  throw new TypeError(`Invalid input type git${typeof input}`)


I think the text "git" prepended before the type may have been unintentional?

Oh.. that's embarrassing 😅
I'm having an issue where the focus is delayed when switching between the IDE and the terminal 😞

Fixed in https://github.com/flackr/scroll-timeline/compare/8e7cf381430bd1d2c7511debec7d954ebaccaf66..4ed64d0898c93a488381f6ad35c42033b39698a8

flackr · 2024-02-03T15:18:37Z

src/tokenizer.js

+    let position = this.index
+    for (let i = 0; i < 3; i++) {
+      const nextCodePoint = this.input.codePointAt(position);
+      if (typeof nextCodePoint !== 'undefined') {


According to the MDN docs, codePointAt only returns undefined if the index is out of range. I think it would be cleaner to change the for loop condition to i < 3 && position < this.input.length, then if the loop runs we always expect it to find a codepoint.

Yeah.. That looks cleaner:
Fixed here: https://github.com/flackr/scroll-timeline/compare/8e7cf381430bd1d2c7511debec7d954ebaccaf66..4ed64d0898c93a488381f6ad35c42033b39698a8

src/tokenizer.js

Update src/simplify-calculation.js Co-authored-by: Bramus <[email protected]> Update src/simplify-calculation.js Co-authored-by: Bramus <[email protected]> Update src/numeric-values.js Co-authored-by: Bramus <[email protected]> Update src/numeric-values.js Co-authored-by: Bramus <[email protected]> Update src/numeric-values.js Co-authored-by: Bramus <[email protected]> Update src/numeric-values.js Co-authored-by: Bramus <[email protected]> Reify e and pi, rename to transformToCSSNumericValue and add jsdoc

flackr

Thanks for the changes, looks good to me!

johannesodland mentioned this pull request Jan 21, 2024

Prevent invalid-regular expression syntax error in Safari < 16.4 #202

Closed

bramus reviewed Jan 21, 2024

View reviewed changes

test/expected.txt Outdated Show resolved Hide resolved

johannesodland marked this pull request as ready for review January 22, 2024 17:09

johannesodland force-pushed the css-numeric-value-parser branch 4 times, most recently from 8bcc515 to e64a826 Compare January 29, 2024 18:04

bramus mentioned this pull request Feb 1, 2024

Handle CSSNumericValue instances while parsing #221

Merged

bramus requested changes Feb 2, 2024

View reviewed changes

johannesodland force-pushed the css-numeric-value-parser branch 3 times, most recently from d2a0a83 to ef9eb58 Compare February 2, 2024 16:34

johannesodland requested a review from bramus February 2, 2024 16:49

flackr reviewed Feb 2, 2024

View reviewed changes

src/numeric-values.js Show resolved Hide resolved

src/tokenizer.js Show resolved Hide resolved

src/tokenizer.js Outdated Show resolved Hide resolved

johannesodland force-pushed the css-numeric-value-parser branch 2 times, most recently from 20ae1ed to 8e7cf38 Compare February 3, 2024 13:19

flackr reviewed Feb 3, 2024

View reviewed changes

johannesodland force-pushed the css-numeric-value-parser branch from 8e7cf38 to 4ed64d0 Compare February 3, 2024 16:07

johannesodland mentioned this pull request Feb 3, 2024

Return "normal" for get rangeStart and rangeEnd #223

Merged

johannesodland force-pushed the css-numeric-value-parser branch from 4ed64d0 to cf22c2b Compare February 3, 2024 17:15

johannesodland force-pushed the css-numeric-value-parser branch from cf22c2b to 0ff6f35 Compare February 3, 2024 23:42

flackr approved these changes Feb 4, 2024

View reviewed changes

bramus merged commit d605aba into flackr:master Feb 4, 2024
2 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement CSSNumericValue.parse() #206

Implement CSSNumericValue.parse() #206

johannesodland commented Jan 21, 2024

bramus commented Jan 26, 2024

johannesodland commented Jan 26, 2024

bramus commented Jan 26, 2024

johannesodland commented Jan 27, 2024

bramus commented Jan 27, 2024

bramus left a comment

johannesodland commented Feb 2, 2024

bramus commented Feb 2, 2024

flackr left a comment

flackr commented Feb 2, 2024

johannesodland commented Feb 3, 2024

flackr Feb 3, 2024

johannesodland Feb 3, 2024

flackr Feb 3, 2024

johannesodland Feb 3, 2024

johannesodland Feb 3, 2024

flackr left a comment

Implement CSSNumericValue.parse() #206

Implement CSSNumericValue.parse() #206

Conversation

johannesodland commented Jan 21, 2024

bramus commented Jan 26, 2024

johannesodland commented Jan 26, 2024

bramus commented Jan 26, 2024

johannesodland commented Jan 27, 2024

bramus commented Jan 27, 2024

bramus left a comment

Choose a reason for hiding this comment

johannesodland commented Feb 2, 2024

bramus commented Feb 2, 2024

flackr left a comment

Choose a reason for hiding this comment

flackr commented Feb 2, 2024

johannesodland commented Feb 3, 2024

flackr Feb 3, 2024

Choose a reason for hiding this comment

johannesodland Feb 3, 2024

Choose a reason for hiding this comment

flackr Feb 3, 2024

Choose a reason for hiding this comment

johannesodland Feb 3, 2024

Choose a reason for hiding this comment

johannesodland Feb 3, 2024

Choose a reason for hiding this comment

flackr left a comment

Choose a reason for hiding this comment