250864 - Right-to-Left support for Chatzilla

Status: RESOLVED FIXED

(Other Applications :: ChatZilla, enhancement, P2)

Description

[Prognathous]

Like most IRC clients, Chatzilla currently only supports Left-to-Right paragraph
direction. Adding an option for Right-to-Left is very likely to be a killer
feature among many BiDi users (e.g. Hebrew and Arabic). 

Open issues:

1. Should switching between RTL and LTR be command or GUI-based? Note that when
the input line is RTL, it may be confusing to switch back to LTR using a
command, since the / ends up on the wrong side of the sentence.

2. If BiDi is implemented using GUI, which context menus should include this
option? IMHO, the Tab-menu and contentarea-menu are prime candidates.

3. Should the direction of the content area and the input line be linked? To
simplify things, I believe that they should.

Prog.

Comment 1

[R.K.Aa.]

Related to bug 128773?

Comment 2

[Asaf Romano (gone)]

(In reply to comment #1)
> Related to bug 128773?

Noop, the issue here is a way to access .style.direction of the content area (and the input area).


----
Taking bug

Comment 3

[Asaf Romano (gone)]

Attachment: patch

What's added:
* /rtl and /ltr: switch both content and input area direction
* "Switch Text Direction" GUI in the contextual menus for tabs and content area

* direction is per channel (for both content and input areas).
* /irtl /iltr for changing (temporary) the *input area only* direction (e.g.
for commands with parameters).

Comment 4

[Asaf Romano (gone)]

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

Requesting review from Silver.

Comment 5

[James Ross]

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

Pendant guide: http://www.hacksrus.com/~ginda/pedant.html

>+         // text-direction aliases
>+         ["rtl",              "text-direction rtl",                CMD_CONSOLE],
>+         ["ltr",              "text-direction ltr",                CMD_CONSOLE],
>+         ["switch-text-dir",  "text-direction switch",                       0],

The usual way parameter to toggle settings is 'toggle' rather than 'switch'.


>+function cmdTextDirection(e)
>+{
>+    var direction;
>+    
>+    switch (e.dir)
>+    {
>+        case "switch":
>+            if (window.getComputedStyle(client.currentObject.frame.contentDocument.body, '').direction == "ltr")

Lines should be kept to 80 characters or less. Probably best to put
|client.currentObject.frame.contentDocument.body| into a variable in this case.
You can also use |null| for the second parameter to getComputedStyle - I think
it's a bit clearer, but it's up to you (it works fine with '').


>+                direction = 'rtl';
>+            else
>+                direction = 'ltr';      
>+            break;
>+        case "rtl":
>+            direction = 'rtl';
>+            break

Missed a semi-colon. ;)

>+        default:
>+            // that is "case "ltr":", but even if !e.dir OR e.dir is an invalid value -> set to default direction
>+            direction = 'ltr';
>+    }  
>+    client.input.style.direction = direction;
>+    client.currentObject.frame.contentDocument.body.style.direction = direction;

Here, and earlier, you use |client.currentObject|. This means it's impossible
to run this on any other view. You should be able to use |e.sourceObject| as a
direct replacement, and with the bonus that you can change the text direction
on a view you aren't looking at.


>          ["-"],
>          ["leave", {visibleif: "cx.TYPE == 'IRCChannel'"}],
>          ["delete-view", {visibleif: "cx.TYPE != 'IRCChannel'"}],
>-         ["disconnect"]
>+         ["disconnect"],
>+         ["-"],
>+         ["switch-text-dir"]
>         ]
>     };
> 
>@@ -312,7 +314,9 @@
>          ["-"],
>          ["leave", {visibleif: "cx.TYPE == 'IRCChannel'"}],
>          ["delete-view", {visibleif: "cx.TYPE != 'IRCChannel'"}],
>-         ["disconnect"]
>+         ["disconnect"],
>+         ["-"],
>+         ["switch-text-dir"]
>         ]
>     };

These seems odd, adding a separator and the item to the bottom. Does it
actually look ok there?


>         singleInputBox.setAttribute ("collapsed", "true");
>         splitter.setAttribute ("collapsed", "false");
>         multiInputBox.setAttribute ("collapsed", "false");
>+        // muliInput should have the same direction as singleInput
>+        multiInput.style.direction = window.getComputedStyle(singleInput,'').direction;

Pedant: >80 characters.


>         client.input = multiInput;
>     }
>     else  /* turn off multiline input mode */
>@@ -1876,6 +1878,8 @@
>         splitter.setAttribute ("collapsed", "true");
>         multiInputBox.setAttribute ("collapsed", "true");
>         singleInputBox.setAttribute ("collapsed", "false");
>+        // singleInput should have the same direction as multiInput
>+        singleInput.style.direction = window.getComputedStyle(multiInput,'').direction;

Same. :)

Also, is it actually nessessay to 'copy' the direction when changing inputs?
Does it not work if you set the direction on both from cmdInputTextDirection?


>     if (client.PRINT_DIRECTION == 1)
>         scrollDown(obj.frame, false);    
>-
>+    
>+    // Input area should have the same direction as the output area
>+    client.input.style.direction = window.getComputedStyle(client.currentObject.frame.contentDocument.body,'').direction;

You know what I'm going to say here. I do also wonder if we can really be safe
assuming |.frame.contentDocument.body| will be there in each case...


>+cmd.rtl.help  = Switches text direction to Right-to-Left
>+cmd.ltr.help  = Switches text direction to Left-to-Right
>+
>+cmd.irtl.help = Switches input area direction to Right-to-Left
>+cmd.iltr.help = Switches input area direction to Left-to-Right

Pedant: help text should end with a full-stop (or other punctuation).


>+
>+cmd.switch-text-dir.label = Switch Text &Direction
>+

Doesn't _D_ conflict with _D_isconnect? Perhaps _T_ext?

Comment 6

[Asaf Romano (gone)]

Attachment: Fixed patch

Addressing most comments (most means we decided to leave some things as they
are after discussing them on #chatzilla).

Keyboard shortcut has been also added (Accel+Shift+E, as in bidiui:
http://bidiui.mozdev.org).

Comment 7

[James Ross]

Comment on attachment 153429 [details] [diff] [review]
Fixed patch

Allignment of []s in commands.js is a little off.

r=[e-mail] with that fixed.

Comment 8

[Simon Montagu :smontagu]

Fix checked in with the alignment corrected. Thank you, Asaf!