299239 - BiDi: Caret is stuck when reaching a single LTR character in RTL text

Status: RESOLVED FIXED

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

Description

[Prognathous]

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.

Comment 1

[Prognathous]

Attachment: Testcase A

Comment 2

[Uri Bernstein]

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.

Comment 3

[Uri Bernstein]

Attachment: Testcase A2

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).

Comment 4

[Uri Bernstein]

Attachment: patch (from bug 207186)

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).

Comment 5

[Uri Bernstein]

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

Comment 6

[Uri Bernstein]

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

Comment 7

[Eyal Rozenberg]

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.

Comment 8

[Eyal Rozenberg]

Attachment: alternative patch

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 9

[Eyal Rozenberg]

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).

Comment 10

[Eyal Rozenberg]

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).

Comment 11

[Uri Bernstein]

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 12

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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 13

[Uri Bernstein]

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).

Comment 14

[Blake Kaplan (:mrbkap) (inactive)]

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

Comment 15

[Asaf Romano (gone)]

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

Comment 16

[Asaf Romano (gone)]

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

Comment 17

[(no longer active)]

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