330460 - Bidi: After moving back to end of line ending with reverse-direction text, "backspace" misbehaves

Status: RESOLVED FIXED

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

Description

[Uri Bernstein]

In an LTR textarea, when a line ends with RTL text, using left-arrow to move into the end of the line, and then pressing backspace, results in the last letter of the RTL text being seleted, and the caret remaining at the end (right) of the line.

Expected: no deletion should actually happen, the caret should move to the left of the RTL text.

The symmetric case (RTL textarea) also has the same problem, but only since the fix to bug 303884 has landed. In other words, fixing bug 303884 caused the two cases to be symmetrical (and broken).

Annotated testcase coming up.

Comment 1

[Uri Bernstein]

Attachment: testcase

Instructions included.

Comment 2

[Uri Bernstein]

Attachment: testcase #2

This is an HTML testcase (for Composer), showing a similar problem with an inline container instead of a line break.
These problems are the result of the "more fundamental problems" mentioned by Simon in bug 303781 comment #5.

Comment 3

[Uri Bernstein]

Attachment: patch

So, after trying to think what CheckBidiLevelForDeletion() should be doing, I realized that we already have a method which does exactly that (I thought) - namely, nsSelection::GetPrevNextBidiLevels(). I had to jump trough some hoops in order to actually use it, but once I did it, this bug (as well as bug 330457) were fixed.
But then I discovered that nsSelection::GetPrevNextBidiLevels() doesn't look beyond line boundaries, which caused the LTR testcase for bug 303781 to break.
Removing the line boundary restriction caused havoc with caret positioning, so I had to add a parameter (aJumpLines) to GetPrevNextBidiLevels to specify which of the behaviors I want (it's false everywhere, except for the new callee).

Comment 4

[Uri Bernstein]

Attachment: same patch, diff -w

Since most of the change in GetPrevNextBidiLevels() is indentation, here's apatch created with diff -w.

Comment 5

[Uri Bernstein]

Attachment: patch

oops, forgot to include nsTextEditRules.h.

Comment 6

[Uri Bernstein]

Attachment: patch v2 (diff -w)

Take care of beginning/end of document (or, actually, beginning/end of scrollframe) cases.

Comment 7

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 215755 [details] [diff] [review]
patch v2 (diff -w)

+  NS_IMETHOD GetPrevNextBidiLevels(nsPresContext *aPresContext, nsIContent *aNode, PRUint32 aContentOffset, PRBool aJumpLines,

Please document the new parameter.

Comment 8

[Uri Bernstein]

Checked in, with new parameter documented:
Checking in editor/libeditor/text/nsTextEditRulesBidi.cpp;
/cvsroot/mozilla/editor/libeditor/text/nsTextEditRulesBidi.cpp,v  <--  nsTextEditRulesBidi.cpp
new revision: 1.19; previous revision: 1.18
done
Checking in editor/libeditor/text/nsTextEditRules.h;
/cvsroot/mozilla/editor/libeditor/text/nsTextEditRules.h,v  <--  nsTextEditRules.h
new revision: 1.80; previous revision: 1.79
done
Checking in editor/libeditor/text/nsTextEditRules.cpp;
/cvsroot/mozilla/editor/libeditor/text/nsTextEditRules.cpp,v  <--  nsTextEditRules.cpp
new revision: 1.203; previous revision: 1.202
done
Checking in editor/libeditor/html/nsHTMLEditRules.cpp;
/cvsroot/mozilla/editor/libeditor/html/nsHTMLEditRules.cpp,v  <--  nsHTMLEditRules.cpp
new revision: 1.323; previous revision: 1.322
done
Checking in layout/base/nsIFrameSelection.h;
/cvsroot/mozilla/layout/base/nsIFrameSelection.h,v  <--  nsIFrameSelection.h
new revision: 1.69; previous revision: 1.68
done
Checking in layout/base/nsCaret.cpp;
/cvsroot/mozilla/layout/base/nsCaret.cpp,v  <--  nsCaret.cpp
new revision: 1.153; previous revision: 1.152
done
Checking in layout/generic/nsSelection.cpp;
/cvsroot/mozilla/layout/generic/nsSelection.cpp,v  <--  nsSelection.cpp
new revision: 3.219; previous revision: 3.218
done