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.

Sign up for GitHub

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

Jump to bottom

fix: webkit properties not being recognized #112

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

fix: webkit properties not being recognized #112

wants to merge 4 commits into from

Conversation

eps1lon
Copy link
Contributor

@eps1lon eps1lon commented Jan 23, 2020

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

@codecov-io
Copy link

codecov-io commented Jan 23, 2020 •
edited

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%> (ø) ⬆️
lib/parsers.js 80.35% <0%> (-0.3%) ⬇️

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

jsakas commented Jan 23, 2020

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

eps1lon
eps1lon commented Jan 23, 2020
View reviewed changes
scripts/generate_implemented_properties.js

.map(propertyFile => camelToDashed(propertyFile.replace('.js', '')))
.map(property => {
const isVendorSpecific = /^(o|moz|ms|webkit)-/.test(property);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sneaking in support for other vendor prefixes. I'm not sure if you're opinionated about other vendor prefixes. I'm looking at this purely from a testing perspective where I'd like to see support for all vendor prefixes. Otherwise it's hard to write tests that cover all browsers at once.

Copy link

@ExE-Boss ExE-Boss Apr 1, 2020 •
edited

Choose a reason for hiding this comment

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

The real bug is in:

cssstyle/lib/parsers.js

Line 712 in 62b3b1b

var first_segment = /^\([^-]\)-/;

It should be:

var first_segment = /^([^-]+)-/;

That’s what I’m doing in #116.

P.S.: JSDOM projects don't use the Angular commit style.

@eps1lon
Copy link
Contributor Author

eps1lon commented Jan 23, 2020

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

domenic commented Jan 23, 2020

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

@eps1lon
Copy link
Contributor Author

eps1lon commented Jan 23, 2020

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

This statement clashes with

cssstyle/lib/allExtraProperties.js

Lines 4 to 5 in ebd2dab

* This file contains all implemented properties that are not a part of any
* current specifications or drafts, but are handled by browsers nevertheless.

@eps1lon eps1lon mentioned this pull request Jan 23, 2020
Merged
@SimenB SimenB mentioned this pull request Jan 24, 2020
Merged
necolas pushed a commit to facebook/react that referenced this pull request Jan 25, 2020
@SimenB @necolas
Upgrade to jest 25 (#17896)
cf00812 
The changes to the test code relate to changes in JSDOM that come with Jest 25:

* Several JSDOM workarounds are no longer needed.
* Several tests made assertions to match incorrect JSDOM behavior (e.g. setAttribute calls) that JSDOM has now patched to match browsers.
  * https://codesandbox.io/s/resets-value-of-datetime-input-to-fix-bugs-in-ios-safari-1ppwh
* JSDOM no longer triggers default actions when dispatching click events.
  * https://codesandbox.io/s/beautiful-cdn-ugn8f
* JSDOM fixed (jsdom/jsdom#2700) a bug so that calling focus() on an already focused element does not dispatch a FocusEvent.
* JSDOM now supports passive events.
* JSDOM has improved support for custom CSS properties.
  * But requires jsdom/cssstyle#112 to land to support webkit prefixed properties.
@eps1lon
Copy link
Contributor Author

eps1lon commented Mar 13, 2020

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

acdlite added a commit to acdlite/react that referenced this pull request Mar 24, 2020
@acdlite
Revert "Upgrade to jest 25 (facebook#17896)"
c099e5a 
This reverts commit cf00812.

The changes to the test code relate to changes in JSDOM that come with Jest 25:

* Several JSDOM workarounds are no longer needed.
* Several tests made assertions to match incorrect JSDOM behavior (e.g. setAttribute calls) that JSDOM has now patched to match browsers.
  * https://codesandbox.io/s/resets-value-of-datetime-input-to-fix-bugs-in-ios-safari-1ppwh
* JSDOM no longer triggers default actions when dispatching click events.
  * https://codesandbox.io/s/beautiful-cdn-ugn8f
* JSDOM fixed (jsdom/jsdom#2700) a bug so that calling focus() on an already focused element does not dispatch a FocusEvent.
* JSDOM now supports passive events.
* JSDOM has improved support for custom CSS properties.
  * But requires jsdom/cssstyle#112 to land to support webkit prefixed properties.
acdlite added a commit to acdlite/react that referenced this pull request Mar 24, 2020
@acdlite
Revert "Upgrade to jest 25 (facebook#17896)"
776fcfb 
This reverts commit cf00812.

The changes to the test code relate to changes in JSDOM that come with Jest 25:

* Several JSDOM workarounds are no longer needed.
* Several tests made assertions to match incorrect JSDOM behavior (e.g. setAttribute calls) that JSDOM has now patched to match browsers.
  * https://codesandbox.io/s/resets-value-of-datetime-input-to-fix-bugs-in-ios-safari-1ppwh
* JSDOM no longer triggers default actions when dispatching click events.
  * https://codesandbox.io/s/beautiful-cdn-ugn8f
* JSDOM fixed (jsdom/jsdom#2700) a bug so that calling focus() on an already focused element does not dispatch a FocusEvent.
* JSDOM now supports passive events.
* JSDOM has improved support for custom CSS properties.
  * But requires jsdom/cssstyle#112 to land to support webkit prefixed properties.
acdlite added a commit to acdlite/react that referenced this pull request Mar 24, 2020
@acdlite
Revert "Upgrade to jest 25 (facebook#17896)"
a622dad 
This reverts commit cf00812.

The changes to the test code relate to changes in JSDOM that come with Jest 25:

* Several JSDOM workarounds are no longer needed.
* Several tests made assertions to match incorrect JSDOM behavior (e.g. setAttribute calls) that JSDOM has now patched to match browsers.
  * https://codesandbox.io/s/resets-value-of-datetime-input-to-fix-bugs-in-ios-safari-1ppwh
* JSDOM no longer triggers default actions when dispatching click events.
  * https://codesandbox.io/s/beautiful-cdn-ugn8f
* JSDOM fixed (jsdom/jsdom#2700) a bug so that calling focus() on an already focused element does not dispatch a FocusEvent.
* JSDOM now supports passive events.
* JSDOM has improved support for custom CSS properties.
  * But requires jsdom/cssstyle#112 to land to support webkit prefixed properties.
acdlite added a commit to facebook/react that referenced this pull request Mar 24, 2020
@acdlite
Revert "Upgrade to jest 25 (#17896)" (#18376)
fc7835c 
This reverts commit cf00812.

The changes to the test code relate to changes in JSDOM that come with Jest 25:

* Several JSDOM workarounds are no longer needed.
* Several tests made assertions to match incorrect JSDOM behavior (e.g. setAttribute calls) that JSDOM has now patched to match browsers.
  * https://codesandbox.io/s/resets-value-of-datetime-input-to-fix-bugs-in-ios-safari-1ppwh
* JSDOM no longer triggers default actions when dispatching click events.
  * https://codesandbox.io/s/beautiful-cdn-ugn8f
* JSDOM fixed (jsdom/jsdom#2700) a bug so that calling focus() on an already focused element does not dispatch a FocusEvent.
* JSDOM now supports passive events.
* JSDOM has improved support for custom CSS properties.
  * But requires jsdom/cssstyle#112 to land to support webkit prefixed properties.
@jsakas
Copy link
Member

jsakas commented Mar 30, 2020

@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
Copy link
Contributor Author

eps1lon commented Mar 31, 2020

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

cssstyle/lib/allExtraProperties.js

Lines 4 to 5 in ebd2dab

* This file contains all implemented properties that are not a part of any
* current specifications or drafts, but are handled by browsers nevertheless.

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

ExE-Boss
ExE-Boss suggested changes Apr 1, 2020
View reviewed changes
scripts/generate_implemented_properties.js

.map(propertyFile => camelToDashed(propertyFile.replace('.js', '')))
.map(property => {
const isVendorSpecific = /^(o|moz|ms|webkit)-/.test(property);
Copy link

@ExE-Boss ExE-Boss Apr 1, 2020 •
edited

Choose a reason for hiding this comment

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

The real bug is in:

cssstyle/lib/parsers.js

Line 712 in 62b3b1b

var first_segment = /^\([^-]\)-/;

It should be:

var first_segment = /^([^-]+)-/;

That’s what I’m doing in #116.

P.S.: JSDOM projects don't use the Angular commit style.

@ExE-Boss ExE-Boss mentioned this pull request Apr 1, 2020
Open
@nyroDev nyroDev mentioned this pull request Nov 23, 2023
Open
@nyroDev
Copy link

nyroDev commented Nov 23, 2023 •
edited

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

nyroDev commented Nov 23, 2023

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

@nyroDev nyroDev mentioned this pull request Nov 26, 2023
Open
@eps1lon
Copy link
Contributor Author

eps1lon commented Nov 27, 2023

Not working on this one at the moment

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ExE-Boss ExE-Boss ExE-Boss requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@eps1lon @codecov-io @jsakas @domenic @nyroDev @ExE-Boss