344423 - [Will|Did]DeleteSelection startOffset off by 1 for backspace in plain text editing

Status: RESOLVED FIXED

(Core :: DOM: Editor, defect)

Description

[Aaron Leventhal]

WillDeleteText(nsISelection *aSelection) is our notification for html input and XUL textbox for text deletions.

Two problems:
When backspace or delete is pressed the startOffset and endOffset are the same, when the endOffset should show that a char is being deleted. We can work around that problem.
The bigger problem is that the startOffset is too high, off by 1.

Comment 1

[Aaron Leventhal]

The problem is StartBatchChanges
(http://lxr.mozilla.org/seamonkey/source/layout/generic/nsSelection.cpp#2826)

When EndBatchChanges() is finally called, the selection is updated. However, [Will|Did]DeleteSelection both get called before that, and it still thinks the selection is to the right of the character to be deleted.

Comment 2

[Aaron Leventhal]

Mats, do you have any ideas?

Comment 3

[Aaron Leventhal]

Attachment: Works so far, but still testing. Model backspace action after ctrl+backspace which deletes 1 word back, and has no problems with [Will|Did]DeleteSelection

I'm not sure, but I think this means we can remove some ePrevious handling for the collapsed action in nsTextEditRules::WillDeleteSelection.

Incidentally, the bug only occurs for plain text editing, not for HTML.

Comment 4

[Aaron Leventhal]

This fixes bug 260921

Comment 5

[Aaron Leventhal]

Attachment: Works so far, but still testing. Model backspace action after ctrl+backspace which deletes 1 word back, and has no problems with [Will|Did]DeleteSelection

Comment 6

[Aaron Leventhal]

Attachment: Works so far, but still testing. Model backspace action after ctrl+backspace which deletes 1 word back, and has no problems with [Will|Did]DeleteSelection

Comment 7

[Aaron Leventhal]

Attachment: Model backspace action after ctrl+backspace which deletes 1 word back, and has no problems with [Will|Did]DeleteSelection

Comment 8

[Aaron Leventhal]

Attachment: Alternative patch goes even farther by implenting collapsed forward delete this way, thus allowing the removal of much unneeded code and providing consistent results for [Will|Did]DeleteSelection

Comment 9

[Uri Bernstein]

(In reply to comment #8)
> Created an attachment (id=232137) [edit]
> Alternative patch

I think you attached the same patch again.

I'll only be able to have a good look at this sometime next week, but at a first glance, I'm not sure this is the right way to go. In fact, when I touched the word-delete code, I commented somewhere that I think it should not go through nsSelection::MoveCaret, but instead be implemented more like single-character deletion. The reason is that "delete" is a pure logical operation, whereas much of the code in PeekOffset (used by CaretMove) is there to enable various visual quirks associated with caret movement.

As an example, I suspect that with this patch, placing the caret at the beginning of a wrapped line and hitting "backspace" once won't actually delete anything, but just move the caret to the end of the previous line. This is the (apparently intended) behavior for pressing left-arrow, and while it's (arguably) correct in that case, it's certainly wrong for backspace.

I'll take a closer look (and perhaps be able to come up with a suggestion for an alternative fix) sometime early next week.

Comment 10

[Aaron Leventhal]

Attachment: Alternative patch goes even farther by implenting collapsed forward delete this way, thus allowing the removal of much unneeded code and providing consistent results for [Will|Did]DeleteSelection

Comment 11

[Aaron Leventhal]

(In reply to comment #9)
> As an example, I suspect that with this patch, placing the caret at the
> beginning of a wrapped line and hitting "backspace" once won't actually delete
> anything, but just move the caret to the end of the previous line.e.
You're right.

That extra "smart" behavior with wrapped text is not necessary for left/right arrow. None of the applications I test with do that:
Notepad, MS Word, IE, Opera

Honestly that's just weird. We don't even do that in rich text editing (Midas, thunderbird etc.). We should be consistent with ourselves and other apps, and remove the extra complicated code. That would allow this patch to work. A big benefit of the alternative patch which does this for Delete as well is the removal of lots of code -- that's not a bad thing :)

> I'll take a closer look (and perhaps be able to come up with a suggestion for
> an alternative fix) sometime early next week.

Thanks!

Comment 12

[Aaron Leventhal]

> This is the (apparently intended) behavior for pressing left-arrow, and while 
> it's (arguably) correct in that case, it's certainly wrong for backspace.

I found another reason that it's a bad idea. When your press shift+right or shift+left to select by character, it pauses and does nothing once when you move pas the line boundary. That's weird. A screen reader user would certainly be confused by that.

So we have 5 reasons to remove that special case:
1) Inconsistent with our rich text editor
2) Inconsistent with other apps
3) Can remove lots of code via the alternative patch, fixing this bug at the same time
4) No confusing visual pause for shift+left and shift+right
5) Won't consuse screen reader users arrowing through the text. I just noticed that they get a beep when they arrow through the end of the line where there is no character.

Comment 13

[Uri Bernstein]

(In reply to comment #12)

> So we have 5 reasons to remove that special case:

The more I think of it, the more I agree with you on this. My current thinking is that this was probably not even done on purpose in the first place. The flip side is that removing it (without breaking other cases) might not be straightforward. I'm amazed that in 6 years or so, no bug (AFAIK) was filed on this behavior.

That said, I brought this specific case just as an example. While I can't think of other examples where delete should behave differently than caret movement off the top of my head, I'd like to think about this a bit more.

I definitely agree that if we're going to do this, we should do it for both directions (backspace and delete), so I'll concentrate on your second alternative when reviewing.

Comment 14

[Aaron Leventhal]

One exception I see in Notepad/Opera/IE/Word is that pressing end or shift+end leaves you at the end of the current line. However, the next right arrow then goes to the 2nd character in the next line. In other words, there is *never* an invisible or extra stop allowed. 

In Composer/Midas an end/shift+end press puts you before the space at the end of the line. The next right arrow puts you at the beginning of the next line.

Comment 15

[Uri Bernstein]

Doing some archeology, I found that the current caret behavior at end of line (or at least the "forward" half of it) was requested in bug 9433, which says:
"[Pressing the right arrow when after the space at end of a wrapped line] should move [the caret] to the beginning of the [next] line (just before the first
character.) This is tricky since technically, this is not a change in location,
but it is what should happen in any word processor."

This was implemented as part of bug 9195.

So this was, apparently, done intentionally. Still, whether 1999-era word processors really behaved this way or not, I agree it's not desirable behavior now. I'll probably file a bug on it sometime tomorrow when I have time.

Comment 16

[Aaron Leventhal]

Attachment: Close but has a couple of flaws, see comment

Uri, I like the idea of a separate bug to fix PeekOffset.

This patch tries but has problems at the end of a line which is so long as to fill the textarea width, and isn't deleting when you hit Delete after an End key press. I did have that working, and it has to do with the logic in nsFrame::GetFrameFromDirection that I was messing with. Hard to make that work right without a new member variable in aPos, like aPos->mKeepGoingAfterLineEdge or somthing. I was trying to use mVisible for it.

Comment 17

[Aaron Leventhal]

Attachment: Takes care of both problems, caret movement near ends of lines and the original bug

The problem with long lines in the text area was really bug 335560.

Comment 18

[Uri Bernstein]

(In reply to comment #16)
> Uri, I like the idea of a separate bug to fix PeekOffset.

Done - see bug 347494. Aaron - with your permission, I'd like to take a stab at that bug myself (although I might end up using parts of your proposed fix).
Any fix to that bug will have to be tested in a wide range of plaintext and HTML situations, including bidi.

Let's concentrate the discussion here on the original bug and your proposed fix for it (i.e., attachment 232140 [details] [diff] [review]). I hope to be able to test it and give some more comments later today, or perhaps tomorrow.

Comment 19

[Aaron Leventhal]

Sounds great! Thank you.

Comment 20

[Uri Bernstein]

Some more random comments and questions, in no particular order:

One thing I noticed about the patch is that it removes the calls to CheckBidiLevelForDeletion(), and thereby kills the bidi "don't delete if caret is not next to deleted character" feature. This should be restored.

Another question is whether you could remove the "if (bCollapsed)" block from nsHTMLEditRules::WillDeleteSelection, like you did in nsTextEdit rules.

It also seems like you might be able to get rid of nsEditor::CreateTxnForDeleteInsertionPoint() and nsEditor::CreateTxnForDeleteCharacter().

>-        case eToEndOfLine:
>-          result = selCont->IntraLineMove(PR_TRUE, PR_TRUE);
>-          aAction = eNext;
>-          break;
Notice that it says "eNext", not "eNone" in this branch. Initially I though this might have been a typo, but apparently it's not. So you're probably changing behavior in this case by always setting to eNone.

Finally, note that this patch makes deletion susceptible to any other existing caret-movement bugs, such as bug 346445, and perhaps other bugs I'm not aware of.

I'll continue looking at the patch later this week. The problem is that I don't really know the editor code that well (as opposed to the selection code), so I'm somewhat nervous about reviewing this. Perhaps someone more familiar with the editor module should give this a look as well?

Comment 21

[Aaron Leventhal]

Wow, I did not even realize that nsPlaintextEditor::DeleteSelection() is called for rich text deletes. Bad assumption on my part, based on the class name.

Okay, so on the plus side this means we can eliminate even more code. That's great. On the minus side, as you said, we'll have to deal with all the caret navigation bugs, which I do not want to do. Someone should do this so we can remove all the unnecessary code, but probably not me.

> One thing I noticed about the patch is that it removes the calls to
> CheckBidiLevelForDeletion(), and thereby kills the bidi "don't delete if caret
> is not next to deleted character" feature. This should be restored.
If we decide to still go with this approach I'll remove that.

> Another question is whether you could remove the "if (bCollapsed)" block from
> nsHTMLEditRules::WillDeleteSelection, like you did in nsTextEdit rules.

> Notice that it says "eNext", not "eNone" in this branch. Initially I though
> this might have been a typo, but apparently it's not. So you're probably
> changing behavior in this case by always setting to eNone.
I wasn't able to duplicate bug 67847 even without that line. They fixed that bug by setting it to eNext. But now that I test with rich text, I see not that bug but we're not deleting the entire selection when I hit delete. That's probably because I am shifting selection without even checking to see if it's collapsed.
I'd like to find a way to fix that without requiring setting it to eNext, because that means we'd still need the if (bCollapsed) sections.

Comment 22

[Aaron Leventhal]

> I'm somewhat nervous about reviewing this. Perhaps someone more familiar with
> the editor module should give this a look as well?

I'm nervous about fixing it. I'm not sure who the current module owner is but I don't think anyone is active. Editor is orphaned as far as I know.

If there is a simpler fix I think we should go for it.

Comment 23

[Aaron Leventhal]

Uri, given the problems with doing this for rich text -- there will be regressions, how about we only do this for plain text?

After all this is only a bug for the plaintext editor anyway.

Comment 24

[Uri Bernstein]

(In reply to comment #23)

> After all this is only a bug for the plaintext editor anyway.

Is that because something is preventing the bug from happening in HTML, or just because we don't care about HTML in this case?

Comment 25

[Uri Bernstein]

(In reply to comment #22)
> 
> If there is a simpler fix I think we should go for it.
> 

Perhaps you can pass on the action into [Will|Did]DeleteSelection (as it is passed to nsTextEditRules::WillDeleteSelection), and then use it (if it's not eNone) to "compensate" for the wrong offset?

Comment 26

[Aaron Leventhal]

Hi Uri, it's a bug in plaintext only because for HTML we are getting 100% accurate WillDeleteText calls. WillDeleteSelection isn't used for the collapsed case there.

> Perhaps you can pass on the action into [Will|Did]DeleteSelection (as it is
> passed to nsTextEditRules::WillDeleteSelection), and then use it (if it's not
> eNone) to "compensate" for the wrong offset?
You're saying [Will|Did]DeleteSelection would get a new aCollapsedAction parameter? I didn't want to have to change nsIEditActionListener, because I don't know who all the consumers of that are outside of Mozilla. What do you think?

Maybe we should make it use [Will|Did]DeleteText for the collapsed case, as HTML does?

Comment 27

[Uri Bernstein]

(In reply to comment #26)
> You're saying [Will|Did]DeleteSelection would get a new aCollapsedAction
> parameter? I didn't want to have to change nsIEditActionListener, because I
> don't know who all the consumers of that are outside of Mozilla. What do you
> think?
>

I guess that's what I was saying. As I said before, I don't really know this code well -  I was just throwing an idea up in the air. But I think your suggestion below is a better idea.
 
> Maybe we should make it use [Will|Did]DeleteText for the collapsed case, as
> HTML does?

This sounds like a very good idea to me. After all, in this case we're not really deleting a selection. Assuming this is not too difficult to do, I suggest going this way.

Comment 28

[Aaron Leventhal]

Attachment: 1) Call [Will|Did]Delete[Text|Node] correctly instead of assuming [Will|Did]DeleteSelection, 2) Invalidate accessible children from dom node edit changes 3) Correct offset/length for WillDeleteNode

Comment 29

[Uri Bernstein]

Comment on attachment 234955 [details] [diff] [review]
1) Call [Will|Did]Delete[Text|Node] correctly instead of assuming [Will|Did]DeleteSelection, 2) Invalidate accessible children from dom node edit changes  3) Correct offset/length for WillDeleteNode

The new code in nsEditor::DeleteSelectionImpl will only work for the plaintext editor. Fortunately, it will never actually be reached in the HTMLEditor case, but we need to make sure of that. So I suggest ASSERTing that this is, in fact, a plaintext editor, and also adding a comment explaining why this code will not be reached by the HTML editor.

Additionally, the only non-text node that we might encounter in the plaintext editor is a nsHTMLBRElement, so it might be worth asserting that this is indeed the case, if deleteCharData is null. Also, add a comment saying that the handling of non-text nodes could be removed altogether once bug 240933 is fixed.

I'm not sure how the changes to nsHyperTextAccessible are relevant, and in any case, I'm not in the position to review them - but I assume you know what you're doing there.

Other than that, looks good. r=me with those asserts and comments.

Comment 30

[Aaron Leventhal]

Comment on attachment 234955 [details] [diff] [review]
1) Call [Will|Did]Delete[Text|Node] correctly instead of assuming [Will|Did]DeleteSelection, 2) Invalidate accessible children from dom node edit changes  3) Correct offset/length for WillDeleteNode

I will add the asserts and comments as recommended.

Ginn, could you r+ the mozilla/accessible changes?

Comment 31

[Aaron Leventhal]

BTW, the InvalidateChildren() calls in nsHyperTextAccessible were necessary so that the accessibility cache is updated after a <br> is inserted or deleted. Otherwise bad things happen, such as events not being fired, or incorrect information in events.

The other change in mozilla/accessible was because we weren't passing the correct information to DOMPointToOffset when a node was deleted.

Comment 32

[Ginn Chen]

Comment on attachment 234955 [details] [diff] [review]
1) Call [Will|Did]Delete[Text|Node] correctly instead of assuming [Will|Did]DeleteSelection, 2) Invalidate accessible children from dom node edit changes  3) Correct offset/length for WillDeleteNode

r=me for accessibility part

Comment 33

[[e-mail]]

Attachment: )

I think you got your )s mixed up...

Comment 34

[(not reading, please use [e-mail] instead)]

aaron, it looks like the code added to nsEditor.cpp has introduced a crash.

see bug #353515.  additionally, is there a typo in this comment:

  // XXX After bug 240933 is fixed we will never be deleting a node         
  // a node, there will always be a char data

At least, I don't get the "a node a node" part.

Comment 35

[Aaron Leventhal]

s/a node a node/a node

Comment 36

[Aaron Leventhal]

Basically after that bug is fixed (if it is fixed) there will no longer be <br>'s, so plain text will be plain text.