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.
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.
> 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
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.
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.
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
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.
Created attachment 195621 [details] [diff] [review] patch v2.1 SetCaretBidiLevel is back, but now outside GetCaretFrameForNodeOffset(). Some more nits, picked by Simon.
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+
Created attachment 195626 [details] [diff] [review] patch v2.1.1 Updated per Simon's last comment. Carrying over r+.
Created attachment 195627 [details] [diff] [review] patch v2.1.1 D'oh! This time did it right (I hope).
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 #201459 - Flags: review?(smontagu) → review+
Attachment #201459 - Flags: superreview?(roc) → superreview+
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
Fix checked in by smontagu, 2005-11-16 01:37
Status: ASSIGNED → RESOLVED
Last Resolved: 13 years ago
Resolution: --- → FIXED
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: 188.8.131.52; previous revision: 1.35 done Checking in mozilla/layout/base/nsCaret.h; /cvsroot/mozilla/layout/base/nsCaret.h,v <-- nsCaret.h new revision: 184.108.40.206; previous revision: 220.127.116.11 done Checking in mozilla/layout/base/nsCaret.cpp; /cvsroot/mozilla/layout/base/nsCaret.cpp,v <-- nsCaret.cpp new revision: 18.104.22.168; previous revision: 22.214.171.124 done Checking in mozilla/layout/generic/nsSelection.cpp; /cvsroot/mozilla/layout/generic/nsSelection.cpp,v <-- nsSelection.cpp new revision: 126.96.36.199; previous revision: 188.8.131.52 done
Target Milestone: mozilla1.9alpha → mozilla1.8.1
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.