LeaVerou / prefixfree

Break free from CSS prefix hell!
http://projects.verou.me/prefixfree/
MIT License
3.83k stars 712 forks source link

fix value property replacement #6147

Open joyously opened 5 years ago

joyously commented 5 years ago

The properties that can have other properties as values can have multiple ones in a list. They were not all getting replaced. I made the fix function use multiline mode, I removed the negative lookahead for a colon (since there can be many), and I handled the case where there is no semi-colon.

For #6146

LeaVerou commented 5 years ago

Hello, thanks for submitting a PR!

Please note that multiline mode makes no difference if you have no ^ and $.

What do you mean there can be many colons? The negative lookahead just checks that the property name is not immediately followed by a colon, i.e. it's part of a value.

It seems to me that the fix on line 227 would be more than sufficient to fix this bug.

joyously commented 5 years ago

The multiline mode was needed in my test case, and some of the expressions do have ^ and $.

I didn't mean that there could be many colons. I meant that there could be many values after the colon. will-change: color, outline-radius, background-color; With the assertion, the value needing the prefix had to be the first value.

LeaVerou commented 5 years ago

Having multiple values after the colon has nothing to do with what this assertion is preventing. The reason this assertion exists is to avoid looking at properties that are not part of a value, i.e. they are used as properties. I.e. in your declaration, it avoids looking at will-change itself, but only at its values (since prefixing the property itself is handled elsewhere).

With the assertion, the value needing the prefix had to be the first value.

That does not follow from the regex. The properties are joined with an OR (|) and then enclosed in parentheses.

Actually, I just tried will-change: appearance, appearance and both properties get prefixed. What was your actual case where they weren't?

joyously commented 5 years ago

I was testing with some real CSS that I added more will-change cases to.

[id='toggle-heart']:checked + label {
  color: #e2264d;
  will-change: outline-radius;
  will-change: outline-radius, transform;
  will-change: outline-radius ;
  animation: heart 5s cubic-bezier(0.17, 0.89, 0.32, 1.49);
}
[id='toggle-heart']:checked + label:before {
  animation-name: bubble;
  will-change: transform,
  outline-radius , border-width;
  will-change: transform, outline-radius, border-width;
  will-change: transform, border-width, outline-radius
}

My test case had more CSS, and it was all handled correctly with this PR, but not without it. I was using Firefox 30 to test, and the outline-radius was prefixed correctly in all cases with this PR.

joyously commented 5 years ago

I tested the original code again, and if there is a space after the property (before the colon), the prefix is not applied. The problem is in my PR also, so I will fix it. But meanwhile, this is the test CSS I am using:

[id='toggle-heart']:checked + label {
  color: #e2264d;
  will-change: outline-radius;
  will-change: outline-radius, transform;
  will-change: outline-radius ;
  will-change : outline-radius;
  will-change : outline-radius, transform;
  will-change : outline-radius ;
  animation: heart 5s cubic-bezier(0.17, 0.89, 0.32, 1.49);
}
[id='toggle-heart']:checked + label:before {
  animation-name: bubble;
  will-change: appearance,
  outline-radius , border-width;
  will-change  : transform, outline-radius, border-width;
  will-change : transform, border-width, outline-radius
}
LeaVerou commented 5 years ago

Which browser are you testing in?

I guess it's Firefox, but if not: outline-radius is not supported in Chrome, with or without a prefix.

This may be better for testing:

[id='toggle-heart']:checked + label {
  color: #e2264d;
  will-change: appearance;
  will-change: appearance, transform;
  will-change: appearance ;
  will-change : appearance;
  will-change : appearance, transform;
  will-change : appearance ;
  animation: heart 5s cubic-bezier(0.17, 0.89, 0.32, 1.49);
}
[id='toggle-heart']:checked + label:before {
  animation-name: bubble;
  will-change: user-modify,
  appearance , border-width;
  will-change  : user-modify, appearance, border-width;
  will-change : user-modify, border-width, appearance
}

So the issues I see are:

All of these fixes are on line 227.