Skip to content
This repository has been archived by the owner on Apr 26, 2024. It is now read-only.

fix value property replacement #6147

Open
wants to merge 2 commits into
base: gh-pages
Choose a base branch
from
Open

Conversation

joyously
Copy link
Contributor

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

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.
@joyously joyously mentioned this pull request Apr 28, 2019
@LeaVerou
Copy link
Owner

LeaVerou commented May 3, 2019

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

joyously commented May 3, 2019

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

LeaVerou commented May 3, 2019

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

joyously commented May 3, 2019

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

joyously commented May 3, 2019

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

LeaVerou commented May 4, 2019

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:

  • Properties are not prefixed in value if the declaration property has a space before the colon. This can be fixed by adding \\s* before the colon in ':(.+?);' on line 227.
  • Multiline values are not prefixed properly. This is because the . does not match line breaks, so the property value is matched incorrectly. It could be fixed by either replacing the dot with the standard [\s\S], or your more specific to this use case [^};]
  • Values are not replaced if the declaration is terminated via a } and not a semicolon. This is also due to the ; that ':(.+?);' ends with, and should be fixed by your [;}].

All of these fixes are on line 227.

removed multiline mode
restored the negative lookahead for the colon
line 227 regex - added whitespace before colon
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants