jsdom / cssstyle

A Node.js implementation of the CSS Object Model CSSStyleDeclaration interface

MIT License

109 stars 70 forks source link

fix: webkit properties not being recognized #112

Open eps1lon opened 4 years ago

eps1lon commented 4 years ago

Seems like they were never tested. I'm not sure if the two added tests can be fixed with independent commits.

codecov-io commented 4 years ago

Codecov Report

Merging #112 into master will decrease coverage by 0.31%. The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #112      +/-   ##
==========================================
- Coverage   37.39%   37.07%   -0.32%     
==========================================
  Files          87       87              
  Lines        1182     1176       -6     
  Branches      227      225       -2     
==========================================
- Hits          442      436       -6     
  Misses        633      633              
  Partials      107      107

Impacted Files	Coverage Δ
lib/allWebkitProperties.js	`100% <100%> (ø)`	:arrow_up:
lib/parsers.js	`80.35% <0%> (-0.3%)`	:arrow_down:

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 62b3b1b...c01df24. Read the comment docs.

jsakas commented 4 years ago

@eps1lon is this a regression we missed in the last PR that ended up being 2.2.0 - or new functionality?

eps1lon commented 4 years ago

@eps1lon is this a regression we missed in the last PR that ended up being 2.2.0 - or new functionality?

Tried the tests in 2.1.0 (git checkout v2.1.0) and they were failing as well. I couldn't find any tests related to webkit properties. I can try to create some tests related to webkit properties that are passing in 2.1.0.

domenic commented 4 years ago

Only webkit properties are in the spec; other engine prefixes should not be added.

eps1lon commented 4 years ago

Only webkit properties are in the spec; other engine prefixes should not be added.

This statement clashes with

https://github.com/jsdom/cssstyle/blob/ebd2dab41a3396419513a7698cdd37494727d0da/lib/allExtraProperties.js#L4-L5

eps1lon commented 4 years ago

@jsakas Is there anything I can do to get this merged?

jsakas commented 4 years ago

@eps1lon sorry for the delay here. Are you able to resolve the conflict for me? Also, based on @domenic's comment I think only webkit should be supported at this time.

eps1lon commented 4 years ago

Are you able to resolve the conflict for me?

Sure thing.

Also, based on @domenic's comment I think only webkit should be supported at this time.

Personally I would prefer if this also handled other prefixed properties. This is in line with https://github.com/jsdom/cssstyle/blob/ebd2dab41a3396419513a7698cdd37494727d0da/lib/allExtraProperties.js#L4-L5

But I understand if you want to change that stance to "we only support properties in the spec".

nyroDev commented 1 year ago

Hi @eps1lon, do you intend to fix conflict for this pull request? I encounter an error with a webkit CSS property not being recognized correctly, and this issue should fix it. I you're not willing to do it, I'll be happy to do it.

What I also noticed is that webkit specific value are accessible through 2 names:
webkitTextFillColor and WebkitTextFillColor

I'm currently not sure if this PR will implement both of them (test case looks like it is)

nyroDev commented 1 year ago

BTW allProperties define only 1 webkit property alone, -webkit-line-clamp I think this PR should remove this line, introduced lately on January

eps1lon commented 12 months ago

Not working on this one at the moment