331444 - Bidi: Caret sometimes moves incorrectly when moving into HTML paragraph starting/ending with reverse-direction text

Status: RESOLVED FIXED

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

Description

[Uri Bernstein]

This is a follow-up to bug 330815.

Sometimes (depending on whether there is or isn't whitespace betwen the paragraphs), when moving the caret into a pargraph starting [ending] with text of reverse direction (relative to that pargraph), the caret moves to the wrong side of the reverse-direction text (that is, not to the visual start/end of the paragraph). 

In bug 330815, comment #6, I wrote about this:

> As far as I can tell, there are two hint-related problems:
>
> 1. The hint is flipped for each line-jump, so in the case of unrendered
> whitespace between two paragraphs, a single step involves two line-jumps, which
> results in two hint-flips, which is wrong. The solution to this is fairly
> simple: the hint should be flipped (in GetFrameForDirection) if the amount is
> eSelectCharacter, but not if it is eSelectNoAmount (indicating that we already
> jumped a line).
> This problem is not bidi-related, but in practice I couldn't come up with a
> non-bidi case which shows a visible bug due to this.
> 
> 2. For bidi, it seems that the correct rule is to flip the hint if the final
> destination frame is a reverse-direction frame (relative to its own paragraph).
> The current code flips the hint for a reverse-direction frame when the amount
> is eSelectCharacter, (which is not necessarily the last step), and also flips
> it back during a line jump if we moved to a frame with an opposite direction.

I'll attach a testcase soon.

Comment 1

[Uri Bernstein]

Attachment: testcase

Use in caret browsing mode or composer.

Notice that in some cases whitespace between the paragraphs is what's causing the problem, and in other cases it's the lack of such whitespace.

Comment 2

[Uri Bernstein]

Attachment: patch

Enough with the hint-flipping madness!

It took me a while, but finally I realized that if we can count on PeekOffset to return the correct frame in mResultFrame, we can use that to easily infer the correct hint. It turned out that usually this was true, except for one case where PeekOffset didn't return immediately after calling itself recursively, but continued to override the correct frame. I couldn't think of any reason why that was correct, so I changed it.
The hint is now determined solely by nsSelection::MoveCaret().

I'd really like to altogether remove mPreferLeft from nsPeekOffsetStruct, but there seem to be other unrelated methods using it, so I'm leaving it there for now.

Comment 3

[Uri Bernstein]

The patch also fixes bug 328656, as expected.

Comment 4

[Simon Montagu :smontagu]

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

Sweet! As you will have seen in the comments you removed, I wanted to do exactly this in the original Bidi check-in, but I didn't have the guts.

Comment 5

[Uri Bernstein]

Attachment: patch v.1.1

After some more testing, I found out that in the eSelectLine (up/down) case, mResultFrame might be an inline containing the target frame, not the actual text frame. So I need to use nsBidiPresUtils::GetFrameEmbeddingLevel instead of just NS_GET_EMBEDDING_LEVEL.

Also, removed preferLeft completely from nsFrame::GetFrameFromDirection().

Comment 6

[Uri Bernstein]

(In reply to comment #5)
 
> Also, removed preferLeft completely from nsFrame::GetFrameFromDirection().

Or at least I meant to. I'll do this before checkin, if this patch gets r/sr. 

Comment 7

[Uri Bernstein]

Comment on attachment 216306 [details] [diff] [review]
patch v.1.1

No, sorry. This was not a perfect solution to the up/down-into-inline problem, because the hint calculation is now wrong. I guess I'll have to revert to the previous way of doing things for everything but left/right, where I know I can rely on mResultFrame.

Comment 8

[Uri Bernstein]

Attachment: patch v2

Like I said above:
- For left/right, use frame and offset to determine hint
- For up/down and home/end, use offset and hint to determine frame (as before).

Comment 9

[Uri Bernstein]

Checked in:
Checking in layout/generic/nsSelection.cpp;
/cvsroot/mozilla/layout/generic/nsSelection.cpp,v  <--  nsSelection.cpp
new revision: 3.222; previous revision: 3.221
done
Checking in layout/generic/nsTextFrame.cpp;
/cvsroot/mozilla/layout/generic/nsTextFrame.cpp,v  <--  nsTextFrame.cpp
new revision: 1.560; previous revision: 1.559
done
Checking in layout/generic/nsFrame.cpp;
/cvsroot/mozilla/layout/generic/nsFrame.cpp,v  <--  nsFrame.cpp
new revision: 3.635; previous revision: 3.634
done