CommentFileSizeAuthor
#123 shortcut.png558 bytesaspilicious
#117 toolbar.png29.39 KBaspilicious
#117 toolbar-rtl-final-fix.patch4.56 KBaspilicious
#112 ie-toolbar.png197.43 KBJeff Burnz
#105 toolbar-shortcuts-rtl_740182.patch5.14 KBJeff Burnz
#103 IE7-Toolbar-RTL.png36 KBreglogge
#98 toolbar.png5.29 KBtsi
#98 toolbar+shortcut-rtl14.patch5.2 KBtsi
#95 toolbar+shortcut-rtl13.patch5.29 KBtsi
#90 toolbar-rtl+shortcut-rtl.patch5.19 KBcasey
#90 smallbuttons.png6.91 KBcasey
#82 toolbar+shortcut-rtl12.patch5.17 KBtsi
#74 toolbarIE6.png7.43 KBaspilicious
#73 toolbar+shortcut-rtl11.patch6.23 KBcasey
#61 toolbarRtlNoOverlay.PNG38.17 KBaspilicious
#61 toolbarRtlOverlay.png37.15 KBaspilicious
#61 rtl-toolbar-shortcutsV10.patch5.97 KBaspilicious
#59 rtl-toolbar-shortcutsV9.patch6.01 KBaspilicious
#57 rtl-toolbar-shortcutsV8.patch5.71 KBaspilicious
#55 rtl-toolbar-shortcutsV7.patch5.76 KBaspilicious
#33 rtl-toolbar-shortcutsV6.patch8.76 KBrealityloop
#28 rtl-toolbar-shortcutsV5.patch8.7 KBaspilicious
#27 ie6RtlToolbar.png6.72 KBaspilicious
#25 rtl-toolbar-shortcutsV4.patch8.61 KBaspilicious
#24 rtl-toolbar-shortcutsV3.patch8.68 KBaspilicious
#20 rtl-styling.png102.87 KBaspilicious
#19 shortcut.png558 bytesaspilicious
#19 rtl-toolbar-shortcutsV2.patch5.17 KBaspilicious
#17 toolbar_rtlv2.patch4.56 KBaspilicious
#15 toolbar_rtl.patch4.36 KBaspilicious
#13 Selection_015.png1.87 KBcosmicdreams
#13 Selection_014.png4.52 KBcosmicdreams
#10 740182-10.patch4.09 KByoroy
#8 rtl-toolbar-shortcuts.patch4.05 KBaspilicious
#2 rtl-toolbar-shortcuts.patch4.22 KByoroy
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Damien Tournoud’s picture

Title: Top level links no good in RTL languages » Toolbar and shortcuts lack RTL styling
yoroy’s picture

Status: Active » Needs review
FileSize
4.22 KB

Yes, that's a better title, thanks. First draft of a patch

Status: Needs review » Needs work

The last submitted patch, rtl-toolbar-shortcuts.patch, failed testing.

Bojhan’s picture

Status: Needs work » Needs review

#2: rtl-toolbar-shortcuts.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, rtl-toolbar-shortcuts.patch, failed testing.

aspilicious’s picture

Status: Needs work » Needs review

#2: rtl-toolbar-shortcuts.patch queued for re-testing.

Jacine’s picture

Status: Needs review » Needs work

Just tried to test this, but the patch no longer applies.

aspilicious’s picture

Status: Needs work » Needs review
FileSize
4.05 KB

A reroll

bleen’s picture

Status: Needs review » Needs work
+++ modules/shortcut/shortcut.css	24 Mar 2010 22:34:35 -0000
@@ -73,8 +73,8 @@
+  padding-left:10px; /* LTR */

missing space

Powered by Dreditor.

yoroy’s picture

Status: Needs work » Needs review
FileSize
4.09 KB

Fixed that and another similar missing space

cosmicdreams’s picture

cool, I'll give this one a test.

cosmicdreams’s picture

I'm testing this right now. I just wanted to report that I didn't experience the issue reported by the OP when I started from a fresh cvs install of d7 and switched english to right to left through the Locale module. I'll see if this clears up some the issues I am seeing though (these issues are not related to the toolbar).

cosmicdreams’s picture

FileSize
4.52 KB
1.87 KB

ok here are some results : (see images)

as the pictures show there are still some issues with the order of the menu items and the overlapping of ui elements and words. I am suspicious of the accuracy of my test though, since there seems to be no change from the cvs HEAD after applying the patch.

Tested with Chrome 5 beta and Firefox 3.5
Testing method: compare and contract the ui before and after the patch, flushed cache. With Firefox I disabled browser-side caching.

cosmicdreams’s picture

So I guess this brings up questions about what we intend the page to look like after the RLT styling is full implemented. What should a perfect RTL toolbar look like?

aspilicious’s picture

FileSize
4.36 KB

I made a new patch, fixing the toolbar.
Needs review :)

aspilicious’s picture

Deleted original comment, need more thinking time...

Edit: I need to reroll this cause we need to reverse the order of the menu items

aspilicious’s picture

FileSize
4.56 KB

Reroll

aspilicious’s picture

This is not done yet...

The add shortcut button, need a proper rtl styling...
You need to add these lines to shortcut.css rtl version to reverse the rounded border:

div.add-or-remove-shortcuts a:hover span.text {
  -moz-border-radius: 5px 0 0 5px;
  -webkit-border-top-right-radius: 5px;
  -webkit-border-bottom-right-radius: 5px;
  border-radius: 5px 0 0 5px;
}

We also need to add rtl style images to shortcut.png (in rtl we always see the hover box starting on the right of the add button)

aspilicious’s picture

FileSize
5.17 KB
558 bytes

And we are learning a lot about patching ;).
This one works great!

Still no support for IE7 or below, but hey it's a start...
You need to replace the shortcut image with the new one I made.

You need the overlay rtl patch if you want some nice results ;)
Opening that issue soon, and I also will refer to the contextual rtl patch!

Seven needs rtl styling to, thats for another patch...

aspilicious’s picture

FileSize
102.87 KB

this patch in combination with this one #766170: Overlay lacks rtl styling gives the result seen in the screenshot :)!

aspilicious’s picture

aspilicious’s picture

See previous post...

sun’s picture

Status: Needs review » Needs work
+++ modules/shortcut/shortcut.css	8 Apr 2010 22:00:19 -0000
@@ -15,8 +15,8 @@
+  margin-left: 5px;

@@ -58,23 +58,23 @@
+  margin-left: 8px;

Overridden in RTL, but does not use /* LTR */ pointer.

+++ modules/shortcut/shortcut.css	8 Apr 2010 22:00:19 -0000
@@ -84,8 +84,8 @@
+  cursor: pointer; ¶

Trailing white-space here.

+++ modules/toolbar/toolbar.css	8 Apr 2010 22:00:19 -0000
@@ -56,9 +56,8 @@
-#toolbar ul li,
-#toolbar ul li a {
-  float: left;
+#toolbar ul li, #toolbar ul li a {

Should be on separate lines.

+++ modules/toolbar/toolbar-rtl.css	1 Jan 1970 00:00:00 -0000
@@ -0,0 +1,25 @@
\ No newline at end of file

+++ modules/shortcut/shortcut-rtl.css	1 Jan 1970 00:00:00 -0000
@@ -0,0 +1,41 @@
\ No newline at end of file

This should not happen.

Powered by Dreditor.

aspilicious’s picture

Status: Needs work » Needs review
FileSize
8.68 KB

Ok another try...

aspilicious’s picture

Ok I fixed IE7 (a stupid float that couldn't be overridden)...

To summarize what is done for lazy people, or people with not much time.
This patch gives full rtl support for the toolbar and shirtcut module.

It is tested in:
IE7
IE8
IE9 (technical preview)
Chrome
Firefox 3.0
Firefox 3.6
Opera 10.5
Safari

Can't test in IE6.
This patch needs the update shortcut.png that you can find in #19...
If you wonna test this please install the patches listed in #21, without overlay rtl patch / seven rtl patch this one is hard to test

FYI: if you install IE9 technical preview you can have developer tools for most ie versions (IE7, IE8 and IE9), thats how I found the IE7 problem

james.elliott’s picture

Subscribing

aspilicious’s picture

FileSize
6.72 KB

I installed IE6 in my brand new xpmode virtual machine.
It looks bad but it "works" as good as the non rtl version, screenshot attached.

aspilicious’s picture

This adds a lil IE6 tweak

realityloop’s picture

Status: Needs review » Needs work

Horizontal scroll with goes crazy in certain areas of the site, see screenshot taken in FireFox 3.6.3

http://img.skitch.com/20100421-t1jnw3byue7i51q4en58m95ny5.png

yoroy’s picture

Any hint on what is causing this?

aspilicious’s picture

I guess some position:relative issues or something like that

realityloop’s picture

I've been able to ascertian that the skip link is causing it, setting it to display: none fixes layout on page, but obviously not the way to deal with it.

realityloop’s picture

Status: Needs work » Needs review
FileSize
8.76 KB

This should fix horizontal scrolling issue from #29

Elijah Lynn’s picture

Status: Needs review » Reviewed & tested by the community

Tested. Looks good.

tsi’s picture

Tested on IE6+, FF 3.x, Chromium.
IE6 is not perfect obviously (rounded corners, transparent PNGs) but who cares...
Looks good.

yoroy’s picture

Cool, thanks all. rtbc indeed.

aspilicious’s picture

#33 I'm curious what you changed...

realityloop’s picture

Status: Reviewed & tested by the community » Needs work

#37 it's in the patch.. added following to modules/shortcut/shortcut-rtl.css

+div#skip-link {
+ margin-left: 500000px;
+}

realityloop’s picture

Status: Needs work » Reviewed & tested by the community

oops.. I accidentally changed status in previous reply

aspilicious’s picture

seems like a little hack, can't we fix this with something nicer?

yoroy’s picture

Status: Reviewed & tested by the community » Needs work

Ew! :) yeah could do with a nicer solution.

realityloop’s picture

Any suggestions? It doesn't need to be that many pixels if thats the issue.. it's just a random number I chose..

sun’s picture

I can't help, because I cannot review the actual changes. Please remove those additional CSS clean-ups - they have nothing to do with this critical issue.

aspilicious’s picture

Come on sun...

1) I just moved some css, so that the order is alphabeticly
I didn't change one line!

2) Look at the rtl version if you want to review...
This makes me sad :(

3) It's not a huge patch so can you please look over the small cleanup?

And if this is a critical issue the other rtl patches are critical too

yoroy’s picture

Well, sun may be strict here, but he's also right. You really have to stick to the actual issue at hand. Sneaking in other changes disrupts other peoples workflow, no matter how harmless they are.

aspilicious’s picture

Argh :(...
I started with patching this thing 2 weeks ago, if at least one of you can promise me to review this today I'll clean this patch. ;)
(I need the promise to overcome the sadness ;) )

realityloop’s picture

I don't quite understand how this is a hack, given that everything has been switched rtl, it seems to make sense to me that the margin for taking the skip link off screen would also have to switch to the other side as well..?

Does anyone have time to enligten me, I'd like to understand.

aspilicious’s picture

Do we have the margin in the normal css to?
Oow...

Wait... I remeber it I think...
Well I'll clean this patch and reroll it and hopefully sun or jacine will give their opinion.

realityloop’s picture

Yes, the default style.css has the following:

#skip-link {
  left:50%;
  margin-left:-5.25em;
  margin-top:0;
  position:absolute;
  width:auto;
  z-index:50;
}

I tried a negative right-margin but it didn't work, hence my solution.

This will still be accesible to screen readers which I think is the primary issue..?

aspilicious’s picture

The negative margin is probably to hide the text.

realityloop’s picture

#50, which is what my large positive margin is for on rtl version :)

sun’s picture

+++ modules/shortcut/shortcut-rtl.css	1 Jan 1970 00:00:00 -0000
@@ -0,0 +1,41 @@
+div#skip-link {
+  margin-left: 500000px;
+}

#skip-link doesn't belong to Shortcut module.

Also, where is the LTR style for this? (it needs the /* LTR */ pointer)

Lastly, this margin will result in a horizontal scrollbar.

+++ modules/shortcut/shortcut-rtl.css	1 Jan 1970 00:00:00 -0000
@@ -0,0 +1,41 @@
+div.skip-link

Broken selector/code here.

Powered by Dreditor.

aspilicious’s picture

Hmm can you try to find the lowest negative value we can use?

realityloop’s picture

#52 It doesn't result in a horizontal scrollbar, it has been tested in multiple browsers (#35), though I agree that it doesn't belong in Shortcut module.

I will split it out and add the pointer at the code sprint in San Francisco today.

aspilicious’s picture

Here is a cleaned version of the patch (I hope I didn't miss anything)

aspilicious’s picture

#52 damnit, thats why we need seven RTL styling!!!!!

That is the place to put that in...
And I fixed the other thingie sun is mentioning...

I'll remove the skip link.

aspilicious’s picture

Status: Needs work » Needs review
FileSize
5.71 KB

Reroll see #56 and #52

sun’s picture

+++ modules/toolbar/toolbar-rtl.css	1 Jan 1970 00:00:00 -0000
@@ -0,0 +1,26 @@
+#toolbar div.toolbar-menu {
+  padding: 5px 10px 5px 50px;
+}

Don't see a LTR pointer for this in this patch?

+++ modules/toolbar/toolbar-rtl.css	1 Jan 1970 00:00:00 -0000
@@ -0,0 +1,26 @@
+* html #toolbar {
+  left: 0px;
+  padding-left: 0; ¶

Trailing white-space here. "px" can be omitted.

+++ modules/shortcut/shortcut-rtl.css	1 Jan 1970 00:00:00 -0000
@@ -0,0 +1,41 @@
+div#toolbar div.toolbar-shortcuts ul {
+  float: right;
+  margin-right: 5px;
+}

(and possibly elsewhere) I believe the margin-left additionally needs to be reset to zero.

94 critical left. Go review some!

aspilicious’s picture

Ok fixed everything, thnx sun!
This needs screenshot reviews now.

I'll also test it.

realityloop’s picture

Status: Needs review » Needs work

So the Skip link text fix not present now?

Screenshot shows result of v9 patch, possibly still needs work.

http://img.skitch.com/20100422-rark2jepqqu181yfg5i94t2jwu.png

This may have been cache issue at my end, it's displaying ok now.
Hopefully someone else can check and mark RTBC

aspilicious’s picture

Status: Needs work » Needs review
FileSize
5.97 KB
37.15 KB
38.17 KB

Install the other rtl patches (see #21) to test this one, cause seven theme, overlay, ... doesn't have rtl at the moment...
After installing those I can't see any rtl error (caused by shortcut or toolbar).
The skip link is part of seven-rtl.css (other issue)

So please install those patches first, else this one never gets commited...

EDIT: PatchV10 fixes a small issue with add shortcut hover links

realityloop’s picture

Status: Needs review » Needs work

#61 feel free to test my patch for "Seven" :)
http://drupal.org/node/778964

aspilicious’s picture

Status: Needs work » Needs review

Your seven patch is far from complete ;). I marked it as duplicate.
Work on #766458: Seven theme lacks rtl styling if you wonna help.

tsi’s picture

Tested last patch on IE7, IE8 (with IETester) FF3.x, Chrome, safari4win on winXP
FF3.x and Chromium on Ubuntu

* Probably nothing - No gray background on hover in IE8 via IETester but that might be related to IETester.
* Menu elements are floated to the left in RTL, that means that they don't flip their order, in RTL - last menu item should be on the left, but I couldn't fix this without breaking IE7.

aspilicious’s picture

TSI

1) please show screenshots. :)
2) "menu items" is that has to do something with the overlay/shortcuts?

tsi’s picture

1) I can create screenshots but there's nothing to show really, it looks the same as your last one in #61
2) I'm not sure I understand the question, but I don't think it does, my point is that "content" - the first menu item (in the toolbar) - should be placed on the right of "structure" - the menu item that comes right after him and so on...

aspilicious’s picture

Ow... I had a patch with that change... who deleted that part :s

aspilicious’s picture

Status: Needs review » Needs work

Gonna figure it out

EDIT: hmm I deleted it to fix IE7, but that wasn't the right way... Need help with it...

EDIT2:

you need to add in toolbar-rtl.css if you wonna help us figure this out (that line doesn't work in IE7)

#toolbar ul li {
  float: right;
}
tsi’s picture

Tried that already, that's why I wrote "but I couldn't fix this without breaking IE7"

Jeff Burnz’s picture

Status: Needs work » Needs review

#61: rtl-toolbar-shortcutsV10.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, rtl-toolbar-shortcutsV10.patch, failed testing.

aspilicious’s picture

Bumping...

Someone with a soluton for the IE problem?

casey’s picture

Status: Needs work » Needs review
FileSize
6.23 KB

Floating elements to the right inside a liquid left floated element seems to be impossible in IE6/IE7; giving the ULs a width makes the list items show up again, but we can't set no width as the number of list items is not fixed.

Therefox I suggest a CSS hack for IE6/IE7 that uses float:left on list items.

aspilicious’s picture

FileSize
7.43 KB

Casey you just disabled rtl for that part of the toolbar. It doesn't look ugly anymore but it isn't really a fix...

aspilicious’s picture

And her eis a discusing about the problem: http://www.sitepoint.com/forums/showthread.php?t=624584

JohnAlbin’s picture

Status: Needs review » Needs work

Subscribe.

catch’s picture

Priority: Critical » Normal

Valid bug but does not block release.

aspilicious’s picture

It does block release as rtl styling is a must for core themes...
We *need* more effort on this....

catch’s picture

An admin-only-rtl-language-only-optional-module-only bug does not meet the definition of critical. That doesn't mean it can't be fixed before release, but we need perspective on what critical issues actually are.

Jeff Burnz’s picture

Status: Needs work » Needs review

#73: toolbar+shortcut-rtl11.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, toolbar+shortcut-rtl11.patch, failed testing.

tsi’s picture

The last patch applied for me only after deleting the first lines :

### Eclipse Workspace Patch 1.0
#P drupal 7

anyway I thought there must be a better way than putting IE hacks in core (how many kittens die when this happens ?), then I remembered that IE really hates floating a elements so I gave them float:none and fixed some issues caused by that move and here is the result, it is not perfect but I guess this is the best we can do with the current structure of the toolbar.

tsi’s picture

Status: Needs work » Needs review

Needs review...

tsi’s picture

Speechless ?

I'll start - the week point of this patch is that it is not a perfect mirror of the ltr style, but in this case a perfect mirror just won't do the trick.

bleen’s picture

RE #84 ... I dont think that is a weakness at all. The only factors that should affect the RTL styling of the toolbar are what looks best RTL and what is the best UX for RTL users. The LTR experience should have nothing to do with it.

tsi’s picture

@bleen18 - The problem is maintenance, when a change is done in the ltr style, it is very easy to adjust the rtl version when they are complementary. but, again, here it just won't do.

yoroy’s picture

Priority: Normal » Major

Bumpity

yoroy’s picture

#82: toolbar+shortcut-rtl12.patch queued for re-testing.

casey’s picture

Status: Needs review » Reviewed & tested by the community

Tested in all browsers available for windows.

Lookin' good!

casey’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
6.91 KB
5.19 KB

Except for the small toolbar buttons...

Added display:block to toolbar-rtl.css:

#toolbar ul li a {
  display: block;
  float: none;
}
aspilicious’s picture

Ie 7-8 looks bad. I will make a screenshot tomorow, also overlay is totally broken in IE6 with this. Don't know if it is related.
Bad review but I don't have a lot of time today.

yoroy’s picture

Status: Needs review » Needs work

I believe you on your word though: needs work.

casey’s picture

I will need a screenshot though; not sure what is going wrong.

I found however that the user links aren't RTLed; Needs work anyway.

aspilicious’s picture

Whoops I was wrong, I was looking at an overlay page, and overlay rtl is not in yet... (we need those rtl patches get in soon, very soon)
But it still needs work for the user links as you pointed out...

tsi’s picture

Status: Needs work » Needs review
FileSize
5.29 KB

OK, this is slightly better,
right to left order in the user links seems impossible in IE so using display: inline in

#toolbar ul#toolbar-user li {
  display: inline;
  float: none;
}

That makes it RTL at least for FF and doesn't break other browsers, needs testing.
I had to remove the line added in #90 and solved the resulting problem with some padding.

casey’s picture

Status: Needs review » Needs work

Almost there: shortcut links in rtl have same small buttons as described in #90 (at least FF).

Jeff Burnz’s picture

+++ modules/shortcut/shortcut.css	2 Aug 2010 17:18:01 -0000
@@ -87,14 +87,14 @@
+  border-top-right-radius: 5px; /* LTR */
+  border-bottom-right-radius: 5px; /* LTR */
+  -moz-border-radius-topright: 5px; /* LTR */
+  -moz-border-radius-bottomright: 5px; /* LTR */
+  -webkit-border-top-right-radius: 5px; /* LTR */
+  -webkit-border-bottom-right-radius: 5px; /* LTR */

Why use the longhand here when we use the shorthand in RTL? Also, the actual border-radius property should come after the browser extensions.

Powered by Dreditor.

tsi’s picture

This one uses the shorthand form in shortcut.css (#97)
@casey - I don't understand, see my screenshot taken from FF/linux

tsi’s picture

Status: Needs work » Needs review
aspilicious’s picture

tsi the black background has to be bigger on the shortcut bar.

bleen’s picture

Status: Needs review » Needs work

In addition to aspilicious' comment in #100 ... IE7 (and IE6 has the same problems it seems) needs some love:
Windows 7 [Running]

yoroy’s picture

Status: Needs work » Needs review

#98: toolbar+shortcut-rtl14.patch queued for re-testing.

reglogge’s picture

FileSize
36 KB

I see different issues with IE6/IE7 and also IE8.

IE6/IE7:
- Highlighting in the Toolbar looks fine
- Highlighting in the shortcuts is off by some pixels

IE8:
- No highlighting at all

Screenshot of IE7 attached. IE6 looks identical.

reglogge’s picture

Just a quickie: for the off-center highlighting in the shortcuts in IE7, you need to do this:

div#toolbar div.toolbar-shortcuts ul li a {
  display: block;
  margin-left: 5px;
  margin-right: 0;
  padding: 0 5px;
}

Without display: block the link doesn’t get its padding applied in IE7

Jeff Burnz’s picture

Issue tags: +RTL
FileSize
5.14 KB

I re-rolled the patch in #99 with a couple of very minor adjustments - mainly adding zoom:1; as this seems to trigger a display: block like behavior in IE6/7 and actually display: inline-block for everyone else. Removed some of the padding on links etc and I think we're either there, or one step closer.

This is not perfect but its very good - we can strive for perfection or live with some warts (not many). tsi has really done a great job here and I really take my hat off, its really something that you got this working so well in IE6/7 at all.

EDIT: BTW - the reason why hover styles do not work in IE8 is because of the filters to apply the shadow, if you take them out the hover styles come back for IE8 - so if we want them back we have to rethink the shadow for IE (in other words use an image).

reglogge’s picture

Status: Needs review » Reviewed & tested by the community

This looks very good indeed. I tested this in all IE versions, FF, Chrome and Opera both on Mac and Windows.

Re the hovering styles in IE8:
Jeff is right, this is a problem with IE8 and IE8 only. We have several options here:
- Ignore it
- Check whether IE9 works (according to Microsoft http://msdn.microsoft.com/en-us/library/cc351024(VS.85).aspx , IE9 will support box-shadow) and if it does, ignore IE8 some more.
- Implement something hackish since IE8 renders the backgrounds for hovered links in the toolbar when put into compatibility mode.

I tried playing around with the filter declarations in /modules/toolbar/toolbar.css

  box-shadow: 0 3px 20px #000;
  -moz-box-shadow: 0 3px 20px #000;
  -webkit-box-shadow: 0 3px 20px #000;
  filter: progid:DXImageTransform.Microsoft.Shadow(color=#000000, direction='180', strength='10');
  -ms-filter: "progid:DXImageTransform.Microsoft.Shadow(color=#000000, direction='180', strength='10')";

but to no avail. These were added in a monster issue #535066: Use CSS3 / IE filter to render toolbar shadow which I think nobody wants to revisit.

Overall, I think this is RTBC however. We can always deal with the IE8 issue, which is NOT germane to this particular issue addressing RTL, in a separate issue.

Great job Jeff and tsi!

webchick’s picture

Status: Reviewed & tested by the community » Needs work

Hm. My confidence level that we go back and fix this for IE 8 anytime soon if we don't fix it here is pretty low. And apparently hasn't been tested in IE 9. Sounds like this still needs some work.

reglogge’s picture

@webchick: Please reconsider.

a) Is testing in IE9 a prerequisite for getting something committed? In the entire issue queue I find exactly 10 issues that even mention IE9.
On the Microsoft page which announces that IE9 will support box-shadow, they explicitly state this:
"Note All columns marked "Internet Explorer 9 Beta" are preliminary content. They may be changed substantially prior to commercial release of the software described herein."
In effect we can't know NOW what IE9 will or will not support once it gets released. And even if it will still support box-shadow we don't know how it will render the backgrounds on :hover - whether in some sort of Microsofty compatibility mode or whatever.
Anyway, I don't have Windows 7 or Vista, so I can't test IE9.

b) The rendering issue in IE8 is not limited to this here RTL issue. It's also present when viewing a site with a LTR language. I reopened #535066: Use CSS3 / IE filter to render toolbar shadow, which introduced this bug in the first place, since there are the people who should know much better about IE's arcane handling of -ms-filter, filter et.al. Maybe they can come up with a solution. I can't.

c) Right now there are quite a few RTL issues hanging in the issue queues, some of them waiting for each other to be committed, their patches breaking every time one of them finally gets committed, having to be rerolled over and over again. Committing this here would take one big chunk out of this cluster and allow other important issues to proceed more easily.

So please, let's just get this rightly major one in and be done with it.

Jeff Burnz’s picture

I'll gather some screenshots and test in IE9, lets take an objective look at the differences and see what we can do better.

Jacine’s picture

EDIT: Removing comment that was posted to the wrong issue.

Jacine’s picture

Argh... Wrong issue. Sorry.

Jeff Burnz’s picture

FileSize
197.43 KB

OK, I've tested in IE6, 7, 8, 9

Preliminary findings are that:

IE9 active tab styles don't work (not sure why as yet, works OK in LTR)

IE8 hover styles don't work (because of the filters, both LTR and RTL don't work)

IE7 and 6 - links have more horizontal space, we can fix this with some neg margin.

IE6 shadows bottom is not so good, the sprite image overflows - looks as good in RTL as it does in LTR.

Upshot? Would like to see the active styles working in IE9 and the hover style work in IE8 - at the moment I am not sure if the IE9 issues is releated to this or not (as in the IE8 hover issue is not, that's a filters issue).

The screenshot is from IE9 to 6 going down. Doesnt this highlight how IE6's time has come - why are we supporting 4 version of one browser?

aspilicious’s picture

If you find IE9 issues I recommend placing feedback on the feedback program. They changed their fieldset label rendering after a report of me. (just mention: drupal and millions of ugly sites cause of IE9 and don't forget to tell them how awesome IE9 is compared to IE8 and below)

Jeff Burnz’s picture

@aspilicious - great idea, I've seen a few weird things, like Bartiks search button borked in RTL etc, I'll look that up.

One thing I think is very noticeable in those screenshots is how much better IE9's text rendering is, its just streets ahead -Lucida Grande looks like a dogs breakfast pre IE9, in fact I would be in favour of switching the font stack to verdana first for these versions, its really poor legibility as it is now.

aspilicious’s picture

I saw the search button issue to...

ahmedwali’s picture

Status: Needs work » Needs review
aspilicious’s picture

FileSize
4.56 KB
29.39 KB

@webchick @dries @anybody else

PLEASE commit at least 1! RTL patch.... (and start with this one)

Why?

1) It doesn't affect other stuff in drupal (it only adds css)
2) Few post above

c) Right now there are quite a few RTL issues hanging in the issue queues, some of them waiting for each other to be committed, their patches breaking every time one of them finally gets committed, having to be rerolled over and over again. Committing this here would take one big chunk out of this cluster and allow other important issues to proceed more easily.

So please, let's just get this rightly major one in and be done with it.

3) I actually tested it today and made a special screenshot, don't let this effort be wasted
4) I'm willing to test/review followup patches if they are needed to improve things.
5) please?

I just rerolled the patch so this is RTBC if you want to fix the hover states in IE8 we probably have to wait till drupal 999.
Are we willing to wait that long for we actually fix drupal 7 for all those RTL users??

aspilicious’s picture

Browser order for the screenshot:

IE7
IE8
IE9 (rtl not fully supported yet in IE9 beta)
chrome
opera
firefox

aspilicious’s picture

Status: Needs review » Reviewed & tested by the community

Status...

tsi’s picture

Hover states issue in IE8 is not RTL specific (#535066: Use CSS3 / IE filter to render toolbar shadow).
So RTBC here too.

Dries’s picture

Status: Reviewed & tested by the community » Fixed

Reviewed and committed to CVS HEAD.

aspilicious’s picture

Dries == my hero

aspilicious’s picture

Status: Fixed » Reviewed & tested by the community
FileSize
558 bytes

We lost track of the image that needed to get included.
Can you please replace the shortcut.png in the shortcut module?
Thnx!

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Committed to HEAD. Thanks!

Status: Fixed » Closed (fixed)
Issue tags: -RTL

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