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:
- example1.css
- example2.css
- example3.css
- example1-rtl.css
- example2-rtl.css
- example3-rtl.css
Expected order of CSS files:
LTR:
- example1.css
- example2.css
- example3.css
RTL:
- example1.css
- example1-rtl.css
- example2.css
- example2-rtl.css
- example3.css
- 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.
Comments
Comment #0.0
hass CreditAttribution: hass commenteda
Comment #0.1
hass CreditAttribution: hass commenteda
Comment #0.2
hass CreditAttribution: hass commentedUpdated issue summary.
Comment #1
hass CreditAttribution: hass commentedComment #2
Wim LeersCan 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.
Comment #2.0
Wim LeersUpdated issue summary.
Comment #3
hass CreditAttribution: hass commentedNo problem. Can you give me a hint where the RTL files are added? Cannot find it in D7.
Comment #4
hass CreditAttribution: hass commented#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.
Comment #5
Wim Leers#4: was going to post the same thing :)
Comment #6
hass CreditAttribution: hass commentedUnbelievable - 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 inlocale_css_alter()
.Comment #6.0
hass CreditAttribution: hass commentedA
Comment #6.1
hass CreditAttribution: hass commenteda
Comment #6.2
hass CreditAttribution: hass commenteda
Comment #7
hass CreditAttribution: hass commentedUpdated issue summary to get this in ASAP.
Comment #8
hass CreditAttribution: hass commented@Wim: Are you able to RTBC this patch, please? :-)
Comment #9
tstoecklerFix 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.
Comment #10
hass CreditAttribution: hass commentedI 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... :-(.
Comment #11
hass CreditAttribution: hass commentedNow with tests.
Comment #11.0
hass CreditAttribution: hass commenteda
Comment #12
tstoecklerAwesome! Thanks for the tests. Uploading the same patch with only the tests, to prove their validity.
Comment #13
tstoecklerOops, forgot the -tests-only suffix. Note to committers: In case #12 fails, #11 is the patch to commit!!!
Comment #15
penyaskitoThose tests look awesome, however:
Not sure if this class name explains the behavior tested here.
Don't use t() in assertion messages.
Good catch with the bugfix, that looks clever.
Comment #16
hass CreditAttribution: hass commentedhook_css_alter()
. This is exactly what is tested here. See https://api.drupal.org/api/drupal/modules%21locale%21locale.module/funct...This has changed in D8 #500866: [META] remove t() from assert message, but not in D7. Won't fix.
Comment #17
hass CreditAttribution: hass commentedReattaching both files. I think the failing tests file need the first and the fixing patch the second.
Comment #18
hass CreditAttribution: hass commented@tstoeckler: RTBC? Like to get this in 7.23...
Comment #19
tstoecklerWell, 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++
Comment #20
David_Rothstein CreditAttribution: David_Rothstein commentedt() 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.
Comment #21
David_Rothstein CreditAttribution: David_Rothstein commentedCommitted 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.
Comment #22
David_Rothstein CreditAttribution: David_Rothstein commentedAdding some of the tags that didn't quite make it on.
Comment #23
hass CreditAttribution: hass commentedChange notification looks good to me. THX for this quick commit.
Comment #24
Tor Arne Thune CreditAttribution: Tor Arne Thune commentedComment #25.0
(not verified) CreditAttribution: commentedA