140609 - [PATCH] navigating menu bar with arrows is reversed when UI aligned to the right

Status: RESOLVED FIXED

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

Description

[Tsahi Asher]

build ID: 2002041711 (Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.0rc1)
Gecko/20020417)

Description: when the interface is aligned to the right, e.g. when using a RTL
language (like hebrew or arabic), navigating the menues in the main menu of any
component of Mozilla (Navigator, Messenger etc.) is reversed. pressing the right
arrow on the keyboard moves to the next menu to the left, and pressing the left
arrow on the keyboard moves to the next menu to the right. this is also true in
sub menues.

Steps to reproduce:
1. aligning the interface to the right: add these lines to the file intl.css, in
the locale\en-US\global, in the en-US.jar file (the language pack file, in the
chrome folder):

window,dialog,wizard,page {
  direction: rtl;
}

menu { direction: rtl; }

outliner { direction: rtl; }

2. start mozilla
3. open one of the menus, either with the mouse or by pressing the Alt key (on
Windows) once.
4. press the right arrow on the keyboard

Actual Results: the menu to the left of the current one becomes active.

Expected Results: the menu to the right of the current one should become active.

Comment 1

[Simon Montagu :smontagu]

Attachment: Suggested patch

Comment 2

[Simon Montagu :smontagu]

Comment on attachment 84530 [details] [diff] [review]
Suggested patch

On reflection, I'm not happy with this approach: it's too much of a band-aid.

I think it would be a lot better to go a bit further and completely separate
the navigation direction from the keycode.

Comment 3

[Simon Montagu :smontagu]

Attachment: Separate direction from keycode

This patch is designed to be scalable to any flow orientation, not just
left-to-right and right-to-left.

Comment 4

[:deanis74]

Referring to "start" and "end" sound more like what "home" and "end" imply. 
They really mean moving just one item next/previous, don't they?

Comment 5

[Simon Montagu :smontagu]

I had a problem with "start" and "end". I wanted to use something as close as
possible to the CSS3 / XSLT terminology for flow orientation, but meaning
"moving towards" rather than "located at", and couldn't think of anything except
ugly neologisms like "startwards" and "endwards". Suggestions are welcome.

Comment 6

[:deanis74]

Next/Previous?  But that gets a little confusing with Before/After.

Comment 7

[Hixie (not reading bugmail)]

The CSS terminology is BASE, which maps out as:

            Before
          +--------+
    Start |        | End
          +--------+
            After

...in top-to-bottom left-to-right text such as typical written English.

Comment 8

[Keyser Sose]

Marking NEW for patch loving.

Comment 9

[Simon Montagu :smontagu]

Attachment: New patch with more comments

Nobody has suggested better terminology, so I have added fuller comments
including a diagram based on comment 7.

Comment 10

[:deanis74]

Comment on attachment 90111 [details] [diff] [review]
New patch with more comments

The diagram helps.  The terminology still is a little vague, but at least
there's a reference to fall back on.  Have you tested this on XUL menulists and
the sort to make sure their keyboard nav isn't broken?

r=me with the above test

Comment 11

[Simon Montagu :smontagu]

I've just tested some XUL menulists and existing keyboard navigation isn't
broken by this patch when dir=ltr.

There seems to be a separate bug that direction isn't being inherited by the
menuitems which are children of the menulist, so I can't test that case yet.

Comment 12

[Simon Montagu :smontagu]

>There seems to be a separate bug that direction isn't being inherited by the
>menuitems which are children of the menulist, so I can't test that case yet.

This may not be true. XUL menu elements with dir=rtl seem not to have direction:
rtl, but I'm not sure if this is a bug or not. When I explicitly add
style="direction: rtl", keyboard navigation works as expected.

Comment 13

[Johnny Stenback (:jst)]

Comment on attachment 90111 [details] [diff] [review]
New patch with more comments

sr=jst

Comment 14

[Simon Montagu :smontagu]

Fix checked in