406330 - Winstripe and Gnomestripe look bad in RTL mode

Status: RESOLVED FIXED

(Firefox :: Theme, defect, P3)

Description

[Tomer Cohen :tomer]

Attachment: Hebrew Minefield 3.0b2pre GTK theme

The last change to make Minefield theme more native is a one step backward for RTL compatibility. 

A. The navigation arrows should be flipped. 
B. There is no tab ending for the last tab (actually, it is hide between the last tab and the previous one). 
C. The GO button is missing.
D. The arrow of the Go button appear at the location of the star for blank tabs.
E. The search box flips when the text cursor enter that field 

These are only what I saw in a ten minute test for the main window.


Mozilla/5.0 (X11; U; Linux i686; he; rv:1.9b2pre) Gecko/2007120104 Minefield/3.0b2pre

Comment 1

[(no longer active)]

This should block Firefox 3 I think, since it breaks the RTL localizations in the GTK theme.

Comment 2

[Dão Gottwald [:dao]]

B could be bug 405670.
C and D are expected behaviour.
E isn't specific to Linux.

Comment 3

[Dão Gottwald [:dao]]

I'll take this for now to fix A and E.

Comment 4

[Dão Gottwald [:dao]]

Attachment: patch for A, E (and D)

This fixes D in that it puts the Go (and star) button to the left.

Comment 5

[Mike Connor [:mconnor]]

Comment on attachment 291077 [details] [diff] [review]
patch for A, E (and D)

>Index: browser/themes/gnomestripe/browser/browser.css
>===================================================================
>RCS file: /cvsroot/mozilla/browser/themes/gnomestripe/browser/browser.css,v
>retrieving revision 1.140
>diff -u -8 -p -r1.140 browser.css
>--- browser/themes/gnomestripe/browser/browser.css	1 Dec 2007 07:00:38 -0000	1.140
>+++ browser/themes/gnomestripe/browser/browser.css	2 Dec 2007 09:39:20 -0000
>@@ -61,29 +61,19 @@
> }
> 
> /* ..... fix searchbar "add engine" padding issue ..... */
> 
> .searchbar-popup .open-engine-manager { 
>   -moz-padding-start: 4px; 
> }
> 
>-#searchbar[empty="true"] > .searchbar-textbox {
>+#searchbar[empty="true"] > .searchbar-textbox > .autocomplete-textbox-container > .textbox-input-box > html|*.textbox-input {
>   color: GrayText;
>-  direction: ltr !important;
>-}
>-
>-#searchbar[empty="true"] > .searchbar-textbox > hbox > hbox > html|input {
>-  direction: ltr !important;
>-  text-align: left !important;
>-}
>-
>-#searchbar[chromedir="rtl"][empty="true"] > .searchbar-textbox > hbox > hbox > html|input {
>-  direction: rtl !important;
>-  text-align: right !important;
>+  direction: ltr;

I don't think this ends up with the right fix, since it means the searchbar is always ltr... and why aren't these rules in searchbar.css anyway?

> /* 24px primary toolbar buttons */
>-#back-button {
>+#back-button ,
>+#forward-button[chromedir="rtl"] {
>   list-style-image: url("moz-icon://stock/gtk-go-back?size=toolbar");
> }
>-#back-button[disabled="true"] {
>+#back-button[disabled="true"] ,
>+#forward-button[chromedir="rtl"][disabled="true"] {
>   list-style-image: url("moz-icon://stock/gtk-go-back?size=toolbar&state=disabled");
> }
> 
>-/* GTK takes care of the RTL for us. Yay for libraries! */
>-#forward-button {
>+#forward-button ,
>+#back-button[chromedir="rtl"] {
>   list-style-image: url("moz-icon://stock/gtk-go-forward?size=toolbar");
> }

er, um.  If GTK takes of the RTL for us, why is this necessary (and won't it be buggy?)  I guess GTK takes care of this based on system locale?  This seems like it'd break when running an RTL locale, based on the comment...

Comment 6

[Dão Gottwald [:dao]]

Michael V.: To what extent do you think GTK takes care of RTL?

Tomer: What's your system locale?

Comment 7

[Dão Gottwald [:dao]]

(In reply to comment #5)
> and why aren't these rules in searchbar.css anyway?

Because #searchbar isn't specific to the searchbar binding.

Comment 8

[Dão Gottwald [:dao]]

(In reply to comment #5)
> >-#searchbar[empty="true"] > .searchbar-textbox {
> >+#searchbar[empty="true"] > .searchbar-textbox > .autocomplete-textbox-container > .textbox-input-box > html|*.textbox-input {
> >   color: GrayText;
> >-  direction: ltr !important;
> >-}
> >-
> >-#searchbar[empty="true"] > .searchbar-textbox > hbox > hbox > html|input {
> >-  direction: ltr !important;
> >-  text-align: left !important;
> >-}
> >-
> >-#searchbar[chromedir="rtl"][empty="true"] > .searchbar-textbox > hbox > hbox > html|input {
> >-  direction: rtl !important;
> >-  text-align: right !important;
> >+  direction: ltr;
> 
> I don't think this ends up with the right fix, since it means the searchbar is
> always ltr...

So I think the direction and text-align hacking doesn't make sense at all. Looks like this is unreviewed code from bug 353673, maybe meant as a temporary hack. Removing it altogether should just work as expected, as far as I understand this now.

Comment 9

[Michael Ventnor]

(In reply to comment #6)
> Michael V.: To what extent do you think GTK takes care of RTL?

So much so that in the stock icon documentation they show you what the arrow looks like on an RTL locale.
Of course I suppose not every distribution is cooperative, and this depends on system locale rather than the downloaded Firefox locale.

Comment 10

[Dão Gottwald [:dao]]

Attachment: patch for the searchbar (E)

Comment 11

[is+mozilla]

There were similar issues to the forward/back button in Emacs over which were resolved in CVS HEAD on August 28.  I know next nothing about GTK programming (or XUL programming, for the matter), but I thought I'd suggest this as a possible source of additional info.  Some broken themes (like gtk-qt) don't report their RTL/LTR status, so it might be good to allow customization of forward/back to allow work arounds.  Here's a snippet from my discussion with the emacs people:

------------------------------------------------------------------------
>OK, so it is a problem with some themes and not others. In particular with
>gtk-qt. However, if the problem really is with the theme, why is it
>that the next/previous buttons are OK, and only the back/forward
>buttons don't look right. Perhaps whatever changes to next/previous
>could also be applied to forward/back so that they also look OK in all
>themes.

The difference is that next/previous are not Gtk+ stock icons (i.e. defined
by the theme), but forward/back are.  Gtk+ handles RTL/LTR by itself when
stock icons are used, but only if the theme supplies that information.
-------------------------------------------------------------------------

Comment 12

[is+mozilla]

Attachment: Broken drop downs in web pages when operating in RTL mode.

This is a long-standing bug, but the fact that web pages now use gtk rather than xul widgets makes this worse.  The text inside a drop-down box is moved too far to the right, overlapping with the down arrow.  This would be a very good thing to fix.  (Well, I thought this was a known bug, but I am unable to find it. Should this be its own separate bug?)

Comment 13

[Dão Gottwald [:dao]]

(In reply to comment #12)
> Should this be its own separate bug?

Yes.

Comment 14

[is+mozilla]

Both of the issues I mention above were actually caused by the fact that I was using gtk-qt. (I thought I had set the theme to Clearlooks, but as a result of a typo in my gtkrc  I was still in gtk-qt.)  So when working with non-buggy themes, the forward/back buttons are correctly set _by the locale_. The fact that overall UI direction is controlled independently by bidi.direction is an issue.  If bidi.direction=1 but LANG=he-IL.UTF-8, then the buttons are in RTL directions in a LTR toolbar! Seems to me the real issue is that bidi.direction doesn't inherit automatically from locale. 

Comment 15

[Michael Ventnor]

Attachment: Fix gnomestripe rtl

This should in theory fix the arrow RTL issues since this uses some (probably undocumented) different GTK image names. It works fine in recent GTK versions and allows us to control RTL.

Comment 16

[Ryan Flint [:rflint] (ping via IRC for reviews)]

Comment on attachment 293758 [details] [diff] [review]
Fix gnomestripe rtl

Looks good.
A couple minor nits:
>Index: toolkit/themes/gnomestripe/help/help.css
>+#HelpToolbar[chromedir="rtl"] #help-back-button {
>+  list-style-image: url("moz-icon://stock/gtk-go-back-rtl?size=toolbar");
>+}
>+
>+#HelpToolbar[chromedir="rtl"] #help-back-button[disabled="true"] {
>+  list-style-image: url("moz-icon://stock/gtk-go-back-rtl?size=toolbar&state=disabled");
> }
>+#HelpToolbar[chromedir="rtl"] #help-forward-button {
>+  list-style-image: url("moz-icon://stock/gtk-go-forward-rtl?size=toolbar");
>+}
>+
>+#HelpToolbar[chromedir="rtl"] #help-forward-button[disabled="true"] {
>+  list-style-image: url("moz-icon://stock/gtk-go-forward-rtl?size=toolbar&state=disabled");
> }
Both buttons have chromedir attributes, so you can just check those directly instead.


>+#back-button[chromedir="rtl"][disabled="true"] {
>+  list-style-image: url("moz-icon://stock/gtk-go-back-rtl?size=toolbar&state=disabled");
> }
Rule matching is done from right to left - sticking the chromedir attribute selector at the end of this rule (and others like it) will allow the processor to bail slightly faster for LTR themes.

Comment 17

[Michael Ventnor]

Attachment: Fix gnomestripe rtl 2 (checked in)

Comment 18

[Reed Loden [:reed]]

Comment on attachment 294172 [details] [diff] [review]
Fix gnomestripe rtl 2 (checked in)

Checking in browser/themes/gnomestripe/browser/browser.css;
/cvsroot/mozilla/browser/themes/gnomestripe/browser/browser.css,v  <--  browser.css
new revision: 1.148; previous revision: 1.147
done
Checking in browser/themes/gnomestripe/browser/places/organizer.css;
/cvsroot/mozilla/browser/themes/gnomestripe/browser/places/organizer.css,v  <--  organizer.css
new revision: 1.6; previous revision: 1.5
done
Checking in browser/base/content/browser-context.inc;
/cvsroot/mozilla/browser/base/content/browser-context.inc,v  <--  browser-context.inc
new revision: 1.36; previous revision: 1.35
done
Checking in browser/base/content/browser-menubar.inc;
/cvsroot/mozilla/browser/base/content/browser-menubar.inc,v  <--  browser-menubar.inc
new revision: 1.123; previous revision: 1.122
done
Checking in toolkit/themes/gnomestripe/global/findBar.css;
/cvsroot/mozilla/toolkit/themes/gnomestripe/global/findBar.css,v  <--  findBar.css
new revision: 1.2; previous revision: 1.1
done
Checking in toolkit/themes/gnomestripe/help/help.css;
/cvsroot/mozilla/toolkit/themes/gnomestripe/help/help.css,v  <--  help.css
new revision: 1.2; previous revision: 1.1
done
Checking in toolkit/content/widgets/findbar.xml;
/cvsroot/mozilla/toolkit/content/widgets/findbar.xml,v  <--  findbar.xml
new revision: 1.20; previous revision: 1.19
done

Comment 19

[Bill Gianopoulos [:WG9s]]

This broke opening bookmarks in the sidebar.  See bug 409481.

Comment 20

[is+mozilla]

Perhaps this is a feature rather than a bug, but the latest nightly no longer reverses the arrow directions when bidi.direction is set to 2. An older nightly (I think 12.16) does set the arrow directions correctly, as does Beta 2.  Perhaps arrow directions are now more tightly tied to local? I tried installing the latest he.xpi to see if that worked, but then I got 
שגיאה בניתוח XML:‏ ישות לא מוגדרת מיקום: chrome://browser/content/browser.xul שורה מספר 185, עמודה 5:    <key id="key_errorConsole" key="&errorConsoleCmd.commandkey;" oncommand="toJavaScriptConsole();" modifiers="accel,shift"/>
----^
The Hebrew says: XML parse error: undefined entity. Place: ... Line number 185, Column 5.  So is there away to simulate a RTL interface without having the Hebrew lang pack installed? 

Also notice in latest nightly (and Beta 2, oddly enough). When bidi.direction is set to 2, the search bar is in its LTR orientation until it is selected, when it magically jumps to its RTL orientation.  Again, I don't know if this is because bidi is now set using some higher magic or if this is a bug. 

Any info/answers regarding my questions would be most appreciated. 

Comment 21

[Dão Gottwald [:dao]]

(In reply to comment #20)
> So is there away to simulate a RTL interface without having the
> Hebrew lang pack installed?

You can modify <http://mxr.mozilla.org/firefox/source/dom/locales/en-US/chrome/global.dtd>.

> Also notice in latest nightly (and Beta 2, oddly enough). When bidi.direction
> is set to 2, the search bar is in its LTR orientation until it is selected,
> when it magically jumps to its RTL orientation.

This is what attachment 291196 [details] [diff] [review] fixes.

Comment 22

[is+mozilla]

Comment on attachment 293434 [details]
Broken drop downs in web pages when operating in RTL mode. 

As noted previously, this was a bug my gtk theme, not firefox. Obsoleting.

Comment 23

[Reed Loden [:reed]]

Comment on attachment 294172 [details] [diff] [review]
Fix gnomestripe rtl 2 (checked in)

Ugh, I somehow forgot to commit the places.xul part of this patch. :/

Checking in browser/components/places/content/places.xul;
/cvsroot/mozilla/browser/components/places/content/places.xul,v  <--  places.xul
new revision: 1.101; previous revision: 1.100
done

Comment 24

[Dão Gottwald [:dao]]

Comment on attachment 291196 [details] [diff] [review]
patch for the searchbar (E)

fixed in bug 406095

Comment 25

[Dão Gottwald [:dao]]

this means the tabs are yet to be fixed (B)

Comment 26

[Teune van Steeg]

The tabs have been fixed by the implementation of native tabs: bug 265698 (see also bug 402247).

 (In reply to comment #25)
> this means the tabs are yet to be fixed (B)
> 
Assuming that that was the last outstanding bug here, I resolve this bug.

Comment 27

[(no longer active)]

Mass-assigning the new rtl keyword to RTL-related (see bug 349193).