Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Bojhan’s picture

I am willing to help on this, we should have propper support on this.

Bojhan’s picture

Priority: Critical » Normal

Given that, unless Batrik is in core - this is not critical

jensimmons’s picture

Yes, please. Let's do it. Write the code someone!

JohnAlbin’s picture

Title: http://wap.avacs.net » Add RTL styling to Bartik

Reverting title after comment spam.

Ahmad Abubakr’s picture

I'm interested in this issue I've been working on Drupal RTL (Arabic) websites for about a year now.

delmarr’s picture

FileSize
67.45 KB

Bartik has issues with rtl and fieldsets, the title of the fieldset floats far to the left off the page. I've attached a screenshot.

willmoy’s picture

Project: Drupal core » Bartik
Version: 7.x-dev »
Component: other » Code
Priority: Normal » Critical

Now we have a bartik project, moving there. Marking as critical because it blocks bartik in core.

Bojhan, ball's in your court if you want it!

yoroy’s picture

Version: » 7.x-1.x-dev

From style.css @ arabicalistapart.com:

body, h1, h2, h3, h4, h5, ul, li, p {
  direction:rtl;
  margin:0;
  padding:0;
}

Is that how Bartik should do it as well? Is there a conditional body class available to hang this RTL layout off of?

.rtl h1,
.rtl h2
etc… {
  direction: rtl;
}
jensimmons’s picture

re #10: Let's keep this as critical. The whole point is to get Bartik into core.

re #9 — that's a screenshot of Seven in the overlay. Is the problem it's having unique to Bartik? If Garland is the current public-facing theme, doesn't Seven do the same thing? If it's a problem w/ Seven, file the issue over there. If somehow Seven is being affected by Bartik (?), then let's track down what's happening and fix it.

yoroy’s picture

there's an epic fieldset fixing issue over here: #676800: Fieldsets break design badly that's not yet fully fixed. revisit this as a seperate issue if needed after that issue is completely fixed.

yoroy’s picture

Talked this over with johnalbin in irc:

Unless there is a specific "direction: ltr;" in Bartik's css, you don't need to override it with a "direction: rtl" in the rtl stylesheet.
It takes some getting used to. You can test it by creating a fake language. I had a en-foo language that was RTL on my test site.
So, basically, when Drupal changes the language to an RTL language, it sets all the doctype/html head thingies it needs to get the site in RTL.
So you don't have to specify anything in the CSS for the language direction.
You only need to look for any styling that is relative to the left or right side of an element. And flip it in the corresponding -rtl.css file.

So a patch would add rtl variation of styles that add right and/or lef margins/paddings
& floats?
also check for any background image positioning.

tsi’s picture

I'm on it.
I will be working on the github version and place a patch here, is that the way to go ?

as I understand it I will create :
1. layout-rtl.css
2. style-rtl.css
3. a patch against style.css (probably just to add /* LTR */ comments)
4. and one against layout.css (/* LTR */ comments)

If all is right and I find the time we can expect first results tommorow, this shouldn't be too hard.

emmajane’s picture

Please work on the version that's in CVS on d.o. It is up to date.

iamjon’s picture

Hi TSI,
I can help you out motze shabbat if you need just let me know

iamjon’s picture

FileSize
56.59 KB
48.36 KB

Attached are two screenshots of Bartik in it's current form, with a user logged in and logged out.
I have to update the translations on my test site and add a bit more content but here it is,

tsi’s picture

FileSize
74.62 KB
76.88 KB

@emmajane - I have checked out the version from cvs and it doesn't look as good as the github version.
I still have a lot to learn about cvs so I might not have downloaded the right version.
Attached are two screenshots of Bartik, one from github and one from cvs with the differences marked.
please point me to the right path in cvs.
I can work on it in the next few hours so a quick response will be appreciated.

yoroy’s picture

TSI: any glitches in the CVS version are bugs we should fix. Do use a CVS checkout of Bartik HEAD please.

Jacine’s picture

Ah, this was actually caused by an old sytax error in the #navigation. It's fixed in CVS now. Thank you for pointing it out.

tsi’s picture

FileSize
3.29 KB

As promised...
Here are the files needed for basic RTL support in Bartik.
This is only about adding the directionality overrides - nothing more, nothing less. I will probably open other issues with a few other things I've noticed that need some love.

in the tar :
1. layout-rtl.css
2. style-rtl.css
3. a patch against style.css with /* LTR */ comments
4. and one against layout.css with /* LTR */ comments
5. flipped comment-arrow-rtl.gif

I tested on a few browsers (FF, Chrome, IE7-8) but NOT thoroughly, IE7 has some issues but I don't think they are RTL related.
If someone have problems setting up an RTL test-site I can attach a few screenshots.

[edit] this is my first work against cvs, I hope it is done right.

yoroy’s picture

hmm, thanks but not ideal :) A single patch that creates the two new files would be best. Let me try to reroll what you did. Maybe http://www.yoroy.com/2009/create-drupal-patches-aptana-tutorial can be helpful?

Let me try to reroll what you did.

yoroy’s picture

Status: Active » Needs review
FileSize
113 bytes
4.9 KB

Lets see. This is only converting #22 to a single patch, I did not review nor test the actual styles.

tsi’s picture

Status: Needs review » Active

Still learning the art of patch work, thanks for the link.

yoroy’s picture

Status: Active » Needs review

no problem. status back to needs review

tsi’s picture

Status: Needs review » Active
FileSize
13.5 KB

Your patch won't make the changes to style.css and layout.css, won't it ?
I think now I've got it right :

tsi’s picture

Status: Active » Needs review

don't know why it changed... again...

tsi’s picture

FileSize
13.08 KB

small fix

yoroy’s picture

Correct, i forgot the changes to the existing stylesheets. You're getting the hang of it! :)

stephthegeek’s picture

FileSize
8.52 KB

Rerolled to keep up with HEAD but made no other changes. I spent a couple of hours testing this with some Arabic content and could not find any issues that were related to the theme. My interface translation was only partial and testing was done with a dozen nodes/comments, but the patch looks really solid, nice job!

There's an issue with horizontal scrollbars permanently appearing in RTL in Chrome and Opera. Oddly enough, it's the opposite of the chromium bug reported here (http://code.google.com/p/chromium/issues/detail?id=10533) but the same fix (going from dir="rtl" on the <body> to #page-wrapper { direction: rtl; }) fixes it, but considering this is consistent with core's html.tpl.php, this doesn't feel like an issue to be addressed in Bartik itself -- just noting the issue.

stephthegeek’s picture

Assigned: Unassigned » stephthegeek

Will need updating for #713444: Add a second sidebar

stephthegeek’s picture

Assigned: stephthegeek » Unassigned
FileSize
9.36 KB

Sincerely hoping I'm doing this whole patching thing correctly... here's an updated patch for the second sidebar.

stephthegeek’s picture

FileSize
142.8 KB

And a screenshot of the layout with two sidebars and some RTL content.

jensimmons’s picture

This looks good to me, with a quick review. I am committing it as-is so that we can keep working on the CSS that is affected by this patch without making this patch out-of-date.

This STILL NEEDS TESTING however. Please do test the Bartik head and make sure that Bartik works happily RTL. If we need additional patches, let's talk about that and make them below.

In other words, committed but not fixed!

Jacine’s picture

Status: Needs review » Needs work

Where is the styles-rtl.css file? :)

iamjon’s picture

I'll test it out with later today and post some screenshots

Bojhan’s picture

published on, seems located at the wrong place.

jensimmons’s picture

I committed the patch in #33.
I also committed the image that was in #24.

Is looks from reading the patch in #33 that css/layout-rtl.css and css/style-rtl.css didn't make it into the patch. Dang CVS!
http://drupal.org/files/issues/bartik_rtl_706862_2.patch

Could someone either make a new patch, or simply attach those files to this issue, and I'll get them into CVS.

tsi’s picture

They are in #22 and #24 creates them (but before second sidebar fixes)

jensimmons’s picture

Ah, there we go! Thanks TSI.

Ok, I just added layout-rtl.css and style-rtl.css to Bartik, and committed that add.

Please do test and see if those two stylesheets need updates for the new second column. I bet they do. Steph if you already made the changes to the CSS in those files, you should be able to patch against the latest Bartik Head successfully. Adding new files to CVS is tricky — now that the files are there, you can modify them much more easily. Just make another patch and your changes will go in.

More feedback?

stephthegeek’s picture

FileSize
650 bytes

Here's my layout-rtl.css that was updated for the new sidebar. No changes were made in style-rtl.css from the earlier patch. I suspect I just did something wrong with the patch and it didn't include the new files.

jensimmons’s picture

Status: Needs work » Needs review

Committed revised layout-rtl.css. Fabulous. Thanks steph and everyone.

So NOW we should have all the code and images for RTL support that has been created to date. Please test and let us know what you discover.

Jeff Burnz’s picture

Status: Needs review » Fixed

Can we change this to fixed, to take it off the radar, its a bit confusing to have something that's been committed marked as needing review, if issues crop up we can re-open as needing work.

Status: Fixed » Closed (fixed)

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

Gábor Hojtsy’s picture

Looks like the Bartik theme as it is in Drupal 8 broke this, see #1979392: Bartik layout broken in RTL.