Affected core versions
D7 only

Bug:
locale_css_alter() documents it will add the RTL CSS file directly after the original CSS file. This is incorrect and causes several overriding issues as I expect that the RTL override comes directly after the normal CSS file. The current logics has very strange side effects on overriding and often requires double overriding.

Currently broken order in RTL:

  1. example1.css
  2. example2.css
  3. example3.css
  4. example1-rtl.css
  5. example2-rtl.css
  6. example3-rtl.css

Expected order of CSS files:

LTR:

  1. example1.css
  2. example2.css
  3. example3.css

RTL:

  1. example1.css
  2. example1-rtl.css
  3. example2.css
  4. example2-rtl.css
  5. example3.css
  6. example3-rtl.css

Background and Solution:
locale_css_alter() adds a tiny value of 0.01 to the CSS weight, but this is a larger weight than the next LTR file has. drupal_add_css() adds a tiny value to the weight by counting $options['weight'] += count($css) / 1000; what results in a smaller value than 0.01. By adding a value of 0.0001 we are always smaller than 1/1000 and it now doesn't matter how many CSS files are added or not, the weight of the RTL file will always higher than it's LTR file and always smaller than the next LTR file. This make sure it really works as documented in locale_css_alter().

// Replicate the same item, but with the RTL path and a little larger
// weight so that it appears directly after the original CSS file.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

hass’s picture

Issue summary: View changes

a

hass’s picture

Issue summary: View changes

a

hass’s picture

Issue summary: View changes

Updated issue summary.

hass’s picture

Issue tags: +Needs backport to D7
Wim Leers’s picture

Can we fix this after #352951: Make JS & CSS Preprocessing Pluggable lands, which should allow us to write a unit test for this? I already greatly expanded test coverage for CSS in that patch.

Wim Leers’s picture

Issue summary: View changes

Updated issue summary.

hass’s picture

No problem. Can you give me a hint where the RTL files are added? Cannot find it in D7.

hass’s picture

Version: 8.x-dev » 7.x-dev
Issue tags: -Needs backport to D7

#2015789: Remove language_css_alter() (RTL stylesheets) in favor of HTML 'dir' attribute got committed. We no longer have -rtl.css files in D8. Moving to D7 only.

Wim Leers’s picture

#4: was going to post the same thing :)

hass’s picture

Component: base system » locale.module
Status: Active » Needs review
FileSize
748 bytes

Unbelievable - how easy this was to fix. *G*

Just to explain, why this cannot fail again and why the current weight addition is not correct. drupal_add_css() adds a tiny value to the weight by counting $options['weight'] += count($css) / 1000;. Now with this small value we are smaller than 1/1000 and it now doesn't matter how many CSS files are added or not, we will always higher than the LTR file and always lower than the next LTR file. This make sure it really works as documented in locale_css_alter().

// Replicate the same item, but with the RTL path and a little larger
// weight so that it appears directly after the original CSS file.

hass’s picture

Issue summary: View changes

A

hass’s picture

Issue summary: View changes

a

hass’s picture

Issue summary: View changes

a

hass’s picture

Title: CSS files order is broken » CSS files order is incorrect in RTL

Updated issue summary to get this in ASAP.

hass’s picture

@Wim: Are you able to RTBC this patch, please? :-)

tstoeckler’s picture

Status: Needs review » Needs work
Issue tags: +Needs tests

Fix looks great, but I think it needs a comment. Also we should write a test. This is something that could easily be broken by further refactoring without anyone noticing.

hass’s picture

I do not believe that this get's any re-factoring. We are late in D7 and this is no longer in D8... But do you have an idea for this comment? I have myself no idea how to test this with simpletest... :-(.

hass’s picture

Status: Needs work » Needs review
Issue tags: -Needs tests
FileSize
3.04 KB

Now with tests.

hass’s picture

Issue summary: View changes

a

tstoeckler’s picture

Awesome! Thanks for the tests. Uploading the same patch with only the tests, to prove their validity.

tstoeckler’s picture

Oops, forgot the -tests-only suffix. Note to committers: In case #12 fails, #11 is the patch to commit!!!

Status: Needs review » Needs work

The last submitted patch, Issue-2028643-by-hass-CSS-files-order-is-broken_1.patch, failed testing.

penyaskito’s picture

Those tests look awesome, however:

+++ b/modules/locale/locale.testundefined
@@ -3096,3 +3096,48 @@
+/**
+ * Functional tests for CSS alter functions.
+ */
+class LocaleCSSAlterTest extends DrupalWebTestCase {

Not sure if this class name explains the behavior tested here.

+++ b/modules/locale/locale.testundefined
@@ -3096,3 +3096,48 @@
+    $this->assertRaw('@import url("' . $base_url . '/modules/system/system.base.css' . $query_string . '");' . "\n" . '@import url("' . $base_url . '/modules/system/system.base-rtl.css' . $query_string . '");' . "\n", t('CSS: system.base-rtl.css is added directly after system.base.css.'));
+    $this->assertRaw('@import url("' . $base_url . '/modules/system/system.menus.css' . $query_string . '");' . "\n" . '@import url("' . $base_url . '/modules/system/system.menus-rtl.css' . $query_string . '");' . "\n", t('CSS: system.menus-rtl.css is added directly after system.menus.css.'));
+    $this->assertRaw('@import url("' . $base_url . '/modules/system/system.messages.css' . $query_string . '");' . "\n" . '@import url("' . $base_url . '/modules/system/system.messages-rtl.css' . $query_string . '");' . "\n", t('CSS: system.messages-rtl.css is added directly after system.messages.css.'));

Don't use t() in assertion messages.

Good catch with the bugfix, that looks clever.

hass’s picture

Status: Needs review » Needs work

Not sure if this class name explains the behavior tested here.

hook_css_alter(). This is exactly what is tested here. See https://api.drupal.org/api/drupal/modules%21locale%21locale.module/funct...

Don't use t() in assertion messages.

This has changed in D8 #500866: [META] remove t() from assert message, but not in D7. Won't fix.

hass’s picture

Reattaching both files. I think the failing tests file need the first and the fixing patch the second.

hass’s picture

Status: Needs work » Needs review

@tstoeckler: RTBC? Like to get this in 7.23...

tstoeckler’s picture

Status: Needs review » Reviewed & tested by the community

Well, I think the class name/doc could be improved, but I don't really have any suggestions on that.

I think @hass is right that we can translate test strings in D7, but I'm not sure. @David_Rothstein will know, however.

So tentatively setting to RTBC. Progress++

David_Rothstein’s picture

t() has been removed from assertion messages almost everywhere in Drupal 7 core. (Locale is one of the last stragglers but there's an RTBC patch at #1797364: Remove t() from assertion messages in tests for the locale module.)

So we should remove them here too. I did that in the attached patch.

Looks great otherwise, so as long as this one passes tests I'll go ahead and commit it. I wonder if this will cause issues anywhere (if someone changed their CSS as a workaround for this problem?) but I think we have to fix it either way; I'll just make sure to mention it in the release notes.

David_Rothstein’s picture

Title: CSS files order is incorrect in RTL » Change notification: CSS files order is incorrect in RTL
Category: bug » task
Priority: Major » Critical
Status: Reviewed & tested by the community » Needs review
Issue tags: +Needs change record, +7.23 release notes, +7.23 release blocker

Committed to 7.x - thanks! http://drupalcode.org/project/drupal.git/commit/81e6074

Actually decided it would be a good idea to have a real change notification for this one, so went ahead and wrote an initial one here: https://drupal.org/node/2058463

Reviews welcome.

David_Rothstein’s picture

Adding some of the tags that didn't quite make it on.

hass’s picture

Status: Needs review » Fixed
Issue tags: -Needs change record

Change notification looks good to me. THX for this quick commit.

Tor Arne Thune’s picture

Title: Change notification: CSS files order is incorrect in RTL » CSS files order is incorrect in RTL
Category: task » bug
Priority: Critical » Major

Automatically closed -- issue fixed for 2 weeks with no activity.

Anonymous’s picture

Issue summary: View changes

A