Rewrite #140
Rewrite #140
Conversation
Co-authored-by: Timothy Gu <[email protected]>
Codecov Report
@@ Coverage Diff @@
## master #140 +/- ##
===========================================
+ Coverage 37.39% 88.55% +51.16%
===========================================
Files 87 95 +8
Lines 1182 2001 +819
Branches 227 465 +238
===========================================
+ Hits 442 1772 +1330
+ Misses 633 200 -433
+ Partials 107 29 -78
Continue to review full report at Codecov.
|
cdoublev
force-pushed
the
rewrite
branch
2 times, most recently
from
b0c39bb
to
363fd1d
Compare
cdoublev
force-pushed
the
rewrite
branch
6 times, most recently
from
1986e57
to
d51e8f0
Compare
cdoublev
force-pushed
the
rewrite
branch
2 times, most recently
from
33adb04
to
57799cc
Compare
cdoublev
force-pushed
the
rewrite
branch
2 times, most recently
from
cd66949
to
feebfaa
Compare
cdoublev
force-pushed
the
rewrite
branch
2 times, most recently
from
9878bec
to
3ae3298
Compare
This fork merges jsdom/cssstyle#140.
|
@cdoublev: I’m not a JSDOM maintainer or anything, but I just really wanted to say that I really appreciate your effort into this PR! I have decided to take a read on your changes, and I have been thinking: Wouldn’t it make more sense to parse the values into some kind of AST? And the apply some form of normalization to them depending on the property name. You might even be able to use something like the CSS typed OM for it. I think it’d be neat to have something like: string → list of tokens → CSS typed OM → normalized CSS typed OM I feel like that’d make more sense than using strings for everything, and it would also make sense to implement the typed OM in JSDOM. (Though of course the typed OM would need to be implemented in JavaScript. There is a library, but it doesn’t seem accurate enough, and also seems abandoned.) |
|
It would have made even more sense to rewrite the whole package and implement the procedures from CSS OM, CSS Syntax, and CSS Typed OM, indeed... which is what I did (95% done). It was a lot more work than I expected, but it is absolutely necessary eg. for parsing the new CSS math functions, backtracking while parsing some complex grammars like A few days ago, I started thinking about how to get the best chances to get a rewrite of Anyway, I should also note that I'm aware that some things are wrong in this PR. My plan was to close it after the work on my rewrite would be done (max. 2 weeks), and to share a link to the corresponding repo so that people can use it for their own needs (JSDOM or any other context). |
|
Hey folks, dropping in here as, I guess, the main jsdom maintainer, to give some background and ideas on the path forward. jsdom's CSS processing has always been offloaded to other packages, maintained outside the "core team" by various folks. The layering has always been a bit scattered, with a bit of code living in jsdom, a chunk in cssom, and a chunk in cssstyle. (We even had an attempted from-scratch rewrite at https://github.com/Zirro/css-object-model which never got finished.) The main constraint is maintainers. A while back, primary maintainership of cssstyle transitioned from chad3814 to jsakas. As part of that we moved the repository into this organization. It looks like recently jsakas hasn't had as much time to review and merge PRs, which is very understandable---I'm barely keeping up on the main jsdom repo itself, doing work in weekend spurts. It's a hard job. So that resulted in various ambitious PRs, like this one and #116, not getting merged. So, where do we go from here? Well, I guess the realization I'm having is that the reason we transitioned the repo into the jsdom org, is to give the jsdom team the ability to appoint new maintainers. And given the intense effort being invested by @cdoublev, I'd love to nominate them, if they accept. That is, give them write permissions to this repository, and let them be the new person driving the cssstyle npm package and its jsdom integration. Here are some concrete ideas for what that would probably look like:
Here are some suggestions (not requirements) for how to run the cssstyle repository:
Anyway, let me know if this sounds like a good plan, and I'm happy to give you write and publish access. |
|
Thank you Domenic, I'm aware of the personal time investment that JSDOM represents for you. I didn't expect to be read by you here, honestly. My initial motivation was to make Despite this, I started from scratch an implementation strictly based on the relevant CSS specs, and I have to say that I spent more time on this task than would have been reasonable. This is the reason why my plan (as mentioned above) was to complete this implementation, publish it on Github under the MIT license, and leave a comment here inviting everyone to make further progress on the goal of improving CSS support in JSDOM. The goal of this plan for me is to restore my personal time. If no JSDOM user/maintainer takes hold of the "collective" goal, I will invest my personal time again to grasp the integration of WPTs tests, which I look at quite often, which I have already installed but which I failed to run (the result of a brief attempt). Another remaining step indeed seems to consolidate a complete implementation of the CSS interface into a single library, obviously using I already had a brief knowledge of the CSS processing layers in JSDOM because I had to fork it to replace this library by a fork providing support for I was also aware of Anyway, I haven't covered all topics but to answer you, I think it would be better to wait until my own rewrite is actually finished, I can measure more precisely the remaining steps, and take a step back on my capacity to handle them as an official maintainer. |
|
Already 2 weeks since my previous comment... I just created the Github repository for my rewrite of By replacing
So why have I published this rewrite now? Because I declared this intent in my previous comment but also because it can now be used by someone able to progress faster than me who wants to use it for its own rewrite. 1.
|
|
EDIT: nevermind, I found a clean solution to return a Domenic, I've successfully implemented almost the whole CSS interface, but there is an architectural issue that I'm struggling with since almost 2 weeks and that is partially related to A Currently, my implementation of Can you see how to create a Below is a (extremely) simplified representation of the dependencies between the required parts. /**
* 1. Requiring parseCSSStyleSheet() requires ...
* 2. ... CSSStyleSheet, which requires ...
* 3. ... CSSStyleSheetImpl, which requires ...
* 4. ... CSSRuleSubClass, which requires ...
* 5. ... CSSRuleSubClassImpl, which requires ...
* 6. ... parseCSSRule(), which requires CSSRuleSubClass (repeat from step 4)
*/
function parseCSSStyleSheet(input) {
// ... parse input against CSS grammar
return CSSStyleSheet.create(document, undefined, { cssRules: input })
}
// Note: only the first argument is defined by the corresponding procedure in CSS Syntax
function parseCSSRule(input, parentStyleSheet, parentRule) {
// ... parse input against CSS grammar
return CSSRuleSubClass.create(document, undefined, { ...input, parentStyleSheet, parentRule })
}
class CSSRuleSubClassImpl extends CSSRuleImpl {
constructor(globalObject, args, { parentStyleSheet, parentRule, ...privateData }) {
this.parentStyleSheet
this.parentRule
}
insertRule(input) {
this.cssRules.push(parseCSSRule(input), this.parentStyleSheet, this.parentRule)
}
}
// webidl2js wrapper
const CSSRuleSubClass = {
create: (...args) => new CSSRuleSubClassImpl(...args)
}
const typeMap = {
CSSRuleSubClass,
}
class CSSStyleSheetImpl {
constructor(globalObject, args, { cssRules }) {
this.cssRules = cssRules.map(rule =>
typeMap[rule.type].create(globalObject, undefined, { ...rule, parentStyleSheet: this }))
}
insertRule(input) {
this.cssRules.push(parseCSSRule(input, this))
}
}
// webidl2js wrapper
const CSSStyleSheet= {
create: (...args) => new CSSStyleSheetImpl(...args)
}This is the best workaround I got for now: // cssom.js
module.exports = {
CSSStyleSheet: require('./cssom/CSSStyleSheet.js'),
CSSRule: require('./cssom/CSSRule.js'),
// ...
}
// interface.js
const { CSSStyleSheet, ...cssom } = require('./cssom.js')
function parseCSSStyleSheet(input) {
// ...
return CSSStyleSheet.create(document, undefined, { cssom, cssRules: input })
}
function parseCSSRule(input, parentStyleSheet, parentRule) {
// ...
return cssom[input.type].create(document, undefined, { ...input, parentStyleSheet, parentRule })
}
// cssom/CSSStylesheet-impl.js
class CSSStyleSheetImpl {
constructor(globalObject, args, { cssom, cssRules, ...privateData }) {
// ...
this.cssRules = cssRules.map(rule =>
cssom[rule.type].create(globalObject, undefined, { cssom, parentStyleSheet: this, ...rule })
}
insertRule(input) {
this.cssRules.push(parseCSSRule(input, this))
}
}
// cssom/CSSRuleSubClass-impl.js
class CSSRuleSubClassImpl {
constructor(globalObject, args, { cssom, parentStyleSheet, ...privateData }) {
// ...
this.cssRules = cssRules.map(rule =>
cssom[rule.type].create(globalObject, undefined, { cssom, parentRule: this, ...rule })
}
insertRule(input) {
this.cssRules.push(parseCSSRule(input, this.parentStyleSheet, this))
}
} |
|
Hi here, I'm sorry because:
I had quite a hard time with parsing selectors. Domenic may somewhat know what I'm talking about, even if matching selector (
These rules required significant changes, which are visible in this commit. The W3C team does an amazing job with developing and maintaining all these specs, imho. But some CSS specs still have parts that are rather nebulous, to put it mildly. Many issues are reported on the The length of my todo list is still pretty demoralizing. This is one of the reason I do not publish it, and it is for this reason that I prefered to push "big" commits like the one referenced above, to spend less time reporting issues on I am aware that the deprecation date of NodeJS v12 is scheduled for April 22. Aiming for that date gives me time to fix the remaining issues and implement the missing CSS features supported in all browsers. I am thinking in particular of Experimental features or features not implemented or partially implemented in browsers, such as |
CSSStyleDeclarationto WebIDL2JS #116var()and computedcalc()both with nestedcalc()andvar()(related: WIP: Implement calc #101, makes Enable vars and nested calc&var #127 and keep property definition using calc and var #141 obsolete)'src()'Please follow the commits to review this pull request. You can check that tests pass for each one, or that a commit that adds a new test is fixed by the next commit. You may have to reload the test script and/or reinstall dependencies.
Other fixed issues (non-exhaustive):
1e1and1e+1should be10instead ofundefined1e-1should be0.1instead ofundefined.10unitshould be0.1unitinstead of0.1unit1Unitshould be1unitinstead ofundefined0should be0deginstead ofundefinedurl(),url( ),url(""), should beundefinedinstead ofurl()url(val\\\nid.url)should beundefinedinstead ofurl(val\\\nid.url)url(file\\0 \\g.jpg)should beurl(file\\0\\g.jpg)instead ofundefinedurl(file.jpg)andurl('file.jpg')should beurl("file.jpg")instead ofurl(file.jpg)"\n"should beundefinedinstead of"\n"'string'should be"string"instead of'string'#1234567should beundefinedinstead ofrgb(18, 52, 86)#ffffffffshould bergb(255, 255, 255)instead ofrgba(255, 255, 255, 1)REDshould beredinstead ofREDRGb(0, 0, 0)should bergb(0, 0, 0)instead ofundefinedrgb(300, 300, 300, 2)should bergb(255, 255, 255)instead ofundefinedrgb(-1, -1, -1, -1)should bergba(0, 0, 0, 0)instead ofundefinedhsl(540, 100%, 50%)should bergb(0, 255, 255)instead ofrgb(0, 0, 0)hsla(400, 200%, 200%, 200%)should bergb(255, 255, 255)instead ofundefinedhsla(-1, -1%, -1%, -1%)should bergba(0, 0, 0, 0)instead ofundefinedrgba(245.5, 245.5, 0, 50.1%)should bergba(246, 246, 0, 0.5)instead ofundefinedrgba(0, 0, 0, 49.9%)should bergba(0, 0, 0, 0.498)instead ofundefinedrgba(5%, 10%, 20%, 0.4)should bergba(13, 26, 51, 0.4)instead ofrgba(12, 25, 51, 0.4)rgb(33%, 34%, 33%)should bergb(84, 87, 84)instead ofrgb(84, 86, 84)<position>)sideshould beundefinedinstead ofside1should beundefinedinstead of1left leftshould beundefinedinstead ofleft lefttop topshould beundefinedinstead oftop topleft top centershould beundefinedinstead ofleft top centerleft top 50%should beundefinedinstead ofleft top 50%top 50%should beundefinedinstead oftop 50%50% leftshould beundefinedinstead of50% left0 0should be0px 0pxinstead of0 00%should be0% centerinstead of0%leftshould beleft centerinstead oflefttopshould becenter topinstead oftopcentershould becenter centerinstead ofcentertop leftshould beleft topinstead oftop left