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 :)
Comment | File | Size | Author |
---|---|---|---|
#41 | drupal-766170-41.patch | 758 bytes | tim.plunkett |
#39 | drupal-766170-39.patch | 967 bytes | tim.plunkett |
#36 | ie7-overlay-after-patch.PNG | 31.25 KB | xjm |
#35 | ie8-overlay-after-patch.png | 26.29 KB | xjm |
#30 | ff-after.png | 30.19 KB | xjm |
Comments
Comment #1
aspilicious CreditAttribution: aspilicious commentedComment #2
aspilicious CreditAttribution: aspilicious commentedI'm gonna reroll this later today cause of the
"\ No newline at end of file"
Comment #3
aspilicious CreditAttribution: aspilicious commentedReroll and css cleanup
Comment #4
tsi CreditAttribution: tsi commentedThis 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)
Comment #5
casey CreditAttribution: casey commentedI 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
Comment #6
aspilicious CreditAttribution: aspilicious commentedCan't apply this anymore, ca you reroll.
And this needs a proper review specially for the opera part.
Comment #7
casey CreditAttribution: casey commentedReroll
Comment #8
aspilicious CreditAttribution: aspilicious commentedAdding tag and needs reroll
Comment #9
mightyiam CreditAttribution: mightyiam commentedsubscribe
Comment #10
aspilicious CreditAttribution: aspilicious commentedSUMMARY
----------
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.
Comment #11
aspilicious CreditAttribution: aspilicious commentedComment #13
aspilicious CreditAttribution: aspilicious commentedNew patch with IE7 support (after applying #766458: Seven theme lacks rtl styling)
Comment #14
aspilicious CreditAttribution: aspilicious commentedComment #15
tsi CreditAttribution: tsi commentedThere are some files missing from the last patch, did some merging so that this patch will add them.
Otherwise looks very good.
Comment #17
catchI 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.
Comment #18
catchComment #19
tsi CreditAttribution: tsi commentedWas this failing because my git mistakes ?
Comment #20
tsi CreditAttribution: tsi commentedHooray !!
Now let's get this committed
Comment #21
iamjon CreditAttribution: iamjon commentedadding myself so I can test this
Comment #22
xjmTagging issues not yet using summary template.
Comment #23
xjmI checked with ksenzee about the opera-specific jquery for the scrollbar issue, and she says it is acceptable in this instance:
Comment #24
xjmI tested the patch as follows:
modules/overlay/images/
.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.
Comment #25
tsi CreditAttribution: tsi commentedI think something went wrong on your test.
This is how it looks from here :
Comment #26
xjm#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.
Comment #27
tsi CreditAttribution: tsi commented@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.
Comment #28
xjmFollowup 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.
Comment #29
xjmJust 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.
Comment #30
xjmComment #31
tsi CreditAttribution: tsi commentedWell, 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.
Comment #32
xjm#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.
Comment #33
tsi CreditAttribution: tsi commentedThe close button should be exactly there, please read #31.
Comment #34
xjm#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. :)
Comment #35
xjmTested 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.
Comment #36
xjmAnd, finally, testing in IE7. IE7 is fine; in fact, it doesn't even have the minor toolbar margin issue IE8 does.
Comment #37
xjmAdded issue summary. I think we are RTBC unless anyone has issues I've missed. :)
Comment #37.0
xjmUpdated issue summary.
Comment #38
webchickWow, GREAT job on the testing in this issue. Thanks!
Committed and pushed to 8.x and 7.x.
Comment #39
tim.plunkettIt 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.
Comment #41
tim.plunkettNot sure why that one image bit was pulled in, here's another try.
Comment #42
aspilicious CreditAttribution: aspilicious commentedOk good to go
Comment #43
webchickOops. :P Thanks, Tim!
Committed and pushed to 8.x and 7.x.
Comment #44.0
(not verified) CreditAttribution: commentedMissed a period.