cssstyle icon indicating copy to clipboard operation
cssstyle copied to clipboard

Migrate `CSSStyleDeclaration` to WebIDL2JS

Open ExE-Boss opened this issue 5 years ago • 7 comments

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)

ExE-Boss avatar Apr 01 '20 16:04 ExE-Boss

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.

codecov-io avatar Apr 11 '20 02:04 codecov-io

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.

ExE-Boss avatar Apr 11 '20 03:04 ExE-Boss

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

TimothyGu avatar Apr 11 '20 16:04 TimothyGu

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.

TimothyGu avatar Apr 11 '20 17:04 TimothyGu

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.

jsakas avatar Apr 13 '20 16:04 jsakas

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?

cdoublev avatar Jun 10 '21 19:06 cdoublev