juan becerra [:juanb] Reporter
	Description • 18 years ago

The search field in the Mac build for Hebrew (2.0.0.1) looks ugly. The right-to-left flip of the toolbar in the Mac doesn't work as well as in Windows because the search field is rounded in the Mac.

You can also see that tabs need improvement.

STR:
1. Install 2.0x Hebrew build on Mac.

Expected: Layout of search field should display correctly (just right-to-left)

Actual: The search field field looks made up of three separate elements.

	Simon Montagu :smontagu
	Comment 2 • 18 years ago

In the screenshot, it seems that all of the UI elements that should be reversed in RTL locale are not reversed.

	Tomer Cohen :tomer
	Comment 3 • 18 years ago

Sorry for the lame question, but are you using the default theme?

	Marek Stępień [:marcoos, inactive]
	Comment 4 • 18 years ago

I can confirm this with an English (US) Mac OS X 10.2, running Fx 2.0.0.1 with a new profile and the default theme.

	Marek Stępień [:marcoos, inactive]
	Comment 5 • 18 years ago

...and Arabic is as broken as Hebrew.

	Asaf Romano (gone)
	Comment 6 • 18 years ago

(In reply to comment #3)
> Sorry for the lame question, but are you using the default theme?
> 

Pinstripe is not RTL-compatible.

	Marek Stępień [:marcoos, inactive]
	Comment 7 • 18 years ago

So, Asaf, this is not a regression? It didn't work in 1.5 as well?

This problem renders Arabic and Hebrew Mac Firefox almost unusable, we should either fix the theme (maybe if we can't make Pinstripe RTL-friendly, consider using Winstripe for Mac RTL builds) or pull Mac ar and he builds from the downloads page (like we did e.g. with gu-IN)...

	Asaf Romano (gone)
	Comment 8 • 18 years ago

(In reply to comment #7)
> So, Asaf, this is not a regression? It didn't work in 1.5 as well?

Right.

	Reuven Gonen
	Comment 9 • 18 years ago

I startes working on a fix for the pinstripe theme in Bug 221824 (attachment 205080 [details] [diff] [review]). if you have the time and knowledge feel free to finish it. (the patch may be a bit outdated because of changes in the theme for Firefox 2.0)

	is+mozilla
	Comment 11 • 17 years ago

These issues persist in the new Proto theme. 

I am requesting blocker status for this bug. The same issues in gnomestripe (bug 406330) are blocking-firefox3+. It seems to me that the default Mac theme deserves the same treatment. 

	Mike Connor [:mconnor]
	Comment 12 • 17 years ago

This is not a blocker for Fx3 as we have never supported RTL on Mac.  We have always supported RTL on Linux, which is why that bug blocks.

	is+mozilla
	Comment 13 • 17 years ago

Per comment #230 in bug #397723, it seems to me that this bug should block 397723.  Could someone with appropriate privileges make this happen?  

	(no longer active)
	Comment 14 • 17 years ago

(In reply to comment #13)
> Per comment #230 in bug #397723, it seems to me that this bug should block
> 397723.  Could someone with appropriate privileges make this happen?  

Done.

	Mike Connor [:mconnor]
	Comment 15 • 17 years ago

I think that

	Mike Connor [:mconnor]
	Comment 16 • 17 years ago

Oops.

I think that this is orthogonal to the new theme.  We should look at making it RTL compatible.

	Mike Beltzner [:beltzner, not reading bugmail]
	Comment 17 • 17 years ago

Mano: apparently proto makes this a lot worse; did many people even try to use Mac builds? Just trying to figure out if this is a regression ...

	Asaf Romano (gone)
	Comment 18 • 17 years ago

I don't know what do you mean by "worse", it's as non-supported in both themes. I don't think we should be shipping RTL builds on mac, they were never usable

	Mic Berman
	Comment 19 • 17 years ago

this is an image i captured on my mac osx 10.4 platform for the beta 4 build of Arabic
here is a link to an image captured by tchung for hebrew beta 4 build
http://img391.imageshack.us/my.php?image=hetoolbarjr1.png

	Tomer Cohen :tomer
	Comment 20 • 17 years ago

Asaf: Is it a rendering issue? I guess not, and if so we should at least try to solve it. 

	Mic Berman
	Comment 21 • 17 years ago

trying this attachment again, apologies

	Mic Berman
	Comment 22 • 17 years ago

full screen view of arabic demonstrating rendering issue on Mac

	Axel Hecht [:Pike]
	Comment 23 • 17 years ago

I know this is late in the game, but to add my 2 cts:

Fact is that we shipped RTL localizations on the mac on firefox 2. They might not have looked perfect, or had the weirdest hacks in intl.css, but in the end, we shipped something.

Ending up with something totally non-shippable would be a regression to me.

	Asaf Romano (gone)
	Comment 24 • 17 years ago

Did we hack intl.css? As far as I can tell it was as broken.

	is+mozilla
	Comment 25 • 17 years ago

I've been using RTL Mac on OS X and Linux since 2004. As I recall, this more or less the sequence of events
1) Firefox 1.0 had no RTL support built in, but you could install a RTL version of the classic theme (maybe also modern) that was created by the Hebrew localization team. I think this was a descendant of the corresponding navigator theme. 
2) Firefox 1.5 had RTL support in winstripe and gtkstripe but not pinstripe.  Pinstripe looked more or less the same as in 1.0.  The back and forward buttons were reversed, but most of the other glitches were relatively minor
3) Visual Referesh in Firefox 2.0.  These actually makes pinstripe look even worse than in Firefox 1.5 . Tabs rounded the wrong way, various other aspects. It's so bad installing the Ubuntu Human theme (allegedly designed for Win and Linux) actually improves the experience and becomes my default theme. 

Summarizing and updating my observations from comment #116 to bug #397723, here are the most glaring issues based on my (admittedly light) use of recent betas:
1) All buttons in the toolbar (forward/back/history) are still in the LTR orientation.
2) The search-engine icon and search button are rounded towards rather than away from the search field. (I guess this is the same as #1.)
3) Tab bar: the corners are rounded towards rather than away from the tab itself.
4) URL bar: this actually has a complete RTL orientation, except that is wrong, as all (or almost all) urls are in English. It should be favicon on the left, url left-justified and rendered as LTR text, the drop down list on the right. In the screenshot (attachment #292197 [details], created for proto 0.8 but still accurate except for the new history buttons) you'll notice that the trailing slash appears on the left of the url rather than the right, the tell-tale sign of English text rendered as RTL instead of LTR. 

I think all of 1-3 should be easy to fix, as I recall there are forward-rtl, back-rtl, etc. icons that can be set. (I haven't been able to find docs on theme changes in FF3, at least as affects RTL. But clearly you guys are the experts so I'll defer to your judgment.) I think a  simple horizontal reflections to create new icons will suffice (or id swap, for cases where the icon pairs are symmetric). For 4, I'm not sure what's involved, but gtkstripe does it correctly by default so I think the ability should be built-in to gecko. 

Finally, let me just say thanks for finally taking a look at this.  I'm happy to contribute testing---just add a comment telling me to grab the latest nightly.  I'm not an expert on mozilla code, but I can apply patches and build from source, so if there is something would like to test before a commit I can do that also. Oh,  I remember encountering difficulty in figuring out how to do a "RTL build." So instructions on that would be appreciated if you want me to test a patch. 

	Yehuda
	Comment 27 • 17 years ago

Search field is highlighted.
Also note the RTL layout of the URL causes the last forward slash of the URL to appear in front of the "http:" prefix

	Behnam Esfahbod [:zwnj]
	Comment 28 • 17 years ago

> Also note the RTL layout of the URL causes the last forward slash of the URL to
> appear in front of the "http:" prefix

We (fa, he, etc) fix this in intl.css by setting the direction and alignment of the address bar.  Please report it as a bug for the locale.

	Tomer Cohen :tomer
	Comment 29 • 17 years ago

(In reply to comment #28)
> We (fa, he, etc) fix this in intl.css by setting the direction and alignment of
> the address bar.  Please report it as a bug for the locale.
> 

I prefer to get rid of any intl.css 'hacks'. If the URL bar is *always* LTR, it should be mentioned in the chrome, and not require each locale to specify it manually. 

Same should also be the guidelines for extensions - If the extension is not available for the RTL locales, and fallback to English but stays in RTL layout, which most of the time make the interface very complicated and non clear. Currently we [he] have rule in intl.css for adblock inside the shipped locale package, and I hope to get it removed one day.

	Yehuda
	Comment 30 • 17 years ago

Notice a small bug in the Linux address area, but otherwise represents the expected behavior for the Mac version.
Of course, Mac's rounded buttons require more work.

	Axel Hecht [:Pike]
	Comment 31 • 17 years ago

Does this block the B5 release of RTL locales on Mac?

	Tomer Cohen :tomer
	Comment 32 • 17 years ago

As a start point, can you please give us some basic information about the proto theme? Does it have some kind of shading on the buttons that may make it difficult to mirror the theme?

	Mic Berman
	Comment 33 • 17 years ago

axel, per comment #31, we had been discussing that in email with Tomer and Ayman and l10n drivers - we are trying to assess if it would be a blocker. the user experience is very bad and it seems Mac usage in general in these areas may be lower. we did block it for beta 4. if the user experience doesn't improve then it may be that we'd do same for b5. thoughts?

	Tomer Cohen :tomer
	Comment 34 • 17 years ago

Update: Thanks to a Yehuda (from comment #30) we now have Firefox Proto theme with basic RTL support (the main navigation toolbar at the moment). We'll review it and post it here. It may be too harmful to commit directly to the tree without reviewing, so please pay the required attention. Expect RTL Mac theme in the very soon future, I hope we will be on time for RC1. 

	Tomer Cohen :tomer
	Comment 35 • 17 years ago

I've reviewed the changes on top of my Linux box. I most admit that Yehuda is doing wonderful job, and there is a huge progress toward resolving this bug.

	is+mozilla
	Comment 36 • 17 years ago

Are there version of theme available (or can be made available)? I'd like to do some testing on on Mac (currently running Tiger). 

	Tomer Cohen :tomer
	Comment 37 • 17 years ago

Attached is Yehuda's fixes I've reviewed yesterday. It is not final yet, but if you are testing it please publish you thoughts and bugs here.

	is+mozilla
	Comment 38 • 17 years ago

I'm not having much success. Perhaps I'm doing it wrong.  I downloaded attachment 312687 [details].  I then copied the contents of classic/global to toolkit/themes/pinstripe/global, and I copied the contents of classic/browser into browser/themes/pinstripe/browser and rebuilt.  The result is that the URL bar is now rounded correctly and the url text is left-justified, but I have no forward or back buttons, and the rounded edges of the search bar are entirely missing.  

I'm not sure if this is and problem with my build or if its an issue still not fixed, but the corners on the tabs are still rounded the wrong direction.

	Yehuda
	Comment 39 • 17 years ago

(In reply to comment #38)
First of all, thanks for checking this patch.
The search edges bug and the missing back and forward arrows can be explained by the build process you used. The patch was intended to be applied after a successful build (or even on top of a downloaded nightly build). I guess that your process failed because the final classic.jar did not include the new *-rtl images. I need to figure out how to do that.

On top of that, there was a change in browser.css and browser.xml on Mar 31, which I was not aware of until now. If you do a checkout and build just before applying my patch, it can cause further issues with the back and forward buttons. 

I will post an updated patch later today or tomorrow which should be compatible with the latest changes in the CVS. The tabs' rounding are still under construction and were not meant to be included in this patch  - this time it is not a CVS conflict or build issue. Hopefully they will be included in this later patch as well.

	Tomer Cohen :tomer
	Comment 40 • 17 years ago

(In reply to comment #38)
> I then copied the contents of classic/global to
> toolkit/themes/pinstripe/global, and I copied the contents of classic/browser
> into browser/themes/pinstripe/browser and rebuilt. 

As this is not a real patch, you'll need to extract it manually or add the attachment files into the classic.jar.

At the moment the files are not on the CVS, and I've not requested any review flag on the file, as it is still incomplete. At the moment it will be complete or at least near that point, I'll start patching the files and request the required reviews. I don't think it will be ready before RC1 freeze, but I hope it will be ready before RC2. 

	Axel Hecht [:Pike]
	Comment 41 • 17 years ago

Putting some Mikes on CC to get them into the loop here, now that there is action.

	is+mozilla
	Comment 42 • 17 years ago

When I didn't find any directories named skin in Minefield.app, I assumed I needed to build from source.  I now have it running with the 3/26 nightly.  Since I haven't used the trunk too much, I'm not sure how much of the following is theme specific and how much is toolkit bugs, but I've noticed the following. (Undoubtedly some of these things are in the process of being fixed)

1) I can't seem to access any bookmarks from the bookmarks menu.  I'm fairly certain this was _not_ a bug before I patched the theme. The only bookmark in the menu is "Bookmarks Toolbar", but it appears empty in the menu, even though I can see and use bookmarks in the actual toolbar.

2) In bookmarks organizer, the forward/back buttons are backwards and the rounded edge on the is on the wrong side of the drop-down buttons.

3) If the search field is selected and I click on the search-engine selection button (or whatever its actual name is), the highlight disappears from around that button. I remains around the rest of the search field, however.

4) This may be a bug in Proto generally, but the preview image in the Theme Manager looks remarkably identically to the one from pinstripe and not look how the theme actually looks now. 

5) In the add-ons manager generally, the little add-on icon (which is on the right, at least in RTL mode), is cut half. It appears only the right-half is shown.  Resizing the window fixes the issue, but clicking on another tab in the manager re-intrdouces the cut off.

I don't have time to create screen-shots just now, but if people want them I can do that tonight.  If I need to open new bugs, let me know. 

Even though the theme is incomplete, this is much better than what we had a week ago.  Keep up the good work!

	is+mozilla
	Comment 43 • 17 years ago

OK, we can scratch #1. I don't what the problem was, but after a restart my bookmarks menu is working again.  The other bugs have persisted. 

	Yehuda
	Comment 44 • 17 years ago

This is another work-in-progess set of files for testers. It includes compatibility fixes with the latest changes in browser.css code, as well as fixes for the tabs display.
To test this set: Download the latest nightly (Apr 1st or later); change directory to Contents/MacOS/chrome; extract content of classic.jar; extract this set to override/add the files; and then re-pack classic.jar (including added files). Restart Firefox to see the changes.

	Yehuda
	Comment 45 • 17 years ago

(In reply to comment #42)
> 
> 1) I can't seem to access any bookmarks from the bookmarks menu.  
Not repeatable as noted

> 2) In bookmarks organizer, the forward/back buttons are backwards and the
> rounded edge on the is on the wrong side of the drop-down buttons.
This issue is on my to-do list.
 
> 3) If the search field is selected and I click on the search-engine selection
> button (or whatever its actual name is), the highlight disappears from around
> that button. I remains around the rest of the search field, however.
This issue is repeatable in the original theme. Currently, out of my scope.

> 4) This may be a bug in Proto generally, but the preview image in the Theme
> Manager looks remarkably identically to the one from pinstripe and not look how
> the theme actually looks now. 
Ditto

> 5) In the add-ons manager generally, the little add-on icon (which is on the
> right, at least in RTL mode), is cut half. It appears only the right-half is
> shown.  Resizing the window fixes the issue, but clicking on another tab in the
> manager re-intrdouces the cut off.
I could not reproduce this one. Can you add a screenshot?

Once again, thanks for testing

	Tomer Cohen :tomer
	Comment 46 • 17 years ago

(In reply to comment #44)

I prefer to extract the classic.jar and than change the manifest file to point to the extracted files instead. This remove the need to repack classic.jar again after each change. 

	Tomer Cohen :tomer
	Comment 47 • 17 years ago

You'll need the following files from the previous zip file in order to get a usable RTL theme - 

cvs diff: browser/themes/pinstripe/browser/Toolbar-rtl.png is a new entry, no comparison available
cvs diff: browser/themes/pinstripe/browser/urlbar/endcap-focused-rtl.png is a new entry, no comparison available
cvs diff: browser/themes/pinstripe/browser/urlbar/endcap-rtl.png is a new entry, no comparison available
cvs diff: browser/themes/pinstripe/browser/urlbar/startcap-active-rtl.png is a new entry, no comparison available
cvs diff: browser/themes/pinstripe/browser/urlbar/startcap-focused-rtl.png is a new entry, no comparison available
cvs diff: browser/themes/pinstripe/browser/urlbar/startcap-rtl.png is a new entry, no comparison available
cvs diff: toolkit/themes/pinstripe/global/menu/menu-arrow-dis-rtl.gif is a new entry, no comparison available
cvs diff: toolkit/themes/pinstripe/global/menu/menu-arrow-hov-rtl.gif is a new entry, no comparison available
cvs diff: toolkit/themes/pinstripe/global/menu/menu-arrow-rtl.gif is a new entry, no comparison available


This is also a friendly request to mark the patch as blocking RC1 or something. :-)

	is+mozilla
	Comment 48 • 17 years ago

I've tested the Add-on manager with the latest nightly and update RTL patch.  The behaviour has changed somewhat. First, it has become intermittent.  Second, I no longer need to resize the window in order to fix. Usually switching to a different tab will fix it. Third, sometimes half the icons are missing, and sometimes their distorted.  Here's a picture of them being distorted.  If succeed in reproducing the half of the icons are missing situation, I'll upload a picture of that also.

	Yehuda
	Comment 49 • 17 years ago

(In reply to comment #48)
> Created an attachment (id=313265) [details]
> screen shot of distortion of icons in Add-On Manager in Proto RTL
> 

Thanks for posting the screen shot. Nevertheless, I could not reproduce this issue...

	Tomer Cohen :tomer
	Comment 50 • 17 years ago

Changelog from previous patch:

Fixed images in the RTL toolbar
browser.xml fixed to send chromedir parameter to the browser.css

Please remoember that you can't build the RTL theme from this patch, and will need also to add the images as described in comment 47.

	Reed Loden [:reed]
	Comment 51 • 17 years ago

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

Need review first before asking for approval.

	(no longer active)
	Comment 52 • 17 years ago

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

	Tomer Cohen :tomer
	Comment 53 • 17 years ago

Please note that we are still looking for a review...

Yehuda - As I remember this is still not the final patch. Do you remember what is still missing?

	Ryan Flint [:rflint] (ping via IRC for reviews) Assignee
	Comment 54 • 17 years ago

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

The patch is incomplete and has a number of issues (some the result of the LTR flavor of pinstripe). I'm currently writing a new version so this has a chance of landing for RC1.

	Yehuda
	Comment 55 • 17 years ago

(In reply to comment #53)
> Yehuda - As I remember this is still not the final patch. Do you remember what
> is still missing?

Work on the places manager is still in progress. And, as Ryan noted, changes to the LTR theme (mainly due to bug #427555) should be applied to this patch as well.

	Mike Beltzner [:beltzner, not reading bugmail]
	Comment 56 • 17 years ago

Ryan: thanks - a question, though. Would it be worth landing the lion's share of this which *does* work, in order to get the RTL theme into a state where it's shippable? I'm willing to accept some amount of visual error, especially on less seen bits of the UI, and we can then fix the rest as follow ups if time permits.

	Ryan Flint [:rflint] (ping via IRC for reviews) Assignee
	Comment 57 • 17 years ago

This covers everything I could see that can be fixed in the theme. I was hoping bug 427935 would land first, but it'll be easy enough to update this once it does.

	Mic Berman
	Comment 58 • 17 years ago

i've nominated this for blocking, if it's at all reasonable to fix this for fx3 it would be great for hebrew and arabic (or RTL) languages on Mac platform. it's been an outstanding issue for a while and it would be wonderful if we could address it. 

	Mike Connor [:mconnor]
	Comment 59 • 17 years ago

Nice to have, not blocking.  That said, I'll try to review tonight.

	Mike Connor [:mconnor]
	Comment 60 • 16 years ago

Comment on attachment 315034 [details] [diff] [review]
Patch v2

r=me, but please unbitrot after kevin lands the last theme drop tonight

	Mike Beltzner [:beltzner, not reading bugmail]
	Comment 61 • 16 years ago

Comment on attachment 315034 [details] [diff] [review]
Patch v2

!renztleb=a

	Ryan Flint [:rflint] (ping via IRC for reviews) Assignee
	Comment 62 • 16 years ago

mozilla/toolkit/themes/pinstripe/global/menu/menu-arrow-dis-rtl.gif 1.1
mozilla/toolkit/themes/pinstripe/global/menu/menu-arrow-hov-rtl.gif 1.1
mozilla/toolkit/themes/pinstripe/global/menu/menu-arrow-rtl.gif 1.1
mozilla/toolkit/themes/pinstripe/global/console/console.css 1.14
mozilla/toolkit/themes/pinstripe/global/dialog.css 1.5
mozilla/toolkit/themes/pinstripe/global/findBar.css 1.13
mozilla/toolkit/themes/pinstripe/global/jar.mn 1.44
mozilla/toolkit/themes/pinstripe/global/listbox.css 1.7
mozilla/toolkit/themes/pinstripe/global/menu.css 1.15
mozilla/toolkit/themes/pinstripe/global/menulist.css 1.16
mozilla/toolkit/themes/pinstripe/global/radio.css 1.7
mozilla/toolkit/themes/pinstripe/global/tree.css 1.17
mozilla/toolkit/themes/pinstripe/mozapps/extensions/extensions.css 1.47
mozilla/browser/components/places/content/places.xul 1.132
mozilla/browser/components/search/content/search.xml 1.125
mozilla/browser/themes/pinstripe/browser/Toolbar-rtl.png 1.1
mozilla/browser/themes/pinstripe/browser/browser.css 1.149
mozilla/browser/themes/pinstripe/browser/browser.xml 1.11
mozilla/browser/themes/pinstripe/browser/jar.mn 1.86
mozilla/browser/themes/pinstripe/browser/searchbar.css 1.24
mozilla/browser/themes/pinstripe/browser/places/menubutton-end-pressed-rtl.png 1.1
mozilla/browser/themes/pinstripe/browser/places/menubutton-end-rtl.png 1.1
mozilla/browser/themes/pinstripe/browser/places/menubutton-start-pressed-rtl.png 1.1
mozilla/browser/themes/pinstripe/browser/places/menubutton-start-rtl.png 1.1
mozilla/browser/themes/pinstripe/browser/places/organizer.css 1.14
mozilla/browser/themes/pinstripe/browser/places/places.css 1.25
mozilla/browser/themes/pinstripe/browser/urlbar/endcap-focused-rtl.png 1.1
mozilla/browser/themes/pinstripe/browser/urlbar/endcap-rtl.png 1.1
mozilla/browser/themes/pinstripe/browser/urlbar/startcap-active-focused-rtl.png 1.1
mozilla/browser/themes/pinstripe/browser/urlbar/startcap-active-rtl.png 1.1
mozilla/browser/themes/pinstripe/browser/urlbar/startcap-focused-rtl.png 1.1
mozilla/browser/themes/pinstripe/browser/urlbar/startcap-rtl.png 1.1
mozilla/toolkit/components/console/content/console.xul 1.16

	Marcia Knous [:marcia]
	Comment 63 • 16 years ago

The he build looks good using Mozilla/5.0 (Macintosh; U; PPC Mac OS X 10.5; he; rv:1.9pre) Gecko/2008042404 Minefield/3.0pr.

	Henrik Skupin [:whimboo][⌚️UTC+2]
	Comment 64 • 16 years ago

Marcia, does it mean we can mark this bug as verified?

	Henrik Skupin [:whimboo][⌚️UTC+2]
	Comment 65 • 16 years ago

This bug was fixed for Firefox 3 a while ago. Lets mark it verified.