Right-to-Left support for Chatzilla

RESOLVED FIXED in mozilla1.8alpha5


15 years ago
15 years ago


(Reporter: bugzillamozilla, Assigned: mano)




Firefox Tracking Flags

(Not tracked)



(1 attachment, 1 obsolete attachment)

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.

Related to bug 128773?
(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
Posted patch patch (obsolete) — Splinter Review
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 on attachment 153403 [details] [diff] [review]

Requesting review from Silver.
Attachment #153403 - Flags: review?(silver)
Comment on attachment 153403 [details] [diff] [review]

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?
Attachment #153403 - Flags: review?(silver) → review-
Posted patch Fixed patchSplinter Review
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:
Attachment #153403 - Attachment is obsolete: true
Priority: -- → P2
Target Milestone: --- → mozilla1.8beta
Comment on attachment 153429 [details] [diff] [review]
Fixed patch

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

r=silver@warwickcompsoc.co.uk with that fixed.
Attachment #153429 - Flags: review?(silver) → review+
Fix checked in with the alignment corrected. Thank you, Asaf!
Closed: 15 years ago
Resolution: --- → FIXED
Product: Core → Other Applications
Target Milestone: mozilla1.8beta → mozilla1.8alpha5
You need to log in before you can comment on or make changes to this bug.