162081 - Wrong letter is underlined as accesskey / mnemonic when widget direction is RTL

Status: RESOLVED FIXED

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

Description

[Tsahi Asher]

Description: when the interface is aligned to the right, e.g. when using a RTL
language (like hebrew or arabic), the wrong letter in menues, buttons etc. is
underlined instead of the the real access key. e.g. if F is the access key for
File, then e is underlined. this happens only with an RTL language (i tested it
on hebrew), and it looks like Mozilla is counting the letters starting from the
left in order to find the letter to be underlined, instead of from the beginning
of the string.

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. localize a menu item, like the word "File" in the main navigator menu (you
will probably need a hebrew/arabic/other RTL enabled OS for that). this item is
in locale/en-US/communicator/utilityOverlay.dtd.
3. start mozilla
4. look at localized UI element

Actual Results: underlined letter is at the opposite side of the string to the
access key.

Expected results: well, the access key should be underlined of course.

Comment 1

[Daniel Wang]

Attachment: testcase

Comment 2

[Daniel Wang]

Attachment: screenshot

WORKSFORME Mozilla 2002081508, Classic Theme, Windows 98

Comment 3

[Daniel Wang]

> XP Toolkit/Widgets: Menus

Comment 4

[Daniel Wang]

> XP Toolkit/Widgets: Menus

Comment 5

[:deanis74]

cc: me, Simon

Comment 6

[:deanis74]

The code that finds the accesskey is here.

http://lxr.mozilla.org/mozilla/source/layout/xul/base/src/nsTextBoxFrame.cpp#759

So we probably need to do different searches depending on what the direction is.
 So something like:

PRBool found;
if (!AlwaysAppendAccessKey()) {
  if Direction == LTR then
    found = FindInReadable;
  else
    found = RFindInReadable;

  if (!found)
    if Direction == LTR then
      found = FindInReadable(caseinsensitive);
    else
      found = RFindInReadable(caseinsensitive);
} else {
  if Direction == LTR then
    found = RFindInReadable(caseinsensitive);
  else
    found = FindInReadable(caseinsensitive);
}

The last chunk there, where AlwaysAppendAccessKey is true, raises the point that
nsTextBoxFrame::UpdateAccessTitle() doesn't respect RTL direction when appending
the accesskey, it always throws it at the end.  Should it put it at the start if
RTL?

Comment 7

[Tsahi Asher]

per comment 2, it works if you use english, but if you use the hebrew word for
"File", and make it's first letter the access key, you would still have the
left-most letter underlined, although in hebrew (and arabic etc.) the first
letter is the right-most letter. i can't put a screenshot now, but maybe tomorrow.

Comment 8

[Tsahi Asher]

Attachment: localized testcase

this is the testcase Daniel attached, only localised to hebrew. you can see how
the left-most letter is underlined, although the right-most letter is the
accesskey.

if you want to view the code of this testcase, be sure to use a viewer that
supports RTL.

Comment 9

[Tsahi Asher]

any work done here? this is a major issue for RTL localizers, and been lying
around for too long.

Comment 10

[Prognathous]

Attachment: Screenshot of misaligned mnemonics

I've tested this bug under WinXP with 1.1 he-IL (20020826) and the problem
seems more severe than the summary suggests. As long as Hebrew input is used,
mnemonics are completely broken. The fact that they are also misaligned is
indeed a major problem, but is secondary.

I suggest to change the summary of this bug accordingly.

Prog.

Comment 11

[Tsahi Asher]

i think there's no point to fix the hebrew input problem before we get the
correct access key underlined. you can open a seperate bug for it, depending on
this one (or have this one depend on the new one).

Comment 12

[Ahmad]

This bug in all OS, please change the OS box

Comment 13

[Prognathous]

Ahmad, can you verify this on operating systems other than Windows?

Prog.

Comment 14

[Tsahi Asher]

see screenshots from debian linux at
http://www.mozilla.org.il/board/viewtopic.php?t=481. mac doesn't have any
accesskeys.

since i reported this, i changed to WinXP, and i get it there too. changing.

Comment 15

[Ahmad]

Yes , I am using Linux and I have this problem

Comment 16

[Asaf Romano (gone)]

Reassign to nobody. :-/

Comment 17

[Asaf Romano (gone)]

question: do hebrew/arabic mnemonic work in Mozilla? I mean, even if
Tsadik-Sofit is marked as the mnemonic, does alt+Kuf opens the File menu?

another one: do we have the same problem for other widgets (buttons, links. etc.)?

Comment 18

[Asaf Romano (gone)]

In answer to my first question: they do.

Comment 19

[Asaf Romano (gone)]

Attachment: patch

First of all, not as I said in my last comment, i see some problems using the
mnemonics, sometimes - at least when i test the localized testcase. When a menu
is opened, the mnemonics (for the opened menu of course) work fine, but: the
top level mnemonics haven't worked-for-me in the suite, but they did work in
firefox, maybe there is a problem in the testcase...

The attached patch fixes the mnemonic placement for RTL text rects (which is
the only case that is going to be tested...). It doesn't fix cases where you
have LTR gui in a RTL langauge (see XXX: comment).

Comment 20

[Asaf Romano (gone)]

Attachment: Before (latest aviary) vs. latest (1.8 with the patch)

(...and i checked, and there is no problem with not-first-letter mnemonic
:-)...)

Comment 21

[Tsahi Asher]

(In reply to comment #17)
> question: do hebrew/arabic mnemonic work in Mozilla? I mean, even if
> Tsadik-Sofit is marked as the mnemonic, does alt+Kuf opens the File menu?
> 

to put things strait, if Kuf is maked in the XUL code as the access key, it will
open the Kovets menu, even if the actual letter underlined is the Tsadik, due to
this bug.

> another one: do we have the same problem for other widgets (buttons, links. etc.)?

it happenes on buttons too. by links, do you mean links in the content area (web
pages)?

(In reply to comment #20)
> (...and i checked, and there is no problem with not-first-letter mnemonic
> :-)...)

are you sure about this? from what i've seen, the underline is simply counted
from the left, regardless of the direction. if the second letter is the access
key, then the second letter from the left is underlined, even if the word starts
at the right (as it is the case in RTL).

i'll upload a testcase in a minute.

Comment 22

[Tsahi Asher]

Attachment: test case with not-first-letter mnemonic

this test case shows that even when the access key is not the first letter, the
problem persists. the access key for the first menu is Vav, the second letter.
however, the underlined letter is the Beit, which is second from left. the line
itself is narrow, to accomodate the thin Vav.

Comment 23

[Asaf Romano (gone)]

Attachment: Screnshot of last testcase

Comment 24

[Asaf Romano (gone)]

(In reply to comment #21)
> (In reply to comment #17)
> > question: do hebrew/arabic mnemonic work in Mozilla? I mean, even if
> > Tsadik-Sofit is marked as the mnemonic, does alt+Kuf opens the File menu?
> > 
> 
> to put things strait, if Kuf is maked in the XUL code as the access key, it will
> open the Kovets menu, even if the actual letter underlined is the Tsadik, due to
> this bug.

Anyway, i didn't change the reg. code of the accesskeys. If there is a problem
(And for me: there is, at least with the testcase), we'll file another bug...

> > another one: do we have the same problem for other widgets (buttons, links.
etc.)?
> 
> it happenes on buttons too. by links, do you mean links in the content area (web
> pages)?
> 

Yep

> (In reply to comment #20)
> > (...and i checked, and there is no problem with not-first-letter mnemonic
> > :-)...)
> 
> are you sure about this? from what i've seen, the underline is simply counted
> from the left, regardless of the direction. if the second letter is the access
> key, then the second letter from the left is underlined, even if the word starts
> at the right (as it is the case in RTL).
> 
> i'll upload a testcase in a minute.

I meant *after* patching (see last screenshot)

Comment 25

[Tsahi Asher]

(In reply to comment #24)
> (In reply to comment #21)
> > 
> > it happenes on buttons too. by links, do you mean links in the content area (web
> > pages)?
> > 
> 
> Yep
> 

well, accesskeys in web pages are not underlined by default, unless you
explicitly use the <u> tag, so it's not a problem there.

Comment 26

[Simon Montagu :smontagu]

(In reply to comment #19)
> First of all, not as I said in my last comment, i see some problems using the
> mnemonics, sometimes - at least when i test the localized testcase. When a menu
> is opened, the mnemonics (for the opened menu of course) work fine, but: the
> top level mnemonics haven't worked-for-me in the suite, but they did work in
> firefox, maybe there is a problem in the testcase...

You might have been seeing bug 177508 some of the time...

> The attached patch fixes the mnemonic placement for RTL text rects (which is
> the only case that is going to be tested...). It doesn't fix cases where you
> have LTR gui in a RTL langauge (see XXX: comment).

Does RTL gui in a LTR language work correctly? This is important because the
Hebrew localization has menu items in English (Chatzilla, DOM Inspector, etc.)

Comment 27

[Asaf Romano (gone)]

Good point Simon....

I shoould also check if we deal with a BiDi char (the accesskey)... Do we have
any method for checking this in bidiutils? or should I check the unicode values?

Comment 28

[Asaf Romano (gone)]

This macro?
http://lxr.mozilla.org/seamonkey/source/content/shared/public/nsBidiUtils.h#209

Expect a patch tonight :-)

Comment 29

[Asaf Romano (gone)]

In my las(In reply to comment #28)

I meant thos 4 macros:
http://lxr.mozilla.org/seamonkey/source/content/shared/public/nsBidiUtils.h#217

Comment 30

[Asaf Romano (gone)]

Attachment: workaround

This fixes rtl accesskeys for the following cases:
* RTL l10n in RTL GUI
* LTR l10n in RTL GUI
* RTL l10n in LTR GUI

..and in a better way (we already store if-frame-is-bidi).

Comment 31

[Asaf Romano (gone)]

Attachment: testcase with both hebrew & english menus

Comment 32

[Simon Montagu :smontagu]

Comment on attachment 154950 [details] [diff] [review]
workaround

This is still only going to work for text in a single direction. Tsahi, do you
think we can assume that individual menu item labels will always be either RTL
or LTR but never mixed[1]? If not, we will need a Bidi-aware version of
nsTextBoxFrame::CalculateUnderline(); if so, that would be overkill and I think
this will be the best solution.

[1] One obvious case where we get mixed-directional text is where the access
key is not localized and we get something like
(F) קובץ
but an isolated LTR character doesn't break this patch.

Comment 33

[Asaf Romano (gone)]

(In reply to comment #32)
> Tsahi, do you think we can assume that individual menu item labels will 
> always be either RTL or LTR but never mixed[1]?

to put things strait, even if it we have something like "חלון Window קטן", and
the mnemonic will be "ל" for example, it will still work, it should also work
for "ק" , "ט" or "ן" , the problematic case is likely to be mixed-direction
string with a LTR mnemonic (only when it is *exist* in the string).

Comment 34

[Tsahi Asher]

we have mixed direction text in file > new > Navigator Window/Navigator
Tab/Composer Page. i left Navigator and Composer in english, because they are
component names that don't have much meaning in english too, in the sense that
they are like brand names. i did translate Address Book and Mail & Newsgroups.

the current access keys for them are english - e.g. P for Composer. i suppose i
could choose hebrew access keys for them to workaroud this problem. are there
any rules on how to select access keys? i think it would be easier to remember
the N in Navigator than the Lamed in Halon.

Comment 35

[Asaf Romano (gone)]

Correct me if i'm wrong, the strings yoyu mentioned are not mixed with two
directions (you don't have any bidi charcter there).

we mean smoething like "חלון Composer חדש". "Composer" itself (even if the box
is RTLed) is not broken.

Comment 36

[Tsahi Asher]

the string is "חלון Navigator" or "דף Composer", if that's mixed enough for you.

please stick to unicode when writing hebrew here. we started with it, and i hate
swiching encodings all the time.

Comment 37

[Asaf Romano (gone)]

You won't have a problem with "[hebrew]+ [english]+", because you are still
counting only in one direction... the problem can come for strings like
"[hebrew]+ [english]+ [hebrew]+" where the accesskey is on "[english]+"

As far as i know, we don't have any string in this form.

Comment 38

[Simon Montagu :smontagu]

Attachment: Patch for multi-directional texts

This sacrifices elegance for performance: the logical place to do this would be
in nsTextBoxFrame::CalculateUnderline() but that would mean duplicating all the
calls to the Bidi engine. It would also be cleaner to pass mAccessKeyInfo
instead of individual members, but the class isn't defined in any public
header, and I'm not sure if there is an appropriate one to move it into.

Comment 39

[Simon Montagu :smontagu]

Attachment: The last testcase with some other evil cases added.

Comment 40

[Simon Montagu :smontagu]

Attachment: testcase, rtl version

Comment 41

[Simon Montagu :smontagu]

(In reply to comment #37)
> You won't have a problem with "[hebrew]+ [english]+", because you are still
> counting only in one direction...

I don't think this is correct. In the case of "חלון Navigator", with access key
"N", the accesskeyIndex will be 5, but the N is visually at the far left if
direction is rtl. I've included this case in the "חדש" submenu in the rightmost
menu in attachment 155036 [details].

Comment 42

[Simon Montagu :smontagu]

Comment on attachment 155034 [details] [diff] [review]
Patch for multi-directional texts

My debug tree is a little out of date. I'll request reviews as soon as I'm sure
that the whole tree builds OK with this patch.

Comment 43

[Asaf Romano (gone)]

(In reply to comment #42)
> (From update of attachment 155034 [details] [diff] [review])
> My debug tree is a little out of date. I'll request reviews as soon as I'm sure
> that the whole tree builds OK with this patch.
> 

I have up-to-dat tree... I will check it soon...

Comment 44

[Asaf Romano (gone)]

Comment on attachment 155034 [details] [diff] [review]
Patch for multi-directional texts

No hunks on patching:
patching file layout/base/public/nsBidiPresUtils.h
patching file layout/base/src/nsBidiPresUtils.cpp
patching file layout/html/base/src/nsPageFrame.cpp
patching file layout/xul/base/src/nsTextBoxFrame.cpp
patching file layout/xul/base/src/tree/src/nsTreeBodyFrame.cpp

Comment 45

[Asaf Romano (gone)]

I'm afraid to say, but the patch doesn't work for me. The status is just like it
was before patching, maybae you patched another file(s) and missed him on "diffing"?

Comment 46

[Simon Montagu :smontagu]

The patch works for me on a clean tree. Maybe there was a problem with your build?

Comment 47

[Asaf Romano (gone)]

Well, on My WinXP Box, i "make"ed with a clean build, on both "mozilla/layout" and even just "mozilla/"

In both cases, no change :-/

Comment 48

[Simon Montagu :smontagu]

Attachment: Screenshot with the patch on linux

Thanks to bz for this screenshot. More investigation is needed.

Comment 49

[Simon Montagu :smontagu]

Attachment: Screenshot with the patch on XP

Comment 50

[Tsahi Asher]

what's up with this bug? we will need this to create good looking official
localized builds, starting Firefox 1.0.

Comment 51

[Simon Montagu :smontagu]

(In reply to comment #50)
> what's up with this bug? we will need this to create good looking official
> localized builds, starting Firefox 1.0.

We need help from someone with a debug build on a system where the patch doesn't
work.

Comment 52

[Asaf Romano (gone)]

(In reply to comment #51)

> We need help from someone with a debug build on a system where the patch doesn't
> work.

...building now.

Comment 53

[Asaf Romano (gone)]

Attachment: Ok, works well on Windows

I dunno what was the problem in previous attempts.

Comment 54

[:deanis74]

Looks like the problem might be linux-only.

Comment 55

[Asaf Romano (gone)]

Comment on attachment 154950 [details] [diff] [review]
workaround

if this one does work as expected on linux, we may want it as a workaround for
ff/tb 1.0.

Comment 56

[Ilya Konstantinov]

Indeed broken on X (exactly the same problem as in the screenshot).
The only point to blame is nsBidiPresUtils::RenderText calling 
aRenderingContext.GetWidth, since this eventually goes to per-platform Gfx code.
The Gtk gfx code returns oversized font metrics, it seems.

It gets even more complicated, since Gtk's has different font metrics
implementations: core X, Xft and Pango. Go figure which one works well and which
one we need to fix.

Comment 57

[Ilya Konstantinov]

gfx/gtk isn't broken. The bug was in the patch. In the RTL case, gfx/gtk was
receiving a "left" string with incorrect width, and starting on the incorrect
start offset. No wonder it miscalculated the width; it was calculating the width
of a NUL terminator!

The fix:
  if (level & 1) {
    aRenderingContext.GetWidth(aText + start,
                               (subRunLength - (aAccessKeyIndex - start + 1)),
                               subWidth, nsnull);
  }

Explanation:
1st argument: aText is in visual order, so we shall begin the string from the
start (e.g. <*start*><TZADI><BET><VAV><KUF> -- where <KUF> is the mnemonic).
2nd argument: aAccessKeyIndex is a logical position within the entire string;
   1. We subtract the start of our run from it (aAccessKeyIndex - start)
   2. We add +1 to it, since aAccessKeyIndex is zero-based.
   3. We subtract the resulting logical position from the start of the run, from
the run's length, to get the visual position within the run.
ie. <TZADI><BET><VAV><*end*><KUF>

Result:
We get the metrics for [<TZADI><BET><VAV>]<KUF>.

Comment 58

[Ilya Konstantinov]

Few more comments about the RenderText function:

1. I wrote that "aText is in visual order". Well, it's passed in logical order
(the function's documentation could as well say so), but the function has a
side-effect of log2vis-ing the string its passed *for input*. We should either
not have this (non-trivial) side-effect, or document it.

2. The two arguments (related to mnemonics) you've added seem a bit ad-hoc here.
Aren't we better off adding something more generic to such a function?
Resolving of logical positions is not a "natural" use of a RenderText function
either, but for sake of optimization (since we do BiDi at this point anyway), we
can offer this functionality in this function.

I propose something like (very similar to the addition parameters to fribidi's
log2vis function):

  struct nsBidiPositionResolve
  {
     // [in] Logical position within string
     PRInt32 logicalPosition;
     // [out] Visual position within string
     PRInt32 visualPosition;
     // [out] Visual metric in twips
     PRInt32 visualMetricTwips;
  };

  /*
   * @param aPosResolve array of logical positions to resolve into visual
positions; can be nsnull if this functionality is not required
   * @param aPosResolveCount number of items in the aPosResolve array
   */
  nsresult RenderText(PRUnichar*           aText,
                      PRInt32              aLength,
                      nsBidiDirection      aBaseDirection,
                      nsPresContext*      aPresContext,
                      nsIRenderingContext& aRenderingContext,
                      nscoord              aX,
                      nscoord              aY,
                      nsBidiPositionResolve* aPosResolve,
                      PRInt32              aPosResolveCount);

Comment 59

[Ilya Konstantinov]

Attachment: Patch (for both reordering and non-reordering gfx-s)

This patch implements 3 changes on top of smontagu's work:
1. A more generic form of RenderText (as I've described in my previous
comment). Resolves an arbitrary number of logical indexes into visual indexes +
left width metric.
2. A minor change in the length passed to GetWidth.
3. RenderText no longer has side-effect on the aText. This forces me to keep a
copy of each run, so I hope it won't affect performance considerably.

Comment 60

[Simon Montagu :smontagu]

(In reply to comment #59)

> 3. RenderText no longer has side-effect on the aText. This forces me to keep a
> copy of each run, so I hope it won't affect performance considerably.

Currently nsTextBoxFrame creates a temporary copy |buffer| to pass to RenderText
to avoid the side effect. You could eliminate that.

Other than that I like the patch, and I can confirm that it works correctly on
Win32.

Comment 61

[Simon Montagu :smontagu]

Comment on attachment 162316 [details] [diff] [review]
Patch (for both reordering and non-reordering gfx-s)

>Index: xul/base/src/nsTextBoxFrame.cpp

>+          posResolve.logicalIndex = mAccessKeyInfo->mAccesskeyIndex;

I'm crashing on this line when entering hebrew in the URI bar. You probably
need to null-check mAccessKeyInfo.

Comment 62

[Ilya Konstantinov]

Attachment: Patch with smontagu's comment #60 and #61

Changes from previous patch:
1. Don't keep unnecessary copy, since RenderText no longer modifies the input
string.
2. Check for null mAccessKeyInfo

Comment 63

[Boris Zbarsky [:bzbarsky]]

It's quite likely I'll need someone on irc to talk me through this patch in
order to review it properly... I don't really know this code, and more
importantly don't really know what it does.  In any case, I'm not going to be
able to look until at least tuesday.

Comment 64

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 162925 [details] [diff] [review]
Patch with smontagu's comment #60 and #61

>Index: base/public/nsBidiPresUtils.h

>+  nsresult RenderText(const PRUnichar*     aText,

Check whether there are other callers who no longer have to allocate and fix
them?

>Index: base/src/nsBidiPresUtils.cpp
>+  for(int nPosResolve=0; nPosResolve<aPosResolveCount; ++nPosResolve)

Space around the '<' please.

>-
>+      

Don't make random whitespace changes?

>+       * If aAccessKeyLeftWidth is not null, we return the width 
of the string

....

Remove this comment, as discussed.

>+          /*
>+           * If this run is only one character long, the width is the x-coord

What width?  You mean the visual position?  May be better to call it that.

>+          /*
>+           * Otherwise, we need to calculate the width of the part of the run to
>+           * the left of the mnemnonic.

"visually to the left of the character whose logical position is
posResolve->logicalIndex" perhaps?

 If the run is right-to-left, this will be
>+           * the substring from the character after the mnemonic up to the end of

"the width of the substring from the character after ..."

>+           * the run; if it is left-to-right it will be the substring from the

Again, "the width of the substring".


>+           * start of the run up to the character before the mnemonic.

Again, we don't really have a mnemonic in sight....  Rephrase the comment
accordingly?

>+              aRenderingContext.GetWidth(aText + posResolve->logicalIndex + 1,
>+                                         start + subRunLength - (posResolve->logicalIndex + 1),

I think the second arg would be clearer as |posResolve->visualIndex -
visualStart|.

>+                                         posResolve->logicalIndex - start,

Same here.  That makes it clearer that we really are looking at the same thing
in both cases...

For that matter, it may be a good idea to put the first arg of the GetWidth()
call in a suitably named local in each branch of that if, and only make one
GetWidth() call.  It would help in two ways: probably smaller code, and more
clarity as to what this magic "aText + posResolve->logicalIndex + 1" quantity
is (the logical start of the string we're measuring).

>Index: xul/base/src/nsTextBoxFrame.cpp

>+           mAccessKeyInfo->mBeforeWidth = posResolve.visualLeftTwips;

Given this, the code in nsTextBoxFrame::CalculateUnderline is somewhat
redundant.  Can part of it be skipped for the case when this code calculates an
mBeforeWidth?

Comment 65

[Ilya Konstantinov]

Attachment: Patch with bzbarsky's comment #62

Implements suggestions by comment #62.

I didn't find any more places where the string passed to RenderText is
duplicated to avoid the "side effect" which RenderText used to have. I only
changed all functions which used RenderText to pass a const PRUnichar*, since
the function now accepts a const string.

Comment 66

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 163634 [details] [diff] [review]
Patch with bzbarsky's comment #62

>Index: base/src/nsBidiPresUtils.cpp
>+            nscoord subWidth;
>+            if (level & 1) {
...
>+              // The delta between the end of the run and the left part's beginning.
>+              PRInt32 visualLeftLength = posResolve->visualIndex - visualStart;
>+              aRenderingContext.GetWidth(visualLeftPart,
>+                                         visualLeftLength,
>+                                         subWidth, nsnull);
>+            }
>+            else {
....
>+              // The delta between the start of the run and the left part's end.
>+              PRInt32 visualLeftLength = posResolve->visualIndex - visualStart;
>+              aRenderingContext.GetWidth(visualLeftPart,
>+                                         visualLeftLength,
>+                                         subWidth, nsnull);
>+            }

The point was that the visuaLeftLength setting and the GetWidth() call are now
identical between the two branches and can be hoisted out of the if/else
entirely.  So you set the visualIndex and visualLeftPart differently (declare
before the if, set in the two branches), then use them.

>Index: xul/base/src/nsTextBoxFrame.cpp
>+        nsBidiDirection direction =
>+                                  (NS_STYLE_DIRECTION_RTL == 

No need for the linebreak after the '=' sign, methinks.  If the line is too
long, you need to outdent more on the line following the equals....

>+               aRenderingContext.GetWidth(mCroppedTitle.get(), mAccessKeyInfo->mAccesskeyIndex,
>+                                      mAccessKeyInfo->mBeforeWidth);

Fix the indent here.

r+sr=bzbarsky with those changes.

Comment 67

[Ilya Konstantinov]

Attachment: Patch with bzbarsky's comment #66

Comment 68

[Ilya Konstantinov]

Attachment: Better patch with bzbarsky's comment #66

Comment 69

[Ian Neal]

Comment on attachment 163816 [details] [diff] [review]
Better patch with bzbarsky's comment #66

Checking in base/public/nsBidiPresUtils.h;
/cvsroot/mozilla/layout/base/public/nsBidiPresUtils.h,v  <--  nsBidiPresUtils.h
new revision: 1.15; previous revision: 1.14
done
Checking in base/src/nsBidiPresUtils.cpp;
/cvsroot/mozilla/layout/base/src/nsBidiPresUtils.cpp,v	<-- 
nsBidiPresUtils.cpp

new revision: 1.56; previous revision: 1.55
done
Checking in html/base/src/nsPageFrame.cpp;
/cvsroot/mozilla/layout/html/base/src/nsPageFrame.cpp,v  <--  nsPageFrame.cpp
new revision: 3.141; previous revision: 3.140
done
Checking in xul/base/src/nsTextBoxFrame.cpp;
/cvsroot/mozilla/layout/xul/base/src/nsTextBoxFrame.cpp,v  <-- 
nsTextBoxFrame.c
pp
new revision: 1.76; previous revision: 1.75
done
Checking in xul/base/src/tree/src/nsTreeBodyFrame.cpp;
/cvsroot/mozilla/layout/xul/base/src/tree/src/nsTreeBodyFrame.cpp,v  <-- 
nsTree
BodyFrame.cpp
new revision: 1.234; previous revision: 1.233
done

Comment 70

[is+mozilla]

Forgive me if this is a stupid question (I'm not a developer), but is it
possible to apply this patch to the aviary1.0 branch? It seems like this would
be a major usability enhancement. 

Comment 71

[Ilya Konstantinov]

Comment on attachment 163816 [details] [diff] [review]
Better patch with bzbarsky's comment #66

Judging from the he-IL nightly builds of Firefox (in which the mnemonics bug is
clearly visible), this fix would be desired for the upcoming Firefox 1.0. Can
it be approved?

Comment 72

[Asa Dotzler [:asa]]

Comment on attachment 163816 [details] [diff] [review]
Better patch with bzbarsky's comment #66

too late for 1.0

Comment 73

[Reuven Gonen]

*** Bug 291453 has been marked as a duplicate of this bug. ***

Comment 74

[Simon Montagu :smontagu]

Comment on attachment 163816 [details] [diff] [review]
Better patch with bzbarsky's comment #66

Requesting branch approval. This is important for all localizations in right-to-left languages

Comment 75

[Simon Montagu :smontagu]

Comment on attachment 163816 [details] [diff] [review]
Better patch with bzbarsky's comment #66

Sorry, this is already in the 1.8 branch

Comment 76

[(no longer active)]

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