502259 - Richtext editor: delete at end of paragraph does nothing

Status: RESOLVED FIXED

(Core :: DOM: Editor, defect, P2)

Description

[liucougar]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-GB; rv:1.9.1) Gecko/20090624 Firefox/3.5
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-GB; rv:1.9.1) Gecko/20090624 Firefox/3.5

in a richtext editor (contentEditable=true or designMode=on), pressing delete key at the end of paragraphs does nothing

Reproducible: Always

Steps to Reproduce:
1. open the attached test case
2. place cursor after "one" (at the end of the first paragraph)
3. press delete
Actual Results:  
nothing happens

Expected Results:  
the first paragraph becomes "onetwo"

the test case uses contentEditable=true. same bug appears if designMode=on on the document is used.

Comment 1

[liucougar]

Attachment: test case for this bug

Comment 2

[liucougar]

Attachment: test case for the bug

Comment 3

[liucougar]

forgot to mention: this is a regression from Firefox 3.0.11

Comment 4

[liucougar]

Attachment: more test cases for this bug

place cursor after each "one" in the html, press "delete", nothing happens

Comment 5

[liucougar]

actually, place cursor after each "two", and press "delete", nothing happens either

Comment 6

[liucougar]

Attachment: proposed patch to fix this regression and a test case

this regression is introduced by http://hg.mozilla.org/mozilla-central/diff/11e029835944/editor/libeditor/html/nsHTMLEditRules.cpp (when fixing Bug 462188 )

This is a critical regression for us (teampatent.com), and we have to block FF3.5 because of it. We really really hope this can make it into 3.5.1 release.

From what I can tell, after that change, delete key press actually triggers the code branch for dealing with plain text (nsHTMLEditRules::WillDeleteSelection does not complete, because after the call to mHTMLEditor->ExtendSelectionForDelete, aAction is set to nsIEditor::eNone), while backspace key press correctly triggers logic in nsHTMLEditRules::WillDeleteSelection

this proposed fix removes eNext handling in nsPlaintextEditor::ExtendSelectionForDelete, so that delete key press will trigger the same code path as backspace key press, which fixes this bug.

a test case is also added for this regression in this patch. I am not sure how to run all the tests, so I don't know whether this patch breaks anything. Let me know how we can help.

FYI: nsPlaintextEditor::ExtendSelectionForDelete was introduced in http://hg.mozilla.org/mozilla-central/rev/34967ab14be8 when fixing Bug 157546

Comment 7

[Martijn Wargers (dead)]

Comment on attachment 387691 [details] [diff] [review]
proposed patch to fix this regression and a test case

You should ask review on the patch to get at least a chance of getting it fixed.

Comment 8

[liucougar]

thanks, didn't know that. 

I looks like you already requested a review on the patch, thanks.

Comment 9

[Theppitak Karoonboonyanan]

(In reply to comment #6)

> this proposed fix removes eNext handling in
> nsPlaintextEditor::ExtendSelectionForDelete, so that delete key press will
> trigger the same code path as backspace key press, which fixes this bug.

Of course, this fix is not desirable for users of the languages in which the Delete key is expected to delete text cluster-wise. Let's investigate what's wrong in the previous patches.

Comment 10

[liucougar]

Attachment: updated patch and a test case

revised the patch so that it does not change behavior for cluster-based languages

what this patch does is: if action is eNext (delete), and it is set to eNone by ExtendSelectionForDelete, set it back to eNext

Comment 11

[Theppitak Karoonboonyanan]

This patch looks OK for me. But as I'm not authorized to review or change it, I think you had better request for a review from peterv.

Comment 12

[Martijn Wargers (dead)]

Comment on attachment 387921 [details] [diff] [review]
updated patch and a test case 

Yeah, I don't think Theppitak has review privileges, PeterV has.

Comment 13

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 387921 [details] [diff] [review]
updated patch and a test case 

How about just removing "*aAction = eNone;"  in the eNext case of nsPlaintextEditor::ExtendSelectionForDelete()? The ePrevious case doesn't clear *aAction. That seems to work, and preserves the Thai text behaviour. You should add a comment saying why you're doing it, referencing the bug.

Comment 14

[liucougar]

Attachment: New patch and test case

modified this patch according to Chris' suggestions

Comment 15

[Theppitak Karoonboonyanan]

(In reply to comment #13)
> (From update of attachment 387921 [details] [diff] [review])
> How about just removing "*aAction = eNone;"  in the eNext case of
> nsPlaintextEditor::ExtendSelectionForDelete()? The ePrevious case doesn't clear
> *aAction. That seems to work, and preserves the Thai text behaviour. You should
> add a comment saying why you're doing it, referencing the bug.

Yes, it seems to work with plain text editor, too. That is because nsEditor::CreateTxnForDeleteSelection() [via nsEditor::DeleteSelectionImpl(), which is called after nsPlaintextEditor::ExtendSelectionForDelete() in nsTextEditRules::WillDeleteSelection()] respects the collapsing status of the current selection and overrides the aAction argument in case it's not collapsed.

Clearing aAction to eNone in nsPlaintextEditor::ExtendSelectionForDelete() is kind of double-safety measurement to tell further running code not to touch the selection any more. But, as you noticed, it appears to be unnecessary here, just like the case of eToEndOfLine, where aAction still needs to be set to eNext for further processing. So, leaving the eNext case unchanged can also be similarly reasoned. So, I agree with the change.

Note, however, that it has nothing to do with the ePrevious case. There's no symmetry here.

Comment 16

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 390407 [details] [diff] [review]
New patch and test case

Delete "*aAction = eNone;", don't comment it out. If we need it back, we can get it from version control history.

You need to get peterv's (or an editor peer's) sign off on this. Send peterv a direct but polite email to request review, that usually works. ;)

Comment 17

[Theppitak Karoonboonyanan]

Wait. Suspecting that similar problem should have happened to eNextWord and ePreviousWord cases as well, I've tried it with the test case and found no problem. So, I also tried the normal Delete key with the *unpatched* fresh trunk checkout, and the problem has gone!

Could someone confirm this? I can be wrong.

Comment 18

[liucougar]

I tried Jul24 nightly build of firefox3.6pre1, and I can still reproduce this bug (don't know whether your trunk code is firefox3.6pre1 or not)

in FF3.5.1, if I ctrl+delete at the end of a paragraph, the first word in the next paragraph is removed, which looks like the expected behavior

Comment 19

[Theppitak Karoonboonyanan]

Yes, my trunk is 3.6a1pre. And its buildconfig is:

about:buildconfig

Build platform
target
x86_64-unknown-linux-gnu

Build tools
Compiler 	Version 	Compiler flags
gcc 	gcc version 4.3.3 (Debian 4.3.3-14) 	-Wall -W -Wno-unused -Wpointer-arith -Wcast-align -W -Wno-long-long -pedantic -fno-strict-aliasing -pthread -pipe -DNDEBUG -DTRIMMED
c++ 	gcc version 4.3.3 (Debian 4.3.3-14) 	-fno-rtti -fno-exceptions -Wall -Wpointer-arith -Woverloaded-virtual -Wsynth -Wno-ctor-dtor-privacy -Wno-non-virtual-dtor -Wcast-align -Wno-invalid-offsetof -Wno-long-long -pedantic -fno-strict-aliasing -fshort-wchar -pthread -pipe -DNDEBUG -DTRIMMED

Configure arguments
--enable-application=browser --prefix=/home/moz --disable-optimize --disable-debug --enable-tests --with-system-bz2 --with-system-jpeg --with-system-zlib

Comment 20

[liucougar]

do you have any local changes? Can you reproduce the bug in the nightly build from http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/latest-trunk/ ?

Comment 21

[Theppitak Karoonboonyanan]

Nightly build fails. There must be some confusion among my builds, as I'm working on several branches, although I separately compile them in different build dirs.

Being checked at another totally unrelated branch, it still fails. So, the bug must really exist, and needs the patch. So, please go on, and sorry for the false alarm.

Comment 22

[Peter Van der Beken [:peterv]]

Comment on attachment 390407 [details] [diff] [review]
New patch and test case

>+	// Reset action to none here would prevent Delete in 
>+	// richtext editor from merging block elements, see Bug 502259

I'd make the comment: "Don't set aAction to eNone (see bug 502259)."

>+        // *aAction = eNone;

Remove the line instead of commenting it out.

Comment 23

[liucougar]

Attachment: updated patch to incorporate all comments plus unit test

the patch is updated to remove the code comment and the comment is modified

the original patch already has review+ and superreview+, do I still need to request review on this patch?

Comment 24

[Martijn Wargers (dead)]

No, I've just carried over the r+ and sr+, since you've already had those. The patch is now ready to be checked in.

Comment 25

[liucougar]

thanks Martijn.

I see you assigned this ticket to me, as I can't really do commits, what shall I do to proceed please?

Comment 26

[Martijn Wargers (dead)]

Someone will check the patch in for you.

Comment 27

[Chris Pearce [:cpearce (Not reading bugmail)]]

Pushed: http://hg.mozilla.org/mozilla-central/rev/a5d741a12327

Comment 28

[liucougar]

May I know whether this will go into Firefox 3.5.2 (or 3.5.1)? By looking at hg.mozilla.org, I don't know how to tell that. Thanks

Comment 29

[Chris Pearce [:cpearce (Not reading bugmail)]]

liucougar: Thanks for your patch! I've checked it in. Your latest patch used tabs, so I converted it to spaces before committing. Please always use spaces for Mozilla code in future, and match the tab-width with the file you're working in - sometimes it's 2, sometimes 4.

I'd say this would definitely make 3.6, I've set appropriate "blocking?" flags so that the powers that be can triage this.

Comment 30

[liucougar]

Thanks for the information

will look out for tabs when preparing patches

Comment 31

[Chris Pearce [:cpearce (Not reading bugmail)]]

Damn it, the test failed:

44627 ERROR TEST-UNEXPECTED-FAIL | /tests/layout/generic/test/test_backspace_delete.xul | Delete broken in "foobar", offset 3 - got [object Text], expected [object Text]
44668 ERROR TEST-UNEXPECTED-FAIL | /tests/layout/generic/test/test_backspace_delete.xul | [SimpleTest/SimpleTest.js, window.onerror] An error occurred: [ uncaught exception: [Exception... "Index or size is negative or greater than the allowed amount"  code: "1" nsresult: "0x80530001 (NS_ERROR_DOM_INDEX_SIZE_ERR)"  location: "http://localhost:8888/tests/layout/generic/test/test_backspace_delete.xul Line: 28"] ]

Windows: http://tinderbox.mozilla.org/showlog.cgi?log=Firefox-Unittest/1248751416.1248753472.17914.gz
Mac: http://tinderbox.mozilla.org/showlog.cgi?log=Firefox-Unittest/1248752373.1248754915.1479.gz
Linux: http://tinderbox.mozilla.org/showlog.cgi?log=Firefox-Unittest/1248752634.1248755143.4056.gz

I have backed the patch out of the tree. We'll have to modify it and try again.

Merge: http://hg.mozilla.org/mozilla-central/rev/ccf897e90b76
Backout: http://hg.mozilla.org/mozilla-central/rev/3ee48fc94855

Comment 32

[liucougar]

Attachment: patch with updated unit tests [Checkin: Comment 37 & 44]

the patch stays the same, but the unit tests are updated

the unit test for Bug 419217 is not testing the bug properly in trunk, and the bug actually is still reproducable in FF 3.5.1 and trunk (I will reopen that bug)

this patch fixes the unit test for Bug 419217, plus fixed the unit test for this bug

this patch fixes both Bug 419217 and this bug

about changes in the unit test file: when delete causes merging of block elements the text node in the first block element is replaced with a new text node, so I added a function to return the expected anchorNode, which can be called after the delete happens.

Comment 33

[Chris Pearce [:cpearce (Not reading bugmail)]]

(In reply to comment #32)
> Created an attachment (id=391527) [details]
> patch with updated unit tests

This doesn't work for all cases. :(

Based on the comments in Bug 419217, I think we need to *always* to the JoinBlock() in nsHTMLEditRules::WillDeleteSelection(), nsHTMLEditRules.cpp:2537. Just enabling it seems to work, but based on peterv's Bug 419217 comment 5, I think for safety we need to change JoinBlock() so that it can handle the case where the two blocks are not in the same subtree - this is the case that causes trouble.

Comment 34

[Chris Pearce [:cpearce (Not reading bugmail)]]

(In reply to comment #33)
> This doesn't work for all cases. :(

Particularly, in the "more test cases for this bug" testcase, if you put the cursor at the end of the "2. two" <li> and press delete, the cursor just moves down to the start of the subsequent "1. one" <li>. I'd expect the delete to pull the subsequent <li> into the current <ol>, so you'd end up with a subsequent "3. one" <li>.

Opera's contenteditable behaviour seems to be pretty sane in these cases, I think we should try to match their behaviour.

Comment 35

[liucougar]

(In reply to comment #34)
> (In reply to comment #33)
> > This doesn't work for all cases. :(
> 
> Particularly, in the "more test cases for this bug" testcase, if you put the
> cursor at the end of the "2. two" <li> and press delete, the cursor just moves
> down to the start of the subsequent "1. one" <li>...

You are right, this case is not handled. However, this is not a regression from FF 3 (this bug report is more about regression): FF3 does not work in that case either.

(In reply to comment #33)
> Based on the comments in Bug 419217, I think we need to *always* to the
> JoinBlock() in nsHTMLEditRules::WillDeleteSelection(),
> nsHTMLEditRules.cpp:2537. Just enabling it seems to work, but based on peterv's
> Bug 419217 comment 5, I think for safety we need to change JoinBlock() so that
> it can handle the case where the two blocks are not in the same subtree - this
> is the case that causes trouble.
By looking at the JoinBlocks function, it has all the code to deal with merging lists, and merging other block elements with list. It seems to me that just always enable JoinBlocks() should do the trick.

However, I think adding the JoinBlocks() should be in a separate patch/commit, because it is a new feature (or another bug fix). It seems to me that the patch attached here fixes the regression, which is all this ticket is about. (Always enabling JoinBlocks may introduce other regressions, that's why I think it's better to have this fix in before dealing with that issue)

Comment 36

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 391527 [details] [diff] [review]
patch with updated unit tests
[Checkin: Comment 37 & 44]

(In reply to comment #35)
> You are right, this case is not handled. However, this is not a
> regression from FF 3 (this bug report is more about regression)
> ...
> However, I think adding the JoinBlocks() should be in a separate patch

Fair enough, I agree. Test change makes sense. r=cpearce.

The review policies changed recently, we don't actually need super review on this, since it's not an architectural change.

Comment 37

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

http://hg.mozilla.org/mozilla-central/rev/27dc5e92048d

Thanks!

Comment 38

[[e-mail]]

Comment on attachment 391527 [details] [diff] [review]
patch with updated unit tests
[Checkin: Comment 37 & 44]

Thunderbird/SeaMonkey would appreciate this on 1.9.1 branch to fix the equivalent bug in message compose/editor.

Comment 40

[Chris Pearce [:cpearce (Not reading bugmail)]]

(In reply to comment #38)
> (From update of attachment 391527 [details] [diff] [review])
> Thunderbird/SeaMonkey would appreciate this on 1.9.1 branch to fix the
> equivalent bug in message compose/editor.

Agreed, it's an annoying bug it would be nice to have it on 1.9.1. Approval?

Comment 41

[Samuel Sidler (old account; do not CC)]

1.9.1.3 is now code frozen and we're not accepting new patches. When 1.9.1.4 opens for checkins, I'll be going through the 1.9.1.3 list to approve. This bug is on that list.

Comment 42

[Mike Beltzner [:beltzner, not reading bugmail]]

Mass change: adding fixed1.9.2 keyword

(This bug was identified as a mozilla1.9.2 blocker which was fixed before the mozilla-1.9.2 repository was branched (August 13th, 2009) as per this query: http://is.gd/2ydcb - if this bug is not actually fixed on mozilla1.9.2, please remove the keyword. Apologies for the bugspam)

Comment 43

[Daniel Veditz [:dveditz]]

Comment on attachment 391527 [details] [diff] [review]
patch with updated unit tests
[Checkin: Comment 37 & 44]

Approved for 1.9.1.4, a=dveditz for release-drivers

Comment 44

[Serge Gautherie (:sgautherie)]

Comment on attachment 391527 [details] [diff] [review]
patch with updated unit tests
[Checkin: Comment 37 & 44]


http://hg.mozilla.org/releases/mozilla-1.9.1/rev/9c86cab9debe

Comment 45

[Al Billings [:abillings - ex-MoCo]]

Verified for 1.9.1 with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1.4pre) Gecko/20090916 Shiretoko/3.5.4pre (.NET CLR 3.5.30729) (once I found a PC keyboard with a delete key).