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

This reverts most of commit 2d842f1425,
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

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 {

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;
 	}
 

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).

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 );

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 );
 	}

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] ) ) {

tests/phpunit/data/less/module/styles.css

@@ -1,3 +1,4 @@
+/* @noflip */
 .unit-tests {
   color: green;
   border: 2px solid #eeeeee;

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' );
 		}
 	}
 

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); }",

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"
 		);
 	}