Closed Bug 343690 Opened 15 years ago Closed 15 years ago

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

Categories

(Toolkit :: XUL Widgets, defect)

All
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla1.8.1beta2

People

(Reporter: is+mozilla, Assigned: smontagu)

Details

(Keywords: fixed1.8.1, rtl)

Attachments

(2 files, 1 obsolete file)

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.
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.
Component: Preferences → Keyboard Navigation
QA Contact: preferences → keyboard.navigation
Assignee: nobody → smontagu
Component: Keyboard Navigation → XUL Widgets
Product: Firefox → Toolkit
QA Contact: keyboard.navigation → xul.widgets
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Attachment #228289 - Flags: first-review?(bugs.mano)
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.
Attachment #228289 - Flags: first-review?(bugs.mano) → first-review+
Toolkit patch checked in. Leaving open for forthcoming xpfe patch.
Attached patch xpfe patch (obsolete) — Splinter Review
Attachment #228583 - Flags: second-review?(neil)
Attachment #228583 - Flags: first-review?(neil)
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");
For some reason, I read the toolkit patch as |this| was used in the VK_RIGHT case too, sigh.
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).
Attachment #228583 - Flags: second-review?(neil)
Attachment #228583 - Flags: second-review+
Attachment #228583 - Flags: first-review?(neil)
Attachment #228583 - Flags: first-review+
Please use document.defaultView, not window, to get the computed style...
Attached patch for checkinSplinter Review
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.
Attachment #228583 - Attachment is obsolete: true
Checked in.
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Attachment #229403 - Flags: approval1.8.1?
Flags: blocking-firefox2?
Target Milestone: --- → mozilla1.8.1beta2
Flags: blocking-firefox2? → blocking-firefox2+
Comment on attachment 229403 [details] [diff] [review]
for checkin

a=drivers, please land on the 1.8.1-branch
Attachment #229403 - Flags: approval1.8.1? → approval1.8.1+
Keywords: fixed1.8.1
Mass-assigning the new rtl keyword to RTL-related (see bug 349193).
Keywords: rtl
You need to log in before you can comment on or make changes to this bug.