Importantify breaks data-uri's about aphrodite HOT 20 CLOSED

My instinct would be to do 1. Something like:

foo.replace(/(\((?:\\.|[^\\)]*\)|"(?:\\.|[^\\"]*"|'(?:\\.|[^\\']*'|[^\("'])*;/g, '$1 !important;');

Untested.

from aphrodite.

Ahh, thanks for the report @ostrgard!

This does indeed look like a bug. Would be happy to accept test demonstrating the behaviour you're expecting or a PR to fix the problem.

from aphrodite.

Hi @jlfwong ! I'll look into a PR tomorrow :)

from aphrodite.

I've tried to come up with a good solution to this, but actually the problem could also apply to the content prop of the :before and :after pseudo classes – definitely an edge-case though. Are there any other cases, where this could happen? I think the core issue is that the importantify function blindly accepts a semicolon as marking the end of line.

Solution 1
You could check for an opening parentheses, double-quotes, single-quotes etc., that isn't closed. You'd have to match that to the previous string, to make sure you're detecting a closure of the right type.

Solution 2
Pass an end of line identifier to importantify, that it uses to split the string and use it in generateCSSRuleset like so:

const ret = `${kebabifyStyleName(key)}:${stringValue}`;
const eol = 'aphrodite-eol-identifier';
return useImportant === false ? ret + ';' : importantify(ret + eol, eol)

I'm not sure which side-effects this would have.

Tests:

it('supports data-uri\'s', () => {
    assertCSS('.foo', [{
        backgroundImage: 'url(data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAAAEAAAAB)'
    }], '.foo{background-image:url(data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAAAEAAAAB) !important;}');
});

it('supports the content property with a semicolon', () => {
    assertCSS('.foo', [{
        ':before': {
            content: '"foo;bar"',
        }
    }], '.foo:before{content:"foo;bar" !important;}');
});

What's your thoughts @jlfwong ?

from aphrodite.

Ahh, a very good point about use of ; in content:!

IIRC, we really only need the regex version of importantify for cases where a key is prefixed to multiple values.

i.e. in the following case:

> require('inline-style-prefixer')
[Function: Prefixer]
> P = require('inline-style-prefixer');
[Function: Prefixer]
> P.prefixAll({transition: 'none', display: 'flex'});
{ transition: 'none',
  display: '-webkit-box;display:-moz-box;display:-ms-flexbox;display:-webkit-flex;display:flex',
  WebkitTransition: 'none',
  MozTransition: 'none',
  msTransition: 'none' }

We only need to do the regex version for display -- the other properties would work just fine to have the important just appended.

The complexity introduced by solution 1 doesn't appeal to me -- it seems like the variety of thing that would yield new surprising behaviour, and I'm afraid I'm not following how solution 2 would work.

@xymostech may have ideas on how to better handle this.

from aphrodite.

Here's a regex that Facebook uses internally to skip strings and urls when converting .foo/bar to .foo-bar and .public/foo/bar to .public-foo-bar:

https://github.com/facebook/draft-js/blob/a472db9335a1ecf8fe4b8b18afe93561e0eb695e/gulpfile.js#L123

from aphrodite.

But we want to ignore all functions, not just url(), right? My worry is that the parens can be nested, which a regexp can't handle. (Well, I guess they can these days, but it's really inefficient.)

That said, this was my first instinct as well (when I thought quotes was the only thing you'd have to skip over).

from aphrodite.

I don't think any other "functions" take arbitrary strings between the parens. And url() does require escaping parens within the URL if you don't use quotes.

from aphrodite.

@jlfwong let me elaborate on my suggested solution 2. As I wrote, importantify blindly accepts a semicolon as marking the end of line (or end of rule). I meant to pass a string to the function, that it will use to split by, instead of ;, to find the end of line/rule. Does that make sense?

from aphrodite.

@ostrgard I think it might be unclear why we added importantify in the first place.

When we auto-prefix certain properties, we get back a value that contains other keys. This is for cases where it's not the key that needs vendor prefixing, but the value. Here's the example from above again.

> P = require('inline-style-prefixer');
> P.prefixAll({transition: 'none', display: 'flex'});
{ transition: 'none',
  display: '-webkit-box;display:-moz-box;display:-ms-flexbox;display:-webkit-flex;display:flex',
  WebkitTransition: 'none',
  MozTransition: 'none',
  msTransition: 'none' }

If you append an custom EOL to all the transition properties, this strategy will work just fine. But for the case of display (which is why we added importantify in the first place instead of just appending !important to each value), we need to inject !important into the middle of the value.

Perhaps your plan to have a custom EOL would work well, but it would involve modifying our fork of inline-style-prefixer (or the upstream, followed by a merge into our fork). If I'm understanding your plan correctly, this is the behaviour you'd want, roughly:

> P.prefixAll({transition: 'none', display: 'flex'}, {eol: '$$$'});
{ transition: 'none',
  display: '-webkit-box;$$$display:-moz-box;$$$display:-ms-flexbox;$$$display:-webkit-flex;$$$display:flex',
  WebkitTransition: 'none',
  MozTransition: 'none',
  msTransition: 'none' }

Then you could do the transformation you were planning, I believe.

from aphrodite.

Aha! Thank you very much for clarifying that @jlfwong! Then my suggested solution 2 is overly extensive. I continued down your path @spicyj. I suggested in solution 1 to check if str contains non-closed double-quotes, single-quotes or parenthesis. But that's just backwards! The following importantify splits by ;'s not contained in double-quotes, single-quotes or parenthesis:

const importantRegexp = /^([^:]+:.*?)( !important)?$/;
const eolSplitRegexp = /;(?=([^\"]*\"[^\"]*\")*[^\"]*$)(?=([^\']*\'[^\']*\')*[^\']*$)(?![^(]*\))/;

export const importantify = (string) => {
    return string.split(eolSplitRegexp).filter(str => str && str !== '').map(
        str => str.replace(
            importantRegexp,
            (_, base, important) => base + " !important")
    ).join(";") + ';';
};

This passes all tests, including the two i provided in this thread.

I filter out the .split provided undefined's that is returned by the non-matched regex groups. Does anyone know a better way of handling this? Sadly I still have lots to learn about regex.

@jlfwong you wrote earlier: "The complexity introduced by solution 1 doesn't appeal to me -- it seems like the variety of thing that would yield new surprising behaviour, and I'm afraid I'm not following how solution 2 would work." Do you feel the same about the above solution?

from aphrodite.

I can't think of any circumstance where that fails, but I do think this is still unnecessarily complex.

As I mentioned, this filter need only apply to properties that were changed as the result of the auto-prefixing. For all other properties, we can just append !important. Doing a pass over all the values and seeing if the value is different before/after auto-prefixing should accurately predict whether the value was changed.

As in the above example with > P.prefixAll({transition: 'none', display: 'flex'});, if you compare the values of the object before and after prefixing, only the display property has changed (other properties have appeared with vendor prefixes, but we only care about ones that may have had semicolons introduced into the values by the auto-prefixer). In such circumstances, it seems reasonable that the split on ; is safe (I'm not aware of any properties whose values need vendor prefixing and can values that may contain ;, but I may be wrong).

I think the right fix is to modify generateCSSRuleset to do this comparison, roughly like so (untested):

export const generateCSSRuleset = (selector, declarations, stringHandlers,
        useImportant) => {
    const handledDeclarations = runStringHandlers(
        declarations, stringHandlers);

    const prefixedDeclarations = Prefixer.prefixAll(handledDeclarations);

    const rules = objectToPairs(prefixedDeclarations).map(([key, value]) => {
        const stringValue = stringifyValue(key, value);
        let cssRules = [`${kebabifyStyleName(key)}:${stringValue}`];
        if (handleDeclarations.hasOwnProperty(key) && value !== handledDeclarations[key]) {
            // Value was vendor prefixed, e.g. "display: flex",
            // converts "flex" to "flex; display: -ms-flexbox;"
            cssRules = cssRules[0].split(";");
        }
        return cssRules.map(rule => useImportant === false ? `${rule};` : `${rule} !important;`).join("");
    }).join("");

    if (rules) {
        return `${selector}{${rules}}`;
    } else {
        return "";
    }
};

If we do that, we can probably eliminate the importantify function completely.

I believe doing that will pass all of the existing test cases and the ones you've helpfully contributed in this issue :)

I'm going to be off the grid for a few weeks, and fear I've missed some of the rationale for introducing the initial importantify regex instead of using a simple split, so I'm going to defer to @xymostech for this one.

from aphrodite.

I havn't had a complete overview of the rest of the codebase, so I can't really say if it's better to eliminate importantify rather than expand upon it. It seems simpler though, which is obviously likeable. I just checked as well – your snippet does pass all tests.

Looking forward to hear what @xymostech thinks on this. Have a happy few weeks off the grid, @jlfwong :)

from aphrodite.

Any updates on this issue? We just ran into this problem earlier.

from aphrodite.

I completely didn't see this issue, but I think I fixed it with #71

from aphrodite.

Still breaking :(

from aphrodite.

I think this is fixed on master, but we haven't cut a new release. @xymostech 0.3.2 time?

from aphrodite.

@ianvdhSBM This should be working with 0.3.3 (I botched the publish of 0.3.2). When you get the chance, can you try to update and see if it's working for you now?

from aphrodite.

@jlfwong It works! Thanks!

from aphrodite.

@ianvdhSBM Glad to hear it, thanks for confirming :)

from aphrodite.