From dc834f39337887b2617c183e8771ce15d2a0b102 Mon Sep 17 00:00:00 2001 From: Timo Tijhof Date: Thu, 1 Sep 2016 16:31:14 -0700 Subject: [PATCH] Implement support for specifying type=styles T87871 formally introduced the concept of a styles module, which sets mw.loader.state to "ready" when loaded through addModuleStyles(). Previously, addModuleStyles couldn't safely do that because a module may contain scripts also, in which case mw.loader must still load the (rest) of the module (causes styles to load twice). In MediaWiki core or extensions this is easily avoided by calling not calling both addModules() and addModuleStyles(). For Gadgets we call both as a workaround to allow users to provide styles (without a FOUC), but also to provide scripts+styles. Since we don't declare which one is intended (and some gadgets do both), we loaded them both ways. This will no longer be allowed in the future (see T92459). The new 'type=styles' Gadget attribute promises to ResourceLoader that a gadget only contains styles. Impact: * [Bug fix] When mw.loader requires a styles module that already loaded, it will not load again. * [Feature] It is possible for a general scripts+styles gadget to depend on a styles gadget. Previously this caused the styles to load twice. * Specifying type=styles will load the module through addModuleStyles() only. Use this for modules that contain styles that relate to elements already on the page (e.g. when customising the skin, layout, or article content). * Specifying type=general will load the module through addModules() only. Use this if your module contains both scripts and styles and the styles only relate to elements created by the script. This means the styles do not need to be loaded separately through addModuleStyles() and will not apply to noscript mode. Effective difference: * Gadgets with only styles: We assume type=styles. This fixes the main bug (styles loading twice) and requires no migration! * Gadgets with only scripts: We assume type=general. This requires no migration! (And: No more empty stylesheet request) * Gadgets with scripts (with or without styles): We assume type=general, but unless type=general was explicitly set we'll still load it both ways so that the styles apply directly on page load. If this is not needed, set type=general. If this is needed, it should become two separate modules. We do not support a single module having two purposes (1: apply styles to the page, 2: provide scripts+styles). The styles module should be separate. It can be made hidden, and listed as dependency of the other module. The latter case is detected on page load and results in a console warning with a link to T42284. Bug: T42284 Bug: T92459 Change-Id: Ia3c9ddee243f710022144fc2884434350695699a --- GadgetHooks.php | 23 ++++- Gadgets_body.php | 24 +++++- includes/GadgetResourceLoaderModule.php | 10 +++ includes/MediaWikiGadgetsDefinitionRepo.php | 4 + .../GadgetDefinitionContentHandler.php | 1 + .../content/GadgetDefinitionValidator.php | 1 + tests/phpunit/GadgetTest.php | 85 ++++++++++++++++++- 7 files changed, 143 insertions(+), 5 deletions(-) diff --git a/GadgetHooks.php b/GadgetHooks.php index a48d930a..0cee1328 100644 --- a/GadgetHooks.php +++ b/GadgetHooks.php @@ -170,6 +170,7 @@ class GadgetHooks { $lb = new LinkBatch(); $lb->setCaller( __METHOD__ ); $enabledLegacyGadgets = array(); + $typelessMixedGadgets = array(); /** * @var $gadget Gadget @@ -183,8 +184,15 @@ class GadgetHooks { } if ( $gadget->isEnabled( $user ) && $gadget->isAllowed( $user ) ) { if ( $gadget->hasModule() ) { - $out->addModuleStyles( Gadget::getModuleName( $gadget->getName() ) ); - $out->addModules( Gadget::getModuleName( $gadget->getName() ) ); + if ( $gadget->getType() === 'styles' ) { + $out->addModuleStyles( Gadget::getModuleName( $gadget->getName() ) ); + } elseif ( $gadget->getType() === 'general' ) { + $out->addModules( Gadget::getModuleName( $gadget->getName() ) ); + } else { + $out->addModuleStyles( Gadget::getModuleName( $gadget->getName() ) ); + $out->addModules( Gadget::getModuleName( $gadget->getName() ) ); + $typelessMixedGadgets[] = $id; + } } if ( $gadget->getLegacyScripts() ) { @@ -197,6 +205,9 @@ class GadgetHooks { foreach ( $enabledLegacyGadgets as $id ) { $strings[] = self::makeLegacyWarning( $id ); } + foreach ( $typelessMixedGadgets as $id ) { + $strings[] = self::makeTypelessWarning( $id ); + } $out->addHTML( WrappedString::join( "\n", $strings ) ); return true; @@ -208,11 +219,17 @@ class GadgetHooks { return ResourceLoader::makeInlineScript( Xml::encodeJsCall( 'mw.log.warn', array( "Gadget \"$id\" was not loaded. Please migrate it to use ResourceLoader. " . - ' See <' . $special->getCanonicalURL() . '>.' + 'See <' . $special->getCanonicalURL() . '>.' ) ) ); } + private static function makeTypelessWarning( $id ) { + return ResourceLoader::makeInlineScript( Xml::encodeJsCall( 'mw.log.warn', array( + "Gadget \"$id\" styles loaded twice. Migrate to type=general. " . + 'See .' + ) ) ); + } /** * Valid gadget definition page after content is modified diff --git a/Gadgets_body.php b/Gadgets_body.php index 9e75abdd..29ecf28f 100644 --- a/Gadgets_body.php +++ b/Gadgets_body.php @@ -36,6 +36,7 @@ class Gadget { $onByDefault = false, $hidden = false, $position = 'bottom', + $type = '', $category; public function __construct( array $options ) { @@ -53,6 +54,7 @@ class Gadget { case 'targets': case 'onByDefault': case 'position': + case 'type': case 'hidden': case 'category': $this->{$member} = $option; @@ -88,6 +90,7 @@ class Gadget { 'dependencies' => $data['module']['dependencies'], 'messages' => $data['module']['messages'], 'position' => $data['module']['position'], + 'type' => $data['module']['type'], ); return new self( $info ); @@ -286,5 +289,24 @@ class Gadget { public function getPosition() { return $this->position; } -} + /** + * Returns the load type of this Gadget's ResourceLoader module + * @return string 'styles', 'general' or '' + */ + public function getType() { + if ( $this->type === 'styles' || $this->type === 'general' ) { + return $this->type; + } + if ( $this->styles && !$this->scripts ) { + // Similar to ResourceLoaderWikiModule default + return 'styles'; + } + if ( !$this->styles && $this->supportsResourceLoader() && $this->scripts ) { + return 'general'; + } + // Real default is in GadgetResourceLoaderModule so that beforePageDisplay + // can distinguish between explicit and fallback. + return ''; + } +} diff --git a/includes/GadgetResourceLoaderModule.php b/includes/GadgetResourceLoaderModule.php index 14156b70..6be3563b 100644 --- a/includes/GadgetResourceLoaderModule.php +++ b/includes/GadgetResourceLoaderModule.php @@ -79,6 +79,16 @@ class GadgetResourceLoaderModule extends ResourceLoaderWikiModule { return $this->getGadget()->getPosition(); } + /** + * Overrides ResourceLoaderWikiModule::getType() + * @return string ResourceLoaderModule::LOAD_STYLES or ResourceLoaderModule::LOAD_GENERAL + */ + public function getType() { + return $this->getGadget()->getType() === 'styles' + ? ResourceLoaderModule::LOAD_STYLES + : ResourceLoaderModule::LOAD_GENERAL; + } + public function getMessages() { return $this->getGadget()->getMessages(); } diff --git a/includes/MediaWikiGadgetsDefinitionRepo.php b/includes/MediaWikiGadgetsDefinitionRepo.php index 6cbae5ad..f7176501 100644 --- a/includes/MediaWikiGadgetsDefinitionRepo.php +++ b/includes/MediaWikiGadgetsDefinitionRepo.php @@ -212,6 +212,10 @@ class MediaWikiGadgetsDefinitionRepo extends GadgetRepo { case 'top': $info['position'] = 'top'; break; + case 'type': + // Single value, not a list + $info['type'] = isset( $params[0] ) ? $params[0] : ''; + break; } } diff --git a/includes/content/GadgetDefinitionContentHandler.php b/includes/content/GadgetDefinitionContentHandler.php index 7bee7f49..4ed98dee 100644 --- a/includes/content/GadgetDefinitionContentHandler.php +++ b/includes/content/GadgetDefinitionContentHandler.php @@ -57,6 +57,7 @@ class GadgetDefinitionContentHandler extends JsonContentHandler { 'dependencies' => array(), 'messages' => array(), 'position' => 'bottom', + 'type' => '', ), ); diff --git a/includes/content/GadgetDefinitionValidator.php b/includes/content/GadgetDefinitionValidator.php index e17c166f..24270700 100644 --- a/includes/content/GadgetDefinitionValidator.php +++ b/includes/content/GadgetDefinitionValidator.php @@ -23,6 +23,7 @@ class GadgetDefinitionValidator { 'module.dependencies' => array( 'is_array', 'array', 'is_string', 'string' ), 'module.messages' => array( 'is_array', 'array', 'is_string', 'string' ), 'module.position' => array( 'is_string', 'string' ), + 'module.type' => array( 'is_string', 'string' ), ); /** diff --git a/tests/phpunit/GadgetTest.php b/tests/phpunit/GadgetTest.php index 226c072b..a327a2a9 100644 --- a/tests/phpunit/GadgetTest.php +++ b/tests/phpunit/GadgetTest.php @@ -12,10 +12,17 @@ class GadgetsTest extends MediaWikiTestCase { $repo = new MediaWikiGadgetsDefinitionRepo(); $g = $repo->newFromDefinition( $line, 'misc' ); $this->assertInstanceOf( 'Gadget', $g ); - return $g; } + private function getModule( Gadget $g ) { + $module = TestingAccessWrapper::newFromObject( + new GadgetResourceLoaderModule( array( 'id' => null ) ) + ); + $module->gadget = $g; + return $module; + } + public function testInvalidLines() { $repo = new MediaWikiGadgetsDefinitionRepo(); $this->assertFalse( $repo->newFromDefinition( '', 'misc' ) ); @@ -49,6 +56,82 @@ class GadgetsTest extends MediaWikiTestCase { $this->assertEquals( array( 'jquery.ui' ), $g->getDependencies() ); } + public function testPosition() { + $g = $this->create( '* foo[ResourceLoader]|bar.js' ); + $this->assertEquals( 'bottom', $g->getPosition(), 'Default position' ); + + $g = $this->create( '* foo[ResourceLoader|top]|bar.js' ); + $this->assertEquals( 'top', $g->getPosition(), 'Position top' ); + } + + public static function provideGetType() { + return array( + array( + 'Default (mixed)', + '* foo[ResourceLoader]|bar.css|bar.js', + '', + ResourceLoaderModule::LOAD_GENERAL, + ), + array( + 'Default (styles only)', + '* foo[ResourceLoader]|bar.css', + 'styles', + ResourceLoaderModule::LOAD_STYLES, + ), + array( + 'Default (scripts only)', + '* foo[ResourceLoader]|bar.js', + 'general', + ResourceLoaderModule::LOAD_GENERAL, + ), + array( + 'Styles type (mixed)', + '* foo[ResourceLoader|type=styles]|bar.css|bar.js', + 'styles', + ResourceLoaderModule::LOAD_STYLES, + ), + array( + 'Styles type (styles only)', + '* foo[ResourceLoader|type=styles]|bar.css', + 'styles', + ResourceLoaderModule::LOAD_STYLES, + ), + array( + 'Styles type (scripts only)', + '* foo[ResourceLoader|type=styles]|bar.js', + 'styles', + ResourceLoaderModule::LOAD_STYLES, + ), + array( + 'General type (mixed)', + '* foo[ResourceLoader|type=general]|bar.css|bar.js', + 'general', + ResourceLoaderModule::LOAD_GENERAL, + ), + array( + 'General type (styles only)', + '* foo[ResourceLoader|type=general]|bar.css', + 'general', + ResourceLoaderModule::LOAD_GENERAL, + ), + array( + 'General type (scripts only)', + '* foo[ResourceLoader|type=general]|bar.js', + 'general', + ResourceLoaderModule::LOAD_GENERAL, + ), + ); + } + + /** + * @dataProvider provideGetType + */ + public function testType( $message, $definition, $gType, $mType ) { + $g = $this->create( $definition ); + $this->assertEquals( $gType, $g->getType(), "Gadget: $message" ); + $this->assertEquals( $mType, $this->getModule( $g )->getType(), "Module: $message" ); + } + public function testPreferences() { $prefs = array(); $repo = TestingAccessWrapper::newFromObject( new MediaWikiGadgetsDefinitionRepo() );