Tested with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b2) Gecko/20050629 Firefox/1.0+ To reproduce: 1. Open the attached testcase. 2. Put the caret to the right of the Hebrew letter Alef (first line, right corner) 3. Press the LeftArrow button 3 times (or more) Result: The caret moves to the right of the number 1 (first line), but then remains stuck in this position regardless of any additional key presses. - The LTR textarea is not effected by the "LeftArrow bug" (but read on...) - The same problem happens with the next line, the one that ends with an 'a'. - The same problem happens when pressing Ctrl+LeftArrow (to quickly jump between words). This bug effects both the LTR and the RTL textareas. This bug is a spinoff from bug 207186, comment #0. More to come. Prog.

Actually, the LTR character doesn't have to be at the end of the line for this to happen - it could be anywhere next to RTL text. Also - this is not symmetrical: it happens only for an LTR character in RTL text, but not for an RTL character in LTR text. This happens both when the textarea is RTL and when it is LTR. Changing summary to reflect these comments. I also suggest that the [Ctrl/Option]+Arrow issue be treated as a separate bug, as it seems to be a more complicated case.

Testcase to demonstrate comment #2: The problem exists on lines 1, 2, and 3 in both textareas (trying to step over the "1" when coming from the Hebrew letters), but does not exist on lines 4, 5 and 6 (single RTL letter).

This is (almost) the patch I attached earlier to bug 207186 (attachment 184595 [details] [diff] [review]). I removed the "hack" comment since i'm more-or-less convinced this is a correct solution (although I'm still looking for something more general which might also fix some of the ctrl+arrow bugs).

Very annoying problem, and we have a patch. Requesting blocking1.8a4.

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

I'd like to explain to ourselves what happens w.r.t. testcase A2 without the patch and what happens with it. We have 2 frames on the first line: the first is an RTL frame which has the aleph-aleph-space, and the second is an LTR frame which has just the 1. frame #1 does PeekOffset() to get the 'previous' (i.e. next to the left) character, and it calls PeekOffset() of frame #2 since there's nothing to the left of its third character, the space; it also flips the preferLeft from 1 to 0. But! It needs to do that only if it looks for the position within itself, and since it's calling its neighbor, there is no reason to make this flip - its RTLness is completely invalid. Frame #2 get the flipped preferLeft and it can't tell this situation apart from a call from a non-RTL neighbor frame, which would not have flipped the value. And frame #2 keeps the flipped value since its directionality embedding level is even (it's LTR). The point is that the 'preferLeft' usually acts like 'prefer the end of a previous frame w.r.t. a left-then-down order, rather than the beginning of the next frame'. But when your document or textarea or whatever are RTL, the visual order is different, meaning that 'preferLeft' is not really "prefer whatever is on the left". But instead of flipping it by XOR we should just set it in a reasonable way.

This patch is like Uri's, with a few differences: 1. I've noted that you don't need an extra variable, you can simply set the mPreferLeft the way you need it based on other information instead of XORing the value you've gotten (i.e. you don't need to even look at the value you've gotten AFAICT) 2. I've added lots more comments, which I hope will be useful; although it may be the case that some of these comments may eventually need to be placed somewhere else, e.g. near the function declaration. But we'll cross that bridge if we get to it (and don't get stuck at some point along the way :-) ... bad pun). 3. Uri only applies his change to eSelectCharacter, I also apply it to eSelectWord. This means that in testcase https://bugzilla.mozilla.org/attachment.cgi?id=131484 we now do not skip the third word on the first line, but rather get stuck at it. I find that the patch change (in either Uri's version of mine) is reasonable in itself, so this shouldn't be thought of as a regression but rather as a different manifestation of the plethora of bugs we have. Plus I believe it's better to get to a word and be stuck then not get to it at all. 4. I still don't understand where the aPos->mPreferLeft we're changing or not changing comes into play, because nsTextFrame does not directly use it other than setting it at the beginning, and in the testcases (e.g. A2) when we get stuck at 1, and when we don't (with the patch), only nsTextFrame::PeekOffset() calls are made - no calls to nsFrame::PeekOffset() or nsBRFrame::PeekOffset() AFAICT.

Comment on attachment 188819 [details] [diff] [review] alternative patch BTW, Uri says he agrees not using the extra var is a good idea, and that I can go ahead and submit my patch; I'm not invalidating his though (both because that's his decision to make and because of the eSelectWord issue).

regarding point 3. in comment 8 - it might only be relevant when layout.word_select.eat_space_to_next_word = false (as is the default on MacOS but not on Windows).

Eyal, I'm re-assigning this to you with the recommendation that you move the review request on your patch from Simon to someone else (probably roc), so we can get this in for 1.8b4 (Simon is on vacation until 17/8, I think).

Comment on attachment 188819 [details] [diff] [review] alternative patch This looks good. I appreciate the comments. Frankly, you two understand this code a lot better than I do at this point. You might as well request review from each other and just ask me for sr.

Comment on attachment 188819 [details] [diff] [review] alternative patch Requesting approval1.8b4 on Eyal's behalf, so this can get landed ASAP (hopefully before the branch).

For Shaver's benefit: I agree with approving this patch for 1.8b4.

Trunk checkin: Checking in nsTextFrame.cpp; /cvsroot/mozilla/layout/generic/nsTextFrame.cpp,v <-- nsTextFrame.cpp new revision: 1.514; previous revision: 1.513 done

1.8 Branch checkin: Checking in nsTextFrame.cpp; /cvsroot/mozilla/layout/generic/nsTextFrame.cpp,v <-- nsTextFrame.cpp new revision: 1.513.4.1; previous revision: 1.513 done

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