Page MenuHomePhabricator

@import styles broken in modules that combine multiple stylesheets
Closed, DeclinedPublic

Description

When a stylesheet containing @import is concatenated with another stylesheet, the @import rule breaks because it has to stay at the top.

For core and extensions we do not support usage of @import, however for site and user modules it can't be missed (i.e. @import global.js from meta, @import css from another sub page, @import css for cross-wiki gadgets..).

bug 33305 introduced this breakages for <style> tag in the front-end, that can be fixed. However it turns out this isn't the first time we concatenate style sheets. the load.php for only=styles also concatenates stylesheets.

Although it is never to be used with more than 1 module when dealing with user-generated content, the "user" and "site" are loaded separately but exist of more than 1 source page.

user: User:Name/common.css + User:Name/<skin>.css
site: MediaWiki:Common.css + MediaWiki:<skin>.css

Ever since 1.17 @import rules in the 2nd pages have been broken. In common.css they still work fine.


Version: 1.17.x
Severity: minor

Details

Reference
bz35562

Event Timeline

bzimport raised the priority of this task from to Lowest.Nov 22 2014, 12:11 AM
bzimport set Reference to bz35562.

Solution: Combine, but output from load.php as array.

Client will combine as efficient as possible keeping @import in mind.

Done in I3e8227ddb87fd9441071ca935439fc6467751dab.

However in only=styles request this obviously still happens, so don't mark it fixed yet.

Gadgets that load modules will work, but the hardcoded <link> tags can't be split up.

looks like this is done enough for release. pushing the rest till later.

(In reply to comment #3)

looks like this is done enough for release. pushing the rest till later.

Yes, the fix in I3e8227ddb87fd9441071ca935439fc6467751dab is good enough for release. Granted that it is reviewed/merged into 1.20.

We've introduced various modules since 1.19 that most wikis will hit the 31 limit without this feature, so we should have it in the release, as without it it'd be a regression that random user interface components will be unstyled in IE.

Is this ever going to be fixed for user skin stylesheets, or do we just have to put all @imports in the common.css now?

(In reply to comment #5)

Is this ever going to be fixed for user skin stylesheets, or do we just have
to
put all @imports in the common.css now?

The notion of "now" is incorrect. Even before ResourceLoader, before Gadgets, before all of that, the user skin scripts/stylesheets were loaded together in 1 request. Which means it has always been risky to use @import because if there are any stylesheets prepended before the one @import is used in, it would break (because CSS only supports @import at the top of the stylesheet).

This hasn't regressed, it never worked in the first place (for almost 10 years I'd say).

I'd recommend avoiding use of @import in the first place, for it is inefficient. Try using modules where possible. If this is a user-specific script, you can use importStylesheetURI instead which serves the same purpose and works from any user script (regardless of concatenation order). It isn't pretty, but it works and should help you in the mean time.

It was working in April. Same scripts, but the MediaWiki was treating them differently.

Modularity is the entire point of using @import, and something that is especially important when dealing with scripts that need to be maintained in a central location, a particular issue on wikis such as enwp with too many gadgets already. While javascript is an arguable fallback, it is not a good one, and if it proves necessary when dealing wish CSS then frankly that would simply suggest that MediaWiki is broken.

Widgets shouldn't have @imports at all. Gadgets have a field to specify which CSS file to include.

I think of 2 solutions for this:

  • Split again CSS resources on different requests (which is bad IMO as it defeats the purpose of RL)
  • After minimizing CSS files, move all @import declarations at the top. I think this can be done pretty easily.

(In reply to comment #7)

It was working in April. Same scripts, but the MediaWiki was treating them
differently.

Simply impossible. User modules have been loaded with ResourceLoader since Februari 2011, and before that they were loaded with another system that combined the user-stylesheets.

Maybe the order has changed (skin.css + common.css vs. common.css + skin.css) I don't know, but they certainly weren't loaded separately so @import was always only working from one of them.

If it worked from April I suspect that it didn't really work, but you might have used a different order. I'd be interested to know how this worked for you in April, maybe we can come up with a short-term fix for this. If you can explain how it worked then, I'm sure we can make it work again :)

(In reply to comment #8)

  • After minimizing CSS files, move all @import declarations at the top. I

think this can be done pretty easily.

I've considered this, but it was reverted because it breaks cascading order.

It could have worked if common.css was empty, thus the @import in the <skin>.css was the first rule in the file.

(In reply to comment #10)

I've considered this, but it was reverted because it breaks cascading order.

What? I think the reason for allowing only @imports at the top was to avoid people relying on cascading order between rules on that file and on the @import-ed one, so moving @imports to top shouldn't break anything.

Well, if done, it should preserve the order as they appear.

(In reply to comment #11)

It could have worked if common.css was empty, thus the @import in the
<skin>.css was the first rule in the file.

Yeah, looks like I added some site-specific css around then, nevermind.

(In reply to comment #10)

I've considered this, but it was reverted because it breaks cascading order.

What? I think the reason for allowing only @imports at the top was to avoid
people relying on cascading order between rules on that file and on the
@import-ed one, so moving @imports to top shouldn't break anything.

Well, if done, it should preserve the order as they appear.

Indeed.

Also, if the reason for removing the ability to import stuff at all was order-related, that seems rather backwards regardless, since slightly off functionality is generally still better than no functionality at all.

(In reply to comment #12)

Also, if the reason for removing the ability to import stuff [..]

It was never removed.

(In reply to comment #13)

(In reply to comment #12)

Also, if the reason for removing the ability to import stuff [..]

It was never removed.

I meant this:

(comment #10)

(In reply to comment #8)

  • After minimizing CSS files, move all @import declarations at the top. I

think this can be done pretty easily.

I've considered this, but it was reverted because it breaks cascading order.

Krinkle: Still working (or still planning to work) on this issue?
If not, should the assignee be set back to default and the bug status changed from ASSIGNED to NEW/UNCONFIRMED? Thanks.

Reindexing as low priority bug.

For any modules loaded through ResourceLoader properly (mediawiki core modules, extension modules, gadgets) @import works fine because:

  • Server provides stylesheets separately as an array.
  • Client only combines if things like @import are not used, creates separate <style> tag otherwise.

For modules minified using ResourceLoader but served from load.php directly (site scripts, user scripts), this does not work because they don't have a way of dynamically combining them on the client-side. If this were a regression then this could maybe prioritised, but it is not.

Even prior to ResourceLoader and MediaWiki 1.17, user styles were combined in a single request (back then through a system powered by "title=-", aka "title equals hyphen").

A user may have broken their own @import by unknowingly creating a second page (e.g. having common.css, creating monobook.css or visa versa), but that's nothing new.

If anything, things are slightly better since at least we don't load site styles and user styles in the same request. Those used to be combined as well (session bound).

It works for non-legacy modules.

And for legacy modules it never worked (not now, and not prior to ResourceLoader's existence). The end result can be trivially achieved by other means (by placing imports in the relevant skin user stylesheet or in common.css only).

I'm open to ideas for how this could be made possible feasible.