Closed Bug 502259 Opened 11 years ago Closed 10 years ago

Richtext editor: delete at end of paragraph does nothing

Categories

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

x86
Windows XP
defect

Tracking

()

RESOLVED FIXED
mozilla1.9.3a1
Tracking Status
status1.9.2 --- beta1-fixed
status1.9.1 --- .4-fixed

People

(Reporter: liucougar, Assigned: liucougar)

References

Details

(Keywords: regression, testcase, verified1.9.1)

Attachments

(2 files, 6 obsolete files)

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.
Attached file test case for this bug (obsolete) —
Attached file test case for the bug (obsolete) —
Attachment #386758 - Attachment is obsolete: true
forgot to mention: this is a regression from Firefox 3.0.11
Keywords: regression, testcase
place cursor after each "one" in the html, press "delete", nothing happens
Attachment #386759 - Attachment is obsolete: true
actually, place cursor after each "two", and press "delete", nothing happens either
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 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.
Attachment #387691 - Flags: review?(peterv)
thanks, didn't know that. 

I looks like you already requested a review on the patch, thanks.
(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.
Attached patch updated patch and a test case (obsolete) — Splinter Review
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
Attachment #387691 - Attachment is obsolete: true
Attachment #387921 - Flags: review?(thep)
Attachment #387691 - Flags: review?(peterv)
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.
Attachment #387921 - Flags: review?(thep) → review?(peterv)
Comment on attachment 387921 [details] [diff] [review]
updated patch and a test case 

Yeah, I don't think Theppitak has review privileges, PeterV has.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Attachment #387921 - Flags: review?(chris)
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.
Attached patch New patch and test case (obsolete) — Splinter Review
modified this patch according to Chris' suggestions
Attachment #387921 - Attachment is obsolete: true
Attachment #390407 - Flags: review?(chris)
Attachment #387921 - Flags: review?(peterv)
Attachment #387921 - Flags: review?(chris)
(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 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. ;)
Attachment #390407 - Flags: superreview?(peterv)
Attachment #390407 - Flags: review?(chris)
Attachment #390407 - Flags: review+
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.
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
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
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/ ?
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 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.
Attachment #390407 - Flags: superreview?(peterv) → superreview+
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?
Attachment #390407 - Attachment is obsolete: true
Attachment #390849 - Flags: superreview+
Attachment #390849 - Flags: review+
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.
Keywords: checkin-needed
Assignee: nobody → 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?
Someone will check the patch in for you.
Pushed: http://hg.mozilla.org/mozilla-central/rev/a5d741a12327
Status: NEW → RESOLVED
Closed: 10 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
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
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.
status1.9.1: --- → ?
Flags: wanted1.9.2?
Flags: blocking1.9.2?
Thanks for the information

will look out for tabs when preparing patches
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
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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.
Attachment #390849 - Attachment is obsolete: true
Attachment #391527 - Flags: review?(chris)
(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.
(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.
(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 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.
Attachment #391527 - Flags: review?(chris) → review+
Depends on: 507936
http://hg.mozilla.org/mozilla-central/rev/27dc5e92048d

Thanks!
Status: REOPENED → RESOLVED
Closed: 10 years ago10 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
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.
Attachment #391527 - Flags: approval1.9.1.3?
Whiteboard: [tb3needs]
Flags: wanted1.9.2?
Flags: blocking1.9.2?
Flags: blocking1.9.2+
Priority: -- → P2
Duplicate of this bug: 511102
(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?
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.
Whiteboard: [tb3needs] → [tb3needs][awaiting branch approval]
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)
Keywords: fixed1.9.2
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
Attachment #391527 - Flags: approval1.9.1.3? → approval1.9.1.4+
Keywords: checkin-needed
Whiteboard: [tb3needs][awaiting branch approval] → [tb3needs][needs landing on 191]
Target Milestone: --- → mozilla1.9.3a1
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
Attachment #391527 - Attachment description: patch with updated unit tests → patch with updated unit tests [Checkin: Comment 37 & 44]
Keywords: checkin-needed
Whiteboard: [tb3needs][needs landing on 191]
Version: unspecified → Trunk
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).
Keywords: verified1.9.1
You need to log in before you can comment on or make changes to this bug.