RTL-ize the core pushbutton theme

Any updates on this issue? It would be nice to have a patch so others can more easily experiment locally.

Tabs images and borders seem not to be correctly RTL'ed.
See screenshot from http://dev.drupal.org.il/he/admin (using Opera).

Please note Firefox 2.0 has a bug with displaying this.. don't waste your time on this..

The code freeze is postponed by four weeks, so we have time to get in RTL support for all core themes. I'd rather commit a properly working theme change then having this broken on purpose.

There is a weird text-indent style in some of the RTL paragraphs. I couldn't find where it comes from, and why it does not appear in all RTL paragraphs...
We have, again, the Tabs issue, which we hope FF3 will solve.

Besides that, it looks fine.

It happens also in the LTR version so i think it's not RTLing problem
I have check also in ie7 LTR & RTL and it's the same the right column in floated down

I don't think it's a fair request. Gabor should be busy enough reviewing and applying patches, we can't ask him to actually create them.

Are these exact width values used in other themes? Otherwise they look arbitrary and given how liquid is pushbutton, there does not seem to be a reason to have these fixed.

Fixed value is a must??? How is bluemarine admin panel working without a fixed value then? It is surely not a must.

Gabor, pushbutton is just a phptemplate conversion of a previous drupal 4.6 xtemplate theme, which gives a migration path for Drupal 4.6 users. It isn't the most buzzword-compliant theme in the world, but you've decided to include it with Drupal 6, seemingly for the sake of compatibility.

Drupal 6 is far from released, so saying that it is decided that a theme or another will get included in Drupal 6 is quite a stretch. We will see what themes come out of the two Google Summer of Code projects dedicated to this task, and we will see what goes into Drupal 6. It is far from decided now.

I've wasted hours until finding this solution which works. For me this does the work and it's sufficient. As an extra bonus it fixes a bug on the English core theme, which is great!

Why don't copy how bluemarine does it? It is visibly not width constrained.

What about a patch to let people help and collaborate on this issue?

Updated title.

Can someone *please* make a patch out of the RTL CSS?

Here's a patch.
There are new images in the patch. I hope it's fine.
If not, please direct me as to how to create the patch with the new images.

It seems there is an over-use for the 'inherit' value in the style-rtl.css file, even for non-directional properties like font, color, height etc.

'inherit' in CSS means to inherit from a parent element in the document (e.g. body {font:inherit} means that it uses the font that is defined in the 'html' element). We used 'inherit' in this context only for properties we do not want to override with values e.g. when we change 'padding-left' in style.css with 'padding-right' in 'style-rtl.css', the original 'padding-left' property is still being used. Not knowing which value to use for this property, we use 'inherit', so it is taken from a parent element, as it would have happened if it was not defined in 'style.css'.

Please attach a tar.gz (or zip) with the images. Including their existence in the patch does not help :) If attachments do not seem to work well or you, send the images to me by mail. I hope (it seems to be) that this issue gets solved soon, so I will commit the images.

Sent the image files to Gabor, by mail.

I did not receive them (lost all my mail received on Sunday due to crazy system faliures). Please resend.

OK, got the images, and committed them. Let's fix up the CSS patch and get this done.

- yhager already voiced concerns about the inherit properties... why do we need those?
- there are items with LTR? comments in the patch... these should either get removed or replaced with LTR, depending on whether there are overrides in the RTL CSS or not
- the "Needed for the main admin panel to be in two columns" part at the end could use some newlines and identation to make it coding standards compliant (ie. look like all the other rules in the file)

Thanks for keeping up the work!

- Images are in CVS, so they should not appear in the patch.
- LTR? comments are still in the LTR CSS... these should be explained in the RTL CSS, and left LTR (without the question mark) in the LTR CSS

Let's get this patch tested.

RTL CSS file is missing unfortunately from the latest patch. And as said, we would need at least one independent tester who sees this is good RTL-wise.

levavie: independent means "not the one who submitted the patch". So it can be anyone but you :) The patch submitter is assumed to think that his patch is fine :)

I tested the the theme and found a bug... in my FF web developer extension! :-)
It misread the css cascading order, and placed style-rtl.css /before/ style.css, though in the source they are in the right order ...

Besides that - except for the inline li elements, which display bad due to a FF fault - everything looks good.

Patch introduce trailing whitespace.

The blocks page (admin/build/block) has the following behavior:
* text overlaps icons on FF2 on Mac, FF2 on Linux.
* text and icons are far away from each other on Opera 9.23 mac.

Now the strange thing is these are also broken after switching to English!!!! We have tested a clean Drupal 6 installation, in which pushbutton is not broken (thanks Gabor!).

Can any of you guys take a look? Let's finish this already...

I took a closer look at the latest patch and have the following coments

--- themes/pushbutton/style.css	23 Nov 2006 11:05:46 -0000	1.22
+++ themes/pushbutton/style.css	11 Sep 2007 18:46:29 -0000
@@ -8,6 +8,7 @@ body  {
   background-color: #fff;
   margin: 0;
   padding: 0;
+  direction: ltr; /* LTR */
 }

Why is this added to style.css?

@@ -63,6 +64,7 @@ pre {
 }
 .form-item {
   margin-top: 1em;
+  text-align: left; /* LTR */
 } 

Same as above - why do you want to add this to the default style.css?

 .tabs ul.secondary li a {
-  background: #fff url(tabs-option-off.png) left center no-repeat;
-  padding: 10px 0 10px 25px;
+  background: #fff url(tabs-option-off.png) left center no-repeat; /* LTR */
+  padding: 10px 25px 10px 0; /* LTR */
   margin: 0;
 }

Take a closer look at this one - this is what breaks the english version of pushbutton!

Great analysis. Let's clean up the added and modified stuff from the original CSS file.

Here's the cleaned up patch.

The problems with the admin/block page were discovered to be merely FF2 rtl bug. It works correctly on FF3 beta (Gran Paradiso) on Mac.

Haven't had time to review and test this patch yet, but i'd like to please someone of the RTL guys to take a look into case http://drupal.org/node/175121 and verify if color module is broken with RTL CSS files, while there is a hardcoded "style.css" name inside color module...

I have created forum dummy nodes on dev.drupal.org.il
I have found that there is a missing rtl image : icon-comment-rtl.png

another issue is the forum display
i dont know what was the intention
but even in the english version, i get the forum icon under the name of the group

The problem in the screenshot you gave does indeed also happen in the CVS version of pushbutton (i.e. English only, FF2/Linux). Therefore, I suggest not to correlate committing this patch to fixing it.

There is some green background in the screenshot you sent, it was caused by a typo in style-rtl.css. I fixed it, revised patch attached.

Gabor, there are two image files missing from the CVS. I did not include them in the patch, please commit them directly:
http://dev.drupal.org.il/themes/pushbutton/icon-comment-rtl.png
http://dev.drupal.org.il/themes/pushbutton/forum-container-rtl.jpg

OK, added the images. We still need testing.

Hi
another issue i have found
links separator

looks little weired on ff 2.0.0.7 but in ie6 very bad , see attached image
i have looked via developer toolbar , and i think thak the problem is in UL.links LI
the style applied are
/* Rule 48 of /themes/pushbutton/style.css */ UL.links LI { BORDER-LEFT: #ff8c00 1px solid} /* Rule 10 of /themes/pushbutton/style-rtl.css */ UL.links LI { BORDER-RIGHT: #ff8c00 1px dotted}
(why the difference dotted / solid ?)

so i think you should add to style-rtl.css
UL.links LI {
BORDER-RIGHT: #ff8c00 1px dotted
BORDER-RIGHT : none; <---
}

Avior

Take a look here: http://dev.drupal.org.il/forum
On FF, Linux, the container's name background image (forum-link-rtl.png) sits behind the text, since the box of the "a" tag is mis-calculated. Adding display:block to its CSS fixes the problem, but I suspect it to be a rather esoteric bug. Can you have a look on IE and report?

attached ie6 image

I'd like to see this included in beta 2, so we have a complete RTL theme set in the next beta. It would be great if you concentrate on fixing these issues up.

Here's a new patch.
I've cleaned it up from typos and unneeded definitions, and redone some of the images for better quality.

Please test.

Nice, now all that is left is to fix FF :)

z.stolar: if you mean you applied the patch, browsed around the theme, and did not notice issues, then please say that explicitly and mark the issue RTBC. Otherwise we are waiting on people to throughly test the patch.

I checked this some hours ago and there are some bugs inside the layout, but i think this are core (system module) bugs and therefore only inherited... we should fix core quickly and test again. There might come up some minor issues after core is fixed - i don't know for sure.

I committed the latest set of images for the RTL theme, so let's work out the CSS issues and go on :)

Gabor: Yes that's what I meant.
As far as I can tell, this patch can be marked RTBC.

hass: What layout bugs are you referring to?

See my screen shot attached, please.

1. In the right ellipsis there are wrong margins inherited by system.css OL config and not system-rtl.css. Problem is documented in http://drupal.org/node/180596
2. Menu looks broken too as you can see. See also http://drupal.org/node/179957 with more screenshots and other themes. This depends on system styles, too.

And the bugs in http://drupal.org/node/179934

@hass: The bugs you refer to in comments #54 and #55 are either FF bugs, or bugs not in pushbutton.

Are we able to fix or workaround this Firefox bugs? IE doesn't look perfect, too.

Are we able to fix or workaround this Firefox bugs? IE doesn't look perfect, too.

The bugs in firefox were already fixed, will be released in FF3. Release date for FF3 will probably be very close to Drupal 6...

https://bugzilla.mozilla.org/show_bug.cgi?id=121633

Regarding IE - Please post a specific description. A patch would be even better ;)

Are you sure you want to use the "inherit" property for the rtl styles?

Inherit means that it'll take the values of it's parent so I think that has to be changed.

dvessel: Yes. When the original theme does not specify a value it is implicitly inherited. On the RTL theme, I would like the value to be inherited but have to do that explicitly. See here for more info. This works in most cases and is preferable IMO.
Let me know if there is a display problem you see with this.

And why is it preferred? Because of luck? Doesn't look like what we'd want to use.

Look here:
http://www.w3.org/TR/REC-CSS2/cascade.html#value-def-inherit
http://dorward.me.uk/www/css/inheritance/

I'll be more clear. When the LTR style has a left padding set to 5px and you set the left padding to "inherit" for the RTL style, then the RTL style will take the property of the parent container. All elements have different defaults so we shouldn't assume the parent to get our values.

Here are the assumed defaults for various elements when nothing is overridden.

http://www.w3.org/TR/CSS21/sample.html

Another thing to consider is that the parent element may have an their own property set and throw it off even further.

I looked at the other core styles and they all use inherit. I think that has to be reconsidered. From looking at all the reference material, it seems to me that it was not what should have been used.

dvessel: Exactly, all other core styles use 'inherit'. Therefore, I suggest not to block this specific theme on this, and open another issue on the 'inherit'.

Let's consider the following over-simplified case:

== test.html ==
<ul class="primary">
  <li>menu1</li>
  <ul class="secondary">
    <li>menu1.1</li>
    <li>menu1.2</li>
  </ul>
</ul>

And the relevant LTR style:

== style.css v1 ==
ul.primary {border: 1px}
ul.secondary li {border-left: 2px} /* LTR */

If we do not use 'inherit', we can try, naively the following for RTL:

== style-rtl.css v1 ==
ul.secondary li {border-right: 2px}

Which is not what we want, since we will get 2px border both on left and right in the RTL case. So in order to get this right, w/o using inherit, one has to dig in the LTR version of the style, and understand that the left border should be 1px. So one changes the above to:

== style-rtl.css v2 ==
ul.secondary li {border-right:2px;border-left:1px}

But after a while, the original theme maintainer decides to change something in the original theme, so he changes the style.css to:

== style.css v2 ==
ul.primary {border: 3px}
ul.secondary li {border-left: 2px} /* LTR */

He checks and see that he didn't change anything related to directionality (there is no /* LTR */ comment on the line that was changed) he doesn't bother with the RTL version of the theme at all. However, now the RTL version of the theme will have wrong border on the left (1px and not 3px).
Had we use inherit, all this would have worked correctly:

== style-rtl.css v3 ==
ul.secondary li {border-right:2px;border-left:inherit}

I think this method is both easier to implement and maintain, and is actually more correct by definition.

Anyway, this discussion does not belong to pushbutton. If you are still not convinced, PLEASE, open another issue, and let's RTBC this one. Since all the other themes are committed, it would be best to commit this now, and continue discussing this in another thread.

The inherit problem that dvessel points out is going to be compounded by the newly committed css .info overrides patch(es), isn't it?

Let me rephrase what I said above..

When the LTR style has a left padding set to 5px and you set the left padding to "inherit" for the RTL style, then the RTL style will take the property of the *parent* container, *not* what was set inside the LTR style.

He checks and see that he didn't change anything related to directionality (there is no /* LTR */ comment on the line that was changed) he doesn't bother with the RTL version of the theme at all. However, now the RTL version of the theme will have wrong border on the left (1px and not 3px).

The sole purpose of the RTL styles is to override. If there is an override then it's up to the themer to comment his LTR style (even if the style isn't necessary) and comment it /* LTR */ to indicate that there's a corresponding RTL style.

And look at the links I posted. By who's definition is that correct?

@senpai, what you mention isn't really related. This is all about the use of "inherit".

I'll put up another issue about this.

@yhager: Unbelievable that RTL people need to wait for FF 3.0 to see a page properly... ~6-8 years after Netscape... and ~20 years with css... this sounds very strange but i don't know better. There is really no workaround!??? I'm on the fence about installing FF Alpha 3.0... prior beta versions where sooooooo buggy. However I'm really interested how people have build their RTL pages within the last 10 years? *G*

Aside i feel very pissed/stressed if i write static HTML containing Arabic or HE letters... the HTML tags are acting LTR and if it comes to the RTL text the mouse and curser is jumping from right-to-left... is there any trick in Eclipse to stop this behavior or how do you write static HTML containing HE letters? Only for my interests...

@dvessel: In Drupal 5, so far, we're completely over-riding the style.css (as well as all other core stylesheets), meaning we don't even load the LTR styles. Like this, we haven't got the inheritance problem. But it's not the right way to go. We've decided to go with cascading styles in D6, because it's a cleaner, more exact, way of doing things.
I guess there is no escape from being more specific here and there. CSS gives us 98% of the work by cascading only, but we may have to use the *-rtl.css files not only to over-ride, but also to add stuff.
However, we should try and do so only when we have to, that is, only when the inheritance doesn't supply the expected values.
I didn't notice such problems in pushbutton. Do we have any?

@hass: At least in Drupal, the need to right non-latin characters in code is quite rare. We usually use the t() function and then translate through interface.
However most of the editors know how to deal with RTL, and most of the developers know how to deal with those editors who are RTL ignorant ;-)

yhager, in your example with lists, if every li receive global styles it won't work.

Modify styles.css v1 so that all li have a border:

== style.css v1 ==
ul.primary <strong>li</strong> {border: 5px solid green}
ul.secondary li {border-left: 2px solid red} /* LTR */

The secondary li in LTR then has a green border except for the left border, which is red.

If you add the RTL style
ul.secondary li {border-right:2px solid red;border-left:inherit}
The right border will be red, but there will be no left border because it inherits from the ul which has no borders set.

So, it may work in some themes, but may be broken in others.

In reference to my screenshot in #54 i saw the same tab navigation on http://ar.wikipedia.org/wiki/%D8%B9%D8%A8%D8%B1%D9%8A. It works in Firefox. Without a deep review what's different to drupal tab's - it looks fixable without waiting for Firefox 3.

Great insight @hass!

I'm using RTL'ed wikipedia almost daily, but didn't notice that it works there..

Anyway, it seems that adding 'float:right' for the 'li' fixes that on FF!! woohoo :)

I've done the change and it looks fine on my env (FF, opera, konqueror). Can anyone test IE6/7 on this? If this works correctly, I'll post a new patch.

Are you able to provide the patch so we can test, please? :-)

Oh.. aren't you telepathic? ;)

Anyway, here's the patch.

You can also look at http://dev.drupal.org.il, and use admin/bestHebrewCms

My fear with this patch is that it turns the list items to float, which fixes the padding, but may break other things if other floats are present, especially in the IE world. I suggest to quickly test this one, but if it creates other problems, stay with the previous patch.

I confirm the solution works, not only in pushbutton, but also in every other theme I used, including GarlandRTL.
This is a really important discovery :-)
I used FF on Linux.

Remark: While in pushbutton, the UL already has a defined height (1.3em), other themes do not, so in order for this solution to work on other themes, a height might be necessary to be applied to the UL, otherwise it get height of zero (because all of its content is floated).

Hi
this work great
attached screen shot in ff and ie

Avior

Actually, in the image you attached, I identify 3 issues:

• There is a slight difference between the two - the rounded corner is on opposite sides. I guess this is a very minor issue that should not hold us from accepting the patch.
• There is a gap between the tabs and the line beneath - this can and should be fixed by lowering the value of the UL's height. I did not notice it when testing the patch. Could it be a local thing?
• The translation to Hebrew is not done yet... :-) (not at all related to here but worth mentioning)

Please don't forget this patch must go into system module css... this is the source of this tab bugs! As long as the tabs are not fixed in system module - other themes have the same problem... remember http://drupal.org/node/179957. I cannot see system modules css in this patch... so i think this patch needs work to fix core and pushbutton together! Don't workaround this bug in pushbutton them self if this is a core bug, please.

@hass, @z.stolar: I would like to remind you that changing the LI to floats was just a binary experiment. If it doesn't work, we'll revert back to inline list items, as it should be.

Having floats on the page can have other consequences and this is not the scope to handle. Other pages might get broken and I cannot ensure this would not happen. If anyone else is willing to take over this - be my guest. I myself would vote against (my own) last patch and use the one before.

The FF bug HAS BEEN FIXED. Dancing around our tails trying to make it work for the time till FF 3 release, while breaking IE and potentially other stuff is a waste of time IMHO. Not to mention putting this into core system.css.

@yhager: I don't agree with you on that: IE7 is also around for quite a while now, and every single site still needs to deal with IE6 bugs. Thanks to CSS and various CSS hacks, we are able to do it. There is no reason we shouldn't do it with FF.

It's not always fun, fixing those little bugs with ugly, unreasonable bugs, but that's the way websites are built (mostly when it comes to updating a design to work with newer browsers).

As you said "if it doesn't work". But as far as I understand (having no IE myself), the fix didn't break anything, so why not using it? Either way, with or without the last patch, I think it is RTBC.

Heh, let's fix the bugs identified on IE then. The whitespace between the line and the tabs is caused by the tab bar height set to higher then the tab height: height:1.2em; for .tabs ul.primary solves the problem for me and it scales well (ie if you scale the font size up or down, it keeps being there where it should be).
So feel free to fold this into the patch. I don't have IE to test, so I cannot give a solution for the mirrored tab borders.

OK,
I had a very hard time getting pushbutton through cvs checkout (can't figure why - other projects were checked out easily...), so I did a dirty trick - I manually edited the last patch, adding one line to it, and fixing the line counter in the beginning of the chunk... I know it's dirty, but it does the job.

I changed the ul height to 1.2em, and added a line-height of 1em to the same ul (.tabs ul.primary)

Let's get this reviewed!

Guys, as it is explained by lots of people already here. Your use of 'inherit' is not right. You use inherit to solve a problem, but you have incorrect assumptions about how it works. 'inherit' != leave the value which was provided in a previous style sheet in the cascade already, but 'inherit' = get the value from the parent HTML element, whatever it was. Here is an example (from http://dorward.me.uk/www/css/inheritance/):

CSS inheritance works on a property by property basis. When applied to an element in a document, a property with the value 'inherit' will use the same value as the parent element has for that property.

For example, given this style sheet:

.foo {
  background-color: white;
  color: black;
}

.bar {
  background-color: inherit;
  color: inherit;
  font-weight: normal;
}

And this HTML fragment:

<div class="foo">
  <p class="bar">
    Hello, world. This is a very short
    paragraph!
  </p>
</div>

The background colour of the div element is white, because the background-color property is set to white. The background colour of the paragraph is also white, because the background colour property is set to inherit, and the background colour of the parent element (the div) is set to white.

Your understanding of the 'inherit' property is bad. The inherit property should not be used in any RTL style sheet. Seems like the previous RTL sheets got committed with pretty bad reviews (shame on me), as most of them contain this 'inherit' cruft. Guys, 'inherit' is not the solution to your problems, it will break the styles and therefore should be removed.

So far, as you use 'inherit' as a protection against future changes not going down to the rtl CSS file, removing all inherits should not be a problem in this patch. If you think you need to add more /* LTR */ comments to the LTR CSS after removing inherits, feel free to do so. Once this patch is committed, we should focus on removing all inherits from all existing RTL CSS files in a new issue.

So:

- remove all use of 'inherit' from this patch
- test your results
- it should be the same as bfore, or even better, with some bugs fixed

Let's get rolling fast, we are about to release a beta 3 in two days and then focus on a release candidate.

I checked the latest patch and it have a working solution for this tab problem reported in http://drupal.org/node/179957... I tried to convert this for system-rtl.css where this is original broken... so i think it would be a good idea to fix core styles first... so we don't need to overrule them here in pushbutton... i feel there are many bugs that have their source outside pushbutton or other core themes...

Do you like to see this going to Drupal 6, or not? Is there anything which is hard to understand in #88 above? The path to success seems to be clear to me. Let's get the patch rerolled without inherits and retested. I'd also like to get all inherits removed from Drupal core, as this was clearly a mistake, and a miserable review oversight on my part. I'd love if you could help us fix our mistakes before Drupal 6 beta 4 is rolled.

For replacement of "inherit" stuff see patch in http://drupal.org/node/195543, please.

All right, we made core inherit-less (as far as RTL styles go) at http://drupal.org/node/195543, so let's redo this without the inherits, test and get in. It would be sad to release without a complete set of RTL themes.

I took the latest patch from comment #86, and removed all the inherit statements.
I made some minor fixes for tabs.primary and tabs.secondary, and now it looks great IMO.

I installed this patch on http://dev.drupal.org.il.

PLEASE TEST, and mark as RTBC if it works!

Great. I tested the demo, and even the tabs looked fine in FF2 (I see the wikipedia workaround used in the CSS), so committed. If you find any remaining RTL bugs in Drupal, feel free to open new issues and point us to them. I hope that Drupal 6 comes out with a solid set of RTL supporting themes now. Thanks for all the work guys.

@Gabor: ok... you asked for it :-). Here are some links of bug's i know about and that bugs me:

http://drupal.org/node/179934
http://drupal.org/node/194590
http://drupal.org/node/180596
http://drupal.org/node/179957 (bugs my theme, too)
http://drupal.org/node/193604

Not really important
http://drupal.org/node/179823
http://drupal.org/node/179756
http://drupal.org/node/179783 (maybe a won't fix)

http://drupal.org/node/179934 apart from the docs this is already committed in another issue
http://drupal.org/node/194590 not an RTL issue, your example is a German page going wrong with table headers
http://drupal.org/node/180596 no patch at all
http://drupal.org/node/179957 seems like a Firefox bug as commented there
http://drupal.org/node/193604 depends on http://drupal.org/node/113607 so review that first

http://drupal.org/node/179823 depends on http://drupal.org/node/179756 which was postponed for D7
http://drupal.org/node/179756 postponed for D7
http://drupal.org/node/179783 that's a by design as the pager arrows are customizable with translation (although as with breadcrumbs, it was explained that it is not required to swap the arrows)

Let's continue discussion in the specific issues, and leave this one fixed. Thanks for the pointers.

http://drupal.org/node/179957 is broken in IE, too.
http://drupal.org/node/180596 there is a patch... not sure if this needs a re-roll

OT here... last comment.