313602 - Bidi: Caret appears incorrectly when using up/down arrows to move into a blank line surrounded by reverse-direction text

Status: RESOLVED FIXED

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

Description

[Uri Bernstein]

In an LTR textarea, when:

- A line (#1) ends with RTL text
- The following line (#2) is blank
- The line after that (#3) starts with RTL text

Then:
Placing the caret on LTR text on line #1 (or #3) line, and pressing down-arrow (or up-arrow) to arrive at line #2, causes the caret to appear at the left of line #3, instead of on line #2.

The caret is logically positioned correctly (i.e. typing anything at this stage will enter text into line #2). It is only displayed incorrectly.

The same holds for the reverse situation (RTL textarea, line #1 ends with RTL, etc.)

Testcase coming up.

Comment 1

[Uri Bernstein]

Attachment: testcase

self-documenting.

Comment 2

[Uri Bernstein]

Attachment: patch

All this patch does is to remove the following condition from around the code setting the caret bidi level after a caret move:

if (frameStart !=0 || frameEnd !=0) // Otherwise the frame is not a text frame, so nothing more to do

This fixes this bug, as well as bug 313596, by allowing the caret bidi level to be set correctly when moving into a BRFrame. Currently the caret bidi level is not changed in these cases, so it remains set according to the previous caret position.

I'm not sure this is *the* correct fix for these bugs (see also bug 303781 comment #5). However, setting the bidi level for BRFrames seems like the right thing to do anyway.

Simon - do you remember why that "if" is there? Are there kinds of frames for which we really do not want to calculate the caret bidi level? Should I try to limit the fix specifically to BRFrames?

Comment 3

[Uri Bernstein]

Attachment: patch -w

Comment 4

[Simon Montagu :smontagu]

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

Yeah, that if was a premature optimization. r=me.

Comment 5

[Uri Bernstein]

Patch checked in by smontagu, 2005-11-16 01:40

Comment 6

[Uri Bernstein]

Checked in to 1.8 branch:
Checking in mozilla/layout/generic/nsSelection.cpp;
/cvsroot/mozilla/layout/generic/nsSelection.cpp,v  <--  nsSelection.cpp
new revision: 3.199.2.5; previous revision: 3.199.2.4
done

Comment 7

[Uri Bernstein]

I backed the patch out of the 1.8 branch, since it caused a regression (which I'll file soon).
Checking in mozilla/layout/generic/nsSelection.cpp;
/cvsroot/mozilla/layout/generic/nsSelection.cpp,v  <--  nsSelection.cpp
new revision: 3.199.2.8; previous revision: 3.199.2.7
done

Comment 8

[Uri Bernstein]

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

Minusing approval‑branch‑1.8.1, so this won't accidentally get checked in to the branch again.

The regression caused by this was bug 328655.