228290 - Incorrect selection/caret movement when crossing cell boundaries in bidirectional tables

Status: NEW

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

Description

[Greg Hewgill]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.5) Gecko/20031007 Firebird/0.7
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.5) Gecko/20031007 Firebird/0.7

When using Shift+Arrow keys to select text in the presence of dir="rtl" text,
the behaviour of the selection boundary movement is strange. For example, after
clicking in the Arabic text in the url mentioned above, repeatedly hitting
Shift+Left will select toward the end of the Arabic text (to the left), but then
will wrap around to the end of the word "Arabic", and repeat. Similarly,
Shift+Right will select toward the beginning of the Arabic text, then suddenly
the selection will jump to the beginning of the document. 

Reproducible: Always

Steps to Reproduce:
1. Click once on Arabic text.
2. Use Shift+Left or Shift+Right to select text.
3. Continue until an edge of the Arabic text is reached.

Actual Results:  
The selection boundaries jump around in unexpected ways depending on which
direction you're going.

Expected Results:  
When moving left, the beginning selection boundary should move up to the end of
the previous line. When moving right, the end selection boundary should continue
into the word "Arabic". This behaviour would match click-and-drag mouse
movements in the same direction as the arrow keys.

Comment 1

[Shoshannah Forbes]

Prog: you have been dealing with similar bug. Can you confirm or dup this one?

Comment 2

[Prognathous]

I can only think of two bugs that I opened and might be related:

Bug 207186 - "Moving the caret in BiDi texts is buggy, especially in
right-to-left forms"
Bug 209430 - "Ctrl+Delete and Ctrl+BackSpace delete words in the wrong direction"

BTW, I did manage to reproduce this behavior using the latest 1.6b nightly, but
I'm not sure that it's not by design. Mozilla doesn't use the same algorithm as
Windows for arrowing the caret through BiDi texts. Ask Simon Montagu for more
details.

Prog.
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.6b) Gecko/20031212

Comment 3

[Prognathous]

Actually, this bug does have similarities with the first testcase that I
provided in bug 207186, comment 1 (-> the "endless loop").

Prog.

Comment 4

[Jesiah S]

I see this also on LInux 2003122007

Comment 5

[Prognathous]

After creating a reduced testcase of this behavior, I found out that it is not
about text selection, but more about caret movement - quite similar to the
behavior described in bug 207186, comment 1.

I will now add this testcase there, and confirm this one as a NEW bug that
depends on a fix for 207186.

Also, Layout/BiDi seems to be the more fitting Component for this bug.

Prog.

Comment 6

[Uri Bernstein]

Attachment: testcase

This is easiest to see in Caret Browsing mode (use F7):

Place the caret at the very left of the RTL cell, and press the left arrow.
Expected: The caret moves out of the table, to the right of "228290".
Observed: The caret moves to the very left of the LTR cell.

Place the caret at the very right of the RTL cell, and press the right arrow.
Expected: The caret moves to the very left of the LTR cell.
Observed: The caret moves to the beginning of the document(!)

Comment 7

[Uri Bernstein]

Seeing on OS X => All/All.

Comment 8

[Prognathous]

Ginn, please see comment #6.

Thanks,

Prog.

Comment 9

[Ginn Chen]

Prog,

I think it is a dup of 207186.

Comment 10

[Uri Bernstein]

(In reply to comment #9)
> Prog,
> 
> I think it is a dup of 207186.

No, I don't think it's a dup of any of the issues currently reported in 207186,
and I'd rather keep it separate instead of it getting lost as comment #456 or
something on that bug.

Also - assigning to myself (this looks fun, I'll give it a try).

Comment 11

[Uri Bernstein]

Attachment: patch

This patch contains two separate fixes, both required to fully fix this bug.

The main fix is in nsFrameTraversal.cpp, to nsVisualIterator.
In order for the iterator to really be visual, the direction of traversing the
(logical) frame tree must be reversed at points where the directionality of the
"current" frame is opposite to the directionality of the original frame. This
affects two places in Next(): selecting the next (visual) sibling becomes
selecting the previous sibling if the direction is reversed, and selecting the
first child becomes selecting the last child. For Prev() the situation is
reversed. Confused? So was I. But I believe I got it right eventually.

The second fix, in nsFrameList.cpp, is much simpler. The visual methods should
treat the various types of table-related frames like they treat block frames
(i.e., just return the prev/next logical sibling). There might be more types of
frames which need to be handled this way, and there probably is a better way of
doing this (without enumerating all these types). However, I couldn't find one.

Comment 12

[Uri Bernstein]

Attachment: patch v. 2

Actually, we need to decide on the "basic" direction once, based on the
direction of the initial element - not the current element whenever Next() or
Prev() are called.
The previous patch caused lockups in certain situations, and worked poorly in
others.

Comment 13

[Uri Bernstein]

Attachment: patch v. 2

Errr... sorry about that. The patch I posted a few minutes ago included a fix
for another bug.

Comment 14

[Uri Bernstein]

Attachment: patch v. 2.1

I realized that this also fixes the underlying problem in bug 274857 and bug
295142, so this version of the patch also removes Ginn's workaround for these
two bugs.

Comment 15

[Uri Bernstein]

Attachment: patch v. 3

Changes from v. 2.1:

- Added comments explaining how nsVisualIterator::Next/Prev() work.
- Do the "drill down to first (last) descendant" part of the algorithm
visually:
-- Take into account the direction of each parent node on the way.
-- Added methods to nsFrameList for getting the first and last visual frames in
the list, and used them during the drilling down.
- Use IsContainingBlock() to determine whether a frame is a block (or
table-related) frame.
- Fix the problem described in bug 309432 comment #4: In GetNextVisualFor(),
only return the next logical sibling if the current frame *and the next frame*
are both block frames (same for GetPrevVisualFor, current *and previous*
frame).
- Various nits picked by Simon on IRC

Comment 16

[Uri Bernstein]

Comment on attachment 196913 [details] [diff] [review]
patch v. 3

Mmm... no. I obviously need to get a better understanding of how how bidi
really works before continuing to work on this.

Comment 17

[Uri Bernstein]

Better summary.

Comment 18

[Uri Bernstein]

Attachment: better testcase

This testcase is more similar to the original page this bug was reported to, and is therefore better than the artificial testcase I attached earlier.

Comment 19

[Uri Bernstein]

(In reply to comment #0)
> Shift+Right will select toward the beginning of the Arabic text, then suddenly
> the selection will jump to the beginning of the document. 

This problem, which might be the only actual valid bug here, was fixed by the patch for bug 303884 (and its follow-up, bug 330284).

As for the other issues reported, I now tend to consider them INVALID, or at least WONTFIX.
Our caret and selection behaviour can be summarized as "visual inside blocks, logical between blocks", that is, moving forwards past the end of a block, the caret (or selection), always arrives at the next (logical) block; moving backwards past the beginning of a block, the caret always arrives at the previous block. (Exactly where in the previous/next block the caret ends up is something that is going to be changed by the patch to bug 330815, if it is approved).
This is what happens here when you move left in the RTL (Arabic) cell. When you move left (forwards) past the leftmost (last) letter, the caret/selection moves into the next cell, which is the one containing the word "Arabic".
Similarly, when moving right (backwards) past the rightmost (first) Arabic letter, the caret/selection moves into the previous cell ("English").

The question is whether we want to change this behaviour in the special case where the blocks in question are table cells. I tend to think the answer is "no", but I'll leave this open for discussion.

Comment 20

[u336495]

is selecting 2 farsi lines at this page the same problem? or should I file another bug?

http://ubiquity.mozilla.com/trac/wiki/TracUnicode

Comment 21

[Simon Montagu :smontagu]

This bug is about tables, so that is a different problem. (It seems to be mostly caused by the Persian lines not having right-to-left direction set, so I'm not sure that it is a bug in Firefox at all)