299535 - BiDi: When pressing Ctrl+Arrow, the caret skips words with oppposite direction (or stops at the end of the word if it hits a punctuation mark)

Status: RESOLVED FIXED

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

Description

[Prognathous]

1. Open the attached testcase
2. Put the caret at the beginning of the first line (RTL textarea)
2. Press Ctrl+LeftArrow twice.

Actual Result:
The caret skips the third word and is placed at the beginning of the fourth
word.

Expected result:
The caret should move to the beginning of the third word.

The same problem happens with the LTR textarea, use Ctrl+RightArrow instead of
Ctrl+LeftArrow to test.

Tested with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b2)
Gecko/20050702 Firefox/1.0+

First reported in Bug 207186, comment #15

Similarly, pressing Ctrl+Delete twice (in step #3) deletes the first three
words, instead of just the first two. Should this be a separate bug?

Prog.

Comment 1

[Prognathous]

Attachment: Testcase E

Comment 2

[Prognathous]

One additional note: If a punctuation mark appears after the word with opposite
direction, the caret stops there. It's still a bug, as the caret should stop at
the beginning of the word rather than the end of it.

To test this, add a period or a comma after the third word in the first line
(i.e. "word," instead of "word"). You can then verify that the caret is at the
end of the word by typing an English character.

Prog.

Comment 3

[Eyal Rozenberg]

Attachment: patch

See the inline comments.

Comment 4

[Eyal Rozenberg]

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

Like I said in code, it's a poor hack rather than a good solution, but I think
the only real solution to all of this is  bug 300131.

Comment 5

[Eyal Rozenberg]

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

Whoops, wrong diff, let me try to get that in order

Comment 6

[Eyal Rozenberg]

Attachment: patch v2

Ok, things should be straightened out now, hopefully.

Comment 7

[Eyal Rozenberg]

Comment on attachment 188866 [details] [diff] [review]
patch v2

Might as well change the review request here as well.

Comment 8

[Uri Bernstein]

Severety: major => normal

Comment 9

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

I think we should focus on bug 300131 rather than land this, unless you think
this bug is important enough to be fixed on the 1.8 branch.

Comment 10

[Eyal Rozenberg]

Comment on attachment 188866 [details] [diff] [review]
patch v2

No, I think this is as important as the rest of the BiDi caret movement bugs,
and it is better to start a refactoring when the existing code has the minimum
amount of bugs, and the maximum amount of explanatory comments (providing
they're not misleading of course :-) ... )


However, the patch needs to change to reflect the check-in of 299239 and the
fix for the regression in bug 305083 (which I've just posted).

So I'm asking you for an SR for the patch the way it is, and when I repost it
I'll move the SR and ask Uri Bernstein for the R.

Comment 11

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

(In reply to comment #10)
> (From update of attachment 188866 [details] [diff] [review] [edit])
> No, I think this is as important as the rest of the BiDi caret movement bugs,
> and it is better to start a refactoring when the existing code has the minimum
> amount of bugs, and the maximum amount of explanatory comments (providing
> they're not misleading of course :-) ... )

That's not correct, because we'll never know whether the existing code has the
minimum amount of bugs. The fact that many of these fixes have caused
regressions suggests that we may in fact already be close to the minimum...

Comment 12

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

Plus, fixing loads of bugs in code you're going to throw away usually isn't
worth it.

Comment 13

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

Comment on attachment 188866 [details] [diff] [review]
patch v2

I'll take a look at the new patch very soon after you post it, I promise.

Comment 14

[Uri Bernstein]

(In reply to comment #12)
> Plus, fixing loads of bugs in code you're going to throw away usually isn't
> worth it.

My take on this is that the word-movement code is pretty much hopeless, and will
require a rewrite. So fixing bugs in it for 1.8 might not be a good use of our time.

I do not think this applies to the rest of the caret movement code, though. I
think generally, this will only require refactoring, not rewriting, so that any
bugs we fix now will benefit the 1.9 code.

Comment 15

[Eyal Rozenberg]

(In reply to comment #14)
> My take on this is that the word-movement code is pretty much hopeless

I won't press the matter, but landing this patch won't make things worse w.r.t.
the hopelessness, plus the comment therein is just as important as the patch
itself because it describes a scenario which needs to be taken into
consideration, either by the existing or during a rewrite. And as for the
rewrite/refactoring - it might take a a short while, or a longer one. Look at
the table borders code for RTL, or the MIME code, for example.

Comment 16

[Uri Bernstein]

For the record, I'm not in any way against accepting this patch, certainly not
against accepting it to the trunk.

As for the branch, it's a question of risk management, and my opinion probably
wouldn't matter even if I had one.

Comment 17

[Uri Bernstein]

Attachment: patch (different approach) (diff -w)

The problem here is that the "wordSelectEatSpaceAfter" behavior is only implemented inside the |if (movementIsInFrameDirection)| block, that is, when moving forward /relative to the current frame/.
This is wrong: since we're dealing with visual movement, when eating spaces after words, "after" should mean "to the right of" in LTR paragraphs, and "to the left of" in RTL paragraphs - regardless of the direction of the word we're skipping.

This patch achieves this by:
1. setting wordSelectEatSpaceAfter to true only if the direction is eDirNext, i.e. if we're moving forward /relative to the paragraph/.
2. copying the entire logic (which deals with wordSelectEatSpaceAfter) from the   |if (movementIsInFrameDirection)| to the |if (!movementIsInFrameDirection)| block, so that these are now completely symmetrical.

Comment 18

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

Comment on attachment 217264 [details] [diff] [review]
patch (different approach) (diff -w)

rubber-stamp!

Comment 19

[Uri Bernstein]

Checked in:
Checking in layout/generic/nsTextFrame.cpp;
/cvsroot/mozilla/layout/generic/nsTextFrame.cpp,v  <--  nsTextFrame.cpp
new revision: 1.566; previous revision: 1.565
done