344226 - Ctrl+Backspace deletes the next word instead of the previous one in RTL text in LTR field

Status: RESOLVED FIXED

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

Description

[Niro]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20060710 Minefield/3.0a1
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20060710 Minefield/3.0a1

Type some RTL text in the address bar (for example: ωμεν)
Press Ctrl+Backspace.

Reproducible: Always



Expected Results:  
last word should be deleted.

Comment 1

[Uri Bernstein]

Technically speaking, this is probably a regression from bug 330815, although ctrl-backspace (and ctrl-delete) weren't working correctly in reverse-direction text even before that (in slightly different situations).

The issue here is that delete-by-word (forward or backward) is a logical operation, but the execution path goes through WordMove() which works "visually" (it is used also for ctrl+right and ctrl+left).

nsPlaintextEditor::DeleteSelection tries to compensate for this by flipping the direction if the caret level is odd (RTL) - however, that's not the correct factor to consider - the correct factor being the *paragraph* direction, which, alas, is not available at that point.

Comment 2

[Uri Bernstein]

Why does assigning a bug to myself have to be so difficult? *sigh*

Comment 3

[Uri Bernstein]

Adjusting summary: Ctrl+backspace (and Ctrl+delete) just work in the opposite direction when in reverse-direction text (e.g., RTL text in an LTR field). In the original scenario, the caret is at the end of the text, so when Ctrl+Backspace attempts to delete the next word (instead of the previous one), nothing actually happens.

Comment 4

[Uri Bernstein]

Attachment: patch

When deleting, "backward" and "forward" should be interpreted as "logical" operations, and the selection itself should also always be done "logically", regardless of bidi.edit.caret_movement_style.

I had to add a method to nsISelectionController in order to achieve this. I made it non-scriptable, because it's really (I think) just for internal use.

Comment 5

[Simon Montagu :smontagu]

Comment on attachment 228950 [details] [diff] [review]
patch

>Index: layout/generic/nsSelection.cpp
>+    case nsIDOMKeyEvent::DOM_VK_BACK_SPACE : 
>+        InvalidateDesiredX();
>+        PostReason(nsISelectionListener::KEYPRESS_REASON);
>+      break;

Not really part of your patch, but this caught my eye: why don't you set pos.mDirection? If you say "I'm relying on the fact that it's initialized to eDirPrevious 20 lines above," I will be very unhappy. If you say, "DOM_VK_UP and DOM_VK_HOME do the same thing," I will be unhappier still. That made a kind of sense back in the mists of code archaeology when the "no break here" comments were true (and good for you for removing some of them), but now it's just confusing. Please remove them all and set pos.mDirection in all cases.

More comments may follow, so don't bother with a new patch yet.

Comment 6

[Uri Bernstein]

(In reply to comment #5)
> (From update of attachment 228950 [details] [diff] [review] [edit])
> >Index: layout/generic/nsSelection.cpp
> >+    case nsIDOMKeyEvent::DOM_VK_BACK_SPACE : 
> >+        InvalidateDesiredX();
> >+        PostReason(nsISelectionListener::KEYPRESS_REASON);
> >+      break;
> 
> Not really part of your patch, but this caught my eye: why don't you set
> pos.mDirection? If you say "I'm relying on the fact that it's initialized to
> eDirPrevious 20 lines above," I will be very unhappy. If you say, "DOM_VK_UP
> and DOM_VK_HOME do the same thing," I will be unhappier still. That made a kind
> of sense back in the mists of code archaeology when the "no break here"
> comments were true (and good for you for removing some of them), but now it's
> just confusing. Please remove them all and set pos.mDirection in all cases.
> 
> More comments may follow, so don't bother with a new patch yet.
> 

"I'm relying on the fact that it's initialized to eDirPrevious 20 lines above". (Not because I think it's a good idea, but because that's what DOM_VK_UP does and I wanted consistency). But you're right, of course. I'll change both of them, and remove the remaining useless comments.

Comment 7

[Simon Montagu :smontagu]

Comment on attachment 228950 [details] [diff] [review]
patch

r=me with the changes in my earlier comment

Comment 8

[Uri Bernstein]

Attachment: patch v1.1

Updated to tip, and addressed the issue in Simon's comment #5.

Comment 9

[Uri Bernstein]

Checked in:
Checking in content/base/public/nsISelectionController.idl;
/cvsroot/mozilla/content/base/public/nsISelectionController.idl,v  <--  nsISelectionController.idl
new revision: 3.22; previous revision: 3.21
done
Checking in layout/generic/nsFrameSelection.h;
/cvsroot/mozilla/layout/generic/nsFrameSelection.h,v  <--  nsFrameSelection.h
new revision: 1.6; previous revision: 1.5
done
Checking in layout/generic/nsSelection.cpp;
/cvsroot/mozilla/layout/generic/nsSelection.cpp,v  <--  nsSelection.cpp
new revision: 3.248; previous revision: 3.247
done
Checking in layout/forms/nsTextControlFrame.cpp;
/cvsroot/mozilla/layout/forms/nsTextControlFrame.cpp,v  <--  nsTextControlFrame.cpp
new revision: 3.227; previous revision: 3.226
done
Checking in layout/base/nsPresShell.cpp;
/cvsroot/mozilla/layout/base/nsPresShell.cpp,v  <--  nsPresShell.cpp
new revision: 3.927; previous revision: 3.926
done
Checking in editor/libeditor/text/nsPlaintextEditor.cpp;
/cvsroot/mozilla/editor/libeditor/text/nsPlaintextEditor.cpp,v  <--  nsPlaintextEditor.cpp
new revision: 1.97; previous revision: 1.96
done

Comment 10

[[e-mail]]

Comment on attachment 229076 [details] [diff] [review]
patch v1.1

>Index: content/base/public/nsISelectionController.idl
Nobody remembered to rev the uuid, so I checked in a fix.

Comment 11

[(no longer active)]

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