From 4c01f8b2bc45da2fa4df29b8dfb14202bb2b4713 Mon Sep 17 00:00:00 2001 From: =?utf8?q?Bartosz=20Dziewo=C5=84ski?= Date: Sat, 20 Sep 2014 23:26:13 +0200 Subject: [PATCH] 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 --- includes/OutputPage.php | 4 ---- includes/libs/CSSJanus.php | 16 ---------------- includes/libs/CSSMin.php | 14 +++++++------- .../ResourceLoaderFileModule.php | 2 -- .../ResourceLoaderUserCSSPrefsModule.php | 2 -- .../ResourceLoaderWikiModule.php | 2 -- tests/phpunit/data/less/module/styles.css | 1 + tests/phpunit/includes/libs/CSSJanusTest.php | 18 +++--------------- tests/phpunit/includes/libs/CSSMinTest.php | 5 +++++ .../resourceloader/ResourceLoaderTest.php | 15 +++++++++++++-- 10 files changed, 29 insertions(+), 50 deletions(-) diff --git a/includes/OutputPage.php b/includes/OutputPage.php index 22a601222a..33217475fc 100644 --- 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 7b7b4071fb..4cfc9b7b71 100644 --- 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 dcaa6857a2..c69e79f5b2 100644 --- 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 137ff62330..dc8b14a23e 100644 --- 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 7abecc77fb..40274c6330 100644 --- 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 3b4c3d9629..99a87393d1 100644 --- 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 e7454afb51..b78780a903 100644 --- 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 e4283b0bf5..8d0224cd20 100644 --- 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 2b4d60d9a0..43c5086950 100644 --- 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 d2e118c543..7664d5b1d6 100644 --- 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" ); } -- 2.20.1