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.

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

fix: webkit properties not being recognized #112

Open
wants to merge 4 commits into
base: main
Choose a base branch
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

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?


.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

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:

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

* 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
necolas pushed a commit to facebook/react that referenced this pull request Jan 25, 2020
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
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
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
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
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

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


.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

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:

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.

@nyroDev
Copy link

nyroDev commented Nov 23, 2023

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
@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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants