search

flackr / scroll-timeline

A polyfill of ScrollTimeline.
Apache License 2.0
891 stars 84 forks source link

Implement CSSNumericValue.parse() #206

Closed johannesodland closed 5 months ago

johannesodland commented 5 months ago

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

bramus commented 5 months ago

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 commented 5 months ago

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 commented 5 months ago

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 commented 5 months ago

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 commented 5 months ago

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

johannesodland commented 5 months ago

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

bramus commented 5 months ago

Thanks!

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

flackr commented 5 months ago

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 commented 5 months ago

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).