Last modified: 2014-11-20 01:12:30 UTC
Here is an example test case which fails: array( '.foo { margin: 1px -4px 3px 2px !important; }', '.foo { margin: 1px 2px 3px -4px !important; }' ),
Hm. There are a few cases where we regex for '[;}]', we should extend that to something like '(?:!important\s*)?[;}]'.
Change 113667 had a related patch set uploaded by Nikerabbit: Workaround for CSSJanus bug 61440 https://gerrit.wikimedia.org/r/113667
Change 113667 merged by jenkins-bot: Workaround for CSSJanus bug 61440 https://gerrit.wikimedia.org/r/113667
Thanks, Bartosz. I submitted a fix (https://github.com/cssjanus/php-cssjanus/pull/3) upstream using your regex change. I also added a few tests.
Note that !important must never be used. It is a valid bug for upstream CSSJanus (since it's valid CSS), but should not bare any relevance to code in Wikimedia environments. There is not a single valid use case for using !important (with the exception of working around upstream code running on the same page that also uses !important, because only !important can trump !important). Any other case, and I mean, *any* other case, has other solutions that are better. Using !important breaks cascading nature and is simply unneeded infectious overkill. Consider it similar to other bad coding patterns that should be disallowed by convention (like using the @-operator in PHP, or loose == comparison and automatic semi-colon insertion in JavaScript). Using irrelevant selectors (as Ic5c575068d911 did) is not necessary. In most cases it's just unnecessary to begin with (e.g. unjustified paranoia). In other cases it may be result of a bug elsewhere in the code. To override a style, always use the same selector as the original style and you'll be fine. If your code is loaded before the other code, something is wrong. If that wrong thing can't be fixed, try harder. Eventually, you may want to resort to one of few workarounds, such as: * Repeat the same selector to increase weight, like .foo.foo.foo.foo (however much weight you need), still leaves you in a better environment than important by allowing multiple code paths to use the same technique and control their weight. * Add [class] selectors (less heavy). * Use default elements as ancestor selector ("body .foo", "html body .foo"). Better than adding in random ancestors that are not relevant to your code. And more maintainable as these don't change. Anything but !important. -- Krinkle [1] 3.14 things you didn't know about CSS: http://vimeo.com/100264064
Thanks Siebrand. I've put it here: https://www.mediawiki.org/wiki/Manual:Coding_conventions/CSS#.21important
(In reply to Krinkle from comment #5) > To override a style, always use the same selector as the original style and > you'll be fine. That isn't appropriate in all cases. Say there are rules for a fooSelector elsewhere. You want to style barSelector. Either "barSelector elements" is a subset of "fooSelector elements", or the sets intersect. In the case of a conflict, you want to override the rules associated with fooSelector. Simply using fooSelector again is not correct. It can unintentionally re-style elements you want to leave alone. If they are overlapping sets, it will also omit elements you *do* want to style (things that are barSelector but *not* fooSelector) However, it's true that adding specificity usually works well in these scenarios. Sometimes this can be done naturally, e.g. with a meaningful container element: .bar-root barSelector However, as you said, !important is part of CSS, whether we like it or not, and it needs to work properly (including in our version). Note, I've ported it to to the JavaScript repo now (https://github.com/cssjanus/node-cssjanus/pull/27)
(In reply to Matthew Flaschen from comment #7) > (In reply to Krinkle from comment #5) > > To override a style, always use the same selector as the original style and > > you'll be fine. > > That isn't appropriate in all cases. Say there are rules for a fooSelector > elsewhere. You want to style barSelector. Either "barSelector elements" is > a subset of "fooSelector elements", or the sets intersect. > > In the case of a conflict, you want to override the rules associated with > fooSelector. > > Simply using fooSelector again is not correct. It can unintentionally > re-style elements you want to leave alone. > I'm not convinced. Poke me on IRC with an example.
Step 1 is done. It's fixed in the CSSJanus upstream node.js repository. However, I realized we also need to do a node.js release and tag, since the PHP version imports the tests by tag: https://github.com/cssjanus/php-cssjanus/blob/master/test/suites/CSSJanusTest.php#L55 . node.js change to set up the release: https://github.com/cssjanus/cssjanus/pull/30 Updated version of the PHP pull request (node.js release needs to be finished before this can be merged, and that's what's causing Travis to fail): https://github.com/cssjanus/php-cssjanus/pull/3 Note, this impacts Flow, but we will be changing the affected area not to use !important in addition to this fix.
(In reply to Matthew Flaschen from comment #9) > node.js change to set up the release: > https://github.com/cssjanus/cssjanus/pull/30 Krinkle tweaked and merged this. > Updated version of the PHP pull request (node.js release needs to be > finished before this can be merged, and that's what's causing Travis to > fail): https://github.com/cssjanus/php-cssjanus/pull/3 This is the last remaining item in the CSSJanus repos (after this, we just need to bump core's composer.json). Since the node.js change has been merged (the tests are imported from the node.js repo), it's now passing.
Change 174593 had a related patch set uploaded by Krinkle: resourceloader: Update cssjanus to v1.1.1 https://gerrit.wikimedia.org/r/174593
Change 174593 merged by jenkins-bot: resourceloader: Update cssjanus to v1.1.1 https://gerrit.wikimedia.org/r/174593
