140636 - menu items are not aligned on the same line when UI aligned to the right

Status: RESOLVED DUPLICATE of bug 311383

(SeaMonkey :: Themes, defect)

Description

[Tsahi Asher]

build ID: 2002041711 (Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.0rc1)
Gecko/20020417)

Description: when the interface is aligned to the right, e.g. when using a RTL
language (like hebrew or arabic), not all menu items are aligned on the same
line. the right ends of the menu items are not all one under the other. this
happens on almost all menues of the Navigator main menu.

Steps to reproduce:
1. aligning the interface to the right: add these lines to the file intl.css, in
the locale\en-US\global, in the en-US.jar file (the language pack file, in the
chrome folder):

window,dialog,wizard,page {
  direction: rtl;
}

menu { direction: rtl; }

outliner { direction: rtl; }

2. start mozilla
3. open one of the main menues
4. observe the menu items alignment

Actual Results: some items are slightly indented (by 1-3 pixels) compared to
other items.

Expected results: all items should be aligned on the same line, one under the other.

Comment 1

[Tsahi Asher]

Attachment: screenshot

i added a thin gray line along the right end of the menu items, to emphasize
the misalignment.

Comment 2

[Boris Zbarsky [:bzbarsky]]

That looks like a problem with the font -- the "r" and "t" glyphs take up more
space than they actually paint...

Comment 3

[Tsahi Asher]

i can show this with hebrew characters as well. hebrew characters are much more
"square" than english characters, yet it happens too. do you want a screenshot?

Comment 4

[Boris Zbarsky [:bzbarsky]]

would be nice, yes.  :)

Comment 5

[Tsahi Asher]

Attachment: screenshot with hebrew menues

this time i don't even have to add a helper line, the misalignment is obvious.
the first line even starts off the menu box.

Comment 6

[Tsahi Asher]

forgot to add, this last screenshot is from a moz0.9.8. i don't have a hebrew
language pack for 1.0 yet.

Comment 7

[Tsahi Asher]

and something else i noticed: in the english locale, there is a gap of about 1cm
between the text and the left edge of the menu box. in hebrew, there is none.
all the strings should start at least 15 pixels to the left of where they are.

Comment 8

[Tsahi Asher]

this still happenes in mozilla 1.5.

Comment 9

[Tsahi Asher]

it just occurred to me, that this bug may be related to bug 74880.

Comment 10

[Lina Kemmel]

Does this bug still occur? I don't see it in mozilla 1.7b (Hebrew system 
locale, untranslated UI).

Comment 11

[Tsahi Asher]

when i reported this, i was using Win98, which has this square font for hebrew
applications, that mozilla used. now i use WinXP, and mozilla uses a more
subtle, round font. i don't know if that's related, but with mozilla 1.6 (i
don't have a language pack for 1.7 yet) the menu items are mostly aligned on the
same line, but i still have the problem of lack of indentation: the text starts
about 3-4 pixels from the right edge of the menu, instead of 15 or so. only menu
items that have bullets or "v" marks at their begining (or folder images for
bookmarks) are indented, so eventually the text is not all on the same line.

i'd like to know how the latest hebrew versions of mozilla look like on Win98,
if someone can post it here.

in any case, the problem happenes when the hebrew language pack is enabled.

Comment 12

[Lina Kemmel]

Tsahi, can you replace occurrences of 'margin-left' to '-moz-margin-start', in:

chrome\modern.jar/skin\modern\global\menu.css      or/and
chrome\classic.jar/skin\classic\global\menu.css

(depending on your skin)?

e.g.
.menu-text {
  -moz-margin-start: 18px !important;
  font-weight: inherit;
}

Does it solve the problem of lacking indentation? If so, I'll release a 
proposed patch for this.

Comment 13

[Tsahi Asher]

nop. i tried it for modern, and it didn't change anything.

Comment 14

[Lina Kemmel]

Sorry, I forgot it won't work for older releases - only starting from 1.7, I 
guess... Did you try it in mozilla 1.6?

Comment 15

[Tsahi Asher]

yes. then i'll try it with a 1.7 nightly i have from last week, later today.

Comment 16

[Tsahi Asher]

(In reply to comment #12)
> Tsahi, can you replace occurrences of 'margin-left' to '-moz-margin-start', in:
> 
> chrome\modern.jar/skin\modern\global\menu.css      or/and
> chrome\classic.jar/skin\classic\global\menu.css
> 
> (depending on your skin)?
> 
> e.g.
> .menu-text {
>   -moz-margin-start: 18px !important;
>   font-weight: inherit;
> }
> 
> Does it solve the problem of lacking indentation? If so, I'll release a 
> proposed patch for this.
> 

yes, this solves it.
please patch both the 1.7 branch and the trunk.

Comment 17

[Lina Kemmel]

Attachment: patch

Replaces left/right margins and paddings with the -moz-*-start/end shorthand
properties.

Custom themes, supplied with their own css, will be still experiencing the 
lack of indentation.

Simon, can you please review the patch?

Comment 18

[Lina Kemmel]

> please patch both the 1.7 branch and the trunk.

Attachment 150366 [details] [diff] is against the trunk.

Comment 19

[Simon Montagu :smontagu]

Comment on attachment 150366 [details] [diff] [review]
patch

I don't believe that I have reviewing rights for themes.

Comment 20

[Tsahi Asher]

you might want to add a comment there explaining why you use -moz-margin- and
not the standard one, so 3rd party theme creators won't be tempted to go back to
the old style.

Comment 21

[Tsahi Asher]

i'm curious: why do you replace margin at
http://bugzilla.mozilla.org/attachment.cgi?id=150366&action=diff#mozilla/themes/classic/global/win/menu.css_sec4
and in other places, but not at
http://bugzilla.mozilla.org/attachment.cgi?id=150366&action=diff#mozilla/themes/classic/global/win/menu.css_sec3
?

Comment 22

[Lina Kemmel]

I only change the places, where the left and the right margins are different.

In the first case, the right margin was 2px, the left - 0px.
In the second case, both the left and the right margins are equal (0px).

(If 4 margins are specified, they relate to the top, right, bottom and left 
respectively.)

Comment 23

[Lina Kemmel]

However, besides menu.css, there are additional XUL-related style sheets that 
specify absolute left / right values. I believe these also should be fixed. - 
But let's see if the patch for menu is reviewed first.

Comment 24

[Lina Kemmel]

[in reply to comment #20]

If the changes are approved, I think it's a good idea to put such a comment, at 
least somewhere at http://themes.mozdev.org.

Comment 25

[Lina Kemmel]

[In reply to comment #21]

Sorry, you're right, it makes sense to use the -moz-* properties even though 
the left and right margins are identical.

Comment 26

[Tsahi Asher]

mozilla 1.7 still uses the themes created for 1.5. i applied this patch to the
RTL-Modern and RTL-Classic themes i made for 1.5 (and used also for 1.6), and
discovered that it disrupts the display of the dropmarkers in the toolbar. in
Classic, they are too wide, and the image is aligned left, and if the mouse is
over the Back arrow and not directly over the dropmarker, the image completely
disappears.

in Modern, the Back dropmarker is compressed to an elipse, and the distance
between the buttons is too big.

i tried to see if menu.css styles the toolbar, but couldn't see any using DOM
Inspector. i just need a direction of how this patch affects the toolbar.

my RTL themes are basicly identical to the originals, eccept that i flipped some
of the graphics, and swaped the words "right" and "left" in many places in the
css. they can be obtained at
https://sourceforge.net/project/showfiles.php?group_id=42516&package_id=74309&release_id=193603

Comment 27

[Lina Kemmel]

[In reply to comment #26]

Using the original mozilla themes, neither of the problems you mentioned seems 
to be reproducible. Please check with 1.7:

1. Do the standard themes misbehave with attachment 150366 [details] [diff] [review] applied?
2. Do your skins display okay without the patch?

And please beware that even a single place, where you swapped "right" 
and "left", can make the patch totally unusable.

Comment 28

[Lina Kemmel]

Attachment: patch for RTL-Classic toolbarbutton.css

Tsahi, I don't think menu.css affects the toolbar.
It seems that you just need to sync with the trunk.
For example, some problems of RTL-Classic can be fixed using the current
toolbarbutton.css; please see the diffs in the attachment.

Comment 29

[Boris Hajduk]

This bug looks very similar to bugs #140759 and #185233
I suppose they are closely related, and probably the same problem cause the 3 bugs.

Comment 30

[Lina Kemmel]

Tsahi, has there been any progress with the RTL* themes?

Comment 31

[Tsahi Asher]

sorry lina, i was very busy recently and didn't have time for this. what is did
so far is this:

took the mozilla 1.5 classic and modern themes, which were also used in 1.6, and
in menu.css, where you replaced *-left with -moz-*-start, i replaced it with
*-right, so the themes are still compatible with 1.5. in addition, i applied
attachment 150969 [details] [diff] [review] to the classic theme, and in a similar manner modified the
modern theme. i then used these themes with mozilla 1.7 with the hebrew language
pack, and the problem was solved (WinXP).

i noticed that despite the fact that mozilla 1.7's theme version is 1.5, it has
a few more files than the themes in mozilla 1.6. i didn't have time to actually
modify the themes that come with 1.7.


(In reply to comment #29)
> This bug looks very similar to bugs #140759 and #185233
> I suppose they are closely related, and probably the same problem cause the 3
bugs.

althoug visually they may look the same, it's pretty clear to me that in the
source of it, the problem is with the CSS of the theme. i think mozilla should
be able to select the right CSS and graphics in the theme, according to the
direction of the language pack, as i wrote in bug 221824.

Comment 32

[Tsahi Asher]

is it possible to apply the patch for RTL classic toolbarbutton.css to the
mozilla code base, and similarly patch the modern theme, in order to save me
some of the work in converting the themes?

Comment 33

[Asaf Romano (gone)]

Simon, can you move the review request to someone else?

Comment 34

[Tsahi Asher]

a few days ago i produced new RTL versions of the default themes, this time
based on the themes that come with mozilla 1.7/win32. among other things, i
manually applied the patch in attachment 150366 [details] [diff] [review] to menu.css, and it worked as
advertised.

classic: http://prdownloads.sourceforge.net/hebmoz/rtl-classic-moz-1.7.xpi?download
modern: http://prdownloads.sourceforge.net/hebmoz/rtl-modern-moz-1.7.xpi?download

Comment 35

[Tsahi Asher]

Neil, what about the review here?

Comment 36

[Tsahi Asher]

probably the right solution for this bug is to create RTL-aware themes, like we have in firefox and thunderbird (bug 221824). i think the infrastructure is there (global/global.dtd: locale.dir in the language pack), so someone just needs to fix the themes to use it. see also comment 34 above.

Comment 37

[Wayne Mery (:wsmwk)]

(In reply to Tsahi Asher from comment #36)
> probably the right solution for this bug is to create RTL-aware themes, like
> we have in firefox and thunderbird (bug 221824). i think the infrastructure
> is there (global/global.dtd: locale.dir in the language pack), so someone
> just needs to fix the themes to use it. see also comment 34 above.

so close this bug in favor of bug 221824? (or dup)

Comment 38

[Philip Chee]

Although bug 221824 is in the SeaMonkey product, it appears to have been hijacked for toolkit specific work. Something more appropriate might be:
Bug 311383 - add RTL theme support to SeaMonkey [helpwanted].

We know what needs to be done (basically just copy what TB and FX have already done). But SeaMonkey is a completely volunteer driven community project and we badly need someone with theming (and graphics) skills to step forward and volunteer to implement this.

Bits and pieces of RTL awareness have been landing once in a while as part of other patches. However our UI-module owner would prefer a more systematic effort rather than piecemeal efforts here and there.

Comment 39

[Philip Chee]

Comment on attachment 150366 [details] [diff] [review]
patch

Cancelling review request due to extreme bit rot. This bug is too old and has too much history.
Please open a new bug with a new patch for the menu item alignment case. Assuming that this still occurs in Modern. Our Default (Classic) theme now picks up the global styles from Toolkit so I assume that RTL menu items don't have any problems in our Default theme.