305798 - Bidi:After deleting last character of reverse-direction text, arrow keys move caret incorrectly

Status: RESOLVED FIXED

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

Description

[Uri Bernstein]

When LTR text is embedded withing RTL text in an RTL textarea, when deleting the
last character of the LTR text, and then using an arrow key to move the caret,
the caret moves as if it were previously located at the other side (i.e. the
beginning) of the LTR text.

The same is true when switching "LTR" and "RTL" in the description above.

This is *not* a regression (not since Firefox 1.0, anyway).

A testcase will follow soon.

Comment 1

[Uri Bernstein]

Attachment: testcase

In the first (RTL) textarea:
- Position the caret inside the word "English".
- Use the right-arrow key to move to the right of the "h"
- Press "backspace" to delete the "h". The caret will be correctly placed to
the right of the "s".
At this point:
- Pressing the right arrow will move the caret to between the "E" and the "n"
- Pressing the left arrow will move the caret to the left of the space left of
the  "E"
- Pressing the down arrow will move the caret to below the position left of the
"E".

The second (LTR) case is symmetrical. Move from within the Hebrew word to the
left of leftmost Hebrew letter and press "backspace" to see the same effect.

Comment 2

[Uri Bernstein]

> This is *not* a regression (not since Firefox 1.0, anyway).

Technically, this *is* a regression, from bug 92124 (so it regressed shortly
before Mozilla 1.0).

Assigning to me, I'll see what I can do.

Comment 3

[Uri Bernstein]

Attachment: patch v1.0

This is a general fix for this bug, as well as several other similar
(non-reported) bugs where, upon moving the caret, it moves from a different
position than that in which it previously appeared. (I can supply testcases and
descriptions of the other bugs, if required).

The idea is to make nsSelection::MoveCaret() go through the bidi caret
positioning algorithm when determining where the caret is initially (before the
move). This ensures that any movement will be based upon the current place
where the caret is actually drawn.

I don't really like the fact that I had to add the extra method to nsICaret.h
(in addition to nsCaret.h) so if you have an idea how to avoid it I'll be happy
to hear.

Comment 4

[Uri Bernstein]

Comment on attachment 193830 [details] [diff] [review]
patch v1.0

Note: need to add 
#include "nsIFrameSelection.h"
To nsICaret.h for this to actually compile.

Comment 5

[Uri Bernstein]

Comment on attachment 193830 [details] [diff] [review]
patch v1.0

This patch won't apply now because of the fix to bug 307533. I'm working on an
updated version.

Comment 6

[Uri Bernstein]

Attachment: patch v2.0

Changes from the previous patch:

- Fixed the arrow-up/down case (which was not fixed by the prevoius patch) by
using GetCaretFrameForNodeOffset() also in nsCaret::GetCaretCoordinates().
- Adjusted for bug 307533.
- GetCaretFrameForNodeOffset() is now side-effect free:
-- It does not set mLastBidiLevel, which is now handled similarly to the other
"last" values. [this is probably the biggest change here]
-- It does not call presShell->SetCaretBidiLevel() in the BIDI_LEVEL_UNDEFINED
case. I believe this is unnecessary.
- Added the missing #include in nsICaret.h

Comment 7

[Uri Bernstein]

Forgot to say:
I also removed some "#ifdef IBMBIDI"s, because adding them everywhere around the
bidiLevel vstuff would have made this terribly ugly, and I got the impression we
want to get rid of them anyway.

Comment 8

[Uri Bernstein]

Attachment: patch v2.1

SetCaretBidiLevel is back, but now outside GetCaretFrameForNodeOffset().
Some more nits, picked by Simon.

Comment 9

[Simon Montagu :smontagu]

Comment on attachment 195621 [details] [diff] [review]
patch v2.1

One nit I missed on IRC: you should change the comment
 // if there is a frameAfter, move into it, setting HINTRIGHT to make sure we
stay there
as you did for the parallel comment with HINTLEFT

Comment 10

[Uri Bernstein]

Attachment: patch v2.1.1

Updated per Simon's last comment. Carrying over r+.

Comment 11

[Uri Bernstein]

Attachment: patch v2.1.1

D'oh! This time did it right (I hope).

Comment 12

[Uri Bernstein]

Attachment: patch v2.2

This is an unbitrotten version of the patch, plus some fixes to error handling in nsTypedSelection::GetPrimaryFrameForFocusNode() (which I did pretty poorly with the previous patches).

So, Simon - you only need to review GetPrimaryFrameForFocusNode(). Everything else should be identical to v2.1.1.

Comment 13

[Uri Bernstein]

Attachment: patch v2.2.1

Same as v2.2, updated for changes made in nsICaret.h.

Comment 14

[Uri Bernstein]

Fix checked in by smontagu, 2005-11-16 01:37

Comment 15

[Uri Bernstein]

Checked in to 1.8 branch:
Checking in mozilla/layout/base/nsICaret.h;
/cvsroot/mozilla/layout/base/nsICaret.h,v  <--  nsICaret.h
new revision: 1.35.2.1; previous revision: 1.35
done
Checking in mozilla/layout/base/nsCaret.h;
/cvsroot/mozilla/layout/base/nsCaret.h,v  <--  nsCaret.h
new revision: 1.43.2.2; previous revision: 1.43.2.1
done
Checking in mozilla/layout/base/nsCaret.cpp;
/cvsroot/mozilla/layout/base/nsCaret.cpp,v  <--  nsCaret.cpp
new revision: 1.144.2.8; previous revision: 1.144.2.7
done
Checking in mozilla/layout/generic/nsSelection.cpp;
/cvsroot/mozilla/layout/generic/nsSelection.cpp,v  <--  nsSelection.cpp
new revision: 3.199.2.7; previous revision: 3.199.2.6
done