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

RESOLVED FIXED in mozilla1.8.1

Status

()

defect
RESOLVED FIXED
14 years ago
11 years ago

People

(Reporter: uriber, Assigned: uriber)

Tracking

({fixed1.8.1})

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

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 6 obsolete attachments)

Assignee

Description

14 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

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

14 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

14 years ago
Posted patch patch v1.0 (obsolete) — Splinter Review
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

14 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

14 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

14 years ago
Posted patch patch v2.0 (obsolete) — Splinter Review
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

14 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

14 years ago
Posted patch patch v2.1 (obsolete) — Splinter Review
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

14 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

14 years ago
Posted patch patch v2.1.1 (obsolete) — Splinter Review
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

14 years ago
Posted patch patch v2.1.1 (obsolete) — Splinter Review
D'oh! This time did it right (I hope).
Attachment #195627 - Flags: superreview?(roc)
Attachment #195627 - Flags: review+
Assignee

Updated

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

Updated

14 years ago
Status: NEW → ASSIGNED
Assignee

Comment 12

14 years ago
Posted patch patch v2.2 (obsolete) — Splinter Review
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)
Attachment #201459 - Flags: review?(smontagu) → review+
Attachment #201459 - Flags: superreview?(roc) → superreview+
Assignee

Comment 13

14 years ago
Posted patch patch v2.2.1Splinter Review
Same as v2.2, updated for changes made in nsICaret.h.
Attachment #201459 - Attachment is obsolete: true
Assignee

Comment 14

14 years ago
Fix checked in by smontagu, 2005-11-16 01:37
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Assignee

Updated

14 years ago
Target Milestone: --- → mozilla1.9alpha
Assignee

Updated

14 years ago
Attachment #203237 - Flags: approval1.8.1?
Assignee

Updated

14 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

14 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

13 years ago
Depends on: 345371

Updated

11 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.