liucougar Assignee
	Description • 15 years ago

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.

	liucougar Assignee
	Comment 3 • 15 years ago

forgot to mention: this is a regression from Firefox 3.0.11

	liucougar Assignee
	Comment 4 • 15 years ago

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

	liucougar Assignee
	Comment 5 • 15 years ago

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

	liucougar Assignee
	Comment 6 • 15 years ago

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

	Martijn Wargers (dead)
	Comment 7 • 15 years ago

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.

	liucougar Assignee
	Comment 8 • 15 years ago

thanks, didn't know that. 

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

	Theppitak Karoonboonyanan
	Comment 9 • 15 years ago

(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.

	liucougar Assignee
	Comment 10 • 15 years ago

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

	Theppitak Karoonboonyanan
	Comment 11 • 15 years ago

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.

	Martijn Wargers (dead)
	Comment 12 • 15 years ago

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

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

	Chris Pearce [:cpearce (Not reading bugmail)]
	Comment 13 • 15 years ago

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.

	liucougar Assignee
	Comment 14 • 15 years ago

modified this patch according to Chris' suggestions

	Theppitak Karoonboonyanan
	Comment 15 • 15 years ago

(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.

	Chris Pearce [:cpearce (Not reading bugmail)]
	Comment 16 • 15 years ago

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. ;)

	Theppitak Karoonboonyanan
	Comment 17 • 15 years ago

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.

	liucougar Assignee
	Comment 18 • 15 years ago

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

	Theppitak Karoonboonyanan
	Comment 19 • 15 years ago

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

	liucougar Assignee
	Comment 20 • 15 years ago

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/ ?

	Theppitak Karoonboonyanan
	Comment 21 • 15 years ago

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.

	Peter Van der Beken [:peterv]
	Comment 22 • 15 years ago

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.

	liucougar Assignee
	Comment 23 • 15 years ago

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?

	Martijn Wargers (dead)
	Comment 24 • 15 years ago

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.

	liucougar Assignee
	Comment 25 • 15 years ago

thanks Martijn.

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

	Martijn Wargers (dead)
	Comment 26 • 15 years ago

Someone will check the patch in for you.

	Chris Pearce [:cpearce (Not reading bugmail)]
	Comment 27 • 15 years ago

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

	liucougar Assignee
	Comment 28 • 15 years ago

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

	Chris Pearce [:cpearce (Not reading bugmail)]
	Comment 29 • 15 years ago

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.

	liucougar Assignee
	Comment 30 • 15 years ago

Thanks for the information

will look out for tabs when preparing patches

	Chris Pearce [:cpearce (Not reading bugmail)]
	Comment 31 • 15 years ago

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

	liucougar Assignee
	Comment 32 • 15 years ago

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.

	Chris Pearce [:cpearce (Not reading bugmail)]
	Comment 33 • 15 years ago

(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.

	Chris Pearce [:cpearce (Not reading bugmail)]
	Comment 34 • 15 years ago

(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.

	liucougar Assignee
	Comment 35 • 15 years ago

(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)

	Chris Pearce [:cpearce (Not reading bugmail)]
	Comment 36 • 15 years ago

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.

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 37 • 15 years ago

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

Thanks!

	neil@parkwaycc.co.uk
	Comment 38 • 15 years ago

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.

	Chris Pearce [:cpearce (Not reading bugmail)]
	Comment 40 • 15 years ago

(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?

	Samuel Sidler (old account; do not CC)
	Comment 41 • 15 years ago

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.

	Mike Beltzner [:beltzner, not reading bugmail]
	Comment 42 • 15 years ago

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)

	Daniel Veditz [:dveditz]
	Comment 43 • 15 years ago

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

	Serge Gautherie (:sgautherie)
	Comment 44 • 15 years ago

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

	Al Billings [:abillings - ex-MoCo]
	Comment 45 • 15 years ago

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).