garland 6 bidi status

two updated files style-rtl.css , fix-ie-rtl
images - added menu-collapsed-rtl.gif (flipped)
page.tpl.php - added code for inclusion of fix-ie and fix-ie-rtl

the rtl theme looks good in firefox

open issues for internet explorer :-(
I needed to hide sidebar images (ie render it wrong, no matter the css no-repeat instructions)

logo - part of the site title is displayed under logo image

layout breaks in admin pages that have tables - (even in LTR pages) as far as i can see this is because tabelheader.js (fixed table headers when scrolling - cool ! )
I have seen an issue on ie bug - http://drupal.org/node/134307

there is a patch waiting for review, hope this will solve this issue

note : I have tested it in IE6 only please test with IE7 too

Avior
Dev Art- Drupal Based Services and development

CommentFileSizeAuthor
#35 garland-rtl-diffs_0.patch17.41 KBdruvision
#32 rtl-with-color.jpg14.76 KBdruvision
#30 secondary-menu.jpg26.1 KBdruvision
#26 garland-rtl-diffs.patch17.44 KBdruvision
#24 page.tpl_.php__3.patch1.02 KBavior
#13 garland6rtl-konq.png89.93 KByhager
#11 il.png118.21 KBmooffie
#10 dev-il.png75.6 KBmooffie
#7 garland6rtl.tgz2.39 KBavior
#5 page.tpl_.php__2.patch1.02 KBavior
#1 garland6_rtl_files.tgz3.69 KBavior
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

avior’s picture

avior CreditAttribution: avior commented
FileSize
garland6_rtl_files.tgz3.69 KB

all files attached
2 styles
1 image
1 page.tpl.php here is the change 

<?php
     $iecss = '<style type="text/css" media="all">@import "' . base_path() . path_to_theme() .'/fix-ie.css";</style>';
     if (defined('LANGUAGE_RTL') && $language->direction == LANGUAGE_RTL) {
        $iecss .= '<style type="text/css" media="all">@import "' . base_path() . path_to_theme() .'/fix-ie-rtl.css";</style>'; 
     } 
    ?>
    <!--[if lt IE 7]>
        <?php print $iecss;?>    
    <![endif]-->
?>
Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy commented

Please attach a patch, so we can apply it to our Drupal 7 copy and test. Compressed packages are not really usable.

druvision’s picture

druvision CreditAttribution: druvision commented

Avior's RTL garland is now installed in the testing environment - http://dev.drupal.org.il. You are welcome to test it.

Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy commented

There is only a default install on the test site, so I could only see some possible problems with the tabs: http://dev.drupal.org.il/user where those tabs seem like glued to each other without any spacing. Is this intended? (AFAIR it was not intended in the previous RTL themes).

avior’s picture

avior CreditAttribution: avior commented
FileSize
page.tpl_.php__2.patch1.02 KB

attached page.tpl.php patch for loading the RTL css too (fix-ie-rtl)
used the same method on core add css

avior’s picture

avior CreditAttribution: avior commented

more issues on IE6
in http://dev.drupal.org.il/node/1 i can see only 1 tab link - 'update' but it's not linkable
the first tab 'view' is hidden somewhere ....

where those tabs seem like glued to each other without any spacing

I guess you viewed it using firefox - I think it's a firefox issue that suppose to be solved in ver 3 - i can't find it now, but i am pretty sure i saw this on one of the rtl themes issues.

Avior

avior’s picture

avior CreditAttribution: avior commented
FileSize
garland6rtl.tgz2.39 KB

Good News !
Hi there
we had a workshop today, and we worked on the garland 6 issue
so thanks to Tom Bigelajzen our css expert we had 99% of the issues solved for ie6
the inner tabs and the header & logo issues

i am uploading 2 rtl css file here
style-rtl.css & fix-ie-rtl.css

you can see a demo site in http://dev.drupal.org.il

todo :
1. test in IE7
2. more tesing in ie6

mooffie’s picture

mooffie CreditAttribution: mooffie commented

>
> you can see a demo site in http://dev.drupal.org.il

In Opera, the site name (that is, what's inside the H1 tag) isn't shown where it's supposed to. (But everything else, in Opera, looks good, AFAICS. Good work!)

avior’s picture

avior CreditAttribution: avior commented

Hi Mooffie
good to see you here (at least here)
can you please attach an image

if you can attach a path for Opera fix it would be great since i dont have Opera and i can not test it

Avior

mooffie’s picture

mooffie CreditAttribution: mooffie commented
FileSize
dev-il.png75.6 KB

>
> can you please attach an image

Attached.

>
> if you can attach a path for Opera fix it would be great

I could fidget with the CSS till the logo showed fine in Opera, but by then it would break in IE. I don't have IE, I can't run it on my system, so I'm not the one to hack Garland RTL.

>
> since i dont have Opera and i can not test it

Why don't you install it?

http://www.opera.com/

mooffie’s picture

mooffie CreditAttribution: mooffie commented
FileSize
il.png118.21 KB

I'm attaching also a screenshot of drupal.org.il, where you can see that Opera can manage it.

Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy commented

I also checked dev.drupal.org.il and drupal.org.il and the former seems to have no spacing between the tabs, while the actual site have these nicely.

yhager’s picture

yhager CreditAttribution: yhager commented
FileSize
garland6rtl-konq.png89.93 KB

Konqueror 3.5.5 tests:
* The title is not in the right place
* The hebrew text (who's online) on the right shows the dash and number reversed (as if direction:rtl is missing on the text)

screenshot attached.

tombigel’s picture

tombigel CreditAttribution: tombigel commented

I believe I have a solution for the Opera bug

It involves adding 2 CSS properties to style-rtl.css:

#header{position:relative}
#wrapper #container #header #logo-floater {width:100%;left:0;top:0}

A little explanation:

#logo-floater is absolutely positioned, and I believe this case it's a bug in the main style.css - the #logo-floater element has no relatively positioned parent to be absolutely positioned by, it can cause bugs in some browsers (as you can see here).

Second, absolutely positioned elements in opera "loose" their width (don't know if it's in LTR too) if it is not explicitly set, and an absolutely positioned element should always have a left and top attributes because IE6 (and apparently Opera) have some issues with it (try and set position:absolute to an element and then give it's parent text-align:center, see what happens in IE6 if left is not set, hint: center = 0).

I didn't create a patch for it, because I don't know how. yet.

Tell me if it works (It worked locally for me)
Tom B.

avior’s picture

avior CreditAttribution: avior commented

I have uploded all updated files to dev.deupal.org.il for re checking

what i did :

1. added /* LTR */ to style.css & fix-ie.css
2. updates and cleaned RTL css files
3. added Tom's fix to Opera at the end of style-rtl.css

todo :
recheck to see that all problems has been fix
check in ie7 - i dont think someone has did it

Avior
Dev Art - Drupal Based Services

Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy commented

I checked in Firefox 2.0.0.6 and Safari 2.0.4. The only problem I noticed is that the list items in the user block are aligned left in Safari. The list bullets are aligned right, but the text itself is aligned left interestingly. Otherwise (from what I can tell), this looks good from the interface.

tombigel’s picture

tombigel CreditAttribution: tombigel commented

There are still some bugs in IE6 and IE7, I found a way to fix them but I still need to refine it, see that they don't break other browsers.
I hope to submit it here or create a patch in a couple of days.

Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy commented

We are approaching a beta release soon (I hope), so it would be really nice to wrap this up sooner than later.

druvision’s picture

druvision CreditAttribution: druvision commented

I think the current situation is good enoguh for the beta. We will always get more feedback - the wider it spreads the more feedback we get. The issue discovered by Gabor doesn't seem to me like a showshopper.

Yuval - if you agree with me - would you wrap it up as a cvs diff?

Amnon

Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy commented

We are expecting to release a beta on or close to Monday. Expecting the theme patch warmly! (Also for pushbutton).

avior’s picture

avior CreditAttribution: avior commented

Hi All
attached tar file with 2 patches for style.css & fix-ie.css + 2 new files style-rtl.css & fix-ie-rtl.css
Thank you Tom for all the fixes !

Gabor I can not post the TAR file here , I will send it to your mail box

druvision’s picture

druvision CreditAttribution: druvision commented

I will ask Yuval to post it as CVS diff

tombigel’s picture

tombigel CreditAttribution: tombigel commented

My notes for the files submitted by Avior:

Fixes:
- A missing closing bracket "}" in style-rtl.css caused Opera to ignore previous fixes
- Fixed IE7, Safari and Opera header width and alignment
- Fixed IE7 background bleed
- Fixed IE7 left and right columns positions
- Fixed IE7 "add content" box width
- Moved some lines from "fix-ie-rtl.css" to "style-rtl.css" to fix some bugs Opera and IE7 too (fix-ie affects only IE < 7)
- Text direction in Safari left column fixed. (not by me, don't know who deserves the credit)

Known issues:
- Left column lists in opera ignore padding and margin, couldn't find the cause - I consider it a minor issue.
- Horizontal scrolling is weird, or missing in all browsers except Opera when window size is smaller then 980px, don't know if it's RTL or global issue, anyway, it's minor.
- Collapsible forms background in admin panels has an offset on the top in IE7. I ignored it for now.

Good weekend
Tom B.

avior’s picture

avior CreditAttribution: avior commented
FileSize
page.tpl_.php__3.patch1.02 KB

page.tpl.php patch
there is a change needed in this file because of the inclusion of both fix-ie.css & fix-ie-rtl.css files

attached here

Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy commented

Committed the RTL menu-collapsed image. Now it should be easy to do non-binary patches with the other files there.

druvision’s picture

druvision CreditAttribution: druvision commented
FileSize
garland-rtl-diffs.patch17.44 KB

Here is a cvs diff of all the garland files involved.

This is the time to thank all people for their support.

Improvements done:

I've moved the page.tpl.php code which selects fix-ie.css into template.php.

Known issues

  • Secondary menu on admin/build/block is left-adjusted. Is there something we can do about it?
  • The color picker module uses the LTR files, not the RTL ones.

Those issues may be solved during the beta, they are not show stoppers. So please commit the patch.

druvision’s picture

druvision CreditAttribution: druvision commented

Known issues (updated):

  • Secondary menu on admin/build/block is left-adjusted. Tom / Avior - Is there something we can do about it? Is it related to Garland, or is it related to the core CSS files?
  • The color picker module uses the LTR files, not the RTL ones. Also, it doesn't support BIDI themes. Any idea on how to correct it?

Those issues may be solved during the beta, they are not show stoppers. So please commit the patch.

Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy commented

Some "cosmetic" (coding style) problems I see:

- all CSS files should contain a CVS Id header, see existing files... A /* $Id$ */ should be replaced at the top by CVS, so it is enough to include this (or copy what is in any of the CSS files at the top, CVS will replace everything).
- fix-ie-rtl.css contains lots of empty lines at the top and double empty lines in the middle, which should not be there
- also, all css rules should have a space after the colon
- "phptemplate_get_ie_styles ()" should not have a space, plus garland_get_ie_styles() might be more appropriate here
- style-rtl.css has lots of non-standard comments, eg (note the too many * signs, lack of whitespace):

+/*
+** Markup free clearing
+** Details: http://www.positioniseverything.net/easyclearing.html
+*/

+/**Fixes for IE7 - Does not break other browsers
+ *(should be in separate file?)
+ */
+/*position:relative on these breaks IE7*/

+/*Apply hasLayout to elements in IE7, using standard property "min-height" (see http://www.satzansatz.de/cssd/onhavinglayout.html)*/
+/*fix background bleed in center column*/

+/*Fix right and left cloumns position breaking on window resize*/

- this file also has lots of newlines at the end
- phptemplate_get_ie_styles() has a non-phpdoc comment, and the global keyword not indented

Let's make this integrate smoother with Drupal!

yhager’s picture

yhager CreditAttribution: yhager commented

* Secondary menu on admin/build/block is left-adjusted. Tom / Avior - Is there something we can do about it? Is it related to Garland, or is it related to the core CSS files?

I am not sure I understand the problem. If you mean the second row of tabs (with theme names) then it is correctly aligned. In what browser did you see the problem? Can you include a screenshot?

* The color picker module uses the LTR files, not the RTL ones. Also, it doesn't support BIDI themes. Any idea on how to correct it?

Here too, I do not see any issue with the color picker - it includes correctly color.css and color-rtl.css. Please include a screenshot and browser versions.

For all other CSS files, we *add* the '-rtl.css' files after the standard CSS files and override the directionality declerations. With fix-ie.css, I see you *replace* the inclusion of fix-ie.css with fix-ie-rtl.css. Why did you choose a different behavior for this file?

druvision’s picture

druvision CreditAttribution: druvision commented
FileSize
secondary-menu.jpg26.1 KB

Secondary menu on admin/build/block is left-adjusted. Tom / Avior - Is there something we can do about it? Is it related to Garland, or is it related to the core CSS files?

This seems like an FF2-only problem. It doens't exist in FF3 or IE6. Hence it's low priority. FF2 will soon be repalced by FF3.

avior’s picture

avior CreditAttribution: avior commented

Hi All

For all other CSS files, we *add* the '-rtl.css' files after the standard CSS files and override the directionality declerations. With fix-ie.css, I see you *replace* the inclusion of fix-ie.css with fix-ie-rtl.css. Why did you choose a different behavior for this file?

The fix-ie uses the same method it appends the rtl css , look at the code

$iecss='<link type="text/css" rel="stylesheet" media="all" href="'  . base_path() . path_to_theme() . '/fix-ie.css" />\n';

  if (defined('LANGUAGE_RTL') && $language->direction == LANGUAGE_RTL) {
    $iecss .= '<style type="text/css" media="all">@import "' . base_path() . path_to_theme() .'/fix-ie-rtl.css";</style>';
}
return $iecss;

the tabs line it's aligned correctly , and the margins (less) are firefox2 bug that will be fixed in ff3

Avior

druvision’s picture

druvision CreditAttribution: druvision commented
FileSize
rtl-with-color.jpg14.76 KB

Color picker issues

The color is indeed changed, but the styles of system modules are LTR.

An example is attached.

druvision’s picture

druvision CreditAttribution: druvision commented

The color module screenshot is on FF2

druvision’s picture

druvision CreditAttribution: druvision commented

Hi Gabor,

I would leave the function name as-is. phptemplate_get_ie_styles is a good name which follows the standards of other function names on the same template.php file.

garland_get_ie_styles would create conflicts when someone renames the theme.

druvision’s picture

druvision CreditAttribution: druvision commented
FileSize
garland-rtl-diffs_0.patch17.41 KB

Here is an updated patch.

Comments format

Most common comments format in style-rtl.css is one-line, exactly the same as in style.css:
For example: /* Prevent the previous directive from showing the content of script elements */
Other comment formats have been migrated.

Inclusion of fix-ie-rtl.css

Yhager wrote:

For all other CSS files, we *add* the '-rtl.css' files after the standard CSS files and override the directionality declerations. With fix-ie.css, I see you *replace* the inclusion of fix-ie.css with fix-ie-rtl.css. Why did you choose a different behavior for this file?

My answer: Not true. Take a deeper look at the CSS and see that fix-ie-rtl.css is there (only there's no newline between them).

yhager’s picture

yhager CreditAttribution: yhager commented

My answer: Not true. Take a deeper look at the CSS and see that fix-ie-rtl.css is there (only there's no newline between them).

Right.. I missed the '.' in the '.=' statement.. :)

However, why did you use

and not for the inclusion of fix-ie-rti.css ?
Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy commented

OK, looked through the code, fixed coding style issues and committed this changeset: http://drupal.org/cvs?commit=80333 I hope I kept everything as intended, only modified some coding style stuff.

I know there are minor issues with this RTLization still, so let's tackle them!

Although we are only talking about Garland here, Minnelli is a nice fixed width subtheme, which you might not checked with the styles. (It might also need a style-rtl.css to be usable, as it has its own style.css).

I'd also like to draw some attention to the pushbutton rtlization issue: http://drupal.org/node/148084 There is not much left to make these themes shine in Drupal 6, but we should go that extra mile to actually make it happen. A fully RTL Drupal 6 would be a great product IMHO. Let's make sure to make it happen.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented
Status: Active » Closed (fixed)

Looks like this was fixed long ago.