302051 - Bidi: "Home" on line containing only text in reverse direction causes caret to be displayed on different line

Status: RESOLVED FIXED

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

Description

[Uri Bernstein]

This was originally described in the second part of bug 207186, comment #2.

If in an RTL textbox, a line contains only LTR text, (or in an LTR textbox, a
line contains only RTL text) pressing the HOME key (cmd+left-arrow on Mac) on
that line causes the caret to be displayed at the beginning of the next line
containing RTL (LTR) text, or, if there is no such line, at the beginning of the
last line.

The insertion point is actually placed at the correct position (logical start of
the line), it's just the caret which is displayed in the wrong place.

Comment 1

[Uri Bernstein]

Attachment: testcase

Place the caret in the first or second line of either textbox and press "Home"
(cmd+left-arrow on Mac).
The caret will appear at the beginning of the third line.

Comment 2

[Uri Bernstein]

Attachment: patch?

This patch just removes some code which is responsible for the bug.
I'm not even requesting official review on this, since I assume that code is
actually useful in some situations. However, I can't say what these situations
are.

Simon - could you please give an example of when that code is (or was) actually
useful? Thanks.

Comment 3

[Simon Montagu :smontagu]

The code is designed for a case where there is some content on the line in the
paragraph direction, and in that case it seems to work well. I think that the
problem here is actually that nsSelection::GetFrameFromLevel() can modify the
value of the aFrameOut argument on failure.

Can you try moving the line
 *aFrameOut = foundFrame;
at http://lxr.mozilla.org/seamonkey/source/layout/generic/nsSelection.cpp#2175
outside the do {} loop and see if this helps?

Comment 4

[Eyal Rozenberg]

I'm not able to reproduce the bug with Win XP using the testcase, build
2005-07-01 05.

Comment 5

[Uri Bernstein]

Eyal:
This is because of the original patch to bug 254278, which caused this bug
(302051) as well as others to be not reproducable between 2004-08-04 and
2005-07-14, when the second patch for 254278 was checked in. See bug 207186
comment #54.

Simon:
Sorry, I got a bit busy with bug 277553. I hope to get back to this bug (and try
your suggestion) soon. 

Comment 6

[Simon Montagu :smontagu]

(In reply to comment #3)
> I think that the
> problem here is actually that nsSelection::GetFrameFromLevel() can modify the
> value of the aFrameOut argument on failure.

That may be a problem, but it's not *the* problem, because GetFrameFromLevel()
is not actually failing. It may need to be changed to make sure we don't leave
the current line.

Comment 7

[Simon Montagu :smontagu]

Attachment: patch??

... like this, but I don't feel I know the frame traversal code well enough to
be sure.

Comment 8

[Uri Bernstein]

Attachment: testcase (related problem)

This testcase demonstrates a different problem with caret positioning after pressing "Home" in bidi text, but I'm posting it here rather than opening a new bug because I think it should affect the way this bug is fixed.

Load the testcase in Composer, and press "Home". The caret is positioned visually between "123" and "456", instead of to the left of "123" as expected.

The fault here, I think, is with the logic for finding the visually first frame on the line. The current logic is: "find the first frame whose level is the same as the paragraph level, and put the caret at the end of the frame before that". This startegy doesn't work in this case.

Instead, perhaps we should use a more standard method for finding the first (last) visual frame on the line, namely nsLineIterator::CheckLineOrder(), followed by drilling down to the first/last descendant.

Comment 9

[Uri Bernstein]

Attachment: patch?!

This attempts to accomplish what I suggested in comment #8, by introducing a "visual" mode to the eSelect[Begin|End]Line case of PeekOffset, and using it for positioning the caret in this specific situation.
Notice that MoveCaret() still always uses the logical mode.

I'm not sure I really like this... Simon - comments?

Comment 10

[Simon Montagu :smontagu]

Comment on attachment 229877 [details] [diff] [review]
patch?!

>+      {
>       it->GetLine(thisLine, &firstFrame, &lineFrameCount, usedRect, &lineFlags);
...
>       }
>+      }

nit: clean up the indentation of this block. r=me with that.

Comment 11

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

Comment on attachment 229877 [details] [diff] [review]
patch?!

+        if ((embeddingLevel & 1) && !lineIsRTL || !(embeddingLevel & 1) && lineIsRTL)

How about
if ((embeddingLevel & 1) == !lineIsRTL)

Comment 12

[Eyal Rozenberg]

(In reply to comment #11)
How about some in-code comments explaining what's being done and what endOfLine signifies exactly? :-(

Comment 13

[Uri Bernstein]

Attachment: patch for checkin

This is what I checked in:
- Fixed indentation per comment #10
- Used roc's suggestion from comment #11
- Added a comment which hopefully clarifies the usage of endOfLine

Comment 14

[Uri Bernstein]

Checked in:

Checking in layout/generic/nsFrameSelection.h;
/cvsroot/mozilla/layout/generic/nsFrameSelection.h,v  <--  nsFrameSelection.h
new revision: 1.7; previous revision: 1.6
done
Checking in layout/generic/nsFrame.cpp;
/cvsroot/mozilla/layout/generic/nsFrame.cpp,v  <--  nsFrame.cpp
new revision: 3.661; previous revision: 3.660
done
Checking in layout/generic/nsSelection.cpp;
/cvsroot/mozilla/layout/generic/nsSelection.cpp,v  <--  nsSelection.cpp
new revision: 3.254; previous revision: 3.253
done
Checking in layout/base/nsCaret.cpp;
/cvsroot/mozilla/layout/base/nsCaret.cpp,v  <--  nsCaret.cpp
new revision: 1.162; previous revision: 1.161
done

Comment 15

[Uri Bernstein]

Reopening to assign to myself. Sorry for this and the following bugspam.