From 2d842f14250646475b5c2ffa2fe4f5a131f94236 Mon Sep 17 00:00:00 2001 From: =?utf8?q?Bartosz=20Dziewo=C5=84ski?= Date: Mon, 18 Aug 2014 17:32:22 +0200 Subject: [PATCH] Make "/*@noflip*/ /*@embed*/" annotation work MIME-Version: 1.0 Content-Type: text/plain; charset=utf8 Content-Transfer-Encoding: 8bit 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 --- includes/OutputPage.php | 4 ++++ includes/libs/CSSJanus.php | 16 +++++++++++++++ .../ResourceLoaderFileModule.php | 2 ++ .../ResourceLoaderUserCSSPrefsModule.php | 2 ++ .../ResourceLoaderWikiModule.php | 2 ++ tests/phpunit/data/css/expected.css | 3 +-- tests/phpunit/data/css/test.css | 3 +-- tests/phpunit/data/less/module/styles.css | 1 - tests/phpunit/includes/libs/CSSJanusTest.php | 20 +++++++++++++++---- .../resourceloader/ResourceLoaderTest.php | 4 ++-- 10 files changed, 46 insertions(+), 11 deletions(-) diff --git a/includes/OutputPage.php b/includes/OutputPage.php index 6ea495372d..c71c19bc07 100644 --- 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 30b92c72ff..ae28163b25 100644 --- 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 8a223b05f5..19b9dd7be4 100644 --- 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 40274c6330..7abecc77fb 100644 --- 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 0472f1a798..58b7fa968c 100644 --- 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 74eb21eff3..03addcb7ab 100644 --- 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 bf589ad4d3..8d0d67089e 100644 --- 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 b78780a903..e7454afb51 100644 --- 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 407f11a64c..e4283b0bf5 100644 --- 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 12ec9ddde2..d72c5e7cde 100644 --- 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" ); } -- 2.20.1