Problem/Motivation

At the moment the seven theme doesn't has a proper rtl css file. This makes drupal almost unusable for rtl users out of the box. Fieldsets are broken, IE7 is broken, tabs are broken in several ways, ...

Proposed resolution

1) This patch adds a ton of rtl styling to fix everything we can find
2) This patch doesn't fix the overlay, thats a seperate issue
3) Don't forget to add the list-item-rtl.png and task-item-rtl.png beneath this summary

Remaining tasks

Patch in #140 is RTBC. Please open followup issues for any additional RTL issues you may find.

A D7 version of the patch is in #151.

For committer review: A test site with the patch applied is available in #143. Screenshots with some before-and-after comparisons are in #120.

  1. Test in IE7, IE8, IE9, Firefox, Chrome, and Safari. done
  2. Confirm whether the issue with unclickable links in IE7 is resolved. done
  3. Test the installer/updater in LTR and confirm that nothing is broken. done for both LTR and RTL
  4. A few code cleanups (see #123). done
  5. Fix non-active tabs in IE7 (see #125). done
  6. Fix visited tabs (see #131) done
  7. The final patch in #140 should be committed along with list-item-rtl.png and task-item-rtl.png (attached below).

Followup issues:

User interface changes

The rendering markup for secondary tabs changes in Seven (adding wrappers).

API changes

None.

CommentFileSizeAuthor
#151 766458-151-d7.patch17.63 KBxjm
#138 766458-138.patch19.37 KBxjm
#138 interdiff-110-138.txt3.71 KBxjm
#140 766458-140-for-aspilicious.patch17.88 KBxjm
#137 766458-137.patch18.73 KBxjm
#137 interdiff-110-137.txt3.75 KBxjm
#135 766458-135.patch16.88 KBxjm
#131 opera-visited.png8.61 KBxjm
#131 ff-visited-tabs.png14.92 KBxjm
#129 766458-seven-rtl-112.patch17.42 KBtsi
#126 766458-seven-rtl-111.patch16.75 KBtsi
#120 filter-tips-after.png110.39 KBxjm
#120 filter-tips-before.png105.67 KBxjm
#120 node-edit-after.png9.34 KBxjm
#120 node-edit-before.png9.57 KBxjm
#120 admin-content-after.png52.54 KBxjm
#120 admin-content-before.png52.93 KBxjm
#120 admin-config-after.png55.79 KBxjm
#120 admin-config-before.png42.85 KBxjm
#110 766458-seven-rtl-110.patch16.78 KBaspilicious
#107 766458-seven-rtl-107.patch16.79 KBaspilicious
#104 auto-complete.jpg123.21 KBiamjon
#104 oh_the_ie7_horror_01.jpg66.14 KBiamjon
#104 oh_the_ie7_horror_02.jpg115.21 KBiamjon
#104 oh_the_ie7_horror_03.jpg77.49 KBiamjon
#104 suggested-20px-left-padding.jpg9.14 KBiamjon
#101 do-not-have-the-energy-to-come-up-with-a-usefull-patch-name.patch16.29 KBaspilicious
#94 overlay-child-rtl.css_.txt496 bytesJeff Burnz
#93 overlay-child-rtl.css_.txt576 bytesJeff Burnz
#91 sevenrtl-766458-91.patch17.58 KBJeff Burnz
#91 close.png704 bytesJeff Burnz
#88 766458-seven-rtl-88.patch16.98 KBaspilicious
#85 766458-seven-rtl-85.patch16.61 KBaspilicious
#84 seven_shortcuts_766458.png2.3 KBJeff Burnz
#84 seven_skipnav_766458.png5.31 KBJeff Burnz
#81 766458-seven-rtl-79.patch15.88 KBaspilicious
#80 task-item-rtl.png178 bytesaspilicious
#80 list-item-rtl.png225 bytesaspilicious
#71 rtld7-IE7-snapshot.png35.86 KBtsi
#70 766458-seven-rtl-70.patch15.92 KBaspilicious
#69 766458-seven-rtl-69.patch16.14 KBaspilicious
#65 766458-seven-rtl-65.patch16.2 KBaspilicious
#58 766458-seven-rtl-done-with-shortcut.patch16.09 KBaspilicious
#57 766458-seven-rtl-done-with-shortcut.patch16.09 KBaspilicious
#55 766458-seven-rtl-done-with-shortcut.patch16.09 KBaspilicious
#53 766458-seven-rtl-done.patch16.09 KBaspilicious
#52 conditional-stylesheet-WTF.png115.25 KBaspilicious
#49 766458- seven-rtl.patch15.7 KBaspilicious
#48 chrome-table-fail- not scrolling.png50.61 KBaspilicious
#48 chrome-table-fail.png47.68 KBaspilicious
#48 opera-table-fail.png50.53 KBaspilicious
#48 opera-table-fail-not.png57.32 KBaspilicious
#48 ie7-fail.png37.15 KBaspilicious
#48 ie7-fail-second.png46.72 KBaspilicious
#48 ie7-fail-second-third.png42.68 KBaspilicious
#41 ie7.png4.17 KBaspilicious
#40 list-item-rtl.png225 bytesaspilicious
#40 task-item-rtl.png178 bytesaspilicious
#39 seven-rtl-ws.patch14.57 KBpoiu
#36 drupal-766458-36.patch14.58 KBtim.plunkett
#27 ie8.jpg63.98 KBhagit
#25 list-item-rtl.png352 bytestsi
#25 task-item-rtl.png169 bytestsi
#25 seven-rtl.patch14.74 KBtsi
#25 seven-ie7.png23.73 KBtsi
#25 seven-ie8.png23.91 KBtsi
#25 seven-firefox.png29.83 KBtsi
#25 seven-chrome.png27.21 KBtsi
#25 seven-opera.png26.29 KBtsi
#25 seven-safari.png27.62 KBtsi
#13 seven-rtl-draftV4.patch12.76 KBJeff Burnz
#6 seven-rtl.png59.71 KBtsi
#5 seven-rtl-draftV3.png62.9 KBaspilicious
#4 seven-rtl-draftV3.patch9.69 KBaspilicious
#3 seven-rtl-draftV2.patch9.71 KBaspilicious
task-item-rtl.png178 bytesaspilicious
list-item-rtl.png225 bytesaspilicious
seven-rtl-draft.png43.17 KBaspilicious
seven-rtl-draft.patch5.92 KBaspilicious
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

aspilicious’s picture

aspilicious’s picture

Srry for spamming but else I forget...

We also need to add this to the rtl file

table.system-status-report th {
  padding-right: 30px;
  background-position: 99% 50%;
}
aspilicious’s picture

FileSize
9.71 KB

Second patch, secondary tabs border is fixed and some other issues.
You still need the original pictures...

aspilicious’s picture

FileSize
9.69 KB

Another update, fixing the fieldsets in FF, and IE8.
In general this patch only lacks IE7/IE6 support!

aspilicious’s picture

FileSize
62.9 KB

New screenshot compare with first post

tsi’s picture

FileSize
59.71 KB

I have some experience with RTL so I thought I might help here.
here is what I did :
* Installed drupal-7-dev from cvs
* applyed your patch
* enabled locale
* enabled Hebrew (RTL)
* switched to Hebrew

it doesn't look good, there is no overlay , tabs are messed up and there's a huge horizontal scrolling.
what haven't I done that you did ?

tsi’s picture

OK, this is interesting, if I make English RTL, I get what you did in your screenshot but not with real RTL languages (Hebrew/Arabic)

aspilicious’s picture

In IE I sometimes got the result you are talking about...

tsi’s picture

I'm not on IE, this screenshot was taken in FF after switching to Hebrew.

[Edit] I actually don't believe this is a seven issue, I just found it working on this issue and I'm not sure where to put it.

tsi’s picture

OK, Found out what is causing my problems : #759844: Overlay does not work with prefixed URL paths
Like I suspected - this is not Seven nor RTL related but Overlay, sorry.

JohnAlbin’s picture

Subscribe

Jeff Burnz’s picture

@aspilicious - your patch in #4 is looking pretty good, the big scroll bar basically comes down to the skip navigation. I added proper overrides for RTL, however it still took a hit in Safari (all other browsers seemed to fine, but Safari gave a big scroll out to the right in RTL).

If we move the skip link offscreen using "top" instead of the left/right, we can solve the issue. The entire new code for skip link LTR and RTL would be:

/**
 * Skip link LTR
 */
#skip-link {
  margin-top: 0;
  left: 50%;
  margin-left: -5.25em;
  position: absolute;
  width: auto;
  z-index: 50;
}
#skip-link a,
#skip-link a:link,
#skip-link a:visited {
  position: absolute;
  display: block;
  text-align: center;
  top: -10000px;
  width: 1px;
  height: 1px;
  background: #444;
  color: #fff;
  font-size: 0.94em;
  text-decoration: none;
  border-radius:0 0 10px 10px;
  -moz-border-radius: 0 0 10px 10px;
  -webkit-border-top-left-radius: 0;
  -webkit-border-top-right-radius: 0;
  -webkit-border-bottom-left-radius: 10px;
  -webkit-border-bottom-right-radius: 10px;
}
#skip-link a:hover,
#skip-link a:active,
#skip-link a:focus {
  position: static;
  width: auto;
  height: auto;
  overflow: visible;
  padding: 1px 10px 2px 10px;
}


/**
 * Skip link RTL
 */
#skip-link {
  margin-left: 0;
  margin-right: -5.25em;
  left: auto;
  right: 50%;
}

There's still some hairy issues to solve - for example the tabs in Opera 10 are real funky in RTL.

Jeff Burnz’s picture

FileSize
12.76 KB

This patch builds on the patch in #4, its mostly the same except I really changed how primary and secondary tabs work, so we can have proper support for Opera - floats for the good browsers, inline for IE6/7.

Cleaned up the Skip link, other than that its the same. It does add an IE7 style sheet.

Not 100% convinced about my work on the tabs here, it would be better if we can find a way to make them work without the special casing for IE6/7, but I can't see the woods for the trees right now so I'll drop this patch in and let some fresh eyes get on it. However it does work in all browsers LTR and RTL (the tabs that is, and the skip link).

A lot more work needed here for full RTL though, IE6 is pretty rotten in some places and Opera also.

aspilicious’s picture

I just quicktested the patch.
Looks nice...

In opera there are two things that need to be fixed:

1) the border of the tables are somehow fucked up
2) the plus sign of the action link need to be reverted

In IE7 the configuration screen is broken. Probably has something to do with position relative or stuff like that

Chrome and firefox looks great :)

I think if opera and most of IE7 is fixed we need to rtbc this cause now it's hard to see what is still going wrong.

Jeff Burnz’s picture

Yeah, agreed, the Opera table borders are whacked, especially with any draggable stuff. Would be good to get it committed, so we can layer on top of it with another iteration. It is really hard to pinpoint errors with all these RTL patches in flux.

yoroy’s picture

Priority: Normal » Major
Jeff Burnz’s picture

#13: seven-rtl-draftV4.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, seven-rtl-draftV4.patch, failed testing.

Jeff Burnz’s picture

Status: Needs work » Needs review
Issue tags: +RTL
reglogge’s picture

Oh boy, this doesn't apply anymore at all. I get

6 out of 21 hunks FAILED

including the attempt to create several files which already exist:

1 out of 1 hunk FAILED -- saving rejects to file themes/seven/template.php.rej
The next patch would create the file themes/seven/style-rtl.css,
which already exists!  Assume -R? [n] n   
Apply anyway? [n] n
Skipping patch.
1 out of 1 hunk ignored -- saving rejects to file themes/seven/style-rtl.css.rej
The next patch would create the file themes/seven/ie7.css,

@Jeff (since you are now the maintainer of Seven theme #927036: Maintainers role for Seven, Co-maintainer for Bartik): What's best to do? Break this up into smaller issues or try to reroll this one in the hope that it lands before others pop up. We also have #925350: Vertical tabs broken which (I think...) will introduce more conflicts with this existing patch from #13.

Jeff Burnz’s picture

Status: Needs review » Needs work

Lets make a start on this again - we can re-roll #13 and merge in the failed hunks. The critical #925350 is going to get in first I imagine so we just account for that later if need be, what we really need is patch to get testing with. I know I did some fairly extensive testing when I wrote #13 but I also know it wasn't *that extensive*. If we can bring in your patch for the autocomplete throbber that would be good also.

Right now I am working on the Toolbar RTL stuff (as in tonight) so if you want to grab this and re-roll that would be great.

reglogge’s picture

Just as a reminder to myself: The patch from this #925698: Autocomplete throbber misplaced for RTL in Seven theme should go in here, too.

sun.core’s picture

Priority: Major » Normal

Demoting to normal. This issue was bumped to major without stating any reason.

tsi’s picture

I would keep it as major, the administration interface of drupal (still) looks bad for rtl users and there's a thread of rtl patches that depend on each other and this is probably the main one, probably not critical but this is why we invented major, isn't it ?
It's been a while, I would like to get this moving, is this still the place or is there any other issue regarding Seven's rtl ?

tsi’s picture

Priority: Normal » Major
Status: Needs work » Needs review
FileSize
27.62 KB
26.29 KB
27.21 KB
29.83 KB
23.91 KB
23.73 KB
14.74 KB
169 bytes
352 bytes

Well, the patch from #13 won't apply anymore and it seems like a lot has changed since it was created, so I decided to start over.
I did keep Jeff's fix for ie7 which works great but couldn't replicate what you did for tabs in Opera - which is still an issue, maybe the only important one left.
This patch was tested, on a lot of browsers, but can always use some more testings, even if not perfect - this is certainly an improvement compared to what we have today and I really hope this will get committed before 7.01
I'm attaching the images to put in the images folder, the patch and some screenshots from ie7, ie8 (in ieTester), chrome, opera, safari (for win), and firefox (for linux).

I'm promoting to major as the current rtl theme is really broken, IMO titles that go off screen (and this time *not* in ie only) is more then just a design issue but an important usability issue.

tsi’s picture

Seriously ? no interest ?

hagit’s picture

Priority: Major » Normal
FileSize
63.98 KB

the last version seem good..
there is only one little final touch-up in ie8. Image is attached.

Jeff Burnz’s picture

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

Moving to D8, back-port to D7 when done. I've listed this on my hit list of things that really need to be fixed in Seven.

tsi’s picture

Jeff, with all due respect, and I really respect all your contribution to this community, I don't see how your last comment helps getting this patch ready.
I've put a lot of work into #25 more then a month ago, rewriting this from scratch, hoping it will get into 7.01 and people will stop working on an admin theme that looks like this (and this is not even IE).
As I see it, pushing to 8.x and adding your personal tag means you take the responsibility for this task to yourself, great community work, lets just hope you'll find the time to do it.

Jeff Burnz’s picture

tsi - not sure if you are aware of this but I am the maintainer for Seven theme so I have overall responsibility to see this fixed, that is why I added the tag (I want to track the really important Seven issues), please don't take that the wrong way, I have not assigned myself to the issue, we need your patch as I can see its very close, its fantastic work and we really need you on this issue.

It makes sense to see this fixed in D8 first then back-port it. The patch will apply to D8, so no issues there, what we need is new enthusiasm as it appears efforts took a downturn the day D7 was released, the new energy in is D8, so lets forge ahead and get this thing fixed.

tsi’s picture

I appreciate that, only that most of the people, at least in our modest Israely community, which I counted on at least for testing this patch, are *still* afraid of touching D7 (partly because of issues like this one), so I believe they will not dare touching anything with a D8 tag over it.
I am aware you are the maintainer of Seven, that's why I hoped a comment from you may get things moving around here and not scare people away.

jcisio’s picture

tsi, back port or forward port, there is an issue for it #1050616: Figure out backport workflow from Drupal 8 to Drupal 7 and it is likely that there will be two parallel issues for each "real" issue in D7 and D8. But currently, it is always "fix in D8, then backport".

About personal tags, IIRC, only Dries and webchick have this permission. Thus, sorry Jeffs, I'm removing it.

Jeff Burnz’s picture

I hear what you are saying in terms of getting people involved, is it possible to form a group (g.d.o?) or locally so you can be better organized in terms of issue identification, patch reviews and discussion? I'm prepared to put in some patch review time for sure, I'm sure others would as well, a testing site would really help reviewers (we do this for accessibility reviewers to save them the trouble of having to install dev versions of Drupal and so on).

Jeff Burnz’s picture

x post,

@jcisio, ok good post to read, frankly I have major issues with the back-port-by-default process and hope this gets resolved.

tsi’s picture

@jeff, we have a local community site at drupal.org.il, but unfortunately it's more about people who try to use drupal and less about people who try to improve drupal.
Can you please elaborate on how a test site could help here ? we need people to look at the code and fix it rather then look at the final result and use it (as in accessibility reviewers).

Turning back to this issue, I believe the only thing we miss here is the tabs fix you did in #13. I've tried to replicate that in #25 but I may have missed something for IE8 and I know there is some problem still in opera's tabs.

finally, whether these last issues get solved sooner or later - what we have in #25 is way better then what is in core ATM, is there an issue about committing an improvement ? even if not a fix ?

tim.plunkett’s picture

FileSize
14.58 KB

The old patch was pre-git, so it didn't apply. Reroll!

poiu’s picture

Priority: Normal » Major

I've tested this on D7 (not D8), and while it's not 100% perfect #36 is a vast improvement over the current state of affairs.

The only reason I'm not changing the status to RTBC is that this is a core patch and http://drupal.org/node/156119 says "thorough review and test by one or more experienced developers". I am changing the priority though, because without this patch the default admin theme, and by extension D7 is currently unusable for RTL websites (see fieldset legends in tsi's screenshot). In fact, I'm having a hard time resisting the urge to bump priority to critical.

If someone can make absolutely sure that this doesn't break anything in LTR sites (I think the only thing that could possibly do that is the new ie7.css file) then please mark this as RTBC or just merge it into core.

aspilicious’s picture

Status: Needs review » Needs work
+++ b/themes/seven/ie7.cssundefined
@@ -0,0 +1,22 @@
+  padding: 0; ¶

trailing white space

Powered by Dreditor.

Srry don't like to do this :s

poiu’s picture

Status: Needs work » Needs review
FileSize
14.57 KB

Removed trailing whitespace.

aspilicious’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
178 bytes
225 bytes

There are still some small problems with rtl, but its hard to track them down without getting this committed.
The IE7 stuff doesn't break the primary tabs. (see screenshot)

RTBC

can put back to needs work when it gets committed but drupal needs this!!!!
I promis I will work on improvements.

PS:
Don't forget to add the rtl images, I attached them again.

aspilicious’s picture

FileSize
4.17 KB

Here is the IE7 screenshot

webchick’s picture

Status: Reviewed & tested by the community » Needs review

Hey, folks! Thanks for your hard work in making Drupal 7 work better for RTL languages! :D

Trying to wrap my head around this patch, and all the thorough screenshots by tsi in #25 really help.

It's a bit hard though, now that D7 is a stable production release with several thousand sites running it, to commit "half fixes". :\ I'm up for discussion on this, but I'd *vastly* prefer to get the last of these issues hammered out before committing this patch. We also definitely need Jeff's sign-off on this code before committing, as the Seven theme maintainer.

Bumping back to "needs review". I think Jeff's statement about a test site is we could "crowd-source" identification of browser issues, and get a hit list ready of additional problems, and make the call on whether they have to be fixed as part of this patch or not.

Jeff Burnz’s picture

Is it possible for someone to setup a live RTL site for us to use for testing, this would be very helpful for testers (me included). If server space is a problem then by all means ping me and I can give you server space, but I really really would love it if someone could set this up and maintain it (apply the patches etc).

We need a proper summary of what this patch fixes, and what it does not. Please list out the specific issue each chunk of code is fixing - this makes it much easier to review and test - especially for Dries and webchick who come into this pretty cold - they don't have hours and hours to poor over and grok these complex theme issues so lets make it easier for them.

tsi’s picture

Setting up a test site.

tsi’s picture

Test site is at http://d7.rtl-themes.co.il, default language is hebrew (RTL) but you can also go to http://d7.rtl-themes.co.il/en/ for an english version (also RTL).
It is a D8.x clone with this patch (only) applied.
I guess overlay's patch is the first one you'll notice missing, I can add it as well.
Front and admin theme are both Seven.
I have also created a test page with some HTML elements (borrowed from Style Guide)

U: test
P: test

aspilicious’s picture

The patch is almost the same as the one in #13. So Jeff should know what is going on.
I admit I put this on rtbc to fast.

SHORT SUMMARY
*************
See #14 and #15 why this should be commited now

"Posted by Jeff Burnz on June 4, 2010 at 1:24pm
Yeah, agreed, the Opera table borders are whacked, especially with any draggable stuff. Would be good to get it committed, so we can layer on top of it with another iteration. It is really hard to pinpoint errors with all these RTL patches in flux."

What is fixed
- tabs (partly ==> needs testing and maybe some tweaking)
- configuration page (almost ==> lines are gone for IE7)
- shortcuts
- skip to content link
- ...

(need to go, better summary will follow tomorow)

mightyiam’s picture

subscribe

aspilicious’s picture

Ok now the real summary
------------------------

A few months before release there were a lot of places in core that hadn't have any rtl styling.
- toolbar
- shortcuts
- dashboard
- seven!
- overlay
- locale
- ...

The stack of css dependencies without rtl made it very hard to to pinpoint errors. Thats why it took ages before a patch got commited

When drupal 7 released only the toolbar and shortcut stuff was fixed. And we gave seven a basic rtl css file. But that file was very very incomplete. This means drupal 7 was almost unusable for our rtl users. As the following things were still broken in rtl.

- fieldsets
- configuration page
- local tasks
- tabs in some browsers
- skip to main content link
- ...

This patch tries to fix all of this. In #13 Jeff proposed a new way to handle tab positioning to make tabs work (for rtl) in every good browser and all ie versions >= 8. Thats why he had to introduce an <= ie7 css specific file. That part got lost in #25 so I reintroduce it again here because tabs were still messy with the latest patch in #39.

I can show you 1000 screenshots off what isn't broken anymore with this patch but I think it's better for us to focus on the stuff that isn't working yet, and discuss if we need to fox it here.

-----------------------------------------------------------------------------------

Problem1: Table drag isn't looking good for some browsers
My opinion: Fix it somewhere else

Why?
- Normal tables are working
- Its possible that it has something to do with basic table drag rtl support (so seven theme can't do any
thing about it)

- A few examples:
* We compare two screenshots of google chrome of the same pages, once we scrolled the other time we didn't. Different results, so it's clearly a javascript (in combination with js css) problem.

chrome-table-fail.png

chrome-table-fail- not scrolling.png

* Opera table drag looks rly rly weird, but normal tables look good

opera-table-fail.png

opera-table-fail-not.png

-----------------------------------------------------------------------------------

Problem1: IE7 has some serious problems
My opinion: Fix it here (if we are going to fix it quickly!!)

I probably did something wrong while merging the IE7 specific stuff.
We have an IE7 specif file in this patch so we can fix the IE7 stuff in there without braken other browsers

- tabs are broken
- config pages are unusable
- shortcuts are broken

ie7-fail.png

ie7-fail-second.png

ie7-fail-second-third.png

aspilicious’s picture

FileSize
15.7 KB

And now the patch

webchick’s picture

Holy shazbot. Now THAT is a summary! GREAT job, aspilicious, that's super helpful.

I trust Jeff to make a call on the full scope of what needs to get fixed in this patch, but speaking as D7 maintainer, at least the IE7 stuff definitely needs to get fixed. We can't have unusable parts of the admin interface.

Status: Needs review » Needs work

The last submitted patch, 766458- seven-rtl.patch, failed testing.

aspilicious’s picture

FileSize
115.25 KB

While trying to fix IE7 I saw the folowing in my results.

Style.css overrides IE7.css ===> WTF o_O

conditional-stylesheet-WTF.png

I also saw some problems with the conditional includes so I fixed the code (I think). Can someone tell me what is going wrong o_O.

/**
* Override or insert variables into the html template.
*/
function seven_preprocess_html(&$vars) {
  // Add conditional CSS for IE8 and below.
  drupal_add_css(path_to_theme() . '/ie.css', array('group' => CSS_THEME, 'browsers' => array('IE' => 'lte IE 8', '!IE' => FALSE), 'preprocess' => FALSE));
  // Add conditional CSS for IE7 and below.
  drupal_add_css(path_to_theme() . '/ie7.css', array('group' => CSS_THEME, 'browsers' => array('IE' => 'lte IE 7', '!IE' => FALSE), 'preprocess' => FALSE));
  // Add conditional CSS for IE6.
  drupal_add_css(path_to_theme() . '/ie6.css', array('group' => CSS_THEME, 'browsers' => array('IE' => 'lt IE 6', '!IE' => FALSE), 'preprocess' => FALSE));
}
aspilicious’s picture

Status: Needs work » Needs review
FileSize
16.09 KB

Well...

- primary tabs fixed
- secondary tabs fixed
- config pages fixed

:D all IE7 issues are fixed and you can use rtl in IE7 now :D :D :D

Used following code

/**
 * Override or insert variables into the html template.
 */
function seven_preprocess_html(&$vars) {
  // Add conditional CSS for IE8 and below.
  drupal_add_css(path_to_theme() . '/ie.css', array('group' => CSS_THEME, 'browsers' => array('IE' => 'lte IE 8', '!IE' => FALSE), 'every_page' => TRUE));
  // Add conditional CSS for IE7 and below.
  drupal_add_css(path_to_theme() . '/ie7.css', array('group' => CSS_THEME, 'browsers' => array('IE' => 'lte IE 7', '!IE' => FALSE), 'every_page' => TRUE));
  // Add conditional CSS for IE6.
  drupal_add_css(path_to_theme() . '/ie6.css', array('group' => CSS_THEME, 'browsers' => array('IE' => 'lt IE 6', '!IE' => FALSE), 'every_page' => TRUE));
}

Based on http://drupal.org/node/1031344

Here is tha patch :D

Jeff Burnz’s picture

Great, when this passes lets get that onto the testing site and I will put aside a few hours tomorrow to go over it, wow I'm so impressed with the overviews and work here, really great and making it much more clear for me (sorry my memory does not extend back one year, lol).

aspilicious’s picture

I missed the shortcut in previous patch. I made them work in all the browsers I have. :D

aspilicious’s picture

I made a test site:

www.juflies.be (I have the seven-rtl and overlay-rtl patch applied)

user: drupal
pass: drupal

Srry for the slow server, I'm a student and can't pay for an aquia server ;)

aspilicious’s picture

Damnit tabs in my patch...

Lets try my new trick...

aspilicious’s picture

And this one doesn't have intendation issues...

* I need to find a way to not mess up my patches *

tsi’s picture

Great work aspilicious,
I have updated the http://d7.rtl-themes.co.il/ test site (U: test P:test) with your patch (only, no overlay patch), I'm not on a windows machine so can't test IE but looks fine on all of my browsers.
I had to manually create ie7.css but that may just be me.

aspilicious’s picture

I still don't know what is wrong with the code in #52 I'm trying to ping people in irc for the past 24 hours without any success. I think we need an answer before we commit this.

tsi’s picture

Don't you say in #53 ":D all IE7 issues are fixed and you can use rtl in IE7 now :D :D :D" ?

aspilicious’s picture

Yes but I rewrote the code.

I replaced

'preprocess' => FALSE));

with

'every_page' => TRUE

Because first one didn't load the IE7.css behind style.css (see screenshot in #52). But I don't know if that is correct. A drupal_add_css expert should help us here.

tsi’s picture

Is this RTL specific ? if not, it better go in a separate issue, don't you think ?

aspilicious’s picture

Not rtl specific but we can't afford to introduce a performance problem.

aspilicious’s picture

FileSize
16.2 KB

Found an additional missing rtl style:

http://www.juflies.be/#overlay=admin/people/permissions/roles
The input box needs to be on the right side

Fixed that in this new patch...

tsi’s picture

The input box is positioned by the user module, not by seven.
We should be careful not to introduce rtl css rules that don't have an analogous rule on the ltr side.
My rewrite in #25 was supposed to be a perfect mirror of the ltr style sheet, I try to validate that by having an /* LTR */ comment in the ltr css for every line in the rtl one.

aspilicious’s picture

tsi you're right #58 is still the patch to review than :).
Either way it needs to be fixed. Someone has to open a new issue for that ;)

aspilicious’s picture

Do I need to beg for a freaking review and an answer to my question :s...
Chances are I need to reroll this again if other patches get in.

aspilicious’s picture

FileSize
16.14 KB

Ok, I just needed to set a weight on the conditional css.

Summary
*******
1) This patch adds LOTS of rtl styling (so it can't break current drupal sites!)
2) It DOES change the way we handle primary tabs because they brake in opera
==> I did A LOT of testing with my mini browser park (chrome, firefox, IE7-IE9)
==> you can see the tabs still work on the screenshots in #48

3) There are 2 test sites (first one loads slowly)

www.juflies.be (english)
user: drupal
pass: drupal

and

http://d7.rtl-themes.co.il/ test site (U: test P:test)

4) I fixed IE7

Tsi says it works, I can't find any issues anymore (that need to be fixed in this issue).
So could we please rtbc this?

Besides for the primary tabs this a no brainer...

aspilicious’s picture

FileSize
15.92 KB

This reroll makes it easier to review...

tsi’s picture

FileSize
35.86 KB

Nice work aspilicious, I've spent the last hour testing around, and it looks good from here, even on ie7, not that it deserves it :), attaching IE7 screenshot for credibility :)
I actually still had to manually create ie7.css but I believe that it's my IDE's partial git support to blame.
I guess my rtbc is not valid here, but FWIW, I give it anyway, now let's get at least one more ?
I've updated my test site to the latest patch - seven as both front-end and admin theme and no overlay :
http://d7.rtl-themes.co.il (hebrew), or http://d7.rtl-themes.co.il/en/ (RTL english) - U:test P:test
so you're welcome to click around.

aspilicious’s picture

You forgot to upload the images from the original report.
Thnx for making these test sites.

tsi’s picture

Hi, you're right. I tested on my local machine and forgot to upload them to the test site.

good_man’s picture

++ for committing with the next version as 7.4 shipped with lack RTL in seven. I've tested it, it looks nice and pretty good job.

I've opened an issue for the user RTL problem #1204844: User module lacks a little RTL.

aspilicious’s picture

Bumping this again... Argh :(

good_man’s picture

+body {
+  direction: rtl;
+}

Why adding this? Drupal already adds dir="rtl" to html tag.

+ul.admin-list.compact li a {
+  margin-right: 0;
+}
+ul.admin-list li div.description a {
+  margin-right: 0px;
+}

Either all zeros with px or without.

tim.plunkett’s picture

Status: Needs review » Needs work

Zero is unitless, no px please.

cosmicdreams’s picture

Thanks for the links to live sites with this RTL themeing turned on.

I'm testing these sites tonight and I just want to calibrate my expectations properly. When RTL styling is applied to a paragraph of text should that paragraph simply be right aligned or should the paragraph's words be transposed so that the order of the words is reversed.

I'm seeing in every place that has a large amount of text the text is right aligned. Now I might be wrong about this, since I'm basing this off of what little my father was able to teach me about the Hebrew language, but aren't the words themselved written right-to-left?

I can't tell if that goal is met by reading throug the Hebrew version of this RTL site. Can someone else verify?

Beyond that, everything else looked good except the advanced help page. On that page the main thing that appears wrong is that I expected that core modules like Blolck, Path, Search, etc. would all be translateable text. Instead I see English words within a sea of Hebrew characters. This sounds like it's not an issue for this patch though.

+1 from me for this patch.

good_man’s picture

No align right is not enough, direction: rtl is the right property for RTL texts. Re. advanced page it's translation issue (if it's valid) not css thing.

aspilicious’s picture

Status: Needs work » Needs review
FileSize
225 bytes
178 bytes

Ok, corrected points noted by#76, he is correct. No functional changes.
Summary in #69 test sites in #71.

I'm also reuploading the pictures needed to get this working.
The correct names are "task-item-rtl.png" and "list-item-rtl.png" in case d.o. renames them.

Any more nitpicks? Or is it done?
I don't think that #78 isn't an issue or not related to this.

aspilicious’s picture

FileSize
15.88 KB

Why does d.o. eats my patch?

good_man’s picture

Status: Needs review » Reviewed & tested by the community

Nothing to add except RTBC :)

good_man’s picture

Issue tags: +Needs backport to D7

ah forgot the D7 tag!

Jeff Burnz’s picture

Status: Reviewed & tested by the community » Needs work
FileSize
5.31 KB
2.3 KB

+++ b/themes/seven/style-rtl.css
@@ -2,11 +2,249 @@
+ul.primary li a.active,
+ul.primary li a:active,
+ul.primary li a:visited,
+ul.primary li a:hover,

Floating pseudo classes, this is nuts, we should break up the primary links declaration block and avoid this sort of verbosity.

+++ b/themes/seven/style-rtl.css
@@ -2,11 +2,249 @@
+  border-left: 1px solid #BEBFB9;

Upper case hex, should be lower case.

+++ b/themes/seven/style-rtl.css
@@ -2,11 +2,249 @@
+  background: transparent url(images/add.png) no-repeat right center;

Just use background position, we should not call the image twice.

+++ b/themes/seven/style.css
@@ -510,7 +523,7 @@
+  border-right: 1px solid #BEBFB9; /* LTR */

Upper case, can we change to lower please. I know there is inconsistencies in Seven, but we should try to not add more or repeat the same mistake etc etc.

seven_shortcuts_766458.png

focus style missing for shortcuts.

seven_skipnav_766458.png

There's some issues with Skip nav in IE9, this a problem in LTR as well - I think we can either clean this up now, or fix it later for both RTL and LTR, feels kinda odd committing something that I know is broken.

aspilicious’s picture

FileSize
16.61 KB

Hmmm

1) The shortcut stuff should not be in seven theme css, I found the error and it's in shortcut-rtl.css. I'll fix it after this gets in.
2) Don't know how to fix your first point, you wrote this code. Can you explain what I should do?
3) I would fix the skip to link in a followup
4) fixed the other stuff

Jeff Burnz’s picture

Shortcuts module stuff I think we can do now, I mean its one line and cleans it up.

Primary links - apply the layout to the anchor only by splitting it into a separate declaration, so we don't need to overwrite pseudo selectors (do this in style.css) ul.primary li a {....}.

Skip link can wait, agreed, could be a bit of refactoring that needs to be done for IE9/10, not in the scope of this patch.

yoroy’s picture

Status: Needs work » Needs review

test the patch in #85…

aspilicious’s picture

FileSize
16.98 KB

Like this? *hopes*

Jeff Burnz’s picture

Status: Needs review » Needs work

The needs work, it breaks the installer and the primary links stuff is not right, I'm assigning this to myself and working up a patch today.

Jeff Burnz’s picture

Assigned: Unassigned » Jeff Burnz

Ops...

Jeff Burnz’s picture

Assigned: Jeff Burnz » Unassigned
Status: Needs work » Needs review
FileSize
704 bytes
17.58 KB

I've fixed up the primary links stuff, added some basic styles to clean up overlay - overlay probably needs more work but can be done in a follow up issues, I just made it good enough to work (title, tabs, close button).

For this patch to work you need the new close.png image attached to this issue.

I have not fixed the installer issue - so this still needs work - the problem is the clear: left; on #content, the installer has a left column so the main content clears it thus breaking the layout during installation.

Setting to needs review to trigger the bot, but really this needs work for the clearing issue.

If this can be fixed up then I think we're good to go, please check my patch also.

Status: Needs review » Needs work

The last submitted patch, sevenrtl-766458-91.patch, failed testing.

Jeff Burnz’s picture

Status: Needs work » Needs review
FileSize
576 bytes

OK, something went wrong with the diff there, it also left out a new file which I am attaching here, I have run out of time for this today, if no one picks this up I can come back to it on Sunday. My apologies.

Jeff Burnz’s picture

Status: Needs review » Needs work
FileSize
496 bytes

OMG, wrong file. Its not my day people...

This is the file that is missing from my patch, I'm not sure why it didnt get added during the diff.

aspilicious’s picture

Wrong file again?

aspilicious’s picture

The overlay is fixed in #766170: Overlay lacks rtl styling. Please don't mix this.

tsi’s picture

Jeff, why are you mixing seven and overlay rtl patches ? we have a working (rtbc imo) patch in #766170: Overlay lacks rtl styling.
About the clearing, maybe I'm missing something but why not give #content clear:right ? wouldn't that override the previous clear:left ?

tsi’s picture

:) I'm slow...

Jeff Burnz’s picture

I didn't know about the other patch. No need to loose your shirt over it guys.

Fix the regression please.

aspilicious’s picture

Not my intention to be hard :). I don't have time to fix it at the moment.

aspilicious’s picture

Status: Needs work » Needs review
FileSize
16.29 KB

EDIT: some told me this post isn't rly constructive so I'm trying to make it sound happier and more constructive

1) I fixed the installer ==> PARTY!!!
2) There are NO issues left (hopefully) ==> BIGGER PARTY!!!
3) We need testers (to test the installer and the rest of the seven theme in normal mode and in rtl)

Side note:
---------

 body.in-maintenance #content {
-  float: right;
+  float: right; /* LTR */
   width: 550px;
-  padding-right: 20px;
+  padding-right: 20px; /* LTR */
+  clear: none;
 }

That last line "clear: none" fixed the installer.
That is the only difference between the last patch and the previous one.

Srry for the crappy patch name.

xjm’s picture

Tagging.

aspilicious’s picture

Don't mind the whitespace I can fix that in a followup if needed.

iamjon’s picture

Ok I did some testing.
Git complained about whitespace somewhere, but the patch applied.

I have to agree with aspilicious's summary. I this seems commit-able, things can always be added later.

This what I found.

Ie7:

  • The links in structure are still not clickable-this one is the only thing that I found that really messed with functionality (oh_the_ie7_horror_03.jpg)
  • When the screen was minimized. Text in the menu went all wacky...on the other hand it dissapeared five minutes later. (oh_the_ie7_horror_01.jpg)
  • The tabs still look funny (oh_the_ie7_horror_02.jpg)

General
The auto complete background image needs to background position: right 0...or have 20 padding on it because it interferes with the text.
(auto-complete.jpg)
And
suggested-20px-left-padding.jpg

iamjon’s picture

Status: Needs review » Reviewed & tested by the community

Changed status

aspilicious’s picture

Status: Reviewed & tested by the community » Needs work

Sadly this needs work, it worked but we broke it again somewhere in the latest patches.

aspilicious’s picture

Status: Needs work » Needs review
FileSize
16.79 KB

- Auto complete is cause by system module
- Toolbar is toolbar.module (and I don't think we can fix that)
- The other issues are caused by the missing ie7 rtl file o_O. The patch in #91 lost that file for some reason.

So I fixed that

tsi’s picture

Good work aspilicious, may you never have to reroll this again.
Let's pray this will be good enough for the git gods, as you wrote before (and edited :) ) we are all sick of this issue, the rest is all here.

You have my RTBC, but I leave the tag change to someone unbiased.

Hamid.D’s picture

When I use command "git apply patch-name" I get the following error.why?
I'm using Drupal 7.4

766458-seven-rtl-107.patch:182: trailing whitespace.
 
error: patch failed: modules/shortcut/shortcut-rtl.css:28
error: modules/shortcut/shortcut-rtl.css: patch does not apply
error: themes/seven/ie7.css: already exists in working directory
error: patch failed: themes/seven/page.tpl.php:11
error: themes/seven/page.tpl.php: patch does not apply
error: patch failed: themes/seven/style-rtl.css:2
error: themes/seven/style-rtl.css: patch does not apply
error: patch failed: themes/seven/style.css:19
error: themes/seven/style.css: patch does not apply
error: patch failed: themes/seven/template.php:17
error: themes/seven/template.php: patch does not apply
aspilicious’s picture

FileSize
16.78 KB
766458-seven-rtl-107.patch:182: trailing whitespace.

Fixed this

error: themes/seven/ie7.css: already exists in working directory

This is impossible this means you alrdy patched the seven theme before you used the latest patch. You should use a clean seven folder and a clean shortcut folder before you can apply this.

Status: Needs review » Needs work
Issue tags: -RTL, -Needs issue summary update, -Needs backport to D7

The last submitted patch, 766458-seven-rtl-110.patch, failed testing.

aspilicious’s picture

Status: Needs work » Needs review

#110: 766458-seven-rtl-110.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, 766458-seven-rtl-110.patch, failed testing.

aspilicious’s picture

Status: Needs work » Needs review

#110: 766458-seven-rtl-110.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, 766458-seven-rtl-110.patch, failed testing.

aspilicious’s picture

Status: Needs work » Needs review
Issue tags: +RTL, +Needs issue summary update, +Needs backport to D7

#110: 766458-seven-rtl-110.patch queued for re-testing.

Bojhan’s picture

Status: Needs review » Reviewed & tested by the community

This is major, and has been re rolled many times. Since http://drupal.org/node/766458#comment-4823582 says its RTBC, and jeff seems to be quite busy - I am going to mark this RTBC.

xjm’s picture

Untagging.

aspilicious’s picture

Don't forget the list en task RTL images from the original report!

xjm’s picture

Issue summary: View changes

Made summary

xjm’s picture

I tested the patch as follows:

  1. Set Seven as default theme. (Probably not necessary since admin paths will use Seven by default anyway, but oh well.)
  2. Enabled locale and disabled overlay.
  3. Set Arabic as default language.
  4. Tested several paths without patch in both Chrome and Firefox on a Mac. Many of the following were spectacularly broken.
    • admin/config/regional/language
    • admin/config/user-interface/shortcut/shortcut-set-1
    • admin/dashboard
    • admin/content
    • node/1/edit
    • filter/tips
    • drupal/admin/config
    • drupal/admin/config/development/performance
  5. Downloaded list-item-rtl.png and task-item-rtl.png to themes/seven/images/.
  6. Applied patch from #110.
  7. Cleared site cache.
  8. Revisited the paths above. Everything was much improved.
  9. Set the default language back to English and visited all the aforementioned paths plus more, looking for regressions. Didn't find any.

There are a few minor outstanding issues with the RTL only, but I think they should be opened as followups. Here's some screenshots demonstrating how broken stuff is before the patch and how it looks after.

xjm’s picture

Updated summary.

xjm’s picture

Issue summary: View changes

Updated issue summary.

Bojhan’s picture

@xjm I noticed the misalignment of the configuration headers too, but yhea - thats a reallly really minor issue.

Bojhan’s picture

Issue summary: View changes

Updated issue summary.

Jeff Burnz’s picture

Status: Reviewed & tested by the community » Needs review

If we can just confirm this does not break the installer or update pages (in LTR) and we have no un-clickable links in IE (RTL) the I am fine with this pending the following cleanups:

+++ b/themes/seven/style-rtl.css
@@ -2,11 +2,245 @@
+
+
+/**
+ * Fieldsets.
+ */

Extra line above fieldsets (all the others have a one line break, this has two.

+++ b/themes/seven/style-rtl.css
@@ -2,11 +2,245 @@
+ul.action-links {
+  padding: 0 20px 0 20px;
+}

Is this actually necessary - its padding: 0 20px 0 20px; by default (could be shorthanded to clean up as well).

+++ b/themes/seven/style-rtl.css
@@ -2,11 +2,245 @@
+ul.action-links a {
+  padding-right: 15px;
+  background-position: right center;
+}

Probably makes no difference but shouldn't this have padding-left: 0;

+++ b/themes/seven/style.css
@@ -860,7 +877,7 @@
+  background: transparent url(images/task-check.png) no-repeat 0px 50%;

Some other minor cleanups have been done, we could remove the redundant px

+++ b/themes/seven/template.php
@@ -17,9 +17,11 @@
+  drupal_add_css(path_to_theme() . '/ie6.css', array('group' => CSS_THEME, 'browsers' => array('IE' => 'lt IE 6', '!IE' => FALSE), 'weight' => 999, 'preprocess' => FALSE));

Why the change here, this appears to be targeting < IE6, it should be just "IE 6", i.e <!--[if IE 6]>. This is the only real thing I question here, the rest is just minor formatting/cleanup.

I've set this back to needs review because nothing here is very serious apart from the IE6 conditional statement and I would like just a little more testing accross browser to know for sure the installer and update pages (when you run update.php) are not broken in LTR. So very close, those things confirmed, cleaned up then RTBC it and move on. Yes I am tired of this patch also, but still we should stick to standards, even if they weren't exactly followed in Seven on previous occasions.

12 days to next Drupal core point release.

Jeff Burnz’s picture

Issue summary: View changes

Updated issue summary.

xjm’s picture

I think maybe it was meant to be lte IE 6?

xjm’s picture

Issue summary: View changes

Updated issue summary.

xjm’s picture

Issue summary: View changes

Updated summary for latest testing.

xjm’s picture

Issue summary: View changes

Updated issue summary.

xjm’s picture

  1. Firefox/Mac: Installed 7.4 in English with the patch applied and verified that all screens of the installation were the same as before with no display errors.
  2. IE8: Set language to Arabic and tested the same paths as above; they were fine.
  3. IE8: Checked update.php in RTL, it looks fine.
  4. IE7: Confirmed that toolbar behaves properly after minimizing.
  5. IE7: Primary tabs are still broken. :( Non-selected tabs disappear sometimes (I think if they are not yet visited?) and are displayed without padding/border/background other times. Might be a hasLayout issue?
  6. IE7: Confirmed that links are clickable at admin paths.
  7. IE7: Tested lots of other stuff. A few very minor rendering errors not present in other browsers (misaligned bullets etc.), but nothing that should block this patch.
  8. Switched back to English.
  9. IE7: Updated to 7.7 with LTR and confirmed update.php looks fine. Confirmed that the tabs are still icky.

Unfortunately I couldn't test in IE6 because my IE6 test environment seems to be hosed.

Also, yes, this did take me 2 hours.

tsi’s picture

FileSize
16.75 KB

Fixed the issues from #123.

Status: Needs review » Needs work

The last submitted patch, 766458-seven-rtl-111.patch, failed testing.

xjm’s picture

#126: That patch is malformed. I get this applying it:

patching file modules/shortcut/shortcut-rtl.css
patching file themes/seven/ie7.css
patching file themes/seven/page.tpl.php
patching file themes/seven/style-rtl.css
patch: **** malformed patch at line 307: @@ -14,11 +248,5 @@

Can you reroll?

tsi’s picture

FileSize
17.42 KB

will this one do the trick ?

tsi’s picture

Status: Needs work » Needs review
xjm’s picture

FileSize
14.92 KB
8.61 KB

I tested #129 in Firefox, Opera, Chrome, and Safari on Mac. Chrome and Safari show some of the same odd resize behavior in #1254248: Admin screens in an RTL language causes display errors when the window is resized in some webkit browsers, but that is outside the scope of this issue.

Firefox and Opera both are missing proper layout for non-active, visited tabs, same as I describe for IE7 in #125. See attached.

xjm’s picture

Assigned: Unassigned » xjm

Looking into a simple css fix for the visited tabs issue.

xjm’s picture

Status: Needs review » Needs work

The visited tab issue happens with both LTR and RTL, as does the disappearing tab in IE7, so important to fix.

xjm’s picture

The visited tab issue is fixed by changing style.css:

ul.primary li a:link,
ul.primary li a.active {
  display: block;
  float: left; /* LTR */
  height: 2.60em;
  line-height: 2.60em;
  padding: 0 18px 8px;
}
ul.primary li a:link,
ul.primary li a.active,
ul.primary li a:active,
ul.primary li a:visited,
ul.primary li a:hover,
ul.primary li.active a {
  background-color: #a6a7a2;
  color: #000;
  font-weight: bold;
  border-width: 1px 1px 0 1px;
  border-style: solid;
  border-color: #a6a7a2;
  -moz-border-radius: 8px 8px 0 0;
  -webkit-border-top-left-radius: 8px;
  -webkit-border-top-right-radius: 8px;
  border-radius: 8px 8px 0 0;
}

to

ul.primary li a:link,
ul.primary li a.active,
ul.primary li a:active,
ul.primary li a:visited,
ul.primary li a:hover,
ul.primary li.active a {
  display: block;
  float: left; /* LTR */
  height: 2.60em;
  line-height: 2.60em;
  padding: 0 18px 8px;
  background-color: #a6a7a2;
  color: #000;
  font-weight: bold;
  border-width: 1px 1px 0 1px;
  border-style: solid;
  border-color: #a6a7a2;
  -moz-border-radius: 8px 8px 0 0;
  -webkit-border-top-left-radius: 8px;
  -webkit-border-top-right-radius: 8px;
  border-radius: 8px 8px 0 0;
}
xjm’s picture

Status: Needs work » Needs review
FileSize
16.88 KB

Attached should resolve both the IE7 visibility issue and the visited tab rendering issues in other browsers. I tested in Chrome, Opera, FF, Safari, IE7, and IE8.

xjm’s picture

Issue summary: View changes

Updated issue summary.

xjm’s picture

Status: Needs review » Needs work

Oops, #135 is missing some hunks for the new files, as is #129. Rerolling....

xjm’s picture

Status: Needs work » Needs review
FileSize
3.75 KB
18.73 KB

Revised patch, with an interdiff against 110 to make sure we didn't lose any hunks this time. :)

xjm’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
3.71 KB
19.37 KB

@aspilicious pointed out that the IE7 CSS is a bit redundant; this fixes it.

xjm’s picture

Issue summary: View changes

Updated issue summary.

xjm’s picture

Issue summary: View changes

Updated issue summary.

xjm’s picture

  1. Applied #138 against current 8.x checkout.
  2. Cleared site cache.
  3. Cleared browser caches and tested the following paths in Opera, FF, Chrome, and Safari on Mac:
    • admin/config/development/performance
    • admin/people
    • admin/people/permissions
    • admin/people/permissions/roles
    • admin/config
    • node/1/edit
    • filter/tips

All behaved as expected.

xjm’s picture

#138 is a bit messy with several commits; here's a cleaner one with just git diff against origin. Edit: This is exactly the same code, just cleaner patch format.

I followed the above testing in IE7 as well and everything was fine.

aspilicious’s picture

Status: Needs review » Reviewed & tested by the community

Did same tests. We are *done* with this. Seriously... :D
Also tested installer, updater etc...

Fixed everything, tested everything. Time for a drink!

xjm’s picture

If you find any outstanding issues with RTL in Seven, please open them as separate followup issues so that this major issue can make the next point release. :)

xjm’s picture

Issue summary: View changes

Updated summary.

xjm’s picture

Issue summary: View changes

Updated issue summary.

xjm’s picture

Issue summary: View changes

Updated issue summary.

tsi’s picture

Status: Reviewed & tested by the community » Needs review

And here is an updated test site (latest D7 with the patch from #140).
http://d7.rtl-themes.co.il or http://d7.rtl-themes.co.il/en for RTL english version.
login with test:test

xjm’s picture

I clicked around the Hebrew test site in Chrome and in IE7. Chrome looked pretty much perfect. IE7 was functional.

So long as there is no LTR regression (and there isn't), we should commit this as-is and then address additional RTL issues (e.g. this border in opera, that bullet in IE7, etc.) as followups. The patch makes RTL functional in Seven, when it is not in origin currently.

tsi’s picture

+1 #144
FF6 looks great too.

Jeff Burnz’s picture

Status: Needs review » Reviewed & tested by the community

OK, let this baby fly, we've all had enough, lol.

New issues to be opened as new issues - definitely.

Jeff Burnz’s picture

Issue summary: View changes

Updated issue summary.

xjm’s picture

Issue summary: View changes

Updated issue summary.

xjm’s picture

Summary updated. :) Thanks Jeff.

xjm’s picture

Issue summary: View changes

Updated issue summary.

xjm’s picture

Issue summary: View changes

Updated issue summary.

xjm’s picture

Issue summary: View changes

Updated issue summary.

Hamid.D’s picture

Status: Needs review » Reviewed & tested by the community

@aspilicious, What do you mean by "clean seven folder"?I replaced seven folder with the original version in drupal-7.7 but I get this error:

Checking patch modules/shortcut/shortcut-rtl.css...
error: modules/shortcut/shortcut-rtl.css: No such file or directory
Checking patch themes/seven/ie7.css...
Checking patch themes/seven/page.tpl.php...
Checking patch themes/seven/style-rtl.css...
Checking patch themes/seven/style.css...
error: while searching for:
  padding: 3px 10px;
}
#dashboard div.block div.content {
  padding: 10px 5px 5px 5px;
}
#dashboard div.block div.content ul.menu {
  margin-left: 20px;
}
#dashboard .dashboard-region .block {
  border: #ccc 1px solid;

error: patch failed: themes/seven/style.css:920
error: themes/seven/style.css: patch does not apply

Thanks

tim.plunkett’s picture

This applies cleanly to 8.x and 7.7, but a slight difference makes it fail against 7.x. It can be rerolled once this is in.

xjm’s picture

#148: I just checked and the latest patch does apply cleanly to 7.7. I suggest applying from the command line, as aspilicious also mentioned difficulties applying using a different client (possibly because it includes a fix to one file outside the theme).

  1. Download http://drupal.org/files/issues/766458-140-for-aspilicious.patch
  2. Place this file in the root of your Drupal directory
  3. Navigate to the root of your Drupal directory on the command line
  4. Type the following at your command prompt:
    patch -p1 < 766458-140-for-aspilicious.patch
    

You should see something like:

[milankovitch:drupal | Fri 20:41:22] $ patch -p1 < 766458-140-for-aspilicious.patch 
patching file modules/shortcut/shortcut-rtl.css
patching file themes/seven/ie7.css
patching file themes/seven/page.tpl.php
patching file themes/seven/style-rtl.css
patching file themes/seven/style.css
patching file themes/seven/template.php
xjm’s picture

Issue summary: View changes

Just secondary tabs, actually.

xjm’s picture

FileSize
17.63 KB

Rolled against D7. The difference is http://drupal.org/commitlog/commit/2/ce4e673d940921ca2e522bc84aac9b0c52c...

Edit: Never mind, the reason #140 applies to 7.7 and not 7.x is http://drupalcode.org/project/drupal.git/commit/29bdcab, which deleted the hunk of CSS rather than restoring the original, nonfunctional CSS. Mystery solved. :P

webchick’s picture

Status: Reviewed & tested by the community » Fixed

AWESOME. So glad to see this one RTBC. aspilicious, you are a frigging machine, man. Bravo.

Committed and pushed to 8.x and 7.x.

tsi’s picture

Unbelievable.

tsi’s picture

makes me wonder, the one tested by us and marked by jeff as RTBC was #140 but I guess the one committed was #151 ? which was never tested or marked as RTBC, how does these kind of things work ?

xjm’s picture

makes me wonder, the one tested by us and marked by jeff as RTBC was #140 but I guess the one committed was #151 ? which was never tested or marked as RTBC, how does these kind of things work ?

#140 would presumably have been committed to 8.x and #151 committed to 7.x. There is a one-line difference between the two patches, because of a minor difference in style.css between the two branches. (See the commits in my comment in #151 for more information.) It wasn't even actually directly related to RTL functionality and they're otherwise the same patch.

Status: Fixed » Closed (fixed)

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

xjm’s picture

Assigned: xjm » Unassigned
xjm’s picture

Issue summary: View changes

Added link to D7 patch.

donquixote’s picture

Status: Closed (fixed) » Needs work

This caused a regression.

 ul.primary {
-  float: right;
+  float: right; /* LTR */
   border-bottom: none;
-  padding: 0.769em 0 5px 8px;
   text-transform: uppercase;
   font-size: 0.923em;
+  height: 2.60em;
+  margin: 0;
+  padding-top: 0;
 }

With the fixed height on the ul, additional tabs are now cut off.
See #1699072: Multiple rows of tabs not spacing properly in Seven theme, which discussed tabs in multiple rows.

Steps to reproduce:
- Open an admin page with tabs.
- Use Firebug or Chrome developer tools to add more tabs, and/or
- Resize the browser window / viewport so the tabs break to multiple lines.
- See tabs disappear.

Feel free to close this issue again, and open a new issue instead.

xjm’s picture

Status: Needs work » Closed (fixed)

@donquixote, I don't see how this issue could possibly have caused a "regression" now since it was committed over five years ago. Please file a separate bug report for anything you find now.

donquixote’s picture

It seems I cannot read dates correctly. Sorry for the disruption.

The issue with the invisible tabs is already reported here:
#2112955: End tabs invisible when content type edit view has many tabs