Page MenuHomePhabricator

ResourceLoaderLESSFunctions::embed does not support CSSJanus flipping for RTL, affecting .background-image-svg() and others
Closed, ResolvedPublic

Description

The LESS embed function in ResourceLoaderLESSFunctions does not support using the flipped version for rtl.

That means that if you use, for example, .background-image() (from mediawiki.mixins.less) it will only use the flipped version for the fallback (for browsers that don't support data URIs).

CSSJanus does not actually flip the image in this case. It just uses a filename ending in -rtl when the original filename ended in -ltr (see https://www.mediawiki.org/wiki/ResourceLoader/Features#Flipping).

This approach would require some refactoring of the CSSJanus class, so embed could call it on a URL or filename.


Version: 1.23.0
Severity: normal
See Also:
https://bugzilla.wikimedia.org/show_bug.cgi?id=67368

Details

Reference
bz66091

Event Timeline

bzimport raised the priority of this task from to High.Nov 22 2014, 3:10 AM
bzimport set Reference to bz66091.

(In reply to Matthew Flaschen from comment #0)

CSSJanus does not actually flip the image in this case. It just uses a
filename ending in -rtl when the original filename ended in -ltr (see
https://www.mediawiki.org/wiki/ResourceLoader/Features#Flipping).

It's a little more complicated that that. It should reuse the same regex/logic as CSSJanus, rather than reinventing it.

I'm finding more and more issues that are caused by this, so it should be addressed some time soon.

I've filed bug 67368 for the removal of the 'embed()' function. See there for more details on how this bug is caused.

Note that we already do this in OOjs UI where the less is compiled by less via Grunt, and thus MediaWiki-specific functions aren't available. And yet there's no sacrifice in efficiency because the /* @embed */ comments are still picked by CSSMin when it loads in MediaWiki.

This could be fixed with or without removing custom LESS, so it's not a blocker (although it might be easier one way than the other).

  • Bug 70742 has been marked as a duplicate of this bug. ***

(Restoring previous bug summary for searchability. I also want to kill embed() (bug 67368), but killing it in bug summaries is not a good idea.)

Change 161245 had a related patch set uploaded by Bartosz Dziewoński:
Fix CSSJanus flipping in LESS mixins and remove broken custom LESS functions

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

  • Bug 68326 has been marked as a duplicate of this bug. ***

Change 161751 had a related patch set uploaded by Krinkle:
Fix CSSJanus flipping in LESS mixins and remove broken custom LESS functions

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

Change 161245 merged by jenkins-bot:
Fix CSSJanus flipping in LESS mixins and remove broken custom LESS functions

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

Change 161751 merged by jenkins-bot:
Fix CSSJanus flipping in LESS mixins and remove broken custom LESS functions

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

  • Bug 66773 has been marked as a duplicate of this bug. ***
  • Bug 68326 has been marked as a duplicate of this bug. ***

Fixed and backported.