Winstripe and Gnomestripe look bad in RTL mode

RESOLVED FIXED in Firefox 3 beta4

Status

()

defect
P3
normal
RESOLVED FIXED
12 years ago
11 years ago

People

(Reporter: tomer, Unassigned)

Tracking

(Blocks 1 bug, {meta, rtl})

Trunk
Firefox 3 beta4
x86
All
Points:
---
Dependency tree / graph
Bug Flags:
blocking-firefox3 +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 4 obsolete attachments)

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
Component: XP Toolkit/Widgets: XUL → OS Integration
Product: Core → Firefox
QA Contact: xptoolkit.xul → os.integration
This should block Firefox 3 I think, since it breaks the RTL localizations in the GTK theme.
Flags: blocking-firefox3?
B could be bug 405670.
C and D are expected behaviour.
E isn't specific to Linux.
I'll take this for now to fix A and E.
Assignee: nobody → dao
Assignee: dao → nobody
Component: OS Integration → Toolbars
OS: Linux → All
QA Contact: os.integration → toolbars
Hardware: All → PC
Summary: [RTL] Default theme looks bad on GTK → Winstripe and Gnomestripe look bad in RTL mode
Assignee: nobody → dao
Posted patch patch for A, E (and D) (obsolete) — Splinter Review
This fixes D in that it puts the Go (and star) button to the left.
Attachment #291077 - Flags: review?(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...
Attachment #291077 - Flags: review?(mconnor) → review-
Michael V.: To what extent do you think GTK takes care of RTL?

Tomer: What's your system locale?
(In reply to comment #5)
> and why aren't these rules in searchbar.css anyway?

Because #searchbar isn't specific to the searchbar binding.
(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.
(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.
Posted patch patch for the searchbar (E) (obsolete) — Splinter Review
Attachment #291077 - Attachment is obsolete: true
Attachment #291196 - Flags: review?(mconnor)
Flags: blocking-firefox3? → blocking-firefox3+
Priority: -- → P3
Target Milestone: --- → Firefox 3 M11
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.
-------------------------------------------------------------------------
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?)
(In reply to comment #12)
> Should this be its own separate bug?

Yes.
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. 
Posted patch Fix gnomestripe rtl (obsolete) — Splinter Review
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.
Attachment #293758 - Flags: review?(rflint)
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.
Attachment #293758 - Flags: review?(rflint) → review+
Attachment #293758 - Attachment is obsolete: true
Keywords: checkin-needed
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
Attachment #294172 - Attachment description: Fix gnomestripe rtl 2 → Fix gnomestripe rtl 2 (checked in)
Depends on: 409481
This broke opening bookmarks in the sidebar.  See bug 409481.
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. 
(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 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.
Attachment #293434 - Attachment is obsolete: true
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
Depends on: 415429
Blocks: fx3-l10n-he
Comment on attachment 291196 [details] [diff] [review]
patch for the searchbar (E)

fixed in bug 406095
Attachment #291196 - Attachment is obsolete: true
Attachment #291196 - Flags: review?(mconnor)
this means the tabs are yet to be fixed (B)
Assignee: dao → nobody
Severity: major → normal
Component: Toolbars → Widget: Gtk
Flags: blocking-firefox3+
Product: Firefox → Core
QA Contact: toolbars → gtk
Target Milestone: Firefox 3 beta3 → ---
Flags: blocking1.9?
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.
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Component: Widget: Gtk → Theme
Flags: blocking1.9?
Product: Core → Firefox
QA Contact: gtk → theme
Target Milestone: --- → Firefox 3 beta4
Flags: blocking-firefox3?
Depends on: 402247
No longer depends on: 409481
Keywords: meta
Flags: blocking-firefox3? → blocking-firefox3+
Blocks: fx35-l10n-fa
No longer blocks: Persian-Fx3.5
Mass-assigning the new rtl keyword to RTL-related (see bug 349193).
Keywords: rtl
No longer blocks: fx35-l10n-fa
Blocks: Persian
You need to log in before you can comment on or make changes to this bug.