Closed Bug 419406 Opened 14 years ago Closed 14 years ago

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

Categories

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

defect

Tracking

()

RESOLVED FIXED

People

(Reporter: uriber, Assigned: thep)

References

(Blocks 1 open bug)

Details

(Keywords: regression, Whiteboard: [needs feedback peterv])

Attachments

(2 files, 3 obsolete files)

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.
Flags: blocking1.9?
Version: unspecified → Trunk
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.
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
Note that fixing this would probably fix bug 350073 as well, and vice versa.
Depends on: 350073
No longer depends on: 350073
Attached patch WIPSplinter Review
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?
Blocks: 350073
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.)
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.
Attachment #305633 - Attachment is obsolete: true
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.
Need feedback from some subset of Uri, Simon and Peter...
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.
(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.
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.
Attachment #305704 - Attachment is obsolete: true
Attachment #305738 - Attachment is obsolete: true
Flags: tracking1.9? → blocking1.9?
Flags: blocking1.9? → blocking1.9+
Priority: -- → P2
Assigning to Theppitak for now. Peterv, we need your feedback.
Assignee: nobody → thep
Whiteboard: [needs feedback peterv]
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.)
(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...
The backout was executed.
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Flags: in-testsuite?
Component: Layout: BiDi Hebrew & Arabic → Layout: Text
QA Contact: layout.bidi → layout.fonts-and-text
You need to log in before you can comment on or make changes to this bug.