Page MenuHomePhabricator

@import are broken due to be concatenated in the middle of output.
Closed, DeclinedPublic

Description

Note: This is not about Less.js handled @import.

@import rules must appear at the beginning of the document before all other rules except @charset. https://www.w3.org/TR/CSS2/cascade.html#at-import

Example: This CSS will cause the browser to render the selectors with the appropriate font.
https://meeseeks.gamepedia.com/load.php?debug=true&lang=en&modules=skins.hydra.netbar&only=styles&skin=hydradark

Example: This CSS will cause the browser to ignore the @import due it being concatenated in the middle of the output.
https://meeseeks.gamepedia.com/load.php?debug=true&lang=en&modules=skins.hydra.smartbanner|skins.hydra.netbar&only=styles&skin=hydradark

It looks like ResourceLoaderFileModule is calling out to CSSMin to handle concatenating all of these files together and in the process handling @embed. CSSMin is detecting the @import blocks so it looks like it might be simple to strip those out and move them to the top.

Event Timeline

Alexia created this task.Nov 30 2018, 9:35 PM
Restricted Application added a project: Performance-Team. · View Herald TranscriptNov 30 2018, 9:35 PM
Restricted Application added a subscriber: Aklapper. · View Herald Transcript
Alexia updated the task description. (Show Details)Nov 30 2018, 10:11 PM
Alexia added a comment.EditedDec 3 2018, 7:04 PM

I wrote a function to hoist @import up to the top of files, but unfortunately in my digging it seems that resource loader is hell bent on keeping every single file separate until the very last moment. I can not find a good place to put this in.

diff --git a/includes/libs/CSSMin.php b/includes/libs/CSSMin.php
index 1ee7a3bc9d..f275fe2972 100644
--- a/includes/libs/CSSMin.php
+++ b/includes/libs/CSSMin.php
@@ -39,6 +39,7 @@ class CSSMin {
 
     const EMBED_REGEX = '\/\*\s*\@embed\s*\*\/';
     const COMMENT_REGEX = '\/\*.*?\*\/';
+    const IMPORT_REGEX = '#(@import\s+?(?:url\()?[\'|"]?.+?[\'|"]?(?:\))?;)#';
 
     /** @var array List of common image files extensions and MIME-types */
     protected static $mimeTypes = [
@@ -220,6 +221,25 @@ class CSSMin {
         }
     }
 
+    /**
+     * Hoist @import blocks to the top.
+     *
+     * @access public
+     * @param string $source CSS data with @imports to hoist.
+     * @return string Hoisted CSS data.
+     */
+    public static function hoistImports( $source ) {
+        $matches = [];
+        if ( preg_match( self::IMPORT_REGEX, $source, $matches ) > 0 ) {
+            unset( $matches[0] );
+            foreach ( $matches as $match ) {
+                $source = str_replace( $match, '', $source );
+                $source = $match . "\n" . $source;
+            }
+        }
+        return $source;
+    }
+
     /**
      * Remaps CSS URL paths and automatically embeds data URIs for CSS rules
      * or url() values preceded by an / * @embed * / comment.

Change 477329 had a related patch set uploaded (by Alexia; owner: Alexia):
[mediawiki/core@master] Hoist @import blocks to the top of concatenated output.

https://gerrit.wikimedia.org/r/477329

Pcj awarded a token.Dec 4 2018, 8:50 PM
Pcj added a subscriber: Pcj.

@Alexia Apologies for the inconvenience resulting from this.

It is intentional on our part for ResourceLoader to not to support @import, with the sole exception of legacy usage within non-gadget wiki modules (e.g. user styles and user scripts) because those are currently not capable of server-side module composition from multiple files.

In the case of modules maintained as part of the software, such as extensions and skins, it is almost always result from a mistake. And even if done intentionally, it would result in poor performance for the web client. Modules composed of multiple files must be declared and composed server-side, not client-side.

There are a number of anti-patterns in this category that are documented and/or detected at run-time with useful warnings for developers. Unfortunately, this particular one has not been triggered before and is currently lacking such documentation and/or run-time detection.

I would propose that:

  • We document this on mediawiki.org.
  • And, we add a PHPUnit structure test that automatically rejects such pattern with a useful error message.
Krinkle triaged this task as Normal priority.Dec 10 2018, 2:47 AM
Krinkle moved this task from Inbox to Backlog on the MediaWiki-ResourceLoader board.

Change 477329 abandoned by Alexia:
Hoist @import blocks to the top of concatenated output.

https://gerrit.wikimedia.org/r/477329

Alexia closed this task as Declined.Dec 10 2018, 5:49 PM
Alexia removed a project: Patch-For-Review.

Understood. I changed our local CSS to embed the output of the Google Font URL call directly.(Which is not recommended, but not the worst thing.) We only use it in two places.

I can put a blurb on mediawiki.org for this once I find a good spot for it.

Understood. I changed our local CSS to embed the output of the Google Font URL call directly.(Which is not recommended, but not the worst thing.)

Resolving that server-side seems like the best solution imho. That's compatible with what ResourceLoader expects and follows performance, availability and privacy best practices for MediaWiki.

It is indeed recommended against by Google because they use User-Agent sniffing in their own back-end to ultra-optimise the CSS output to just the font type supported by the current user's browser. This is an optimisation not compatible with ResourceLoader and is intentionally not supported by us, because that level of cache-fragmentation is not considered worth the infrastructure cost and worth the performance delay it would incur on users due to increased cache misses. In simpler terms: Not doing this optimisation is in itself an optimisation that benefits our users; the infrastructures and balances are quite different between Google and MediaWiki :)

That's not to say this optimisation would be pointless. And at Google scale, it can be done with negligible cost for themselves or end-users.