Closed Bug 305798 Opened 15 years ago Closed 15 years ago

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

Categories

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

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla1.8.1

People

(Reporter: uriber, Assigned: uriber)

References

Details

(Keywords: fixed1.8.1)

Attachments

(2 files, 6 obsolete files)

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.
Attached 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.
> 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
Attached 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)
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!]
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)
Attached 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)
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.
Attached 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)
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+
Attached 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+
Attached 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+
Attachment #195626 - Attachment is obsolete: true
Attachment #195626 - Flags: superreview?(roc)
Attachment #195626 - Flags: review+
Status: NEW → ASSIGNED
Attached 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+
Attached patch patch v2.2.1Splinter Review
Same as v2.2, updated for changes made in nsICaret.h.
Attachment #201459 - Attachment is obsolete: true
Fix checked in by smontagu, 2005-11-16 01:37
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9alpha
Attachment #203237 - Flags: approval1.8.1?
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+
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
Depends on: 345371
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.