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

NEW
Unassigned

Status

()

Core
Layout: Text
--
minor
14 years ago
7 years ago

People

(Reporter: Greg Hewgill, Unassigned)

Tracking

Trunk
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(1 attachment, 6 obsolete attachments)

(Reporter)

Description

14 years ago
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

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

Comment 2

14 years ago
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

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

Prog.

Comment 4

14 years ago
I see this also on LInux 2003122007

Comment 5

14 years ago
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.
Status: UNCONFIRMED → NEW
Component: Internationalization → Layout: BiDi Hebrew & Arabic
Depends on: 207186
Ever confirmed: true
Created attachment 187514 [details]
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(!)
Seeing on OS X => All/All.
OS: Windows 2000 → All
Hardware: PC → All

Comment 8

13 years ago
Ginn, please see comment #6.

Thanks,

Prog.

Comment 9

13 years ago
Prog,

I think it is a dup of 207186.
(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).
Assignee: smontagu → uriber
Created attachment 187672 [details] [diff] [review]
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.
Attachment #187672 - Flags: review?(smontagu)
Created attachment 188059 [details] [diff] [review]
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.
Attachment #187672 - Attachment is obsolete: true
Attachment #188059 - Flags: review?(smontagu)

Updated

13 years ago
Attachment #187672 - Flags: review?(smontagu)
Created attachment 188061 [details] [diff] [review]
patch v. 2

Errr... sorry about that. The patch I posted a few minutes ago included a fix
for another bug.
Attachment #188059 - Attachment is obsolete: true
Attachment #188061 - Flags: review?(smontagu)

Updated

13 years ago
Attachment #188059 - Flags: review?(smontagu)

Updated

13 years ago
No longer depends on: 207186

Updated

13 years ago
Status: NEW → ASSIGNED
Created attachment 194422 [details] [diff] [review]
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.
Attachment #188061 - Attachment is obsolete: true
Attachment #194422 - Flags: review?(smontagu)

Updated

12 years ago
Attachment #188061 - Flags: review?(smontagu)
Created attachment 196913 [details] [diff] [review]
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
Attachment #194422 - Attachment is obsolete: true
Attachment #196913 - Flags: review?(smontagu)

Updated

12 years ago
Attachment #194422 - Flags: review?(smontagu)
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.
Attachment #196913 - Flags: review?(smontagu) → review-
Better summary.
Summary: Behaviour of shift+arrow keys with rtl text → Incorrect selection/caret movement when crossing cell boundaries in bidirectional tables
Created attachment 215570 [details]
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.
Attachment #187514 - Attachment is obsolete: true
Attachment #196913 - Attachment is obsolete: true
(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.
Assignee: uriber → mozilla
Status: ASSIGNED → NEW
QA Contact: amyy → zach

Updated

12 years ago
Attachment #215570 - Attachment mime type: text/plain → text/html

Updated

9 years ago
Component: Layout: BiDi Hebrew & Arabic → Layout: Text
QA Contact: zach → layout.fonts-and-text

Comment 20

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

http://ubiquity.mozilla.com/trac/wiki/TracUnicode
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)

Updated

7 years ago
Assignee: mozilla → nobody
You need to log in before you can comment on or make changes to this bug.