cssstyle icon indicating copy to clipboard operation
cssstyle copied to clipboard

fix: webkit properties not being recognized

Open eps1lon opened this issue 4 years ago • 11 comments

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

eps1lon avatar Jan 23 '20 17:01 eps1lon

Codecov Report

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

Impacted file tree graph

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

codecov-io avatar Jan 23 '20 17:01 codecov-io

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

jsakas avatar Jan 23 '20 17:01 jsakas

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

eps1lon avatar Jan 23 '20 17:01 eps1lon

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

domenic avatar Jan 23 '20 18:01 domenic

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 avatar Jan 23 '20 18:01 eps1lon

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

eps1lon avatar Mar 13 '20 14:03 eps1lon

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

jsakas avatar Mar 30 '20 22:03 jsakas

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

eps1lon avatar Mar 31 '20 09:03 eps1lon

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 avatar Nov 23 '23 14:11 nyroDev

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

nyroDev avatar Nov 23 '23 15:11 nyroDev

Not working on this one at the moment

eps1lon avatar Nov 27 '23 17:11 eps1lon