288789 - BiDi - In an RTL textbox, caret moves to wrong position when using arrow keys to move into a line containing LTR text

Status: VERIFIED FIXED

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

Description

[Uri Bernstein]

In an RTL textbox, if a line contains LTR characters embedded in RTL text, using
the arrow keys to move into this line (from the previous line or next line), the
caret appears at the beginning/end of the RTL fragment, rather than at the
beginning/end of the line.

Please notice that this is a regression. Unlike the various problems described
in bug 207186, this bug was not on the Aviary branch - it only appears on the
trunk.

Comment 1

[Uri Bernstein]

Attachment: testcase

Self-explaining testcase

Comment 2

[Uri Bernstein]

Bah - I got the directions messed up a bit. The summary should read:
"... the caret appears at the beginning/end of the *LTR* fragment, rather than
at the beginning/end of the line."

Comment 3

[Uri Bernstein]

This regressed between 2004-11-25 and 2004-12-05.
Unfortunately, I can't find in the archives any builds in between these two
dates (although I know such builds were made), so I can't give a more precise date.

It should be noted that the Aviary landing took place approximately at that time
frame.

Comment 4

[Simon Montagu :smontagu]

(In reply to comment #3)
> This regressed between 2004-11-25 and 2004-12-05.
> Unfortunately, I can't find in the archives any builds in between these two
> dates (although I know such builds were made), so I can't give a more precise
date.
> 
> It should be noted that the Aviary landing took place approximately at that time
> frame.

Bug 256833 and Bug 256835 are possible candidates.

Comment 5

[Uri Bernstein]

Indeed, backing out the patch to bug 256835 fixes this one. I'll add a note on
that bug.

Comment 6

[Ginn Chen]

I'll work on it when I've time.

Comment 7

[Uri Bernstein]

Requesting 1.8b3 block. BiDi caret movement is buggy enough even without this
regression (see bug 265070). Having the situation worsen is not something people
are looking forward to.

Comment 8

[Uri Bernstein]

Taking. It seems like Ginn's patch only exposed a deeper bug, which I'm trying
to fix.

Comment 9

[Uri Bernstein]

Attachment: patch

Make nsFrameList::GetPrevVisualFor() and nsFrameList::GetNextVisualFor()
actually return the previous (and next) visual frame, based on the paragraph
direction. (before the patch, "next" meant "right" and "prev" meant "left").

This also makes the logic in nsFrame::GetFrameFromDirection() somewhat simpler,
as the "lineJump" case is no longer a special case.

Comment 10

[Uri Bernstein]

Requesting blocking1.8b4. This is a very bad regression, and I have a proposed fix.

Comment 11

[Simon Montagu :smontagu]

Comment on attachment 187360 [details] [diff] [review]
patch

The logic seems correct, but I can't test this right now, so please test with
as many combinations as possible of text in different directions, with line
breaks between words in the opposite direction from the paragraph direction,
and with line breaks between runs in opposite directions from each other.

Comment 12

[Uri Bernstein]

Comment on attachment 187360 [details] [diff] [review]
patch

I tested this in the various cases outlined by Simon, and it seems fine in all
of them (there are still glitches with the visual placement of the caret in
certain situations, but those are independent of this bug).

Comment 13

[Uri Bernstein]

I'll ask someone to check this in sometime next week, when I'll be available to
deal with possible regressions.

Comment 14

[Uri Bernstein]

Patch checked in by timeless at 2005-08-02 10:19.

Comment 15

[Uri Bernstein]

Attachment: my first mochitest

Simple mochitest for this.

Comment 16

[Simon Montagu :smontagu]

Comment on attachment 290025 [details] [diff] [review]
my first mochitest

Nice!

Comment 17

[Uri Bernstein]

Attachment: mochitest, take 2

Now in the right directory, and including Makefile.in.

Comment 18

[Uri Bernstein]

Comment on attachment 290129 [details] [diff] [review]
mochitest, take 2

Hi roc, what are the review requirements for layout tests? do I need an sr+? Do I even need an r+, officially?

Comment 19

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

Comment on attachment 290129 [details] [diff] [review]
mochitest, take 2

review for tests is optional. If you feel comfortable with the test, you can just land it.

Comment 20

[(no longer active)]

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