343690 - arrow keys traverse preference panes in wrong direction in RTL mode.

Status: RESOLVED FIXED

(Toolkit :: XUL Widgets, defect)

Description

[is+mozilla]

User-Agent:       Mozilla/5.0 (X11; U; Linux ppc; he-IL; rv:1.8.0.4) Gecko/20060406 Firefox/1.5.0.4 (Debian-1.5.dfsg+1.5.0.4-1)
Build Identifier: Mozilla/5.0 (X11; U; Linux ppc; he-IL; rv:1.8.0.4) Gecko/20060406 Firefox/1.5.0.4 (Debian-1.5.dfsg+1.5.0.4-1)

If you use the left/right arrow keys to traverse the various preference panes (general, privacy, etc.), and you are using a RTL locale, then pressing the right arrow key will travel to the pane to the left, and pressing the left arrow key activates the pane to the right. This behaviour is counterintuitive :) and should be fixed. I think this is a general gecko issue, since the preferences dialog in Thunderbird shows the same behaviour. 

Reproducible: Always

Steps to Reproduce:
1. Open preferences dialog
2. Click on or tab to one of the pane icons
3. hit left/right arrow key. 

Actual Results:  
The pane to the right/left (respectively) is displayed.

Expected Results:  
The pane to the left/right (respectively) is displayed.

Comment 1

[Simon Montagu :smontagu]

I have a fix for the preference panes (which are a <radiogroup>), but meanwhile Asaf pointed out that buttons have the same problem, e.g. the three options for Home Page in the General pane.

Comment 2

[Simon Montagu :smontagu]

Attachment: Patch radiogroups and buttons

Comment 3

[Asaf Romano (gone)]

Comment on attachment 228289 [details] [diff] [review]
Patch radiogroups and buttons

>Index: toolkit/content/widgets/radio.xml
>===================================================================

>@@ -309,16 +309,20 @@
>         this.checkAdjacentElement(false);
>         event.stopPropagation();
>       </handler>
>+      <!-- left arrow goes back when we are ltr, forward when we are rtl -->

Please add this as a js comment.

>       <handler event="keypress" keycode="VK_LEFT" phase="target">
>-        this.checkAdjacentElement(false);
>+        this.checkAdjacentElement(window.getComputedStyle(this, "")
>+	                                .direction == "rtl");

one more space here.

ditto for the second handler; r=mano otherwise.

Comment 4

[Simon Montagu :smontagu]

Toolkit patch checked in. Leaving open for forthcoming xpfe patch.

Comment 5

[Simon Montagu :smontagu]

Attachment: xpfe patch

Comment 6

[[e-mail]]

Comment on attachment 228583 [details] [diff] [review]
xpfe patch

>         if (event.keyCode == KeyEvent.DOM_VK_UP ||
>-            event.keyCode == KeyEvent.DOM_VK_LEFT)
>+            (event.keyCode == KeyEvent.DOM_VK_LEFT &&
>+              window.getComputedStyle(this.parentNode, "")
>+                    .direction == "ltr") ||
>+            (event.keyCode == KeyEvent.DOM_VK_RIGHT &&
>+              window.getComputedStyle(this.parentNode, "")
>+                    .direction == "rtl"))
>           return window.document.commandDispatcher.rewindFocus();
> 
>         if (event.keyCode == KeyEvent.DOM_VK_DOWN ||
>-            event.keyCode == KeyEvent.DOM_VK_RIGHT)
>+            (event.keyCode == KeyEvent.DOM_VK_RIGHT &&
>+              window.getComputedStyle(this.parentNode, "")
>+                    .direction == "ltr") ||
>+            (event.keyCode == KeyEvent.DOM_VK_LEFT &&
>+              window.getComputedStyle(this.parentNode, "")
>+                    .direction == "rtl"))
>           return window.document.commandDispatcher.advanceFocus();
Wow, this is so ugly, I'd so love it if there was at least a more readable way to express this.

>+        this.checkAdjacentElement(window.getComputedStyle(this, "")
>+                                        .direction == "rtl");
I notice that the above is the only time where you use this instead of this.parentNode; which way is correct and why?
>+        this.checkAdjacentElement(window.getComputedStyle(this.parentNode, "")
>+                                        .direction == "ltr");

Comment 7

[Asaf Romano (gone)]

For some reason, I read the toolkit patch as |this| was used in the VK_RIGHT case too, sigh.

Comment 8

[[e-mail]]

Comment on attachment 228583 [details] [diff] [review]
xpfe patch

>         if (event.keyCode == KeyEvent.DOM_VK_UP ||
>-            event.keyCode == KeyEvent.DOM_VK_LEFT)
>+            (event.keyCode == KeyEvent.DOM_VK_LEFT &&
>+              window.getComputedStyle(this.parentNode, "")
>+                    .direction == "ltr") ||
>+            (event.keyCode == KeyEvent.DOM_VK_RIGHT &&
>+              window.getComputedStyle(this.parentNode, "")
>+                    .direction == "rtl"))
>           return window.document.commandDispatcher.rewindFocus();
> 
>         if (event.keyCode == KeyEvent.DOM_VK_DOWN ||
>-            event.keyCode == KeyEvent.DOM_VK_RIGHT)
>+            (event.keyCode == KeyEvent.DOM_VK_RIGHT &&
>+              window.getComputedStyle(this.parentNode, "")
>+                    .direction == "ltr") ||
>+            (event.keyCode == KeyEvent.DOM_VK_LEFT &&
>+              window.getComputedStyle(this.parentNode, "")
>+                    .direction == "rtl"))
>           return window.document.commandDispatcher.advanceFocus();
I've figured that this would be neater as separate handlers as per the radiogroup, but that's a separate bug. (I'm not convinced that these handlers should belong on the button base binding anyway).

>+        this.checkAdjacentElement(window.getComputedStyle(this.parentNode, "")
>+                                        .direction == "ltr");
Based on Mano's IRC comments this.parentNode should be this because the radiogroup is the container in this case. sr=me with this fixed (I didn't do a full review but you can check this in with r=Mano).

Comment 9

[Boris Zbarsky [:bzbarsky]]

Please use document.defaultView, not window, to get the computed style...

Comment 10

[Simon Montagu :smontagu]

Attachment: for checkin

What I plan to check in as soon as the tree opens, addressing comment 8 and comment 9 for the xpfe patch and bringing toolkit into line with the changes.

Comment 11

[Simon Montagu :smontagu]

Checked in.

Comment 12

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 229403 [details] [diff] [review]
for checkin

a=drivers, please land on the 1.8.1-branch

Comment 13

[(no longer active)]

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