drupal_add_css to select RTL CSS files when necessary and RTLized CSS files

Hm, the PHP part of the patch is quite straightforward, although I noticed that ".css","-rtl.css",$path should have spaces after the commas.

I have two notes though:

- Do you think is it mandatory to have all the CSS files duplicated for RTL usage? Why don't we include a cascade level for RTL sheets, so only the direction should be changed in them? Maintaining two sets of CSS files is quite painful.
- This patch does not seem to have any CSS for the core themes. How much value does RTL core CSS files have without an RTL core theme? I assume you still need to download an RTL(-enabled) even after applying this patch, so Drupal 6 will not get out-of-the box support for RTL usage even with this patch.

BTW I think this should be in the 'language system' or 'theme system', now categorized for the theme system, because it feels better.

Hi

- Do you think is it mandatory to have all the CSS files duplicated for RTL usage? Why don't we include a cascade level for RTL sheets, so only the direction should be changed in them? Maintaining two sets of CSS is quite painful.

there are two issues here
1. maintain the RTLed css files - from this point of view I think it would be much easy to maintain the whole file - because it would be easier to compare / generate an example is Jcss module
2. adding the files from the theme / core - from this point of view it might be easier to just add (and not replace) and overwrite the LTR file with RTL file ,but remember there is more (not needed) data coming to the client that is overwrited

- This patch does not seem to have any CSS for the core themes. How much value does RTL core CSS files have without an RTL core theme? I assume you still need to download an RTL(-enabled) even after applying this patch, so Drupal 6 will not get out-of-the box support for RTL usage even with this patch.

It would be a great idea to RTL out of the box theme but this is the outopia for us now
a great milestone for us would be code freeze of drupal 6 - we want to add this functionallity there to make themers and RTL themers and easier job to load core CSS files
the job of RTLing a core themes can be done after (6.1 ??)
RTLing a new theme will apply on the theme css files

another issue is the compress css files feature - how will it work in multi lingual sites ?

I am working on the rtl css files - what is our timeframe for drupal 6 ?

Avior

Hi Gabor,
Regarding the value of this functionality without a core theme...
We've built several installation profiles with additional hebrew tweaks meaning there is a greater crowd that uses the instaltion profiles to bootstrap their site when installing a fresh installation.
These are obviously built upon the existing version of drupal so we actually do have our "core" themes that will be built upon it.
We have rtl versions for zen and garland but we still need to stabalize them before they become "core" quality.
It may happen towards this milestone but the israeli drupal community has discussed this (hence the multi personal intrest in this thread) an prioritized this as the first and major milestone (anyelse we can achieve towards 6 will be a wonderful bonus).
best regards and thanks for the help
Lior

attached tar file of the rtl version of the files

5 new files in drupal 6
1. color
2. dblog
3. poll

all 3 files have now rtl version

4. locale
5. tracker
both not changed from the ltr version , so it's not included in the tar files

we will upload the files to our drupal 6 and test it

avior: ??? the patch posted by z.stolar already includes RTL CSS files (and it is in the preferred patch format not in a tar gz which is harder to view and review). Maybe a bit more cordination would help here...

avior: the CSS compression / contantenation works by looking at the file list, so if you change the list of files to include RTL files, a new cache file will be created for these. I did not test this, but this is the concept at least. You should test that too.

The code freeze for Drupal 6 is June 1st, before features such as this should get in.

we will upload the files to our drupal 6 testing environment and test it

Yor can view our drupal 6 testing environment here: http://dev.drupal.org.il.

Amnon
_{-
Professional: Drupal Search | Drupal Israel | Web Hosting Strategies
Personal: Hitech Dolphin: Regain Simple Joy :)}

We think the direction is to have a sparate compression of RTL CSS files, and a separate compression of LTR CSS files. This might require us to patch the compression algorithm as well - but serving RTL CSS files uncompressed is a good start.

In the HTML code of http://dev.drupal.org.il I found:

<html xmlns="http://www.w3.org/1999/xhtml" xml:lang="Object" lang="Object">

Looks like the language attributes are incorrect?

Duplicating all the CSS files seems excessive, but might well be the only option. I guess it is not as simple as setting direction: rtl at the highest-level and letting that cascade.

Gabor asked the exact same question, but the answer we got is somewhat confusing -- I'm not it actually answer the question. What is the problem with a global direction: rtl? :)

Dries: "direction" only alters the default text alignment and the layout of things like table cells. It does not magically flip the page around. Float directions, horizontal paddings/margins, etc, will remain LTR. This is especially a problem for tableless layouts.

Doing automatic mirroring is near impossible because some left/rights are structural while others are purely graphical (e.g. rounded corner images). In theory you could mirror every image too and really mirror everything in the CSS, but even then there'd still be some edge cases that cannot easily be solved (specifically, horizontal pixel offsets in background-position).

However, I'm not a fan of duplicating the CSS either. With the CSS preprocessor on, the overhead of a override RTL stylesheet is quite small.

It would make it easier to apply and maintain direction-neutral CSS changes. We could also mark every LTR style that has an RTL override with a simple /* LTR */ comment in the stylesheet. That way, whenever you'd alter a style with this marker, you know you need to do the same change in the RTL version.

+1 for cascading style sheets.

With duplication, *every* change made my a developer must be duplicated to the rtl version of the .css. This is unreasonable to demand from a developer (which might not even be aware that an RTL version exists, since he is changing a non-directional property). This is so unreasonable that it will quickly become obsolete, unless there is an alert maintainer for the 'RTL'ed CSS's.

When working with cascaded style sheets, only directional rules (float, text-align, padding/margin, background-position..) should be duplicated to the RTL version of the CSS, which makes much more sense (at least to me)

Anyway, I estimate that over 80% of the CSS is direction-independent.

The original patch by z.stolar does not include the flipped image for 'misc/menu-collapsed.png' (since it is binary it cannot be included in a patch).

It should go right in 'misc/menu-collapsed-rtl.png'

I think the the big question is Who will do the task of RTLing a css ?
I think we have to seperate the discussion to two :
1. RTL version of the core
2. RTLing (new) themes

if we are talking about Core, we just did that task, and the css files are changed rarely.
did you mean that core developers will now have to maintain the RTL version of core css ?
I think that will be utopia for the RTL users, but it's seems to be unfair to let them load rtl version of drupal and test this (this is the only way testing it)

I think that the RTL community will have to maintain those files, but we just want to make sure that once there are RTL files, they will get loaded from the core
so when a update to the core css files is out we need to update the rtl one as well

about RTLing new themes, it will have the same logic when an rtl version of the file exists it will get loaded in RTL page.

from an css RTLer point of view i find it more easy to have a rtl version of the whole css files rather than having just the parts that need's to be override
doing that on the first time, will add some work (that's ok), but try doing that on the second time there is an update to the LTR css, I wouldn't like doing that task....

comment : the files that I attached here were drupal 6 updated RTLed files
Zohar / Yuval can you please upload them in as a patch ? (I also emailed you the files)

Avior, we are dealing with Drupal core issues here. Drupal 6 can only be considered an RTL supporting release, if RTL CSS and image files are provided for *all core modules* and *all core themes* in the Drupal package! Who does it does not seem to be a problem, as it seems many of you are working on it. You should just unify your powers. There is no discussion about contributed themes here, you should deal with each themer one-on-one, this is out of scope for this issue. Let's not keep blocked by that.

You said it is more convenient to create standalone RTL CSS files, not building on the LTR ones with the cascade. But you would not be interested in updating RTL CSS if the LTR one changes. Steven already offered the solution: use the cascade, mark relevent parts of the LTR sheets with an /* LTR */ comment, so people updating it are altert to update the RTL CSS. Who maintains it later on will turn out. If nobody comes around to maintain it, Drupal 7 will drop CSS support, this is what happens with other parts of Drupal core too. This is how life works. (Although I think we can reasonably hope that someone will be around to maintain the RTL CSS files).

Task list:
- provide cascade RTL CSS files (do not copy what should not be changed from the LTR CSS)
- mark every RTL changed style in the LTR CSS as /* LTR */
- modify the PHP code to *add* the RTL CSS if the language is RTL (not to replace the LTR CSS)
- test with the above code, see if CSS preprocessing works even if you change the language

As far as I see, what to do for Drupal core is clearly identified.

I have a CSS question.

When the original CSS sets

margin-left: 10px;

If I just set in the rtl.css

margin-right: 10px;

Then the unified attribute of this selector would be:

margin-left: 10px;
margin-right: 10px;

which is not what we want to achieve. Actually, we want the 'margin-left' property to behave just as if it wasn't defined for that selector (inherited/default).

I am not sure that setting this to 'auto' will work.. but I probably don't know enough CSS for that.

If you don't want a margin on a cascaded element, you can set it to 0. If you want it to be inherited, you can set it to 'inherit'. CSS has the tools you need.

Trying with cascade anyway.

CSS specification says that using 'inherit' as value, forces the use of the value of the parent. Assuming this is supported by enough browsers (i.e. including ie), I have created the attached patch.

I have tested an installation from scratch.
I have tested with and without CSS preprocessing.

Everything seems to work fine.

Please note the attached patch now includes all files, including the menu-collapsed-rtl.png

The test site at http://dev.drupal.org.il is updated with latest from CVS HEAD and this patch. admin user/pw can be created for anyone who likes to do some more testing on the site.

I suggest the RTL declarations to have the exact same scope of the original declarations. It's much more maintainable. Then, there is no such problem as pointed by yhager in the previous post.

Suggested Convention:

The RTL css file may contain BOTH the original declaration, commented-out, and the new declaration.
This will make life of cascade CSS maintainer much easier.

Convention Examples:

Example 1: Here the RTL style needs to override default, invisible assumed values. Manual migration and testing are a must. _{(That's one of the reasons why a conversion algorithm won't be simple - and won't be justified by the low volume of work and the high probability of errors. auto-rtl is an overkill).}

#header {
/* LTR:    padding-left: 10px; */
/* RTL:Start */ 
              padding-right: 10px;
              text-align: right;
/* RTL:End */
}

Example 2: Single-line replaces: the RTL modification comes exactly after the commented line.

#menu {
/*  LTR      text-align: right; */
/*  RTL */  text-align: left;
  vertical-align: top;
  white-space: nowrap;
}

Example 3: Removed declaration

#mission {
  background-color: #E8E8E8;
        border-left: 10px solid #F5F5F5;
        border-right: 10px solid #F5F5F5;
        border-bottom: 10px solid #F5F5F5;
/*  LTR     padding: 1.5em 2em; */
  color: #000;
}

Tagging the original CSS files is not enough
Marking the LTR CSS file with /* RTL */ tags may also be a good idea, but not enough. Sometimes, additional changes are needed. For example, consider the following LTR declaration:

#mission {
        border-left: 10px solid #F5F5F5;
        border-right: 10px solid #F5F5F5;
}

Now - what happens if the LTR coder decides to increase the right border, or to align the element to the left? Suddently, an RTL version is neeed, even if the element is not marked LTR.

Conclusion: I don't think that marking the original files is enough. RTL functionaly will have to be tested by the RTL migrator anyway.

Comments
Auto-rtl is an overkill - Complex & risky conversion algorithm. And even if we find a great algorithm, browser compatibility may kill it. Migrating a pure-css and maintaining cross-browser compatibility (especially IE6) is sometimes a real challenge.

Suggested added task:
- Markup the RTL CSS files with the notation above. _{This will accurately identify the exact rules modified. Then, the RTL CSS maintainer should DIFF between the new LTR CSS and the old one, and make sure the CSS for the updated rules is relevant.}

Please use cvs diff.

On the PHP code:

- "This file should be have a '-rtl' suffix to its name" is not English, at least remove the "be" :)
- Locale module is not required to be enabled to have a $language->direction with RTL, so that check is both superfluous and misleading

On the PNG:

- the PNG in the diff looks garbage, we already have it here attached, so we can add it in without binary data being in the patch

On the CSS:

- the test site uses the garlandtrl theme, not Garland itself, as RTL CSS and images for Garland are not included in this patch. you plan to work on that after this gets committed?
- I see the RTL CSS files are now quite small, that is good. The comments in the CSS also seem fine.

TODO:

- fix the PHP code issues mentioned above
- use cvs diff
- do not include binary code in the patch
- have others in the RTL community review the changes and comment on them; we can only believe you that the CSS rules introduced here are fine for an RTL language, as we (core maintainers) are not fluent in any RTL language (as far as I know); so it would be nice to have it seen by more eyes before getting committed

Thanks for the feedback. I will fix the issues you mentioned and create a new 'cvs diff' by tomorrow.

- Locale module is not required to be enabled to have a $language->direction with RTL, so that check is both superfluous and misleading

I added this check since I got an error (variable not defined IIRC) during install time. Is there a different check I should use for this?

- the test site uses the garlandtrl theme, not Garland itself, as RTL CSS and images for Garland are not included in this patch. you plan to work on that after this gets committed?

Yes, However, I cannot guarantee ETA, since Garland is one of the more complex themes out there. Currently the garlandrtl is the best we have, and it's far from perfect.
Maybe RTL'ing other themes in the core may be much simpler, though I haven't tried - I will try to get some help on this from the RTL community.

This patch has value even without an RTL theme, as it lays the infrastructure for future RTL themes, even if not included in the core itself (actualy, not having this patch blocks future RTL themes).

Yes, I am well aware of the importance of this patch, in fact as soon as the RTL language identification functionality we developed hit core, I rushed to the Arabizaton Drupal Group to request RTL CSS files (http://groups.drupal.org/arabization). So count me as a supporter of the issue. I only tried to clean the situation up for others so we see where are we going.

BTW you can just do isset($language) to ensure that the variable is there.

See revised patch attached.

Changes from previous patch:
* uses CVS diff
* Does not include binary file
* replaced module_exists('locale') with defined('LANGUAGE_RTL')

OK, why didn't you use isset($language)? You said you got an error of the undefined $language, not an error of the undefined constant in install time.

why didn't you use isset($language)? You said you got an error of the undefined $language, not an error of the undefined constant in install time.

'locale.inc' does not get included if there is no language available at install time (besides English). The error I got was a constant error and not variable error.

Does Drupal 6 includes the functionality of the i18n module enabled?

Guessing so, I tried to test going to http://dev.drupal.org.il/en, or http://dev.drupal.org.il/he. I got the following error:

warning: Invalid argument supplied for foreach() in /home/devdrup/drupal-rtl/includes/menu.inc on line 574

I tried to change the language negotiation configuration from 'None' to 'Path prefix only' but the error remained. It doesn't seem to be connected to RTL, since LTR languages (English) are effectd as well - but please advise

levavie: what about not mixing in unrelated issues?! The issue you experience is clearly not related to RTL CSS file support as you note.

Applied the latest patch to my test environment. It worked nicely with cache turned on or not. Granted, without a CSS theme, this does not do much visually, but is an important groundwork. As the patch addressed Dries' Steven's and my issues, and worked fine, I just needed to clean up the phpdoc on the RTL CSS addition, so people not aware of RTL issues should better understand.

Awaiting RTL patches for core themes. :)

seems like this deserves mention at the D6 theme upgrade docs, no? http://drupal.org/node/132442

or perhaps at the module updated docs? (since i guess it is a change to the drupal_add_css() function, which is normally called by modules, not themes): http://drupal.org/node/114774

i can't really decide the best place, but it should probably be mention in one of those 2 that this change happened for the D6 API.

thanks,
-derek

Derek: good point! I have included a small writeup about the changes in the theme update guide and the module update guide. Linked the module guide piece to the theme guide paragraph, because it is obviously more verbose.

BTW in the meantime I also noticed that some RTL CSS files had the CVS Id comments improperly, so went and fixed them too.

The previous patch seem to have missed RTL-ing the 'misc/print.css' used by the book module.
The attached patch fixes this.

The patch is applied on the test site. See an example here: http://dev.drupal.org.il/he/book/export/html/14

I did not get any feedback about the patch from the previous comment. I believe that without it the issue is not fixed.

Please let me know what do you think.

Thanks for the follow up! I don't think we need the file_exists('misc/print-rtl.css'), there is no such check for print.css either. Otherwise this looks good.

ok, revised patch attached.

Thanks for the patch. Although your last patch missed print-rtl.css, I got that from the previous one. Committed!

Awaiting the remaining themes: a pushbutton patch and a garland/minnelli patch.

Shouldn't we assign the rtl direction to the html not by means of style but as an attribute to the html? It's the content (HTML) that's right-to-left by nature, not the presentation (CSS) anyway. Of course we'll need to change (maybe switch) anything that reads "right" or "left" in the css files.
This way, by adding a dir="rtl" attribute to the tag, when viewing page with no style (or the browser's default style, if you like to call it) you'll still be able to view the hopefully semantic content in full RTL glory! It's more semantic than adding a direction: rtl; to the style, which is separated from content.

robinhood1362: that's a different issue to adding RTL CSS files. Please open a new issue and reference it here with a follow up comment.

I find it extremely awful that the sidebar remains in the same position regardless of the text direction. Let only its consequences on the logo and primary links positioning.

@youcha: well, it only depends on the theme. Not all themes can swap columns with CSS, it depends on the markup. Not all themes ship with column swap. Many themes name their columns "left" and "right" which makes it a bit illogical to swap columns as well.