Make "/*@noflip*/ /*@embed*/" annotation work without CSSJanus hacks

This reverts most of commit 2d842f14250646475b5c2ffa2fe4f5a131f94236,
leaving only the test added in it, and reimplements the same
functionality better.

Instead of stripping /*@noflip*/ annotations in CSSJanus, which is
incompatible with other implementations that preserve it, extend
CSSMin to allow other CSS comments to be present before the
rule-global @embed annotation. (This required making the regex logic
in it even worse than it was, but it's actually slightly less terrible
than I expected it would be. Good thing we have tests!)

Bug: 69698
Change-Id: I58603ef64f7d7cdc6461b34721a4d6b15f15ad79

diff --git a/includes/OutputPage.php b/includes/OutputPage.php
index 22a6012..3321747 100644 (file)
--- a/includes/OutputPage.php
+++ b/includes/OutputPage.php
@@ -3520,8 +3520,6 @@ $templates
                if ( $flip === 'flip' && $this->getLanguage()->isRTL() ) {
                        # If wanted, and the interface is right-to-left, flip the CSS
                        $style_css = CSSJanus::transform( $style_css, true, false );
-               } else {
-                       $style_css = CSSJanus::nullTransform( $style_css );
                }
                $this->mInlineStyles .= Html::inlineStyle( $style_css ) . "\n";
        }
@@ -3572,8 +3570,6 @@ $templates
                        $previewedCSS = $this->getRequest()->getText( 'wpTextbox1' );
                        if ( $this->getLanguage()->getDir() !== $wgContLang->getDir() ) {
                                $previewedCSS = CSSJanus::transform( $previewedCSS, true, false );
-                       } else {
-                               $previewedCSS = CSSJanus::nullTransform( $previewedCSS );
                        }
                        $otherTags .= Html::inlineStyle( $previewedCSS ) . "\n";
                } else {

diff --git a/includes/libs/CSSJanus.php b/includes/libs/CSSJanus.php
index 7b7b407..4cfc9b7 100644 (file)
--- a/includes/libs/CSSJanus.php
+++ b/includes/libs/CSSJanus.php
@@ -174,22 +174,6 @@ class CSSJanus {
                $css = $noFlipClass->detokenize( $css );
                $css = $noFlipSingle->detokenize( $css );
 
-               // Remove remaining /* @noflip */ annotations, they won't be needed anymore
-               // and can interfere with other code (bug 69698).
-               $css = self::nullTransform( $css );
-
-               return $css;
-       }
-
-       /**
-        * Remove @noflip annotations, but don't do any other transforms.
-        * @param string $css stylesheet to transform
-        * @return string Transformed stylesheet
-        */
-       public static function nullTransform( $css ) {
-               $patt = self::$patterns['noflip_annotation'];
-               $css = preg_replace( "/($patt)\\s*/i", '', $css );
-
                return $css;
        }
 

diff --git a/includes/libs/CSSMin.php b/includes/libs/CSSMin.php
index dcaa685..c69e79f 100644 (file)
--- a/includes/libs/CSSMin.php
+++ b/includes/libs/CSSMin.php
@@ -200,10 +200,9 @@ class CSSMin {
                        $remote = substr( $remote, 0, -1 );
                }
 
-               // Replace all comments by a placeholder so they will not interfere
-               // with the remapping
-               // Warning: This will also catch on anything looking like the start of
-               // a comment between quotation marks (e.g. "foo /* bar").
+               // Replace all comments by a placeholder so they will not interfere with the remapping.
+               // Warning: This will also catch on anything looking like the start of a comment between
+               // quotation marks (e.g. "foo /* bar").
                $comments = array();
                $placeholder = uniqid( '', true );
 
@@ -226,12 +225,13 @@ class CSSMin {
 
                $source = preg_replace_callback(
                        $pattern,
-                       function ( $matchOuter ) use ( $local, $remote, $embedData ) {
+                       function ( $matchOuter ) use ( $local, $remote, $embedData, $placeholder ) {
                                $rule = $matchOuter[0];
 
-                               // Check for global @embed comment and remove it
+                               // Check for global @embed comment and remove it. Allow other comments to be present
+                               // before @embed (they have been replaced with placeholders at this point).
                                $embedAll = false;
-                               $rule = preg_replace( '/^(\s*)' . CSSMin::EMBED_REGEX . '\s*/', '$1', $rule, 1, $embedAll );
+                               $rule = preg_replace( '/^((?:\s+|' . $placeholder . '(\d+)x)*)' . CSSMin::EMBED_REGEX . '\s*/', '$1', $rule, 1, $embedAll );
 
                                // Build two versions of current rule: with remapped URLs
                                // and with embedded data: URIs (where possible).

diff --git a/includes/resourceloader/ResourceLoaderFileModule.php b/includes/resourceloader/ResourceLoaderFileModule.php
index 137ff62..dc8b14a 100644 (file)
--- a/includes/resourceloader/ResourceLoaderFileModule.php
+++ b/includes/resourceloader/ResourceLoaderFileModule.php
@@ -870,8 +870,6 @@ class ResourceLoaderFileModule extends ResourceLoaderModule {
 
                if ( $flip ) {
                        $style = CSSJanus::transform( $style, true, false );
-               } else {
-                       $style = CSSJanus::nullTransform( $style );
                }
                $localDir = dirname( $localPath );
                $remoteDir = dirname( $remotePath );

diff --git a/includes/resourceloader/ResourceLoaderUserCSSPrefsModule.php b/includes/resourceloader/ResourceLoaderUserCSSPrefsModule.php
index 7abecc7..40274c6 100644 (file)
--- a/includes/resourceloader/ResourceLoaderUserCSSPrefsModule.php
+++ b/includes/resourceloader/ResourceLoaderUserCSSPrefsModule.php
@@ -83,8 +83,6 @@ class ResourceLoaderUserCSSPrefsModule extends ResourceLoaderModule {
                $style = implode( "\n", $rules );
                if ( $this->getFlip( $context ) ) {
                        $style = CSSJanus::transform( $style, true, false );
-               } else {
-                       $style = CSSJanus::nullTransform( $style );
                }
                return array( 'all' => $style );
        }

diff --git a/includes/resourceloader/ResourceLoaderWikiModule.php b/includes/resourceloader/ResourceLoaderWikiModule.php
index 3b4c3d9..99a8739 100644 (file)
--- a/includes/resourceloader/ResourceLoaderWikiModule.php
+++ b/includes/resourceloader/ResourceLoaderWikiModule.php
@@ -151,8 +151,6 @@ abstract class ResourceLoaderWikiModule extends ResourceLoaderModule {
                        }
                        if ( $this->getFlip( $context ) ) {
                                $style = CSSJanus::transform( $style, true, false );
-                       } else {
-                               $style = CSSJanus::nullTransform( $style );
                        }
                        $style = CSSMin::remap( $style, false, $this->getConfig()->get( 'ScriptPath' ), true );
                        if ( !isset( $styles[$media] ) ) {

diff --git a/tests/phpunit/data/less/module/styles.css b/tests/phpunit/data/less/module/styles.css
index e7454af..b78780a 100644 (file)
--- a/tests/phpunit/data/less/module/styles.css
+++ b/tests/phpunit/data/less/module/styles.css
@@ -1,3 +1,4 @@
+/* @noflip */
 .unit-tests {
   color: green;
   border: 2px solid #eeeeee;

diff --git a/tests/phpunit/includes/libs/CSSJanusTest.php b/tests/phpunit/includes/libs/CSSJanusTest.php
index e4283b0..8d0224c 100644 (file)
--- a/tests/phpunit/includes/libs/CSSJanusTest.php
+++ b/tests/phpunit/includes/libs/CSSJanusTest.php
@@ -15,27 +15,15 @@ class CSSJanusTest extends MediaWikiTestCase {
 
                if ( $cssB ) {
                        $transformedA = CSSJanus::transform( $cssA );
-                       $this->assertEquals(
-                               $transformedA,
-                               str_replace( '/* @noflip */ ', '', $cssB ),
-                               'Test A-B transformation'
-                       );
+                       $this->assertEquals( $transformedA, $cssB, 'Test A-B transformation' );
 
                        $transformedB = CSSJanus::transform( $cssB );
-                       $this->assertEquals(
-                               $transformedB,
-                               str_replace( '/* @noflip */ ', '', $cssA ),
-                               'Test B-A transformation'
-                       );
+                       $this->assertEquals( $transformedB, $cssA, 'Test B-A transformation' );
                } else {
                        // If no B version is provided, it means
                        // the output should equal the input (modulo @noflip annotations).
                        $transformedA = CSSJanus::transform( $cssA );
-                       $this->assertEquals(
-                               $transformedA,
-                               str_replace( '/* @noflip */ ', '', $cssA ),
-                               'Nothing was flipped'
-                       );
+                       $this->assertEquals( $transformedA, $cssA, 'Nothing was flipped' );
                }
        }
 

diff --git a/tests/phpunit/includes/libs/CSSMinTest.php b/tests/phpunit/includes/libs/CSSMinTest.php
index 2b4d60d..43c5086 100644 (file)
--- a/tests/phpunit/includes/libs/CSSMinTest.php
+++ b/tests/phpunit/includes/libs/CSSMinTest.php
@@ -202,6 +202,11 @@ class CSSMinTest extends MediaWikiTestCase {
                                'foo { /* @embed */ background: url(red.gif); }',
                                "foo { background: url($red); background: url(http://localhost/w/red.gif?timestamp)!ie; }",
                        ),
+                       array(
+                               'Embedded file, other comments before the rule',
+                               "foo { /* Bar. */ /* @embed */ background: url(red.gif); }",
+                               "foo { /* Bar. */ background: url($red); /* Bar. */ background: url(http://localhost/w/red.gif?timestamp)!ie; }",
+                       ),
                        array(
                                'Can not re-embed data: URIs',
                                "foo { /* @embed */ background: url($red); }",

diff --git a/tests/phpunit/includes/resourceloader/ResourceLoaderTest.php b/tests/phpunit/includes/resourceloader/ResourceLoaderTest.php
index d2e118c..7664d5b 100644 (file)
--- a/tests/phpunit/includes/resourceloader/ResourceLoaderTest.php
+++ b/tests/phpunit/includes/resourceloader/ResourceLoaderTest.php
@@ -90,6 +90,15 @@ class ResourceLoaderTest extends ResourceLoaderTestCase {
                $this->assertStringEqualsFile( $basePath . '/styles.css', $styles['all'] );
        }
 
+       /**
+        * Strip @noflip annotations from CSS code.
+        * @param string $css
+        * @return string
+        */
+       private function stripNoflip( $css ) {
+               return str_replace( '/*@noflip*/ ', '', $css );
+       }
+
        /**
         * What happens when you mix @embed and @noflip?
         * This really is an integration test, but oh well.
@@ -108,14 +117,16 @@ class ResourceLoaderTest extends ResourceLoaderTestCase {
                $contextLtr = self::getResourceLoaderContext( 'en' );
                $contextRtl = self::getResourceLoaderContext( 'he' );
 
+               // Since we want to compare the effect of @noflip+@embed against the effect of just @embed, and
+               // the @noflip annotations are always preserved, we need to strip them first.
                $this->assertEquals(
                        $expectedModule->getStyles( $contextLtr ),
-                       $testModule->getStyles( $contextLtr ),
+                       $this->stripNoflip( $testModule->getStyles( $contextLtr ) ),
                        "/*@noflip*/ with /*@embed*/ gives correct results in LTR mode"
                );
                $this->assertEquals(
                        $expectedModule->getStyles( $contextLtr ),
-                       $testModule->getStyles( $contextRtl ),
+                       $this->stripNoflip( $testModule->getStyles( $contextRtl ) ),
                        "/*@noflip*/ with /*@embed*/ gives correct results in RTL mode"
                );
        }