Last modified: 2014-06-06 04:27:01 UTC

Wikimedia Bugzilla is closed!

Wikimedia migrated from Bugzilla to Phabricator. Bug reports are handled in Wikimedia Phabricator.
This static website is read-only and for historical purposes. It is not possible to log in and except for displaying bug reports and their history, links might be broken. See T37562, the corresponding Phabricator task for complete and up-to-date bug report information.
Bug 35562 - @import styles broken in modules that combine multiple stylesheets
@import styles broken in modules that combine multiple stylesheets
Status: NEW
Product: MediaWiki
Classification: Unclassified
ResourceLoader (Other open bugs)
1.17.x
All All
: Lowest minor (vote)
: Future release
Assigned To: Krinkle
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-03-28 13:57 UTC by Krinkle
Modified: 2014-06-06 04:27 UTC (History)
8 users (show)

See Also:
Web browser: ---
Mobile Platform: ---
Assignee Huggle Beta Tester: ---


Attachments

Description Krinkle 2012-03-28 13:57:05 UTC
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.
Comment 1 Krinkle 2012-08-30 18:29:39 UTC
Solution: Combine, but output from load.php as array.

Client will combine as efficient as possible keeping @import in mind.
Comment 2 Krinkle 2012-09-25 17:14:45 UTC
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.
Comment 3 Mark A. Hershberger 2012-09-28 18:58:53 UTC
looks like this is done enough for release. pushing the rest till later.
Comment 4 Krinkle 2012-09-28 23:35:07 UTC
(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.
Comment 5 Isarra 2012-12-09 03:26:18 UTC
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?
Comment 6 Krinkle 2012-12-09 16:30:00 UTC
(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.
Comment 7 Isarra 2012-12-09 16:52:02 UTC
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.
Comment 8 Jesús Martínez Novo (Ciencia Al Poder) 2012-12-09 17:07:01 UTC
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.
Comment 9 Krinkle 2012-12-09 17:10:30 UTC
(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 :)
Comment 10 Krinkle 2012-12-09 17:11:16 UTC
(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.
Comment 11 Jesús Martínez Novo (Ciencia Al Poder) 2012-12-09 17:16:55 UTC
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.
Comment 12 Isarra 2012-12-09 18:13:57 UTC
(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.
Comment 13 Krinkle 2012-12-09 18:51:18 UTC
(In reply to comment #12)
> Also, if the reason for removing the ability to import stuff [..]

It was never removed.
Comment 14 Isarra 2012-12-09 18:54:06 UTC
(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.
Comment 15 Andre Klapper 2014-03-09 12:46:50 UTC
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.
Comment 16 Krinkle 2014-06-06 04:26:44 UTC
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).

Note You need to log in before you can comment on or make changes to this bug.


Navigation
Links