Fix parsing of minimized css output

[TimvdLippe]

The problem was the expression [\s;] in the first non-capturing group. This incorrectly consumed the ; from a previous css rule.

This check was introduced at 6433f62 for #1389.

Fixes #2483

[samccone]

R: @tjsavage for slotting

[tjsavage]

Adding @azakus who has been taking a look

[nazar-pc]

There was somewhat similar fix in 883aa5c, just did PR to this PR to improve regexp a bit: TimvdLippe#1

[TimvdLippe]

Yes I have tried that solution, but that fails on detecting incorrect variable names like name--incorrect. It will leave up the name, causing weird CSS to happen. Therefore I decided to explicitly capture more characters in front, in order to circumvent the 'failures'.

You can try this yourself by adding the incorrect variable and inspecting using chrome dev tools when running the unit test. The element should have a CSS rule with warning, indicating invalid CSS is applied to the element.

However, if this CSS weirdness is more desired than capturing everything, your patch seems fine. I will leave that for the core team to decide.

[nazar-pc]

Well, I've tried that and unfortunately both our regexps are not good, they start capturing incorrect CSS property - in your case completely (doesn't harm, incorrect property), in my case partially (might override some custom properties?).
I'll try to find something that will cover all cases.

[TimvdLippe]

What I tried (and did work together with my regex) was throwing a SyntaxError upon catching incorrect CSS variable names. However, this is against the CSS spec if I am not mistaken, but at least clearly notifies the user what is going on. It therefore failed this testcase.

To achieve this I changed https://github.com/Polymer/polymer/blob/master/src/lib/css-parse.html#L150 to

.replace(this._rx.customProp, function(match) {
  if (match.trim().indexOf(this.VAR_START) !== 0) {
    throw new SyntaxError("Invalid CSS variable defined '" + match + "'");
  }
  return '';
})

P.S. The variable declaration is ignored by Polymer, as it is unable to find any var(name--incorrect). All in all these regexes are quite tricky to get completely right, if not impossible.

[nazar-pc]

I think throwing exception is a good idea, never seen -- in the middle of any css property.

[googlebot]

We found a Contributor License Agreement for you (the sender of this pull request) and all commit authors, but as best as we can tell these commits were authored by someone else. If that's the case, please add them to this pull request and have them confirm that they're okay with these commits being contributed to Google. If we're mistaken and you did author these commits, just reply here to confirm.

[TimvdLippe]

I will squash the commits tomorrow to also fix the cla

[nazar-pc]

@googlebot, what is wrong?) I'm OK with that, confirm or whatever word you're expecting

[dfreedm]

googlebot gets confused if the commiter email does not match the author email

[googlebot]

CLAs look good, thanks!

[TimvdLippe]

Commits have been squashed and the PR is again available for review @azakus . If/when this PR is merged, I shall create an issue to discuss the addition of the SyntaxError.

[dfreedm]

LGTM, @sorvell for review

[ebidel]

Ping

[rubenstolk]

Any update on this @sorvell?

[TimvdLippe]

PR rebased onto current master.

[TimvdLippe]

@azakus for some reason the PR did not fail, but when merged into master Travis complained on Safari: https://travis-ci.org/Polymer/polymer/builds/94970278#L1630

I don't have safari myself, but could you confirm/deny the tests break in that browser? For some reason the tests in this PR were not run in safari, as seen at https://travis-ci.org/Polymer/polymer/builds/94621570#L1557

[dfreedm]

Reverting for now, moved into branch JeremybellEU-fix-min-css

[kevinpschaaf]

@JeremybellEU FYI, since we moved our CI runner to Travis, 3rd-party PR's are only tested against browsers that we can run on the Travis VM (Firefox & Chrome); the full browser matrix is only run on Sauce for pushes to non-forked branches, due to Travis's lack of support for a secured test configuration needed to keep Sauce keys private (something we're hoping to work with them to fix).

[nazar-pc]

@kevinpschaaf, in fact Travis have support for private environmental variables, configs in .travis.yml and more. I have such key for decrypting certificate to create digital signatures on releases in one of my projects.
Here are docs:

• https://docs.travis-ci.com/user/environment-variables/#Encrypted-Variables
• https://docs.travis-ci.com/user/encrypting-files/
• https://docs.travis-ci.com/user/encryption-keys/

[dfreedm]

@nazar-pc Yep, we use encrypted variables for the sauce configuration. https://github.com/Polymer/polymer/blob/master/.travis.yml#L18-L21

[kevinpschaaf]

@nazar-pc Travis will only decrypt such keys for first-party branches, not for forks, since a malicious forker could echo the decrypted keys to the log by modifying the .travis.yml file to do just that.

From the page you linked:

Please note that encrypted environment variables are not available for pull requests from forks.

When I say "secured test configuration", I mean we'd like the option to be able to specify the .travis.yml file itself in a secured location (e.g. not in the repo, which the contributor can change in their fork, but on the travis website, for example).