search

jsdom / cssstyle

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

Rewrite #140

Closed cdoublev closed 2 years ago

cdoublev commented 3 years ago

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):

codecov-commenter commented 3 years ago

Codecov Report

Merging #140 (ae7da59) into master (b527ed7) will increase coverage by 51.16%. The diff coverage is 91.61%.

:exclamation: Current head ae7da59 differs from pull request most recent head 3ae3298. Consider uploading reports for the commit 3ae3298 to get more accurate results Impacted file tree graph

@@             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
Impacted Files Coverage Δ
lib/allExtraProperties.js 100.00% <ø> (ø)
lib/properties/azimuth.js 4.44% <2.32%> (+4.44%) :arrow_up:
lib/properties/fontWeight.js 21.42% <16.66%> (+21.42%) :arrow_up:
lib/properties/borderCollapse.js 33.33% <25.00%> (+33.33%) :arrow_up:
lib/properties/fontStyle.js 33.33% <28.57%> (+33.33%) :arrow_up:
lib/properties/fontVariant.js 33.33% <28.57%> (+33.33%) :arrow_up:
lib/properties/lineHeight.js 33.33% <28.57%> (+33.33%) :arrow_up:
lib/properties/borderSpacing.js 69.23% <66.66%> (+69.23%) :arrow_up:
lib/properties/font.js 72.72% <70.00%> (+72.72%) :arrow_up:
lib/properties/clip.js 88.46% <78.57%> (+88.46%) :arrow_up:
... and 100 more

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...3ae3298. Read the comment docs.

ghost commented 2 years ago

@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.)

cdoublev commented 2 years ago

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 <position>, etc.

A few days ago, I started thinking about how to get the best chances to get a rewrite of CSSStyleDeclaration accepted on the JSDOM repo, but I'm afraid that this would be a task even harder than all what I already did... :)

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).

domenic commented 2 years ago

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.

cdoublev commented 2 years ago

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 style.width = { toString: () => '100px' } (valid CSS declaration) work in the tests of another project (JS implementation of Web Animations) that are running in a JSDOM environment, therefore to make this declaration work in this library. This led me to discover its issues with "common" CSS syntaxes, then to realize that jsakas does not have anymore the time to manage this library, and also that a complete rewrite is needed to support some CSS syntaxes. I also think that improving CSS support in JSDOM is not a priority for its users, because most often their tests can workaround an unsupported syntax with an alternative syntax.

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 webidl2js internally.

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 style.width = { toString: () => '100px' }.

I was also aware of Zirro/css-object-model and its use of CSSTree, which came out of my mind afterwards. I believe it was after I finished the backtracking mechanism of the CSS parser that I get another look at CSSTree. But after spending about 1 hour reviewing it, I concluded that it is great for parsing and transforming CSS, but not for processing it like a user agent should, by identifying longhand property values in a shorthand value, by handling property specific rules, by resolving math functions, default values, etc... maybe this is one of the reasons why Zirro / css-object-model never got finished, and maybe it was this conclusion that got me out of my head CSSTree the first time.

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.

cdoublev commented 2 years ago

Already 2 weeks since my previous comment... I just created the Github repository for my rewrite of cssstyle in its near current state, so I'm closing this PR.

By replacing cssstyle now, it would already provide better CSS support in jsdom and allow several issues to be closed, but I think the right time to replace cssstyle will be after some (still, sorry) remaining tasks are done. It is mainly about:

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. @webref/css

This rewrite creates an abstract syntax tree to represent the grammar of a property whose value is specified as input. The definitions (grammars) of the CSS properties and types are imported from @webref/css.

The main issue is that in specifications, CSS type definitions are not always defined with the CSS syntax or the markup required to be extracted by @webref/css and/or transformed into an abstract syntax tree. Another issue is that some properties and types are defined in different specifications with different values.

Because @webref/css is semi-automatically updated after some changes to the specs are published, it seems safer to define a fixed version number in the dependencies. I am trying to transition to its latest version from a previous version that is only a few months old, and it is not easy.

2. Implement more property/type specific rules

While @webref/css is of great help, the specific rules for parsing and serializing some properties and types require a specific implementation. Several types and properties are already parsed and serialized conforming to these rules: terminal types, <color>, <position>, <gradient>, border,background, margin, etc., but there are many more.

This task seems appropriate for external contributions. Which means that some documentation is needed. Writing this documentation is an underlying task that I did not add to the above list.

3. Code cleanup

Some parts of this rewrite are residues of cssstyle or this PR. There are certainly some parts for which I did not get an opportunity to get a review and find optimizations or issues to be fixed. There are also some other parts resulting from back and forth while implementing a feature and for which a clean up requires a special attention.

4. Implement the full CSS interface

CSS is not limited to CSSStyleDeclaration. I think the implemented procedures represent a significative part of what is required to implement the whole CSS object model (rules, queries, imports, stylesheet, etc.), and doing this before an integration to jsdom could prevent issues that would be otherwise discovered while transitioning from a partial to a full CSS implementation.

Another painfull task on which I worked the last few days was about an issue related to recursive backtracking. NodeJS and most browser vendors do not implement tail call optimization. When a CSS value should be matched against a CSS grammar allowing many combinations, recursive backtracking can lead to exceeding the maximum call stack size. Eg. the grammar for background involves 7! (5040) permutations when all its longhands are specified in input, and 6! + 5! + ... (873) (8659) more when some are omitted (total = 13699). The workaround is a trampoline but matching a value for this property is still very slow. The rewrite does not include them, but I found solutions to optimize parsing these kind of grammars.

I will update this thread while progressing on these tasks.

cdoublev commented 2 years ago

EDIT: nevermind, I found a clean solution to return a CSSStyleSheet() from both parse a stylesheet and parse a CSS stylesheet, and a CSSRule subclass from parse a CSS rule. I hope to finish the implementation with a minimum of unit tests next week, before trying to integrate the WPTs, the ESLint rules mentioned above, forking jsdom and prepare a PR, writing some docs, etc...

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 webidl2js.

A CSSStyleSheet is created from the result (CSS rules) of parsing a CSS stylesheet as a string or byte stream. I'm not sure if this result should already be instances of classes inheriting CSSRule or simple representation objects to map to instances of the corresponding classes when creating the CSSStyleSheet, but CSSRule.parentStyleSheet should reference the instance of CSSStyleSheet. Reciprocally, the CSSStyleSheet should reference the CSSRule instances via CSSStyleSheet.cssRules (a getter of CSSRuleList that shares the private list of rules). Finally, CSSStyleSheet.insertRule() appends a new instance of a CSSRule subclass in CSSStyleSheet.cssRules, and the same method may exist for some CSSRule subclasses such as CSSStyleRule or CSSKeyframesRule, which are "intermediary" parent classes of CSSNestingRule or CSSKeyframeRule.

Currently, my implementation of parseCSSStyleSheet(), whose corresponding procedure does not define its return value, does not return a CSSStyleSheet: the user has to run CSSStyleSheet.create(globalThis, undefined, { cssRules: parseCSSStyleSheet() }). I can successfully parse a CSS stylesheet and get the expected tree of CSS objects, but I'm struggling to implement CSSStyleSheet.insertRule() because of circular dependency issues I already had when trying to return a CSSStyleSheet from parseCSSStyleSheet().

Can you see how to create a CSSRule subclass from (the result of) parseCSSRule(), the last statement that is run for CSSStyleSheet.insertRule(), to avoid a circular dependency?

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))
  }
}
cdoublev commented 2 years ago

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 (Element.querySelector()) is currently handled with nwsapi in jsdom. There are rules such as:

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 w3c/csswg-drafts repo, some of which have been around for months or even years. I also reported several issues myself.

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 w3c/csswg-drafts, and above all, to avoid announcing a delay that I may not be able to honor.

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 @font-face, which seems rather complex to implement, or of @import, for which I have not yet decided the behavior to implement. Ideally, the referenced style sheet should be fetched, knowing that fetch will soon be supported in NodeJS.

Experimental features or features not implemented or partially implemented in browsers, such as @layer, @custom-query, new color functions, etc (the list is very large), will have to wait.