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.

Files: 
CommentFileSizeAuthor
#20 locale-css-ordering-2028643-20.patch2.92 KBDavid_Rothstein
PASSED: [[SimpleTest]]: [MySQL] 40,224 pass(es).
[ View ]
#20 interdiff-17-20.txt1.99 KBDavid_Rothstein
#17 Issue-2028643-by-hass-CSS-files-order-is-broken_1_0-tests.patch2.53 KBhass
FAILED: [[SimpleTest]]: [MySQL] 40,509 pass(es), 3 fail(s), and 0 exception(s).
[ View ]
#17 Issue-2028643-by-hass-CSS-files-order-is-broken_1.patch3.04 KBhass
PASSED: [[SimpleTest]]: [MySQL] 40,116 pass(es).
[ View ]
#12 Issue-2028643-by-hass-CSS-files-order-is-broken_1.patch2.53 KBtstoeckler
FAILED: [[SimpleTest]]: [MySQL] 40,411 pass(es), 3 fail(s), and 0 exception(s).
[ View ]
#11 Issue-2028643-by-hass-CSS-files-order-is-broken.patch3.04 KBhass
PASSED: [[SimpleTest]]: [MySQL] 40,483 pass(es).
[ View ]
#6 Issue-2028643-by-hass-CSS-files-order-is-broken.patch748 byteshass
PASSED: [[SimpleTest]]: [MySQL] 40,476 pass(es).
[ View ]

Comments

Issue summary:View changes

a

Issue summary:View changes

a

Issue summary:View changes

Updated issue summary.

Issue tags:+needs backport to D7

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.

Issue summary:View changes

Updated issue summary.

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

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.

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

Component:base system» locale.module
Status:Active» Needs review
StatusFileSize
new748 bytes
PASSED: [[SimpleTest]]: [MySQL] 40,476 pass(es).
[ View ]

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.

Issue summary:View changes

A

Issue summary:View changes

a

Issue summary:View changes

a

Title:CSS files order is brokenCSS files order is incorrect in RTL

Updated issue summary to get this in ASAP.

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

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.

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... :-(.

Status:Needs work» Needs review
Issue tags:-Needs tests
StatusFileSize
new3.04 KB
PASSED: [[SimpleTest]]: [MySQL] 40,483 pass(es).
[ View ]

Now with tests.

Issue summary:View changes

a

StatusFileSize
new2.53 KB
FAILED: [[SimpleTest]]: [MySQL] 40,411 pass(es), 3 fail(s), and 0 exception(s).
[ View ]

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

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.

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.

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.

Status:Needs work» Needs review
StatusFileSize
new3.04 KB
PASSED: [[SimpleTest]]: [MySQL] 40,116 pass(es).
[ View ]
new2.53 KB
FAILED: [[SimpleTest]]: [MySQL] 40,509 pass(es), 3 fail(s), and 0 exception(s).
[ View ]

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

Status:Needs work» Needs review

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

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++

StatusFileSize
new1.99 KB
new2.92 KB
PASSED: [[SimpleTest]]: [MySQL] 40,224 pass(es).
[ View ]

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.

Title:CSS files order is incorrect in RTLChange 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.

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

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

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

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

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

Issue summary:View changes

A