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

RESOLVED FIXED

Status

()

defect
RESOLVED FIXED
13 years ago
11 years ago

People

(Reporter: uriber, Assigned: uriber)

Tracking

Trunk
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 3 obsolete attachments)

Assignee

Description

13 years ago
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.
Assignee

Comment 1

13 years ago
Posted file testcase
Instructions included.
Assignee

Comment 2

13 years ago
Posted file 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.
Assignee

Comment 3

13 years ago
Posted patch patch (obsolete) — Splinter Review
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).
Assignee: mozilla → uriber
Status: NEW → ASSIGNED
Attachment #215176 - Flags: review?(smontagu)
Assignee

Comment 4

13 years ago
Posted patch same patch, diff -w (obsolete) — Splinter Review
Since most of the change in GetPrevNextBidiLevels() is indentation, here's apatch created with diff -w.
Assignee

Updated

13 years ago
Blocks: 330457
Assignee

Comment 5

13 years ago
Posted patch patch (obsolete) — Splinter Review
oops, forgot to include nsTextEditRules.h.
Attachment #215176 - Attachment is obsolete: true
Attachment #215247 - Flags: review?(smontagu)
Attachment #215176 - Flags: review?(smontagu)
Assignee

Comment 6

13 years ago
Take care of beginning/end of document (or, actually, beginning/end of scrollframe) cases.
Attachment #215177 - Attachment is obsolete: true
Attachment #215247 - Attachment is obsolete: true
Attachment #215755 - Flags: review?(smontagu)
Attachment #215247 - Flags: review?(smontagu)
Attachment #215755 - Flags: review?(smontagu) → review+
Assignee

Updated

13 years ago
Attachment #215755 - Flags: superreview?(roc)
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.
Attachment #215755 - Flags: superreview?(roc) → superreview+
Assignee

Comment 8

13 years ago
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
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED

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.