search

jsdom / cssstyle

A Node.js implementation of the CSS Object Model CSSStyleDeclaration interface
MIT License
109 stars 70 forks source link

Migrate `CSSStyleDeclaration` to WebIDL2JS #116

Open ExE-Boss opened 4 years ago

ExE-Boss commented 4 years ago

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.

review?(@jsakas, @TimothyGu)

codecov-io commented 4 years ago

Codecov Report

Merging #116 (c6b72e0) into master (b527ed7) will decrease coverage by 1.37%. The diff coverage is 89.10%.

Impacted file tree graph

@@            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 data Powered by Codecov. Last update b527ed7...c6b72e0. Read the comment docs.

ExE-Boss commented 4 years ago

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.

TimothyGu commented 4 years ago

travis-ci.com seems to have been down for maintenance.

TimothyGu commented 4 years ago

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.

jsakas commented 4 years ago

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.

cdoublev commented 3 years ago

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?

codecov-commenter commented 4 months ago

:warning: Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

Attention: Patch coverage is 89.10891% with 11 lines in your changes missing coverage. Please review.

Project coverage is 36.01%. Comparing base (b527ed7) to head (c6b72e0). Report is 16 commits behind head on main.

Files Patch % Lines
lib/CSSStyleDeclaration-impl.js 92.50% 6 Missing :warning:
lib/parsers.js 78.94% 3 Missing and 1 partial :warning:
lib/properties/borderSpacing.js 0.00% 0 Missing and 1 partial :warning:

:exclamation: Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #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 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.