330815 - Bidi: Caret moves incorrectly when moving between HTML paragraphs of reverse direction, with multiple frames in the first/last line

Status: RESOLVED FIXED

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

Description

[Uri Bernstein]

When a LTR paragraph is followed by an RTL paragraph, and the RTL paragraph has multiple frames on the first line (generated, e.g., by inline cotainers such as <b>), pressing right-arrow at the very end of the LTR paragraph moves the caret to the left edge of the rightmost frame of the first line of the RTL paragraph.

The expected behaviour in this case is a matter for debate, but the approach currently implemented in Mozilla (when there are no inlines), is to move the caret to the left edge of the first line of the RTL paragraph. The other possibility (implemented e.g. in MS Word) would be to move the caret to the right edge of the first line.

This problem occurs also in the other cases of moving the caret between reverse-direction paragraphs (from RTL forwards to LTR, from LTR backwards to RTL, and from RTL backwards to LTR). When moving backwards, the problem naturally depend on the existance of multiple frames on the last line of the preceding paragraph.

This is a regression from my fix for bug 288789.

Testcase coming up.

Comment 1

[Uri Bernstein]

Attachment: testcase

In caret browsing mode or Composer:

Place the caret at the end of the first (LTR) paragraph and press right-arrow. The caret moves into the first line of the RTL paragraph, after (to the left of) the two bolded words. 

Place the caret at the beginning (top right) of the second (RTL) paragraph and press right-arrow. The caret moves to before the words "United States of America".

Comment 2

[Uri Bernstein]

(In reply to comment #0)
>
> This is a regression from my fix for bug 288789.
>

Actually, it isn't. I saw that it regressed between Fx 1.0 and 1.5, so I assumed it's a regression from that bug. But actually, it regressed between 2004-11-25 and 2004-12-06, indicating that it's a regression from bug 256835.

bug 288789 was also a regression from the same bug, somewhat similar to this one. However, fixing it left the current issue unfixed.

Comment 3

[Uri Bernstein]

Attachment: patch

This is based on the patch I posted earlier on bug 330627. It fixes this bug as well as that one, and I thought it would be better for it to be attached to a bug with a more meaningful summary.

What this patch basically does is to change the meaning of eDirNext (in PeekOffset and GetFrameFromDirection) to "right and down in an LTR paragraph, left and down in an RTL paragraph".
Prior to this patch, eDirNext meant "right and down in an LTR paragraph, right and up in an RTL paragraph."
The semantics introduced by this patch match those of the post-bug 288789 nsVisualIterator, making the code outside the iterator much simpler.

By pushing the logic saying "left-arrow means 'prev' in LTR, 'next' in RTL" up to nsSelection::MoveCaret, I safely avoid any loops in PeekOffset and GetFrameFromDirection. This ensures that infinite-loop issues like bug 274857, bug 295142 and bug 330627 will not happen. It also means that the "Bidi Ghost Frames" mechanism can really be removed, since the mechanism described in bug 330627 comment #9 now works in the bidi case just as well as it does in the LTR case.

What this means, though, is that the behavior between reverse-direction paragraphs is now "the other possibility" mentioned in the 2nd paragraph of comment #0. I believe that the existing intended behaviour is inherently bug-prone and difficult to implement. Specifically, it won't allow me to do what I did in this patch. I also happen to thing that the behavior introduced by this patch is actually better, and it seems to also be Microsoft-compatible (OS X is terribly buggy and inconsistent on this matter, so I can't say what its intended behavior is).

I tested this patch for regressions on all the fixed dependencies of bug 207186, and found none.
Although this doesn't actually fix any of the bidi word-selection/word-movement bugs, I hope that by simplifying the code this is making those bugs easier to fix.

Comment 4

[Uri Bernstein]

Attachment: patch (diff -b)

patch with diff -b. Also very slightly updated (removed two more lines dealing with eSelectDir, which is useless as far as I can tell).
Simon - please use this one for reviewing.

Comment 5

[Uri Bernstein]

With the patch, there is still a hint-related issue when moving into a paragraph (note: an HTML block, not just a line), starting/ending with reverse-direction text. The bahaviour seems to depend on whether there are any empty/unrendered frames between the paragraphs.

I'll try to figure out the hint issue (I suspect the logic here can be simplified as well), but I think this is minor enough that it could be pushed off to a follow-up bug.

Notice that this problem does not affect plaintext editors at all.

Comment 6

[Uri Bernstein]

So, 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.

Still, given the relative complexity of both the current patch and the hint-related change, and given the fact that the current patch leaves the system in an "almost correct" state, I'd rather do the hint fixes in a separate patch.

Comment 7

[Uri Bernstein]

Attachment: patch v1.0.1 (diff -b)

No real code changes, only changes are:
- Update to trunk tip (for bug 330881)
- Changed a comment in nsTextFrame::PeekOffset to match the code changes
- Removed a comment on nsFrame::GetFrameFromDirection which no longer makes sense

Comment 8

[Uri Bernstein]

Attachment: patch v2 (diff -b)

At Simon's advice, leave the eSelectDir stuff alone for now.

We probably want to get rid of eSelectDir altogether, as it currently doesn't perform any useful function. I'll file a follow-up bug for that if/when this patch lands.

Comment 9

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

Comment on attachment 215775 [details] [diff] [review]
patch v2 (diff -b)

I won't pretend to understand what's going on here, but it's a net removal of code so I'm for it!

Comment 10

[Uri Bernstein]

Checked in.

I will post followup bugs about the issues mentioned in comment 6 (sorting out hints), and comment 8 (getting rid of eSelectDir).

Checking in layout/generic/nsSelection.cpp;
/cvsroot/mozilla/layout/generic/nsSelection.cpp,v  <--  nsSelection.cpp
new revision: 3.220; previous revision: 3.219
done
Checking in layout/generic/nsTextFrame.cpp;
/cvsroot/mozilla/layout/generic/nsTextFrame.cpp,v  <--  nsTextFrame.cpp
new revision: 1.554; previous revision: 1.553
done
Checking in layout/generic/nsFrame.cpp;
/cvsroot/mozilla/layout/generic/nsFrame.cpp,v  <--  nsFrame.cpp
new revision: 3.632; previous revision: 3.631
done