cssstyle
cssstyle copied to clipboard
Migrate `CSSStyleDeclaration` to WebIDL2JS
This migrates CSSStyleDeclaration to use WebIDL2JS.
Part of https://github.com/jsdom/jsdom/issues/2727.
There’s also https://github.com/jsdom/webidl2js/issues/188, which would simplify the generation of CSSStyleDeclaration‑properties.webidl by allowing the resulting string to be passed to WebIDL2JS directly, instead of having to write it to a temporary file.
- Fixes https://github.com/jsdom/cssstyle/issues/70
- Supersedes and closes https://github.com/jsdom/cssstyle/pull/112
- Supersedes and closes https://github.com/jsdom/cssstyle/pull/166
- Fixes https://github.com/jsdom/jsdom/issues/2945
review?(@jsakas, @TimothyGu)
Codecov Report
Merging #116 (c6b72e0) into master (b527ed7) will decrease coverage by
1.37%. The diff coverage is89.10%.
@@ Coverage Diff @@
## master #116 +/- ##
==========================================
- Coverage 37.39% 36.01% -1.38%
==========================================
Files 87 86 -1
Lines 1182 1155 -27
Branches 227 229 +2
==========================================
- Hits 442 416 -26
+ Misses 633 632 -1
Partials 107 107
| Impacted Files | Coverage Δ | |
|---|---|---|
| lib/allExtraProperties.js | 100.00% <ø> (ø) |
|
| lib/properties/borderSpacing.js | 0.00% <0.00%> (ø) |
|
| lib/parsers.js | 80.09% <78.94%> (-0.55%) |
:arrow_down: |
| lib/CSSStyleDeclaration-impl.js | 92.50% <92.50%> (ø) |
|
| lib/allWebkitProperties.js | 100.00% <100.00%> (ø) |
Continue to review full report at Codecov.
Legend - Click here to learn more
Δ = absolute <relative> (impact),ø = not affected,? = missing dataPowered by Codecov. Last update b527ed7...c6b72e0. Read the comment docs.
The build passed, but it’s not reflected on GitHub due to cssstyle using the legacy service‑based Travis‑CI.org instead of the GitHub App‑based Travis‑CI.com.
travis-ci.com seems to have been down for maintenance.
Also an interesting question would be do we want to add some custom reflection support for CSS attributes? I think it might increase memory usage, but would allow us to get rid of the getter/setter methods on CSSStyleDeclarationImpl.prototype. Would be interesting to experiment.
Looks good as far as I’m concerned. I’ll leave @jsakas to do a final pass and merge however.
Thanks guys for all your work here. Diving into this PR this week.
Hello,
I created #140 before truly understanding this morning what this PR was about. The initial goal of my PR was to convert values passed to an interface of CSSStyleDeclaration into CSSOMString, when that interface expected such type. But this PR already handles that.
My PR fixes many other issues and implements support for several CSS properties and value types. So I allowed myself to rebase my PR on this one. I only kept my tests related to the conversion to CSSOMString.
I would like to add a few notes here, some of which I have already made in other places.
1/ I'm not sure I understand why lib/properties.js should be generated (and transformed by babel). In my PR, I removed this build step because it prevented me from using a "higher order setter" for CSSOMString conversion, but also to handle globally (for all properties) empty string and CSS wide keywords as an initial parsing step, before passing the CSS value to the CSS property setter.
2/ The way CSS values are parsed in this library does not conform to the CSSOM specification and especially the CSS Syntax specification, ie. a "global" parsing step rather than lexing (tokenizing), consuming tokens for parsing, and serializing tokens. A "global" parse/serialize strategy is easier to implement and perhaps more efficient, but it may be preferable and even necessary to conform to those specifications before being forced to a large rewrite. This would also make it possible to avoid using the "higher order setter" mentioned in the first point.
3/ This PR is open since more than 1 year. Would it be possible to have this PR and/or my PR reviewed by someone else? AS noted above, it fixes many jsdom and cssdom reported issues, an many others which have not been reported yet. My motivation for contributing to this library was to be able to use objects implementing toString() as CSS values, and to allow calc() (computed or not) as value and value component, in the jest tests of an animation library. The only way to get these two features now seems to fork jsdom, because HTMLElement.style is created with new cssstyle.CSSStyleDeclaration(). Can you see a "light" alternative workaround?