299842 - BiDi: Recursive caret movement in LTR lines which begin with an RTL word or character

Status: VERIFIED FIXED

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

Description

[Prognathous]

To reproduce:

1. Open https://bugzilla.mozilla.org/attachment.cgi?id=137983
2. In the second textarea (LTR), put the caret anywhere on the Hebrew word 
   ("עברית").‎
3. Hold the LeftArrow button pressed.

Actual result:
The caret moves recursively to left. Every time it reaches the beginning of the
line (leftmost part), it comes back in the beginning of the Hebrew word (to
right of the word).

Expected result:
The caret should stop at the beginning of the line. This is the expected
behavior in Visual Caret Movement, as is the convention in Gecko.

Tested with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b2)
Gecko/20050705 Firefox/1.0+

First reported in Bug 207186, comment #19

Uri, if I understand correctly, your patch for Bug 299239 should also fix this
one. If so, please close this as WORKSFORME after you get that patch checked in.

Thanks,

Prog.

Comment 1

[Uri Bernstein]

> Uri, if I understand correctly, your patch for Bug 299239 should also fix this
> one. If so, please close this as WORKSFORME after you get that patch checked in.

No, this is a separate issue, for which I attached a patch to bug 207186. That
patch was checked in but caused a regression, so it was backed out. I then
suggested an alternative patch. I'll post both these patches here shortly.

Comment 2

[Uri Bernstein]

Attachment: patch A

This is the original patch I proposed in bug 207186 comment 37. It got
r+:smontagu and sr+:roc, and was checked in, but it caused a regression: bug
299371, so it was backed out.
I'm still not sure if the approach here (PeekOffset() returning an error code
when there is nowhere to go) was really wrong, or whether it merely exposed a
problem elsewhere in the code.

Comment 3

[Uri Bernstein]

Attachment: patch B

An alternative patch, originally posted on bug 207186, comment #50.
The idea here is to make sure that nsFrame::GetFrameFromDirection does not flip

mPreferLeft unless it succeeds.

This does not cause the regression caused by patch A.

Comment 4

[Uri Bernstein]

Attachment: testcase

This testcase shows that the problem can also occur in the reverse case (RTL
textarea, strating with LTR text, using right-arrow).

Comment 5

[Uri Bernstein]

Comment on attachment 188432 [details] [diff] [review]
patch B

Since I got no response on bug 299371, let's just go with option "B", which
fixes this bug without causing a regression.
Since Simon is on vacation, and this is a small, simple, patch - moving review
request to roc.

Comment 6

[Asaf Romano (gone)]

Checking in nsFrame.cpp;
/cvsroot/mozilla/layout/generic/nsFrame.cpp,v  <--  nsFrame.cpp
new revision: 3.573; previous revision: 3.572
done

Comment 7

[(no longer active)]

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