Closed Bug 85420 (bidiui) Opened 23 years ago Closed 20 years ago

Need Bidi UI / Menu commands and keyboard shortcut for controling Page & Input fields (textarea) direction for specific locales (or when bidi.browser.ui is set)

Categories

(Core :: Layout: Text and Fonts, defect, P1)

defect

Tracking

()

VERIFIED FIXED
mozilla1.8alpha5

People

(Reporter: mahar, Assigned: asaf)

References

()

Details

(Keywords: fixed-aviary1.0, intl, relnote)

Attachments

(6 files, 9 obsolete files)

143.75 KB, image/png
Details
55.08 KB, image/x-png
Details
1.98 KB, application/x-xpinstall
Details
10.47 KB, patch
asaf
: review+
Details | Diff | Splinter Review
2.49 KB, patch
mconnor
: review+
Details | Diff | Splinter Review
11.25 KB, patch
asaf
: review+
Details | Diff | Splinter Review
We need to have the Bidi options present.
Blocks: 75009
Blocks: 74928
Blocks: 82849
Blocks: 74929
Mass-move all BiDi Hebrew and Arabic qa to me, zach@zachlipton.com. 
Thank you Gilad for your service to this component, and best of luck to you 
in the future.

Sholom. 
QA Contact: giladehven → zach
Not to nag or anything, but what is going on with this bug? Are we going to see
anything related to it?
No longer blocks: 75009
Blocks: 75009
Blocks: 115712
Can i nag again to know what is going on without being a pest?
well, did it anyway...
so- what is going on with this bug?
hi,

I've added the following in my prefs.js file after looking at the code

user_pref("bidi.controlstextmode", 2);
user_pref("bidi.direction", 2);
user_pref("bidi.textstyle", 2);

This still does'nt give me bidi behaviour on text* widgets, though it does
give me orientation.

Could someone let me know if this is the right way to enable bidi until the
UI is enabled?

thx,
prabhat.

PS: I am trying this on *nix (Solaris, Linux)
Michael/Zach,

Is there going to be any movement on this bug? It's set as a blocker but its
status is still new and there has not been any activity in a long time.

Are there any successful implementations/screenshots of this in action yet?

Some bugs that appear to be related are #80352 and #115715 (a meta bug).

Perhaps some dependencies need to be set up but I'll leave that up to one of you
who is more clued in on this issue.
Okay, I think I've confused this with something else. I am thinking of Bidi in
XPFE, i.e. in XUL files and appearing in the user interface. This bug probably
relates to the UI in prefs for general bidi options in web content?

Either way, does anyone know the status of BiDi in XUL / XFFE?
*** Bug 121996 has been marked as a duplicate of this bug. ***
You can Bidi-fy the whole chrome by adding the following to your userChrome.css
stylesheet:

window,dialog,wizard,page {
  direction: rtl;
}
Keywords: nsbeta1
I vote nay!

Why on earth would anyone want this?
The whole RTL UI is something that Microsoft is trying to push down our throats 
for no good reason, other than marketing.

Check out, for example, localised Hebrew versions of Windows... they just don't 
look and feel right. why? because most of the files and applications have Latin 
names.
The outcome is a very messed up user interface.

Don't waste your time on this.

Prog.
umm.., prognathous@hotmail.com , I am not sure we are talking about the same 
this. This bug is about a UI to control bidi-spcific options. NOT a 
Hebrew/Arabic UI for Mozilla (that is handled by the l10n team. For hebrew: 
http://www.mozilla.org.il ).

so rather you like a localized bidi ui or not, if you are a bidi user, you 
still need some options which are lacking at the moment (even on English UI).

Or am I mistaken?

Will we ever get in the UI bidi prefs like the old bidi Netscape had?
(see http://www.forbes.co.il/nana/img/hb_ns.gif  . There also was an option of 
copying visual text as logical, under a diffrent menu- notice that this shot 
has a "bidi options" menu- http://www.forbes.co.il/nana/img/ns_heb.gif )
mpt had a serious look at this bug already?

Are you guys sure we need a BiDi UI? Microsoft seems to manage without it, and I
bet developers would be happier too if the BiDi behavior was consistent and
couldn't be altered by the end-user.
Actually IE on Windows at least has a UI for making a page RTL or LTR.

The readon IE doesn't need a UI is because it has an operating system where 
these things can be set.

Mozilla is intended to be crossplatform, so we can't rely on the OS behavior to 
be there.
I don't recall Windows have any BiDi settings dialog either.

Why won't we just adopt Windows defaults? Its seems like the defacto standard
for Hebrew now, so for things not defined by the real standard, we should
probably adopt Windows' behavior.
In windows, Bidi behavier is defined in the regional settings (in the control
panel), If you have Arabic windws, for example, you can define there what style
digits you prefer. However, if you are not using Arabic windows, you have no
control over these settings (for example).
Also, as mentioned above, windows IE does have UI to change directionality of items.

Since most platforms Mozilla works on is NOT Arabic (Or Hebrew) windows, but
other versions of windows and non-windows systems (which may or may not have
places to defined bidi settings), using the windows settings does not seem to me
to be a smart move.
I'm talking about adopting defaults for things Windows presents no choice for
either. For example, Hebrew Windows doesn't allow any tuning to its BiDi
behavior -- which means Microsoft decided on sensible defaults there.

What I'm saying is -- what Microsoft has decided could have a sensible default
and thus avoid having a UI settings screen for, is good enough for us too.
Actually, that's an interesting question, Simon (and maybe Maha if she still 
reads these)

What does windows do about the clipborad issue and the controls issue?

Is Mozilla just adding better functionality where IE lacks?

Or does IE handle things in some special way?
Not sure what you mean by clipboard problem - so I'd guess:
IE6 copies text into the clipboard as-is. Since the rest of Windows is logical,
copying Logical text works fine, while Visual text is pasted "reversed" in all
Windows apps. I'm not sure how Mozilla handles those issues, but its pretty
useless until it would finally learn to mark multiple BiDi lines.

As to controls, they're always Logical. That's been the "standard" in Israeli
Visual pages for years and nobody expects browsers to behave differently.
Yes and no.
In Arabic windows, for example, IE decides if do display Arabic digits or Indic
digits, based on the settings in the control panel (Hebrew users do not see
this, since Hebrew uses only Arabic digits).
For clipboard, Bidi Netscape 4.6i had an option to convert visual Hebrew copied
to logical on the clipboard- from looking in the about:config of mozilla it
seems this option should be avalible. IE does not offer this function.

IE win has UI for changing the directionality of a page/email (in the encoding
menu). this is a bidi option we should have (bugs are already filed for that),
and we need a UI for it as well.

So we have at least 3 bidi features that need some sort of UI, and that using a
default (MS or otherwise) is not enugh.
Just to clarify, these are the preferences in question (* indicates default)

// ------------------
//  Text Type
// ------------------
// 1 = charsettexttypeBidi *
// 2 = logicaltexttypeBidi
// 3 = visualtexttypeBidi

// ------------------
//  Controls Text Mode
// ------------------
// 1 = logicalcontrolstextmodeBidiCmd *
// 2 = visiualcontrolstextmodeBidi
// 3 = containercontrolstextmodeBidi

// ------------------
//  Clipboard Text Mode
// ------------------
//  1 = logicalclipboardtextmodeBidi
// 2 = visiualclipboardtextmodeBidi
// 3 = sourceclipboardtextmodeBidi *

// ------------------
//  Numeral Style
// ------------------
// 1 = regularcontextnumeralBidi *
// 2 = hindicontextnumeralBidi
// 3 = arabicnumeralBidi
// 4 = hindinumeralBidi

// ------------------
//  Charset Mode
// ------------------
// 1 = doccharactersetBidi *
// 2 = defaultcharactersetBidi
What I don't understand is why are we bench-marking against IE, as Mozilla is 
clearly not only a windows platform browser. I think if we want to bench mark 
against IE, we would do so with all the "good" features and surpass that, and 
find out features that are not there and get them in.

One outstanding point that I would like to raise here is the visual data 
support, for Arabic we can have visual and implicit data represented using the 
same codepage, so this cannot be differentiated using encoding. 
I would like to give a couple of examples here on what adding the bidi ui would 
provide for the end user:

Example 1 (text type)
A UTF-8 encoded html with data in Arabic FExx range, by definition this 
encoding is logical, so it undergoes reordering. But the FExx data is used to 
represent visual data!!.... how to set that?

Example 2 (Text type in Text Areas controls)
most of the customers use AS/400 systems (visual data again),on the conversion 
from visual to logical data loss is inevitable. so the preference is to have a 
way to represent visual data (either through converting to another visual 
codepage like 864, or to unicode FExx range). helping customers with their 
legacy data problems!!
The same argument goes to clipboard text mode, the ability to copy something 
that is logical and paste it to a visual application like PCOM (or any similar 
visual host application)

Example 3 (Numeral Shapes)
If we do pick up the bidi information from the regional settings and it is set 
to National (always view numbers in Arabic used format, how about viewing a 
totally English page - numbers will be totally out of context.
Does the user have to continuously visit Regional setting on his machine to 
change that setting while moving between pages on the net?!
Blocks: 98160
I noticed that "autonomic" visual widgets, whose text mode is considered independently from the entire 
document format, are no longer supported. 

However, "visiualcontrolstextmodeBidi" can be applicative. In the absence of such an UI option, one can 
realize visual controls only by applying "containercontrolstextmodeBidi". Then, he/she may be constrained 
to change the whole document format (if it's "logical"), possibly making it totally unreadable.
Kindly be informed that Ahmad A. Abu-Taha (ahtaha@eg.ibm.com) from IBM Egypt is 
replacing Maha Abou El-Rous (mahar@eg.ibm.com) in monitoring and receiving 
notifications of Mozilla bugs regarding Arabic.
adt: nsbeta1-
Keywords: nsbeta1nsbeta1-
For those who need BiDi UI and don't want to wait until this bug is fixed,
here's a simple workaround based on javascript bookmarklets:

1. Create two bookmarks and place them in your personal bookmarks toolbar.
2. Change the properties of these bookmarks as follows:
   Name: Left-to-Right	  Location: javascript:void(document.dir='ltr')
   Name: Right-to-Left	  Location: javascript:void(document.dir='rtl')
3. When you need to change the page direction, simply click the the
corresponding bookmark.

This is very useful when reading plain-text Hebrew or Arabic emails in
Gecko-based browsers (using webmail interfaces).

Prog.
Hopping that the bookmarklet workaround, as seen in previous comment (25) is
only temporary (hint, hint!) than solution, here is a better switcher, which
require usage of only one bookmark.

javascript:(function(){if (document.dir=='rtl') document.dir='ltr'; else
document.dir='rtl';})()
Way to go, Tomer :-)

Now, can any of you javascript gurus suggest the code to a similar bookmarklet
that would only modify the direction of the current (active) textarea? this
could serve as a nice workaround for Bug 98160.

The best I could come up with so far only changes the *first* textarea, not the
active one:

  javascript:void(document.getElementsByTagName('textarea')[0].dir='ltr')
  javascript:void(document.getElementsByTagName('textarea')[0].dir='rtl') 

Any suggestions?

Prog.
If I may insert a shameless plug, my mozBidiBindings package ( 
http://toast.unwind.co.il/hacks/mozBidiBindings-0.3.tar.gz ) provides handy 
key bindings (XBL) for switching the input direction in all edit boxes and in 
Composer paragraphs. Installation is non-trivial (I should make it into an XPI 
one day) but I'm sure you could figure it if you follow the README. 
 
This should work with Mozilla 1.3. Haven't tested with 1.4 yet. 
 
I am in front of you, already working on that, and asking people about a
solution. Meanwhile, we can create one to change all the <textarea>-s in a page.


Let's move this off-topic discussion, to its place, to suppress the overload of
this bug. 
Ilya, we (at mozilla.org.il) already know yours, and another shorter one, but
the real-world problem with that is the key-binding has to be changed to the
system default/Windows-like.
>Let's move this off-topic discussion, to its place, to suppress the overload of
>this bug.

If you move the discussion somewhere else, please provide a reference here so
interested parties can stay in the loop.

From Comment #28:
>If I may insert a shameless plug, my mozBidiBindings package ( 
>http://toast.unwind.co.il/hacks/mozBidiBindings-0.3.tar.gz

but it only works with mozilla, and not the other Geko based browsers, right?
I don't think that this is a blocker bug.
Severity: blocker → normal
The suggested functionality overlaps much of what Bug 79682 deals with. Anyone
has any idea what can be done to facilitate the addition of such a menu to Mozilla?

Much of the opposition to these bugs was that cluttering the menus with dozens
of BiDi UI options would not be really beneficial to users (not even BiDi
users). This may be somewhat true, but there's at least one setting that is
sorely missing - being able to change the page direction on the fly (in
Explorer: View -> Encoding -> LTR/RTL Document).

I am sure that most BiDi users will settle for having just this one very
important setting. The current bookmarklet hacks are not something that we can
seriously offer as permanent solutions.

Prog.
Blocks: 240501
Instead of deciding to not decide, how about finding an alternative approach,
one that will provide us with hard evidence as to what users really want/need?
this alternative could be a user survey.

In conducting such a survey we should discuss the following issues:
* Current page direction
* Textarea direction
* Interface for inserting control characters
* Interface for inserting special BiDi characters (Browser) and entities
(Composer), e.g. uncommon punctuation marks and biblical cantillation accents
* Numeral shape (contextual, Arabic, Hindi...)
* Default page direction
* Clipboard direction
* Ways to access these functions: menus, context menus, buttons, keyboard
shortcuts, prefs, hidden prefs

IMHO, it would probably be sufficient to provide menu entries for Current page
direction and for Textarea direction, as well as a keyboard shortcut for the latter.

Any opinions?

Prog.
(In reply to comment #36)

> In conducting such a survey we should discuss the following issues:
> * Current page direction

a menu item, e.g. in the character encoding menu, should do that.

> * Textarea direction

a Ctrl+Shift (or other on non-windows systems as appropriate) shortcut for this
will suffice IMO.

> * Interface for inserting control characters

how many users do you reccon know control characters exist, let alone know how
to use them? completely unneeded.

> * Interface for inserting special BiDi characters (Browser) and entities
> (Composer), e.g. uncommon punctuation marks and biblical cantillation accents

i would agree for that for Composer only. i think most people won't bother
inserting things like Geresh or Makaf through a menu item. the right way to to
this is by your activity at the Israeli standards institute, on changing the
keyboard layout.

> * Default page direction

again, this will be good for Composer. for Navigator, the default is set by the
HTML specification (i.e. LTR unless otherwise noted).
The extesion (For now, it's for firefox only, seamonkey version will become
soon) in the following link adds "Invert Direction" to every text field (INPUT
and TEXTAREA).
http://mano.fastmail.fm

I will add page-direction control later.
The extension has benn updated. The new version contains UI to control page
direction and keyboard-shortcut for "Switch Text Direction".

Additional information (Hebrew):
http://www.mozilla.org.il/board/viewtopic.php?p=4765#4765
Any Arabic, Urdu or Farsi speakers in the house? The new BiDi UI extension needs
your help to translate a few strings in order to provide localizations for these
languages.

BiDi UI Project: http://bidiui.mozdev.org
Relevant bug: http://bugzilla.mozdev.org/show_bug.cgi?id=6675

Thanks,

Prog.
see also bug 98160: can't change alignment/directionality in textarea
(1) Version 0.3.3 and up supports Mozilla Suite.

(2) Screenshots:
http://bidiui.mozdev.org/images/SwitchTextDirectionMenu.png
http://bidiui.mozdev.org/images/switch_page_before.png

(3) The next version will come with Arabic localization
(see http://bugzilla.mozdev.org/show_bug.cgi?id=6675).
Depends on: 251820
Now that Chatzilla has UI for switching directionality (bug 250864), I'm
wondering if it can be a so-big problem to include the additional menu items
from BiDi UI (http://bidiui.mozdev.org)?...I am not talking about the ability to
"code" it :-)

Also notice that IE/Win has the ability to switch page direction and Safari/Mac
has UI for switching writing direction (textareas' contexual menu).

To say the truth, we really don't need anything from bug 79676.
Depends on: 252475
Accel+Shift+E has been reserved for directionality control. (bug 251958)
Depends on: 251958
(In reply to comment #44)
> Accel+Shift+E has been reserved for directionality control. (bug 251958)

I'd just like to note that this key-binding doesn't mean that we shouldn't
*also* emulate Windows' behavior - separate key-bindings for each direction. Bug
166240 has to be fixed first to make that possible.

Prog.
On Windows 2000, the presense of BiDi UI in Windows' built-in controls depends 
on the Hebrew language support being installed (in Regional Options), not 
necessarily on selecting a Hebrew locale. 
(Coincidentally, without the "language support" installed, BiDi reordering 
doesn't occur at all.) 
 
On Win2K and higher, we should use UniScribe's ScriptGetProperties 
( http://msdn.microsoft.com/library/en-us/intl/uniscrib_6fn7.asp ), and then 
inspect the langid or the fComplex member of the returned SCRIPT_PROPERTIES 
structures. To prevent DLL dependency on Uniscribe, we should use LoadLibrary. 
On WinNT and Win9x, since the BiDi support on those systems requires a special 
version of the OS, we should use some other way (IBMBIDI already has some 
functions to detect the OS built-in BiDi capabilities, right?). 
Ilya: we need a cross platform solution, what do you advice to do on Linux for
example?

(On Mac OS X, we already have a problem:bug 252475
, but Javier is intnded to work on this soon).
No longer depends on: 251820
The solution would have to be multiplatform but not platform-independent; that 
is, we'll try to match our behavior to what's accepted on each platform. 
 
I present the current state of things, so you'd understand what behavior a 
Linux user expects. 
- On Gtk+, there's no BiDi UI, but there's an ever-present "Insert Control 
Character" menu. 
- On Qt, there's no BiDi UI, but there are optional BiDi accels, activated by 
selecting a checkbox ('Enable enhanced support for Right-to-left languages') in 
Qt Configuration (qtconfig) utility. 
 
Mechanisms we can try to rely on for detection: 
- Presense of an optional BiDi rendering module (a-la UniScribe's "Hebrew" and 
"Arabic" optional modules) 
 
Even when the rendering system support loadable modules (Gtk's Pango library), 
they are usually installed all in one bunch. 
 
- BiDi language locale setting 
 
Relying on the locale is not reliable, since many users don't configure their 
country's locale (except for monetary/date conventions, or translated UI, 
there's no incentive to change it). 
 
- Presense of a BiDi language keyboard mapping 
 
Keyboard mapping files are installed in one bunch, without user intervention. 
 
- Usage of a BiDi language keyboard mapping 
 
The currently-used keyboard mappings cannot be reliably queried without 
resorting to getting familiar with KDE's (kxkb) and GNOME's (gswitchit?) 
keyboard mapping loaders. As last resort, we might employ such strategy. 
However, is it right to tie capabilities required to read BiDi pages with the 
ability to write BiDi text? 
 
---- 
 
On a separate thought, Internet Explorer 5 feels free to interfere with the 
user's browsing the first time he stumbles on a Hebrew page, prompting him to 
download and install a "Hebrew support" package. What if we'll also prompt the 
user the first time he stumbles on a page which uses BiDi (detecting using 
iface provided by bug 251820) ? 
IMHO: your last suggestion is much better than every proposed solution (until
now...), and much more safe as a cross-platform solution.

We should have a hidden pref (something like: "browser.chrome.bidiui"):
- 0: user hasn't been asked for BiDi UI
- 1: user wants BiDi UI.
- 2: user has been asked, and doesn't want BiDi UI.

What do you think?
Having another dialog asking to turn this on is useless, we really should either:

1) figure out if it makes sense to show the UI (system locale, page charset)
2) make if preference based.

the only problem with 1) is performance - we don't want to slow down the browser
nor say the view menu.
(In reply to comment #50)
Why not a simpler solution:
1. Check at startup if BiDi is available
2. If it is, show the menu item active, If not, show the menu item disabled (grayed).
 (In reply to comment #50) 
> Having another dialog asking to turn this on is useless 
 
I guess "useless" was not the word you meant to use, as the use of this dialog 
is clear. 
 
The benefit of this dialog over a pref-only solution, is that: 
a) it saves the user a journey to the Preferences, 
b) most users will accept the dialog's offer (since they've shown interest in 
the BiDi page in the first place), 
c) it doesn't force a Preference on latin-1-ers. 
 
> 1) figure out if it makes sense to show the UI (system locale, page charset) 
 
Didn't my previous comments prove to you that heuristics are bound to be flaky? 
 
> 2) make if preference based. 
 
Obviously this will always be present, to override whatever heuristics we make. 
I suggest we follow the 80:20 paradigm, i.e. implementing 20% of the suggested
heuristics, so that the needs of 80% of our target users are answered. I'm
afraid that implementing them all will push the release of this long overdue
functionality again.

Let's assess the effectiveness of Doron's first option (detecting locale and
charset only):

1. At least (and I believe I'm being conservative here) 80% of our target BiDi
users don't configure locale of another country, but rather have the proper one
set. The vast majority of computers in Israel that come with pre-installed OSes
are already configured as he-IL out of the box. In this implementation, Mozilla
will always display BiDI menus for these users, regardless of charset.

2. Of the 20% who don't have the proper locale configured, the UI will
dynamically display BiDi menus when browsing pages with certain charsets. These
menus are unlikely to bother one who browses BiDi pages (and with 8 bit
charsets, it's difficult to false detect). The only case where the menu may be
inappropriate is when browsing LTR Unicode pages. Would a single addition to the
View menu in this case bother anyone? I'm quite sure that it won't.

Aiming for 100% detection is nice, but it's a costly fine tuning, one that will
complicate the code and will likely put off fixing this bug even further.

As for performance, it's probably best to test the impact of this single system
call *per session*. Anyone care to benchmark this?

Prog.
Prog: instead of "dynamic menus" (...HIG...), we should have a UI for the pref
(browser.chrome.bidiui ?) in the languages pane (after "default charset").

-------
The pref should not be visible for RTL locales.
Are you talking about a visible or a hidden pref? A visible one may look
something like this:

  For Left-to-Right locales, display direction menus:
  [ ] Always
  [o] Only in pages that use Unicode or Right-to-Left character encodings
  [ ] Never

Is that what you have in mind?

Prog.
(In reply to comment #55)
> Are you talking about a visible or a hidden pref? A visible one may look
> something like this:
> 
>   For Left-to-Right locales, display direction menus:
>   [ ] Always
>   [o] Only in pages that use Unicode or Right-to-Left character encodings
>   [ ] Never
> 
> Is that what you have in mind?
> 
> Prog.

Of course visible, but not so complicated (user: "Unicode? ha? charset!?"), just:
 [ ] Include interface for Right-to-Left languages
(In reply to comment #55) 
>   [o] Only in pages that use Unicode or Right-to-Left character encodings 
 
This is incorrect. Unicode text encodings (UTF-8, UTF-16 etc.) would _NOT_ 
cause the 'usesBidi' flag to be true. The only times this flag would be set 
are: 
1) The page contains characters of one of the RTL languages (encoding is 
irrelevant) 
2) The page contains at least one element whose direction is RTL (e.g. 
DIR=RTL). 
 
Only those cases involve activating the IBMBIDI code, thus setting the 
'usesBidi' (or whatever we'll call it) flag. 
 
(In reply to comment #53) 
> At least (and I believe I'm being conservative here) 80% of our target BiDi 
users don't configure locale of another country 
 
This is also incorrect. Locale is only relevant for Unices. On Windows, we have 
a more reliable method which is also more consistent with Windows (see my 
comment #46). Trust me that on most Unices (esp. the so-common Linux 
workstations), the locale is set to en_US. 
Alias: bidiui
Keywords: nsbeta1-intl
Summary: Need Bidi UI → Need Bidi UI / Menu commands and keyboard shortcut for controling Page & Input fields (textarea) direction
Attached patch code! (obsolete) — Splinter Review
Adds an item in the view menu, which gets shown/hidden on startup of the window
for performance reasons.  It still checks the page locale since the code is
shared (isBidiEnabled()), which can be optimized.

Adds an context menu to text input fields.
Doron, thanks for the code!

Few comments:
1. Why do we check the charset's language, instead of asking whether BiDi is
used on the page (using the interface I propose in bug 251820) ? I think the 2nd
option is better, since it works even on Unicode charsets which aren't
designated for any specific language.
2. Should I introduce an iface to get the system's supported languages (with a
Uniscribe-based implementation on Win2K)? For Win2K OS integration, this would
be the correct way (instead of querying the locale).
Comment on attachment 155116 [details] [diff] [review]
code!

Thnaks for working on that!

now to my comments... :-)

>+  try {
>+    var localService = Components.classes["@mozilla.org/intl/nslocaleservice;1"]
>+                                 .getService(Components.interfaces.nsILocaleService);
>+    systemLocale = localService.getSystemLocale().getCategory("NSILOCALE_MESSAGES");
>+  } catch (e){}
>+
>+  rv = ( (systemLocale == "he") || (systemLocale == "ar") || (systemLocale == "fa") 
>+         || (systemLocale == "ur") );

As far as i remember, the returned value for:
  localService.getSystemLocale().getCategory("NSILOCALE_MESSAGES");
is in the syntax of "en-US", "he-IL" etc.


>+function SwitchDocumentDirection(){
>+  if (window.content.document.dir == "ltr")
>+    window.content.document.dir = "rtl";
>+  else
>+    window.content.document.dir = "ltr";
>+}

This is not going to work for pages with frames. Solving this issue in BiDi UI
was done by using the following recursive function:

function get_Frames (frame, documentList) {
    const framesList = frame.frames;

    documentList.push(frame.document);
    for(var i = 0; i < framesList.length; i++)
    {
	get_Frames(framesList[i], documentList);
    }

    return documentList;
}

and then:

invert_page_dir: function() {
    const documents = get_Frames(window.content, new Array());

    for(var i = 0; i < documents.length; i++)
	if (documents[i].dir == 'rtl')
	    documents[i].dir = 'ltr';
	else
	    documents[i].dir = 'rtl';
}


>+        <menuseparator hidden="true" id="documentDirection-separator" />
>+        <menuitem hidden="true" id="documentDirection-swap" 
>+                  label="&bidiMenuItem.label;" accesskey="&bidiMenuItem.accesskey;"
>+                  oncommand="SwitchDocumentDirection()"/>
>       </menupopup>


1. What's the position of the menu item? why separator? screenshots are always
welcome :-)
2. Pedant: from the View menu (not like the contextual-menu-item) you're
switching *Page* direction, not just the text, it needs a separate entity.

>+<!ENTITY bidiMenuItem.label         "Switch Text Direction">
>+<!ENTITY bidiMenuItem.accesskey     "d">

When we have a separate entity for the view menu, please you Pa&ge.

>+      <menuseparator id="context-sep-bidi"/>

What's the position?

>+      <menuitem id="context-bidi-text-direction-toggle"
>+                label="&bidiMenuItem.label;"
>+                accesskey="&bidiMenuItem.accesskey;"
>+                oncommand="SwitchTextEntryDirection(gContextMenu.target)"/>

The method SwitchTextEntryDirection is not included in your patch 



>Index: xpfe/communicator/resources/content/nsContextMenu.js
>===================================================================


does this mean, it will also be available for mail & news? yay!


Last things:
1. Accel+Shift+E is reserved for "Switch Text Direction", please use it.
2. we need this for the Firefox too.

-----
..and again: Thank you for working on this!
After forcing isBidiEnabled to return true:

1. The "Switch Text Direction" (Page! :-) ) menu item is at the end of the View
menu after a sep., this is ok for textboxes, but not for the View menu. IMO: The
best position is after "Character Encodings", without a sep before.
2. This doesn't apply Mail & News as i taught, but for that we have bug 96057
and bug 119857.
this was a work in progress, and yeah, missed a file.

>2. Should I introduce an iface to get the system's supported languages (with a
>Uniscribe-based implementation on Win2K)? For Win2K OS integration, this would
>be the correct way (instead of querying the locale).

a nsISystemLocale that basically did what my code did, plus special stuff for
say win2k/xp/etc would be great.
Ilya, how do you define 'supported languages'? In Win9x/2K/XP you're probably
referring to the options in the language switching applet; what about elsewhere?
KDE has such an applet, and X in general has the keyboard layout and its groups.
Perhaps some of these should be checked? Or how about elements of the locale
(differences between CTYPE and LANG)?
(In reply to comment #63)

See comment 48.

Please don't block this bug for adding this behavior, after we'll finish here,
we can open a separate bug for improved isBiDiEnabled, thnsks.
I agree we should push the locale-checking code in -- but get the 
charset-checking code out! Figuring out whether BiDi is relevant by charset is 
absolutely broken logically-wise.  
 
I'll implemented the isBidiEnabled flag very soon. The Uniscribe code for Win2K 
will take more time, as I don't currently have a Win2K development environment 
set up. 
 
-- 
 
(In reply to comment #63) 
 
I'm referring to Control Panel | Regional Options. There, on Win2K and higher, 
you have a list of languages (not keyboard layouts!) which the OS can render. 
If any of the BiDi languages are installed, BiDi UI will be enabled. On WinXP 
and higher, there's a separate checkbox on whether to enable RTL support. 
 
Win9x series come in localized editions, which come supporting a predefined 
list of languages. There should be a way to figure whether a certain edition 
supports an RTL language: IBMBIDI does this somewhere. 
 
On Linux, the only portable hint is the locale. If Qt is installed, one can 
look in its configuration, to check for useRtlExtensions=true, but unlike Gtk, 
Qt isn't even a dependency for Mozilla. Gtk's BiDi UI is not optional, so Gtk 
doesn't give us a hint regarding what the user wants. 
(In reply to comment #65)
> Figuring out whether BiDi is relevant by charset is 
> absolutely broken logically-wise.  

I completely agree with Ilya here. I've rejected patches in the past that use
charset to determine whether Bidi support is needed.

> Win9x series come in localized editions, which come supporting a predefined 
> list of languages. There should be a way to figure whether a certain edition 
> supports an RTL language: IBMBIDI does this somewhere. 

See nsRenderingContextWin::InitBidiInfo(). This gets complicated and hacky
because what is documented and works in some flavours of Windows doesn't work in
other flavours.
Attached patch equivalent aviary patch. (obsolete) — Splinter Review
(In reply to comment #68)
> Created an attachment (id=155715)
> equivalent aviary patch.
> 

I'm going to update some problems in this patch very soon. Notice that we are
not going to have keyboard shortcut until we can use ctrl+shift on windows and
ctrl+arrows on other platforms, sorry.

I will probably add a keyboard shortcut in the hebrew l10n, and maybe produce an
extension that adds the keyboard shortcut: Accel+Shift+E (Notice that
Accel+Shift+[A-Z] doesn't work on gtk2).
As some have suggested, the switch text direction in a input field could
probably have an entry in the edit menu (for easier discovery) if inside a input
element, and a place to put the eventual key binding.
Attached patch improved patch for aviary (obsolete) — Splinter Review
Changes:
1. the locale check is working now (the format is "he-IL", not just "he").
2. one more locale
2. "Switch Text Direction" in the edit menu.
3. keybinding (!): Accel+Shift+Y (ye.., I know it requiers two hands, but
Accel+Shift+E doesn't work on gtk2).
3. .style.direction for "Switch Text Direction" instead of .dir attribute
4. no ui for bidi.browser.ui :-/

notice my XXX comment:
"+  // XXX: implement controller for cmd_SwitchTextDirection
+  document.getElementById('cmd_SwitchTextDirection').setAttribute('disabled',
!document.commandDispatcher.focusedElement); "

...not that it means something to users.
Attachment #155715 - Attachment is obsolete: true
(/me doesn't know to count :-D)

also notice:
+function SwitchFocusedTextEntryDirection() {
+  if (isBidiEnabled())

That's because we don't want the keyboard shortcut to work when the ui is hidden.
Attachment #155859 - Flags: review?(mconnor)
asaf, didn't you forget the minus character after "syr"?
(In reply to comment #73)
> asaf, didn't you forget the minus character after "syr"?

No, i take first 3 characters
Comment on attachment 155859 [details] [diff] [review]
improved patch for aviary

canceling review request.

from some reason, the contextual-menu duplicates itself, at least on mac.
Attachment #155859 - Flags: review?(mconnor)
i just missed an operator in isBiDiEnabled
Attachment #155859 - Attachment is obsolete: true
Attachment #155870 - Flags: review?(mconnor)
(In reply to comment #71)

> 1. the locale check is working now (the format is "he-IL", not just "he").

What about UTF locales? Are they counted as being bidi enabled?
also, on linux, what is checked exactly? LANG? LC_CTYPE? Something else?

Re. the format: Will he_IL (as used in Linux) be detected, or only he-IL?
(In reply to comment #77)
> (In reply to comment #71)
> 
> What about UTF locales? Are they counted as being bidi enabled?
> also, on linux, what is checked exactly? LANG? LC_CTYPE? Something else?

but you still have a lang id in your locale... right? (we're talking about the
lang of dates, currency, etc.). en_UTF won't get BiDi UI. Anyway, you can still
turn on bidi.browser.ui pref which overides the locale check.

In future, we're going to improve isBiDiEnabled.

> Re. the format: Will he_IL (as used in Linux) be detected, or only he-IL?

Mozilla has its own locale object, he_IL is converted to he-IL. To be sure, you
can test my XPI in bug 252475 (adds "What's my locale?" in the View menu) to
check that.
Attachment #155870 - Attachment description: fixed patch → fixed patch (aviary)
(In reply to comment #78)
 
> but you still have a lang id in your locale... right? (

not in LANG, but in other locale declarations

> To be sure, you
> can test my XPI in bug 252475 (adds "What's my locale?" in the View menu) to
> check that.

It claims my locale is en-US, apprently ignoring my LC_CTYP

My actual locale (from the locale command) is as follows:
LANG=en_US.UTF-8
LC_CTYPE=he_IL.UTF-8
LC_NUMERIC=he_IL.UTF-8
LC_TIME=he_IL.UTF-8
LC_COLLATE=en_US.UTF-8
LC_MONETARY=he_IL.UTF-8
LC_MESSAGES=en_US.UTF-8
LC_PAPER=he_IL.UTF-8
LC_NAME=he_IL.UTF-8
LC_ADDRESS=he_IL.UTF-8
LC_TELEPHONE=he_IL.UTF-8
LC_MEASUREMENT=en_US.UTF-8
LC_IDENTIFICATION=he_IL.UTF-8
LC_ALL=

It looks like mozilla (or at least that extention) only looks for LANG, which in
Linuxes that are set to have English UI, *will not be* he_IL, so we are talking
about a lot of misses here.

Shosh, can you test this on linux?
(In reply to comment #80)
> Created an attachment (id=155931)
> XPI to check NSILOCALE_CTYPE instead of NSILOCALE_MESSAGE

I just checked it on *Windows*. As it returned the expected result, I will
replace NSILOCALE_MESSAGE to NSILOCALE_CTYPE in the patch (on checkin), but I
have to know if it works or not on Linux.
Assignee: mkaply → bugs.mano
Priority: -- → P1
QA Contact: zach → xslf
Target Milestone: --- → mozilla1.8beta
Attachment #155870 - Attachment description: fixed patch (aviary) → fixed patch (aviary), notice comment 81 for a little change
Status: NEW → ASSIGNED
Summary: Need Bidi UI / Menu commands and keyboard shortcut for controling Page & Input fields (textarea) direction → Need Bidi UI / Menu commands and keyboard shortcut for controling Page & Input fields (textarea) direction for specific locales (or when bidi.browser.ui is set)
Whiteboard: [have patch]
Comment on attachment 155870 [details] [diff] [review]
fixed patch (aviary), notice comment 81 for a little change

>Index: browser/base/content/browser-context.inc
>===================================================================
>RCS file: /cvsroot/mozilla/browser/base/content/browser-context.inc,v
>retrieving revision 1.4.4.2
>diff -u -r1.4.4.2 browser-context.inc
>--- browser/base/content/browser-context.inc	24 Jun 2004 02:08:28 -0000	1.4.4.2
>+++ browser/base/content/browser-context.inc	12 Aug 2004 00:35:08 -0000
>@@ -224,6 +224,9 @@
>                 label="&metadataCmd.label;"
>                 accesskey="&metadataCmd.accesskey;"
>                 oncommand="gContextMenu.showMetadata();"/>
>+      <menuseparator hidden="true" id="context-sep-bidi"/>
>+      <menuitem hidden="true" id="context-bidi-text-direction-toggle"
>+                label="&bidiSwitchTextDirectionItem.label;"
>+                accesskey="&bidiSwitchTextDirectionItem.accesskey;"
>+                oncommand="SwitchTextEntryDirection(gContextMenu.target)"/>
>     </popup>
>-    
>-    
>\ No newline at end of file

random whitespace changes are bad, as I mentioned on IRC


>-    
>+
>+  // BiDi UI
>+  if (isBidiEnabled()) {
>+    document.getElementById("documentDirection-separator").hidden = false;
>+    document.getElementById("documentDirection-swap").hidden = false;
>+    document.getElementById("textfieldDirection-separator").hidden = false;
>+    document.getElementById("textfieldDirection-swap").hidden = false;
>+  }
>+

please use .removeAttribute("hidden") for these instead.



>--- browser/base/content/utilityOverlay.js	15 Jul 2004 20:43:54 -0000	1.8.12.2
>+++ browser/base/content/utilityOverlay.js	12 Aug 2004 00:35:43 -0000
>@@ -287,6 +287,8 @@
>   goUpdateCommand('cmd_paste');
>   goUpdateCommand('cmd_selectAll');
>   goUpdateCommand('cmd_delete');
>+  // XXX: implement controller for cmd_SwitchTextDirection
>+  document.getElementById('cmd_SwitchTextDirection').setAttribute('disabled', !document.commandDispatcher.focusedElement); 

I'm assuming there will be a followup bug filed to clean this up?

with the changes I noted, r=me
Attachment #155870 - Flags: review?(mconnor) → review+
Attached patch Final patch for the fox (obsolete) — Splinter Review
in reply to:
> >-	
> >+
> >+  // BiDi UI
> >+  if (isBidiEnabled()) {
> >+	document.getElementById("documentDirection-separator").hidden = false;
> >+	document.getElementById("documentDirection-swap").hidden = false;
> >+	document.getElementById("textfieldDirection-separator").hidden = false;

> >+	document.getElementById("textfieldDirection-swap").hidden = false;
> >+  }
> >+
> please use .removeAttribute("hidden") for these instead.

from #developers:
----
<mconnor> Mano: just as a note, we tend to prefer .removeAttribute("foo") to
.foo = false;
...
<NeilZZZ> mconnor: not for hidden or collapsed
<NeilZZZ> mconnor: .hidden = false is faster than removeAttribute("hidden")
because 
...
<Mano> mconnor: so leave it as it is?
...
<mconnor> in response to your question, yes, leave it as it is
----

The other stuff is fixed (including comment 81).
Attachment #155870 - Attachment is obsolete: true
Attached patch Final patch for the fox (obsolete) — Splinter Review
oops, wrong file
Attachment #156132 - Attachment is obsolete: true
Comment on attachment 156133 [details] [diff] [review]
Final patch for the fox

moving review from mconnor, requesting approval.
Attachment #156133 - Flags: review+
Attachment #156133 - Flags: approval-aviary?
Flags: blocking-aviary1.0PR?
Whiteboard: [have patch] → [have patch], needed-aviary1.0, [l10n impact]
(In reply to comment #77) 
> What about UTF locales? Are they counted as being bidi enabled? 
> also, on linux, what is checked exactly? LANG? LC_CTYPE? Something else? 
 
The "charset" part of the locale is irrelevant. A locale's charset and 
language/country are unrelated. We shall only consider the language/country 
part of the locale to decide whether we need BiDi UI. 
 
(In reply to comment #77)
> on linux, what is checked exactly? LANG? LC_CTYPE? Something else?

On the last patch, LC_CTYPE
(CCing Asa)

Asa, any chance to get this for the PR? (no risks here, really)
Comment on attachment 156133 [details] [diff] [review]
Final patch for the fox

a=mkaply for aviary
Attachment #156133 - Flags: approval-aviary? → approval-aviary+
it was hunked.
Comment on attachment 156294 [details] [diff] [review]
Up-to-date patch for aviary

moving r/a.
Attachment #156294 - Flags: review+
Attachment #156294 - Flags: approval-aviary+
Checked into aviary branch 08/16/2004 14:21.
Keywords: fixed-aviary1.0
(In reply to comment #92)

Thanks!
Flags: blocking-aviary1.0PR?
Whiteboard: [have patch], needed-aviary1.0, [l10n impact]
I'm going to improve this for aviary1.0 (now that the strings are inside, we
have time), in the plan:
1. "Switch Page Direction" in the context menu
2. Accel+Shift+X instead of Accel+Shift+Y
(In reply to comment #94)
> 2. Accel+Shift+X instead of Accel+Shift+Y

I don't think I like this either. Accel+Shift+X seems more suitable for "Cut as
unformatted text" or something of that sort. How about Accel+Shift+Y for GTK and
Accel+Shift+E for everyone else? It's actually very similar to the situation we
have with Accel+M to compose a new mail in Seamonkey - it collides with OS X
Cmd+M (Minimize Window), so we use Accel+Shift+M on the Mac. In other words, we
should use the most suitable shortcut and work-around collisions with
"problematic" platforms.

By the way, Ctrl+Shift in Windows and Accel+Arrows in other platforms would be
welcome additions, but I'd don't think they should replace a (mostly) unified
shortcut. One of the great things about Mozilla is that it works very similarly
on different platforms.

Prog.
I really start to consider Control (not Accel)+Shift+Q for Firefox.

(In reply to comment #95)
> How about Accel+Shift+Y for GTK and
> Accel+Shift+E for everyone else?

It's not up-to-me... CCing mconnor.
The patch contained Mac-style line-endings in browser.js and
contentAreaUtils.js. Fix checked in.

Please let's not introduce more platform-specific keybindings.
Ctrl+Shift+E is dangerous because you may accidentially hit Ctrl+Shift+W, which
closes the window without a warning. That might be a bug though.

Please note that any changes to keybindings need to go into browser.dtd, which
needs to be done before the l10n freeze.
What about Accel+~ ? (which is Accel+Shift+`)
(In reply to comment #98)
> What about Accel+~ ? (which is Accel+Shift+`)
> 

answring to my question: no, it is already used on mac (app's windows toggling).

but maybebe Ctrl+~ (Ctrl+Shift+`)
Does every single action require a keyboard shortcut? I'd prefer none to one
that conflicts or causes us to use a worse combination for some more common action.
(In reply to comment #100)
> Does every single action require a keyboard shortcut? I'd prefer none to one
> that conflicts or causes us to use a worse combination for some more common
action.

"Switch Text Direction" does need a keyboard shortcu, since it's very common,
for BiDi users. (ie has ctrl+right/left shift, but we can't impelement it at the
momemt (need DOM3 events))

Ctrl (not Accel)+Shift+` (ctrl+~) is fine fot me.
http://www.mozilla.org/projects/ui/accessibility/mozkeyintro.html

Ctrl+~ is free, but it's against the HIG.

> No Ctrl key combos on Mac OS
>
> The Mac human interface guidelines ask developers to use Cmd, not Ctrl.
> Using Ctrl for hotkeys would confused many >Mac users. We can, however, offer 
> Ctrl combinations for user-defined keystrokes (once we start offering keyboard
> configurability).

http://www.mozilla.org/projects/ui/accessibility/mozkeyintro.html#modifiers

It's worth noting that Apple's own applications use Ctrl+A and Ctrl+E (for Home
and End of line), so the HIG is not the end of it. All in all, Ctrl+~ seems like
the best compromise.

Asa, this action is common enough to warrant a keyboard accelerator. IE
implements one, and so should we.

Prog.
(In reply to comment #102)
> http://www.mozilla.org/projects/ui/accessibility/mozkeyintro.html
> 
"Emacs editor bindings always go on Ctrl"

Well, Ctrl+~ is not used for conroling text direction in emacs ;) but, as this
is an editor command, it's not-so-bad.
We will probably have Accel+Shift+X, mconnor doesn't agree to Ctrl+~ or to
separate keyboard shortcut for Linux.
Comment on attachment 156294 [details] [diff] [review]
Up-to-date patch for aviary

Please fix all the:
\ No newline at end of file
occurances please.
(In reply to comment #105)
> Please fix all the:
> \ No newline at end of file
> occurances please.
fix checked in.
The changes:

1. "Switch Page Direction" in pages' contextual menu (only when isBiDiEnabled
of course). It's shown for every element but input fields, as it applies to
every element in the page. I believe it should be there because:
 (1) We don't (and we won't) have a keyboard shortcut for this action.
 (2) it's contexual: say yoy are using webmail, you need to switch the
direction for each rtl message.
 (3) for ie bidi users (ie has that).
 (4) access in general

2. I think I finally found better keyboard shortcut: Accel+Shift+P. If I am
missing something, it will be fixed to Accel+Shift+X on checkin.
Attachment #156476 - Flags: review?(mconnor)
Accel+Shift+X and Accel+Shift+P may better be reserved for future variants on
Cut (e.g. unformatted) and Print (e.g. selection), but if these two are the
final candidates, then I would definitely prefer the former. It's simply easier
to press when the left hand is on the keyboard and the right hand on the mouse.

Either way, I still belive that Accel+~ is the best option. I wonder what
mconnor has against it...

Prog.
accel+shift+x looks more comfortable to me.
(In reply to comment #108)
> Accel+Shift+X and Accel+Shift+P may better be reserved for future variants on
> Cut (e.g. unformatted) and Print (e.g. selection), but if these two are the
> final candidates, then I would definitely prefer the former.

In the future we'll have better keyboard shortcuts, on all platforms, trust me :-).

> It's simply easier

ok, 2 against is enough -> Accel+Shift+X

> Either way, I still belive that Accel+~ is the best option. I wonder what
> mconnor has against it...

remember, it was Control, not Accel, we can't use Accel+~ on mac. In addition,
the ~ symbol is somewhat problematic to new users (finding the key, understand
they need to hit shift in addition to control).
Attachment #156476 - Attachment description: polish for aviary → polish for aviary (s/"P"/"X" on checkin)
Comment on attachment 156476 [details] [diff] [review]
polish for aviary (s/"P"/"X" on checkin)

yeesh, sorry for the latency
Attachment #156476 - Flags: review?(mconnor)
Attachment #156476 - Flags: review+
Attachment #156476 - Flags: approval-aviary?
Comment on attachment 156476 [details] [diff] [review]
polish for aviary (s/"P"/"X" on checkin)

a=mkaply for aviary
Attachment #156476 - Flags: approval-aviary? → approval-aviary+
Comment on attachment 156294 [details] [diff] [review]
Up-to-date patch for aviary

checked in. Thank you, Mike!
Attachment #156133 - Attachment is obsolete: true
Is there anything new with the patch for Seamonkey?

Prog.
(In reply to comment #114)
> Is there anything new with the patch for Seamonkey?
> 
> Prog.

today, i hope.
Attached patch Seamonkey patch (obsolete) — Splinter Review
Attachment #160148 - Flags: superreview?(blizzard)
Attachment #160148 - Flags: review?(smontagu)
Attachment #155707 - Attachment is obsolete: true
Attachment #160148 - Flags: review?(smontagu) → review+
Attachment #160148 - Flags: superreview?(blizzard) → superreview?(neil.parkwaycc.co.uk)
Comment on attachment 160148 [details] [diff] [review]
Seamonkey patch

The change to all.js is included in the firefox patch (which is waiting for
trunk checkin).
Comment on attachment 160148 [details] [diff] [review]
Seamonkey patch

I found bugs with this patch straight away.
If you try to switch the page direction of about:config you get a JS error.
If you right-click in an email message you get a JS error.
I was also able to switch the text direction of the URL bar.

>+  var systemLocale;
>+  try {
>+    var localService = Components.classes["@mozilla.org/intl/nslocaleservice;1"]
>+                                 .getService(Components.interfaces.nsILocaleService);
>+    systemLocale = localService.getSystemLocale().getCategory("NSILOCALE_CTYPE").substr(0,3);
>+  } catch (e) {}
>+
>+  rv = ( (systemLocale == "he-") || (systemLocale == "ar-") || (systemLocale == "syr") ||
>+         (systemLocale == "fa-") || (systemLocale == "ur-") );
Typo in localeService. Also I think all this should go inside the try/catch,
also you could use a regexp to test the system locale, rather than matching
substrings.
var systemLocale =
localeService.getSystemLocale().getCategory("NSILOCALE_CTYPE");
rv = /^(he|ar|syr|fa|ur)-/.test(systemLocale);
Although I do wonder about the hardcoding of this list, and whether we should
be using the requested content locale instead of the system locale.

>+    var mPrefs = Components.classes["@mozilla.org/preferences-service;1"].getService(Components.interfaces.nsIPrefBranch);
We already have a global variable (boringly named "pref") for this. mPrefs
would be the wrong naming convention anyway.

>+function GetFrameDocumentsFromWindow(aWindow)
>+{
>+ if (aWindow.getComputedStyle(aWindow.document.body, "").direction == "ltr")
>+    aWindow.document.dir = "rtl";
>+  else
>+    aWindow.document.dir = "ltr";
Why do you have to get the style on the body to set it on the document? Reading
document.dir would appear to suffice.

>+function SwitchDocumentDirection()
>+{
>+  GetFrameDocumentsFromWindow(window.content);
>+}
Any chance you could inline this function? You may want to rename
GetFrameDocumentsFromWindow.

>+  // The keybinding shoudn't work if the menu item is hidden
Keybindings are always hidden elements, they don't understand about other
elements being hidden (although they do understand disabled).

>+        var showBIDI = isBidiEnabled();
This function isn't defined in all the files that use nsContextMenu.js -
assuming you don't need the functions in the other windows then easiest is to
declare a global showBIDI in nsContextMenu.js and set it from navigator.js
(also use this variable in the key handler).

>+  document.getElementById('cmd_SwitchTextDirection').setAttribute('disabled',
>+         !document.commandDispatcher.focusedElement);
Is there a reason why this test is not the one used by the context menu?
Attachment #160148 - Flags: superreview?(neil.parkwaycc.co.uk) → superreview-
I'll update the patch tonight
Attachment #160148 - Attachment is obsolete: true
Sorry for the latency.

as on #composer :)

(In reply to comment #118)
> (From update of attachment 160148 [details] [diff] [review])
> I found bugs with this patch straight away.
> If you try to switch the page direction of about:config you get a JS error.
> If you right-click in an email message you get a JS error.

fixed


> I was also able to switch the text direction of the URL bar.

Well, that's not a bug, we may want to switch url-bar's direction for search
strings, keywords, etc.

> Typo in localeService. Also I think all this should go inside the try/catch,
> also you could use a regexp to test the system locale, rather than matching
> substrings.
> var systemLocale =
> localeService.getSystemLocale().getCategory("NSILOCALE_CTYPE");
> rv = /^(he|ar|syr|fa|ur)-/.test(systemLocale);

fixed, thanks.

> Although I do wonder about the hardcoding of this list, and whether we should
> be using the requested content locale instead of the system locale.


We try to match as many BiDi users as possible. In windows/mac, the locale check
is pretty much the same. That is not the case on linux,  see comment 79.

> >+    var mPrefs =
Components.classes["@mozilla.org/preferences-service;1"].getService(Components.interfaces.nsIPrefBranch);
> We already have a global variable (boringly named "pref") for this. mPrefs
> would be the wrong naming convention anyway.


done

> document.dir would appear to suffice.


> >+function SwitchDocumentDirection()
> >+{
> >+  GetFrameDocumentsFromWindow(window.content);
> >+}
> Any chance you could inline this function? You may want to rename
> GetFrameDocumentsFromWindow.


fixed.
 
> >+  document.getElementById('cmd_SwitchTextDirection').setAttribute('disabled',
> >+         !document.commandDispatcher.focusedElement);
> Is there a reason why this test is not the one used by the context menu?


document.commandDispatcher.focusedElement doesn't point to textfields only, but
it is unlikey that a user focus on  a button, and then go to the edit menu. In
addition,  I can't access gContextMenu from places like utilityOverlay.

Anyway, instead of making the code even more hacky (check tagname+type
attribute), I'll fix the XXX mentioned in utilityOverlay.js later, probably for 1.8.
Attachment #160969 - Flags: superreview?(neil.parkwaycc.co.uk)
Comment on attachment 160969 [details] [diff] [review]
Seamonkey patch - see next comment

sr=me with the nits fixed.

>+  var systemLocale;
>+  try {
>+    var localeService = Components.classes["@mozilla.org/intl/nslocaleservice;1"]
>+                                 .getService(Components.interfaces.nsILocaleService);
>+    systemLocale = localeService.getSystemLocale().getCategory("NSILOCALE_CTYPE");
>+  } catch (e) {}
>+
>+  rv = /^(he|ar|syr|fa|ur)-/.test(systemLocale);
Nit: This should all be inside the try block i.e.
try {
  var localService = ...
  var systemLocale = ...
  rv = ...
} catch (e) {}

>+ if (aWindow.document.dir == "ltr")
>+    aWindow.document.dir = "rtl";
>+  else
>+    aWindow.document.dir = "ltr";
You could use ? "rtl" : "ltr" here.

>+var showBiDi;
Nit: Should be var gShowBidi = false; (gShowBidi because of global variable
style and = false because non-navigator windows should have it explicitly set
to false for the context menu).

>+  if (window.getComputedStyle(aElement, "").direction == "ltr")
>+    aElement.dir = "rtl";
>+  else
>+    aElement.dir = "ltr";
Could use ?: here too if it will fit on one line.
Attachment #160969 - Flags: superreview?(neil.parkwaycc.co.uk) → superreview+
Thanks Neil.

(eta for firefox trunk: 17/10)
Target Milestone: mozilla1.8beta → mozilla1.8alpha5
No longer blocks: 74928, 74929, 75009, 82849
(In reply to comment #36)

> * Numeral shape (contextual, Arabic, Hindi...)

Any Arabic user in the house? I would like to know if we need this 
functionality for Arabic locales.
Attachment #160969 - Attachment is obsolete: true
Comment on attachment 162328 [details] [diff] [review]
Seamonkey: patch for checkin

moving r/sr
Attachment #162328 - Flags: superreview+
Attachment #162328 - Flags: review+
Attachment 162328 [details] [diff] checked in to trunk.
Whiteboard: fixed on trunk
Keywords: relnote
Whiteboard: fixed on trunk → [fixed on trunk] waiting for trunk sync (firefox)
Status: ASSIGNED → RESOLVED
Closed: 20 years ago
Resolution: --- → FIXED
Whiteboard: [fixed on trunk] waiting for trunk sync (firefox)
Blocks: 272705
Status: RESOLVED → VERIFIED
Blocks: 279416
Blocks: 300590
Component: Layout: BiDi Hebrew & Arabic → Layout: Text
QA Contact: xslf → layout.fonts-and-text
You need to log in before you can comment on or make changes to this bug.