Make "/*@noflip*/ /*@embed*/" annotation work

To do it, just remove /*@noflip*/ annotations in CSSJanus after
we're done processing. They are not needed anymore and some obscure
interactions with CSSMin logic for preserving comments caused
`/*@noflip*/ /*@embed*/ background-image: url(…)` not to work
correctly (it would not be embedded).

This also requires us to always do CSSJanus processing, even when we
don't need flipping, to consistently handle the annotations.
I'm not entirely sure if this is worth it, but I still greatly prefer
doing it to documenting this stupid limitation. :)

Bug: 69698
Change-Id: I311b12b08b2dff9d45efb584db08cf4a11318f59

diff --git a/includes/OutputPage.php b/includes/OutputPage.php
index 6ea4953..c71c19b 100644 (file)
--- a/includes/OutputPage.php
+++ b/includes/OutputPage.php
@@ -3515,6 +3515,8 @@ $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";
        }
@@ -3565,6 +3567,8 @@ $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 30b92c7..ae28163 100644 (file)
--- a/includes/libs/CSSJanus.php
+++ b/includes/libs/CSSJanus.php
@@ -179,6 +179,22 @@ 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/resourceloader/ResourceLoaderFileModule.php b/includes/resourceloader/ResourceLoaderFileModule.php
index 8a223b0..19b9dd7 100644 (file)
--- a/includes/resourceloader/ResourceLoaderFileModule.php
+++ b/includes/resourceloader/ResourceLoaderFileModule.php
@@ -867,6 +867,8 @@ 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 40274c6..7abecc7 100644 (file)
--- a/includes/resourceloader/ResourceLoaderUserCSSPrefsModule.php
+++ b/includes/resourceloader/ResourceLoaderUserCSSPrefsModule.php
@@ -83,6 +83,8 @@ 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 0472f1a..58b7fa9 100644 (file)
--- a/includes/resourceloader/ResourceLoaderWikiModule.php
+++ b/includes/resourceloader/ResourceLoaderWikiModule.php
@@ -152,6 +152,8 @@ 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/css/expected.css b/tests/phpunit/data/css/expected.css
index 74eb21e..03addcb 100644 (file)
--- a/tests/phpunit/data/css/expected.css
+++ b/tests/phpunit/data/css/expected.css
@@ -4,8 +4,7 @@
 
 .selector { /*@embed*/ background-image: url(simple-ltr.gif); }
 
-/* Doesn't work! */
-/*.selector { /*@noflip* / /*@embed* / background-image: url(simple-ltr.gif); }*/
+.selector { /*@embed*/ background-image: url(simple-ltr.gif); }
 
 .selector { /*@embed*/ background-image: url(simple-ltr.gif); }
 

diff --git a/tests/phpunit/data/css/test.css b/tests/phpunit/data/css/test.css
index bf589ad..8d0d670 100644 (file)
--- a/tests/phpunit/data/css/test.css
+++ b/tests/phpunit/data/css/test.css
@@ -4,8 +4,7 @@
 
 /*@noflip*/ .selector { background-image: /*@embed*/ url(simple-ltr.gif); }
 
-/* Doesn't work! */
-/*.selector { /*@noflip* / /*@embed* / background-image: url(simple-ltr.gif); }*/
+.selector { /*@noflip*/ /*@embed*/ background-image: url(simple-ltr.gif); }
 
 .selector { /*@embed*/ /*@noflip*/ background-image: url(simple-ltr.gif); }
 

diff --git a/tests/phpunit/data/less/module/styles.css b/tests/phpunit/data/less/module/styles.css
index b78780a..e7454af 100644 (file)
--- a/tests/phpunit/data/less/module/styles.css
+++ b/tests/phpunit/data/less/module/styles.css
@@ -1,4 +1,3 @@
-/* @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 407f11a..e4283b0 100644 (file)
--- a/tests/phpunit/includes/libs/CSSJanusTest.php
+++ b/tests/phpunit/includes/libs/CSSJanusTest.php
@@ -15,15 +15,27 @@ class CSSJanusTest extends MediaWikiTestCase {
 
                if ( $cssB ) {
                        $transformedA = CSSJanus::transform( $cssA );
-                       $this->assertEquals( $transformedA, $cssB, 'Test A-B transformation' );
+                       $this->assertEquals(
+                               $transformedA,
+                               str_replace( '/* @noflip */ ', '', $cssB ),
+                               'Test A-B transformation'
+                       );
 
                        $transformedB = CSSJanus::transform( $cssB );
-                       $this->assertEquals( $transformedB, $cssA, 'Test B-A transformation' );
+                       $this->assertEquals(
+                               $transformedB,
+                               str_replace( '/* @noflip */ ', '', $cssA ),
+                               'Test B-A transformation'
+                       );
                } else {
                        // If no B version is provided, it means
-                       // the output should equal the input.
+                       // the output should equal the input (modulo @noflip annotations).
                        $transformedA = CSSJanus::transform( $cssA );
-                       $this->assertEquals( $transformedA, $cssA, 'Nothing was flipped' );
+                       $this->assertEquals(
+                               $transformedA,
+                               str_replace( '/* @noflip */ ', '', $cssA ),
+                               'Nothing was flipped'
+                       );
                }
        }
 

diff --git a/tests/phpunit/includes/resourceloader/ResourceLoaderTest.php b/tests/phpunit/includes/resourceloader/ResourceLoaderTest.php
index 12ec9dd..d72c5e7 100644 (file)
--- a/tests/phpunit/includes/resourceloader/ResourceLoaderTest.php
+++ b/tests/phpunit/includes/resourceloader/ResourceLoaderTest.php
@@ -110,12 +110,12 @@ class ResourceLoaderTest extends ResourceLoaderTestCase {
 
                $this->assertEquals(
                        $expectedModule->getStyles( $contextLtr ),
-                       str_replace( '/*@noflip*/ ', '', $testModule->getStyles( $contextLtr ) ),
+                       $testModule->getStyles( $contextLtr ),
                        "/*@noflip*/ with /*@embed*/ gives correct results in LTR mode"
                );
                $this->assertEquals(
                        $expectedModule->getStyles( $contextLtr ),
-                       str_replace( '/*@noflip*/ ', '', $testModule->getStyles( $contextRtl ) ),
+                       $testModule->getStyles( $contextRtl ),
                        "/*@noflip*/ with /*@embed*/ gives correct results in RTL mode"
                );
        }