Problem/Motivation

Overlay currently does not provide any RTL styling.

Proposed resolution

Provide RTL layout for overlay, including:

  • rtl text orientation throughout
  • right-aligned title
  • left-aligned tabs
  • left-side close button
  • correct layout relative to scrollbar

Remaining tasks

The patch in #19 has been tested and confirmed to work in:

  • IE7
  • IE8
  • Firefox
  • Chrome
  • Safari
  • Opera

There are some minor irregularities with margins in some browsers, but these issues do not impact the functionality. There is a followup issue for layout bugs when the window is resized in some webkit browsers: #1254248: Admin screens in an RTL language causes display errors when the window is resized in some webkit browsers.

To resolve this issue, commit the patch in #19 with close-rtl.png added in modules/overlay/images.

User interface changes

Correct overlay layout in RTL languages.

API changes

None.

Original report by @aspilicious

This is a prety good start, probably IE7 and below are broken.
You need to add the new image too :)

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

aspilicious’s picture

Status: Active » Needs review
aspilicious’s picture

I'm gonna reroll this later today cause of the

"\ No newline at end of file"

aspilicious’s picture

FileSize
4.78 KB

Reroll and css cleanup

tsi’s picture

This is looking good BUT I guess you were working on an English RTL because it is somehow not working correctly with real RTL languages (hebrew/arabic).
see my issue - #786940: Overlay breaks in any language (with path prefix)

casey’s picture

FileSize
368 bytes
368 bytes
5.81 KB

I unfortunately needed to include a browser test as there seems to be no way to detect the left-sided scrollbar in Opera. Would be nice if we found another solution.

Note that patch needs to be tested in conjunction with patches of
- #740182: Toolbar and shortcuts lack RTL styling
- #766458: Seven theme lacks rtl styling

aspilicious’s picture

Can't apply this anymore, ca you reroll.
And this needs a proper review specially for the opera part.

casey’s picture

FileSize
5.51 KB

Reroll

aspilicious’s picture

Status: Needs review » Needs work
Issue tags: +RTL

Adding tag and needs reroll

mightyiam’s picture

subscribe

aspilicious’s picture

SUMMARY
----------

The overlay is not rly usable in rtl at the moment. This patch fixes that, but has some issues.

1) In IE7 the secondary tabs are on the wrong side, I think it will be fixed once we have fixed #766458: Seven theme lacks rtl styling.
2) Scrolbar and scrolbar detection are a pain for this. We need a javascript expert to look at this. In chrome and firefox there are some weird behaviours with scrollbars.

overlay-strange-toolbar.png

aspilicious’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 766170-overlay-rtl.diff, failed testing.

aspilicious’s picture

New patch with IE7 support (after applying #766458: Seven theme lacks rtl styling)

aspilicious’s picture

Status: Needs work » Needs review
tsi’s picture

FileSize
368 bytes
5.36 KB

There are some files missing from the last patch, did some merging so that this patch will add them.
Otherwise looks very good.

Status: Needs review » Needs work

The last submitted patch, 766170-overlay-rtl.patch, failed testing.

catch’s picture

Priority: Normal » Major

I marked #1166886: [Policy, no patch] Improve RTL support as duplicate, but I'm bumping this one to major in its place - a completely new feature added without any RTL styling is a regression compared to D7.

It looks like #766458: Seven theme lacks rtl styling is the only other properly major RTL issue and that's already marked as such.

catch’s picture

Version: 7.x-dev » 8.x-dev
Issue tags: +Needs backport to D7
tsi’s picture

Status: Needs work » Needs review
FileSize
368 bytes
5.34 KB

Was this failing because my git mistakes ?

tsi’s picture

Hooray !!
Now let's get this committed

iamjon’s picture

adding myself so I can test this

xjm’s picture

Tagging issues not yet using summary template.

xjm’s picture

I checked with ksenzee about the opera-specific jquery for the scrollbar issue, and she says it is acceptable in this instance:

[6:06PM] xjm: if ($.browser.opera) {stuff }
[6:07PM] ksenzee: as a general rule I am okay with browser sniffing to add styling in corner cases like this
[6:07PM] ksenzee: it is obviously bad if what you really want is to know whether the browser supports some JS syntax etc
[6:07PM] ksenzee: in a case like this, you really do need to know whether it's Opera you're dealing with

xjm’s picture

I tested the patch as follows:

  1. Enabled locale.
  2. Set Arabic as default language.
  3. Visited an overlay in a window with scrollbars in Chrome, Safari, FF, and Opera on Mac.
  4. Downloaded close-rtl.png to modules/overlay/images/.
  5. Applied patch from #19.
  6. Cleared site cache and reloaded the page.
  7. Set language back to English and checked that the overlay still behaves as expected in LTR.

FF and Opera were fine; see attached screenshots. Safari and Chrome had some pretty spectacular bugs with scrollbar and window resizing for overlay pages in RTL, regardless of whether the patch was applied or not. (See attached screenshots). So, the patch is a definite improvement, but there are still outstanding RTL overlay issues in webkit.

tsi’s picture

FileSize
42.23 KB
48.23 KB

I think something went wrong on your test.
This is how it looks from here :

xjm’s picture

#25: Did you try resizing the window in Chrome from small to large and back? The same thing happened in Safari (I have both browsers in the screenshot), which led me to believe it's not a user-specific issue. I've also confirmed it in Chrome this morning on a different machine. I'll try a release of Chrome to rule out that it's an issue with the dev channel.

Edit: I will open a separate issue for the window resizing thing, since it does work reasonably (albeit with funky scrollbars) on the initial page load, and it's independent of this patch here shouldn't block it.

tsi’s picture

@xjm - no, actually I didn't try resizing the browser window before, but I did now, and everything is still in place.
Do you see this happenning on the test site (drupal:drupal) as well ?
Admittedly, there is a weird left margin of about 15px (visible in my screen shot), but I wouldn't say this is a commit blocker since the situation without the patch is much worse, sometimes hardly functional.

xjm’s picture

Followup issue for the webkit resize bug:
#1254248: Admin screens in an RTL language causes display errors when the window is resized in some webkit browsers

That shouldn't block this one.

Edit re: #27 -- Yeah, I can confirm the same behavior at that site.

xjm’s picture

Just noticed that my after screenshot for FF shows the wrong close button position, but that's not what I'm seeing in FF atm with browser and drupal caches cleared. Some funkiness going on with uppercase/lowercase of filenames.

I'll try to test in IE 7 and 8 later today.

xjm’s picture

FileSize
30.19 KB
tsi’s picture

Well, I guess we need a third opinion here, I can't reproduce this issue (#28), not on the test site and not on my local machine.
#29 - What do you mean by wrong close button position, on LTR it shows on the right, in RTL - on the left, that is the desired behavior.

xjm’s picture

#31: As I said, I opened a separate issue for the window resizing issue. We can follow up there.

#24 has a bad screenshot. I've corrected it in #30.

tsi’s picture

The close button should be exactly there, please read #31.

xjm’s picture

#33 Yes, that's what I said in #29. To reduce the risk of further confusion, let me just state: we agree and FF is fine. :)

xjm’s picture

FileSize
26.29 KB

Tested in IE8. Tabs and close button look fine. There's the same small margins with the toolbar described above (but at least it doesn't blow up when the window is resized). Screenshot attached.

xjm’s picture

FileSize
31.25 KB

And, finally, testing in IE7. IE7 is fine; in fact, it doesn't even have the minor toolbar margin issue IE8 does.

xjm’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs issue summary update

Added issue summary. I think we are RTBC unless anyone has issues I've missed. :)

xjm’s picture

Issue summary: View changes

Updated issue summary.

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Wow, GREAT job on the testing in this issue. Thanks!

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

tim.plunkett’s picture

Status: Fixed » Needs review
FileSize
967 bytes

It seems that modules/overlay/overlay-child-rtl.css was not committed to either 7.x or 8.x.
For ease of committing, here's just that file as a patch.

Status: Needs review » Needs work

The last submitted patch, drupal-766170-39.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
758 bytes

Not sure why that one image bit was pulled in, here's another try.

aspilicious’s picture

Status: Needs review » Reviewed & tested by the community

Ok good to go

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Oops. :P Thanks, Tim!

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

Status: Fixed » Closed (fixed)
Issue tags: -RTL, -Needs backport to D7

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

Anonymous’s picture

Issue summary: View changes

Missed a period.