Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Migrate CSSStyleDeclaration to WebIDL2JS #116

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

ExE-Boss
Copy link

@ExE-Boss ExE-Boss commented Apr 1, 2020

This migrates CSSStyleDeclaration to use WebIDL2JS.

Part of jsdom/jsdom#2727.


There’s also jsdom/webidl2js#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)

Copy link
Member

@TimothyGu TimothyGu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work! Can't comment on the CSS parts too much, but the Web IDL stuff looks solid. I do of course defer to @jsakas for a full review.

lib/CSSStyleDeclaration-impl.js Outdated Show resolved Hide resolved
scripts/convert-idl.js Outdated Show resolved Hide resolved
scripts/convert-idl.js Outdated Show resolved Hide resolved
lib/parsers.js Outdated Show resolved Hide resolved
scripts/convert-idl.js Outdated Show resolved Hide resolved
scripts/convert-idl.js Outdated Show resolved Hide resolved
src/CSSStyleDeclaration.webidl Show resolved Hide resolved
lib/CSSStyleDeclaration-impl.js Outdated Show resolved Hide resolved
lib/CSSStyleDeclaration-impl.js Outdated Show resolved Hide resolved
lib/CSSStyleDeclaration.test.js Outdated Show resolved Hide resolved
@codecov-io
Copy link

codecov-io commented Apr 11, 2020

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%) ⬇️
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 ExE-Boss force-pushed the feat/migrate-to-webidl branch 3 times, most recently from 56a48b5 to c3d2de8 Compare April 11, 2020 03:33
@ExE-Boss
Copy link
Author

ExE-Boss commented Apr 11, 2020

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 ExE-Boss requested a review from TimothyGu April 11, 2020 03:37
@TimothyGu
Copy link
Member

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

Copy link
Member

@TimothyGu TimothyGu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you also write some documentation on the new setup? Two things in particular: the webidl2js-wrapper, and the fact that the default CSSStyleDeclaration export takes a parameter (but not the one exposed by webidl2js-wrapper).

index.js Outdated Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
lib/CSSStyleDeclaration-impl.js Outdated Show resolved Hide resolved
scripts/convert-idl.js Outdated Show resolved Hide resolved
scripts/convert-idl.js Outdated Show resolved Hide resolved
scripts/generate_implemented_properties.js Outdated Show resolved Hide resolved
lib/CSSStyleDeclaration-impl.js Outdated Show resolved Hide resolved
@TimothyGu
Copy link
Member

TimothyGu commented Apr 11, 2020

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.

@ExE-Boss ExE-Boss requested a review from TimothyGu April 11, 2020 19:44
index.js Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@ExE-Boss ExE-Boss requested a review from TimothyGu April 12, 2020 09:04
Copy link
Member

@TimothyGu TimothyGu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good as far as I’m concerned. I’ll leave @jsakas to do a final pass and merge however.

scripts/download_latest_properties.js Outdated Show resolved Hide resolved
scripts/generate_implemented_properties.js Outdated Show resolved Hide resolved
@jsakas
Copy link
Member

jsakas commented Apr 13, 2020

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
Copy link

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
Copy link

⚠️ 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 ⚠️
lib/parsers.js 78.94% 3 Missing and 1 partial ⚠️
lib/properties/borderSpacing.js 0.00% 0 Missing and 1 partial ⚠️

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

Additional details and impacted files
@@            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              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@ExE-Boss ExE-Boss requested a review from TimothyGu July 24, 2024 23:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CSSStyleDeclaration should support camelCase getter "webkit-*" vs "-webkit-*"
6 participants