Bidi:After deleting last character of reverse-direction text, arrow keys move caret incorrectly

RESOLVED FIXED in mozilla1.8.1

Status

()

RESOLVED FIXED
13 years ago
10 years ago

People

(Reporter: uriber, Assigned: uriber)

Tracking

({fixed1.8.1})

Trunk
mozilla1.8.1
fixed1.8.1
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 6 obsolete attachments)

(Assignee)

Description

13 years ago
When LTR text is embedded withing RTL text in an RTL textarea, when deleting the
last character of the LTR text, and then using an arrow key to move the caret,
the caret moves as if it were previously located at the other side (i.e. the
beginning) of the LTR text.

The same is true when switching "LTR" and "RTL" in the description above.

This is *not* a regression (not since Firefox 1.0, anyway).

A testcase will follow soon.
(Assignee)

Comment 1

13 years ago
Created attachment 193730 [details]
testcase

In the first (RTL) textarea:
- Position the caret inside the word "English".
- Use the right-arrow key to move to the right of the "h"
- Press "backspace" to delete the "h". The caret will be correctly placed to
the right of the "s".
At this point:
- Pressing the right arrow will move the caret to between the "E" and the "n"
- Pressing the left arrow will move the caret to the left of the space left of
the  "E"
- Pressing the down arrow will move the caret to below the position left of the
"E".

The second (LTR) case is symmetrical. Move from within the Hebrew word to the
left of leftmost Hebrew letter and press "backspace" to see the same effect.
(Assignee)

Comment 2

13 years ago
> This is *not* a regression (not since Firefox 1.0, anyway).

Technically, this *is* a regression, from bug 92124 (so it regressed shortly
before Mozilla 1.0).

Assigning to me, I'll see what I can do.
Assignee: mozilla → uriber
(Assignee)

Comment 3

13 years ago
Created attachment 193830 [details] [diff] [review]
patch v1.0

This is a general fix for this bug, as well as several other similar
(non-reported) bugs where, upon moving the caret, it moves from a different
position than that in which it previously appeared. (I can supply testcases and
descriptions of the other bugs, if required).

The idea is to make nsSelection::MoveCaret() go through the bidi caret
positioning algorithm when determining where the caret is initially (before the
move). This ensures that any movement will be based upon the current place
where the caret is actually drawn.

I don't really like the fact that I had to add the extra method to nsICaret.h
(in addition to nsCaret.h) so if you have an idea how to avoid it I'll be happy
to hear.
Attachment #193830 - Flags: review?(smontagu)
(Assignee)

Comment 4

13 years ago
Comment on attachment 193830 [details] [diff] [review]
patch v1.0

Note: need to add 
#include "nsIFrameSelection.h"
To nsICaret.h for this to actually compile.
Attachment #193830 - Attachment description: patch → patch [not for checkin!]
(Assignee)

Comment 5

13 years ago
Comment on attachment 193830 [details] [diff] [review]
patch v1.0

This patch won't apply now because of the fix to bug 307533. I'm working on an
updated version.
Attachment #193830 - Attachment description: patch [not for checkin!] → patch v1.0
Attachment #193830 - Attachment is obsolete: true
Attachment #193830 - Flags: review?(smontagu)
(Assignee)

Comment 6

13 years ago
Created attachment 195575 [details] [diff] [review]
patch v2.0

Changes from the previous patch:

- Fixed the arrow-up/down case (which was not fixed by the prevoius patch) by
using GetCaretFrameForNodeOffset() also in nsCaret::GetCaretCoordinates().
- Adjusted for bug 307533.
- GetCaretFrameForNodeOffset() is now side-effect free:
-- It does not set mLastBidiLevel, which is now handled similarly to the other
"last" values. [this is probably the biggest change here]
-- It does not call presShell->SetCaretBidiLevel() in the BIDI_LEVEL_UNDEFINED
case. I believe this is unnecessary.
- Added the missing #include in nsICaret.h
Attachment #195575 - Flags: review?(smontagu)
(Assignee)

Comment 7

13 years ago
Forgot to say:
I also removed some "#ifdef IBMBIDI"s, because adding them everywhere around the
bidiLevel vstuff would have made this terribly ugly, and I got the impression we
want to get rid of them anyway.
(Assignee)

Comment 8

13 years ago
Created attachment 195621 [details] [diff] [review]
patch v2.1

SetCaretBidiLevel is back, but now outside GetCaretFrameForNodeOffset().
Some more nits, picked by Simon.
Attachment #195575 - Attachment is obsolete: true
Attachment #195621 - Flags: review?(smontagu)
(Assignee)

Updated

13 years ago
Attachment #195575 - Flags: review?(smontagu)
Comment on attachment 195621 [details] [diff] [review]
patch v2.1

One nit I missed on IRC: you should change the comment
 // if there is a frameAfter, move into it, setting HINTRIGHT to make sure we
stay there
as you did for the parallel comment with HINTLEFT
Attachment #195621 - Flags: review?(smontagu) → review+
(Assignee)

Comment 10

13 years ago
Created attachment 195626 [details] [diff] [review]
patch v2.1.1

Updated per Simon's last comment. Carrying over r+.
Attachment #195621 - Attachment is obsolete: true
Attachment #195626 - Flags: superreview?(roc)
Attachment #195626 - Flags: review+
(Assignee)

Comment 11

13 years ago
Created attachment 195627 [details] [diff] [review]
patch v2.1.1

D'oh! This time did it right (I hope).
Attachment #195627 - Flags: superreview?(roc)
Attachment #195627 - Flags: review+
(Assignee)

Updated

13 years ago
Attachment #195626 - Attachment is obsolete: true
Attachment #195626 - Flags: superreview?(roc)
Attachment #195626 - Flags: review+
(Assignee)

Updated

13 years ago
Status: NEW → ASSIGNED
(Assignee)

Comment 12

13 years ago
Created attachment 201459 [details] [diff] [review]
patch v2.2

This is an unbitrotten version of the patch, plus some fixes to error handling in nsTypedSelection::GetPrimaryFrameForFocusNode() (which I did pretty poorly with the previous patches).

So, Simon - you only need to review GetPrimaryFrameForFocusNode(). Everything else should be identical to v2.1.1.
Attachment #195627 - Attachment is obsolete: true
Attachment #201459 - Flags: superreview?(roc)
Attachment #201459 - Flags: review?(smontagu)
Attachment #195627 - Flags: superreview?(roc)

Updated

13 years ago
Attachment #201459 - Flags: review?(smontagu) → review+
Attachment #201459 - Flags: superreview?(roc) → superreview+
(Assignee)

Comment 13

13 years ago
Created attachment 203237 [details] [diff] [review]
patch v2.2.1

Same as v2.2, updated for changes made in nsICaret.h.
Attachment #201459 - Attachment is obsolete: true
(Assignee)

Comment 14

13 years ago
Fix checked in by smontagu, 2005-11-16 01:37
Status: ASSIGNED → RESOLVED
Last Resolved: 13 years ago
Resolution: --- → FIXED
(Assignee)

Updated

13 years ago
Target Milestone: --- → mozilla1.9alpha
(Assignee)

Updated

13 years ago
Attachment #203237 - Flags: approval1.8.1?
(Assignee)

Updated

13 years ago
Attachment #203237 - Flags: approval1.8.1? → branch-1.8.1?(roc)
Attachment #203237 - Flags: approval-branch-1.8.1?(roc) → approval-branch-1.8.1+
(Assignee)

Comment 15

13 years ago
Checked in to 1.8 branch:
Checking in mozilla/layout/base/nsICaret.h;
/cvsroot/mozilla/layout/base/nsICaret.h,v  <--  nsICaret.h
new revision: 1.35.2.1; previous revision: 1.35
done
Checking in mozilla/layout/base/nsCaret.h;
/cvsroot/mozilla/layout/base/nsCaret.h,v  <--  nsCaret.h
new revision: 1.43.2.2; previous revision: 1.43.2.1
done
Checking in mozilla/layout/base/nsCaret.cpp;
/cvsroot/mozilla/layout/base/nsCaret.cpp,v  <--  nsCaret.cpp
new revision: 1.144.2.8; previous revision: 1.144.2.7
done
Checking in mozilla/layout/generic/nsSelection.cpp;
/cvsroot/mozilla/layout/generic/nsSelection.cpp,v  <--  nsSelection.cpp
new revision: 3.199.2.7; previous revision: 3.199.2.6
done
Keywords: fixed1.8.1
Target Milestone: mozilla1.9alpha → mozilla1.8.1

Updated

12 years ago
Depends on: 345371

Updated

10 years ago
Component: Layout: BiDi Hebrew & Arabic → Layout: Text
QA Contact: zach → layout.fonts-and-text
You need to log in before you can comment on or make changes to this bug.