419406 - Forward-delete deletes immediately even if caret not visually adjacent to deleted character

Status: RESOLVED FIXED

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

Description

[Uri Bernstein]

The "del" (forward-delete) key now immediately deletes the next logical character even if the caret is visually not adjacent to it (e.g., in "hello|שלום", when the caret is logically after the "o", pressing del deletes the ש instead of just moving the caret to the right side of it).

This is a regression from bug 157546.

Comment 1

[Theppitak Karoonboonyanan]

Shouldn't the text be deleted in logical sequence rather than visual?

If the expected result is to always be in visual order, we don't need the different  treatments of left/right and backpace/delete in nsFrameSelection::MoveCaret() implementation, then.

Comment 2

[Uri Bernstein]

Yes, deletion always happens in logical order, and that's the way it should be.

However, in the special case when the next logical character is not next to the caret, our behavior used to be (and still is, for backspace), that no character is initially deleted, and instead the caret moves to the position visually before (for backspace: after) the character to be deleted. the next press on delete/backspace does the actual deletion.

This logic is implemented in nsTextEditRules::CheckBidiLevelForDeletion:
http://mxr.mozilla.org/seamonkey/source/editor/libeditor/text/nsTextEditRulesBidi.cpp#48

Comment 3

[Uri Bernstein]

Note that fixing this would probably fix bug 350073 as well, and vice versa.

Comment 4

[Uri Bernstein]

Attachment: WIP

Pull the call to CheckBidiLevelForDeletion() up into nsPlaintextEditor::DeleteSelection(), before expanding the selection. This circumvents the problems described in bug 350073 comment #2 and #3.

It fixes (I think) this bug, and /partially/ fixes bug 350073 (partially, because it looks at the direction of the adjacent character in the deletion direction - which could be a space with a directionality different than the actual word that's going to be deleted).

This a terrible hack, though: It makes CheckBidiLevelForDeletion() public, and relies on some ugly down-casting. The correct thing to do might be to get rid of nsTextEditRulesBidi altogether, and pull CheckBidiLevelForDeletion() itself into nsPlaintextEditor.

Simon - thoughts on the above?

Comment 5

[Theppitak Karoonboonyanan]

Attachment: Another patch which defers selection extension to later stage (WIP)

I'm also trying another approach: by moving the selection extension down to later stage.

By doing the extension after mRules->WillDoAction() call, this fixes the problem for plain text editor. However, this doesn't work for HTML editor because the deletion was already handled by WillDoAction(). This patch will break mochitest in bug 157546.

To go further, I wonder whether WillDoAction() for both editors should know how to handle forward deletion operations by themselves, or the HTML editor should not handle them at all.

(If this approach is accepted, similar fix for bug 350073 can also be done, I suppose.)

Comment 6

[Theppitak Karoonboonyanan]

Attachment: Patch deferring extension, with handling in HTML editor

This patch also adds cell-wise deletion capability to nsHTMLEditRules::WillDeleteSelection(). Although it passes the mochitest, I'm not sure if this would just make it too smart for its functionality.

By this scheme, the nsIEditRules derivatives that claim to handle forward deletions in WillDoAction() must do it perfectly by themselves, and the code in nsPlaintextEditor::DeleteSelection() will provide default implementation otherwise.

Comment 7

[Theppitak Karoonboonyanan]

Attachment: Patch deferring extension, and addressing bug 350073

In reply to comment #5)

> (If this approach is accepted, similar fix for bug 350073 can also be done, I
> suppose.)

And this patch also addresses that bug with the same set of changes.

Comment 8

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

Need feedback from some subset of Uri, Simon and Peter...

Comment 9

[Uri Bernstein]

So basically my approach was to pull the CheckBidiLevelForDeletion() call out from nsHTMLEditRules to nsPlaintextEditor, whereas Theppitak's approach is the reverse, in a way: push the selection extension from nsPlaintextEditor into nsHTMLEditRules.

I don't understand the distinction between these interfaces well enough to comment on which approach is better, or more correct[*], but I do notice that Theppitak's patch introduces some duplication of code (the selection extension code), which is something we might want to try and avoid.

[*] The design here seems to be horribly messed up by the existence of the "never mind, I already did it myself" parameter (aka aHandled). I didn't like getting this kind of response from my mom when I was a kid, and I don't like it here either.

Comment 10

[Theppitak Karoonboonyanan]

(In reply to comment #9)
> I don't understand the distinction between these interfaces well enough to
> comment on which approach is better, or more correct[*], but I do notice that
> Theppitak's patch introduces some duplication of code (the selection extension
> code), which is something we might want to try and avoid.

Yes, I also noticed that. And I've just got another idea to refactor, by adding a method for doing the extension to nsPlaintextEditor. So, it's simply pulled back again. I'll try it today.

As I said, I'm not sure which approach is better, either. Pulling up the bi-di check to nsPlaintextEditor also looks logical to me, and the patch is smaller. Meanwhile, I also feel uncomfortable when seeing the low-level codes claim to do some jobs while it's actually done in different way.

Comment 11

[Theppitak Karoonboonyanan]

Attachment: Patch deferring extension, with refactoring

And here it is.

I notice some funny thing in my patch: the refactored ExtendSelectionForDelete() function accepts an nsISelection, which is supposed to be passed from nsHTMLEditRules::WillDeleteSelection(), but hardly does anything on it directly. Rather, it's done through mSelConWeak selection controller, a member of ns[Plaintext]Editor.

This may reflect a gap between different layers of functionalities.

Comment 12

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

Assigning to Theppitak for now. Peterv, we need your feedback.

Comment 13

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

We're leaving this on the blocking list, with the plan that we back out the
checkin that caused it if this patch isn't ready (which probably means backing
it out shortly, actually, since the backout may have risk).

(This and bug 419217.)

Comment 14

[Mike Schroepfer]

(In reply to comment #13)
> We're leaving this on the blocking list, with the plan that we back out the
> checkin that caused it if this patch isn't ready (which probably means backing
> it out shortly, actually, since the backout may have risk).
> 
> (This and bug 419217.)
> 

I think we should execute the backout plan now...

Comment 15

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

The backout was executed.