327107 - After using backspace[delete] to remove a single LTR character in RTL text, the next press on backspace[delete] has no effect

Status: RESOLVED FIXED

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

Description

[Uri Bernstein]

After using backspace to remove a single LTR character surrounded by RTL text (or at the end of an RTL paragraph, preceded by RTL text), pressing backspace again has no effect. Pressing backspace once more deletes the RTL character to the left of the caret.

The same is true when using "delete" instead of "backspace".
The same is also true after deleting a single RTL character in LTR text.

Comment 1

[Uri Bernstein]

Attachment: testcase

Self-documented.

Comment 2

[Uri Bernstein]

This behaviour is consistent with the current specification of bidi caret movement. See http://www-306.ibm.com/software/globalization/topics/bidiui/multiline.jsp, under "Backspace and Delete".

To fix it, the spec can be amended in one of two ways:

1. In the section referred to above, in paragraph (b), remove the words "and this level has the same parity as the cursor level".

2. (Suggested by smontagu): add after "After the successful removal of a character by Delete or Backspace, the cursor level must be set to that of the removed character," something to the effect that if neither the character before nor the character after the cursor position is the same level (parity?) as the removed character, set the caret level to the level of the next logical character in the deletion direction.

(2) also covers the case of deleting a single level-2 character between level-1 and level-0 text, but is more difficult to implement, as getting the next logical character in the deletion direction is not trivial.

Comment 3

[Uri Bernstein]

Here's a case which would be fixed by the first solution, but not by the second:
1. Place the caret inside an RTL run (between two RTL characters).
2. Switch to an LTR keyboard layout.
3. Press "backspace".
The "backspace" in step three has no apparent effect.

Back when the caret level affected keyboard layout, step 3 would have the effect of switching the keyboard layout back to RTL. But with that feature disabled, the backspace does nothing.

Comment 4

[Uri Bernstein]

Attachment: patch (alternative 1)

This patch implements the first, simple, solution presented above.
Simon, given comment #3, perhaps we should just go with this?

Comment 5

[Simon Montagu :smontagu]

Comment on attachment 212412 [details] [diff] [review]
patch (alternative 1)

r=me, noting that we really need to go through and look for all cases which don't make sense now that changing caret level doesn't change the keyboard input language.

Nit: while you're there, please make the spacing conform to local style, i.e. 
if (levelBefore == levelAfter)

Comment 6

[Uri Bernstein]

Comment on attachment 212412 [details] [diff] [review]
patch (alternative 1)

I'll fix the spacing when I check in, if/when I get sr.

Comment 7

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

Comment on attachment 212412 [details] [diff] [review]
patch (alternative 1)

rubberstamp!

Comment 8

[Uri Bernstein]

Checked in:
/cvsroot/mozilla/editor/libeditor/text/nsTextEditRulesBidi.cpp,v  <--  nsTextEditRulesBidi.cpp
new revision: 1.16; previous revision: 1.15

Comment 9

[Uri Bernstein]

...and naturally I forgot to do the whitespace fix. I did it in a separate checkin.

Comment 10

[Uri Bernstein]

Comment on attachment 212412 [details] [diff] [review]
patch (alternative 1)

People are complaining about this, so it would be nice to be able to tell them it'll be fixed in Fx 2.0.

Comment 11

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

Comment on attachment 212412 [details] [diff] [review]
patch (alternative 1)

I would like to wait for a month or so to see if there are any regressions before we land this on branch.

Comment 12

[(no longer active)]

Mass-assigning the new rtl keyword to RTL-related (see bug 349193).