462188 - Option-Delete / Ctrl-Backspace does not delete words in designmode textareas or Thunderbird message body editor

Status: RESOLVED FIXED

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

Description

[Daniel Einspanjer [:dre] [:deinspanjer]]

User Agent: Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b2pre) Gecko/20081029 Minefield/3.1b2pre

Steps to Reproduce:
1. Visit sandbox forum of vBulletin site in URL field above (note it seems that all websites using vBulletin appear to be affected by this regression.  I've tested four that I know of.)
2. Log in with the credentials
     Username: "Test for Firefox"
     Password: "password"
3. Click the "New Thread" button
4. Type multiple words in the rich textarea
5. Attempt to use Command-Delete to delete a word

Actual Results:
At most, one space is deleted, words are never deleted.

Expected Results:
Any whitespace and the entire previous word up to the next instance of whitespace should be deleted.
Firefox 2.x, 3.0.x, and 3.1b1 exhibit this correct behavior 

I tried to reproduce this with the latest Windows nightly of Minefield, but it crashes upon clicking the "Log In" button after entering credentials in StR above.

Comment 1

[Mardeg]

http://crash-stats.mozilla.com/report/index/02b26173-a5eb-11dd-83e8-001cc45a2ce4

Comment 2

[Boris Zbarsky [:bzbarsky]]

Daniel, does disabling content jit prevent the crash?

We probably want to file a separate bug on the crash, and use this bug for the Command-Delete problem...  I'd much appreciate someone hunting down a one-day regression range here...

Comment 3

[Martijn Wargers (dead)]

Disabling content jit makes the crash go away, I just filed bug 462219 for that crash.

Comment 4

[Daniel Einspanjer [:dre] [:deinspanjer]]

So disabling jit on the latest nightly eliminated the crash for me as well.  Now that I can log in with the latest nightly on Windows, I can verify that the inability to delete words happens on Windows as well, using ctrl-backspace.
Setting platform/os to All/All since I can't select just Mac+Windows.

Comment 5

[Martijn Wargers (dead)]

Attachment: testcase (uses enhanced privileges)

Comment 6

[Martijn Wargers (dead)]

It regressed between 2008-10-15 and 2008-10-16:
http://hg.mozilla.org/mozilla-central/pushloghtml?startdate=2008-10-14+04%3A00%3A00&enddate=2008-10-16+09%3A00%3A00
My guess is a regression from bug 157546.

Comment 8

[Daniel Einspanjer [:dre] [:deinspanjer]]

Trying to make the subject a little more specific and less un-related.

Comment 9

[Theppitak Karoonboonyanan]

Attachment: Call ExtendSelectionForDelete() earlier

This patch fixes my committed patch in Bug 157546. The order of selection extending has been restored back to previous state for the cases unrelated to the bug.

Comment 10

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

Thanks! Could you add this bug to the mochitests and request review from peterv?

Comment 11

[Theppitak Karoonboonyanan]

Attachment: Updated patch, with mochitest added

Mochitest added according to roc's comment. Please review my patch. Thanks.

Comment 12

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

The test isn't going to work on Mac because Mac uses option-backspace to delete a word, not ctrl-backspace. So on Mac you should use altKey. Copy wordModifiers from test_word_movement.html and use that, that should do it.

Comment 13

[Daniel Einspanjer [:dre] [:deinspanjer]]

Erm.. fixing title to reflect the correct metakey for Mac.  Should be Option-Delete (the "Alt" key), not Command (the "Mac" key)

Comment 14

[Theppitak Karoonboonyanan]

Attachment: Updated patch, fixing mochitest for Mac

OK. Updated the mochitest for Mac. Requesting for the review again.

Comment 15

[Theppitak Karoonboonyanan]

Some rationales for the patch, in case it helps the review process:

- ExtendSelectionForDelete() must be called after CheckBidiLevelForDeletion(), to prevent Bug 419406.

- Calling it before 'if (aAction == nsIEditor::eNone) return NS_OK;' will leave the cases that were not handled by WillDoAction() before applying the patch in Bug 157546 unhandled. And nsPlaintextEditor::DeleteSelection() will pass it on to DeleteSelectionImpl() as usual.

- ExtendSelectionForDelete() may have changed the selection. So, startNode and startOffset need to be updated

- Some selection extensions, such as for ePreviousWord direction, modify the selection start, rather than the end. So, both selection ends must be retrieved, not just the ending as done in Bug 157546.

Comment 19

[Susurrus]

I'm also experiencing this bug. It doesn't happen in the latest Firefox nightlies for me (Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b3pre) Gecko/20081206 Shiretoko/3.1b3pre), but it happens in the latest Thunderbird (Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b3pre) Gecko/20081205 Shredder/3.0b2pre). Option-Fn-Delete (deleting forward instead of backwards on my laptop) does work however.

Comment 20

[Peter Van der Beken [:peterv]]

Comment on attachment 348720 [details] [diff] [review]
Updated patch, fixing mochitest for Mac

There are several cases where after calling ExtendSelectionForDelete we still cancel the action (look for |*aCancel = PR_TRUE|). I think this can only happen for aAction==ePrevious and aAction==eToEndOfLine, but we would be changing the selection without deleting. Isn't that wrong?

Comment 21

[Theppitak Karoonboonyanan]

(In reply to comment #20)
> (From update of attachment 348720 [details] [diff] [review])
> There are several cases where after calling ExtendSelectionForDelete we still
> cancel the action (look for |*aCancel = PR_TRUE|). I think this can only happen
> for aAction==ePrevious and aAction==eToEndOfLine, but we would be changing the
> selection without deleting. Isn't that wrong?

OK. But the difference between the two cases is that, for aAction==ePrevious, no selection was changed (or if actually changed as noted in the FIXME comment, aAction would be set to eNone), while it was actually changed for aAction==eToEndOfLine.

In the code before Bug 157546, the selection change would cause bCollapse to be false on start, and the whole "if (bCollapse) ..." block would not be executed at all. To retain that old execution path, bCollapse may also need to be refreshed and re-checked after the ExtendSelectionForDelete() call.

I'll prepare such patch soon. But could you suggest how to test the aAction==eToEndOfLine case? What shortcut is for such command?

Comment 22

[Theppitak Karoonboonyanan]

Attachment: updated patch, with bCollapsed re-checked

Here is the patch as mentioned in comment #21.

Comment 24

[Valery Kondakoff]

Please, consider adding a keyboard shortcut to delete the current line while in Mail Compose window!

Comment 25

[Peter Van der Beken [:peterv]]

(In reply to comment #21)
> In the code before Bug 157546, the selection change would cause bCollapse to be
> false on start, and the whole "if (bCollapse) ..." block would not be executed
> at all. To retain that old execution path, bCollapse may also need to be
> refreshed and re-checked after the ExtendSelectionForDelete() call.

I'm actually wondering if you should use an nsAutoSelectionReset to undo the changes to the selection when canceling.

> I'll prepare such patch soon. But could you suggest how to test the
> aAction==eToEndOfLine case? What shortcut is for such command?

platformHTMLBindings.xml says ctrl-k on OS X and Linux.

Comment 27

[Theppitak Karoonboonyanan]

(In reply to comment #25)
> (In reply to comment #21)
> > In the code before Bug 157546, the selection change would cause bCollapse to be
> > false on start, and the whole "if (bCollapse) ..." block would not be executed
> > at all. To retain that old execution path, bCollapse may also need to be
> > refreshed and re-checked after the ExtendSelectionForDelete() call.
> 
> I'm actually wondering if you should use an nsAutoSelectionReset to undo the
> changes to the selection when canceling.

I'm afraid that would be too big change, which alters the logic of existing code. It may be too risky.

> > I'll prepare such patch soon. But could you suggest how to test the
> > aAction==eToEndOfLine case? What shortcut is for such command?
> 
> platformHTMLBindings.xml says ctrl-k on OS X and Linux.

Unfortunately, I haven't been successful overriding Minefield's ctrl-k shortcut for the search bar. And I don't have enough space to checkout and build Thunderbird for now.. Is there an easy way to disable the search bar shortcut?

Comment 28

[Peter Van der Beken [:peterv]]

(In reply to comment #27)
> I'm afraid that would be too big change, which alters the logic of existing
> code. It may be too risky.

I don't see where we were changing the selection *and* canceling in the existing code?

Comment 29

[Theppitak Karoonboonyanan]

(In reply to comment #28)
> I don't see where we were changing the selection *and* canceling in the
> existing code?

No, there was no such attempt. So, I try to keep up with that logic, to bring back the previously working mechanism before applying patch in Bug 157546, i.e. attachment #342183 [details] [diff] [review], as explained in comment #21.

Before that patch was applied, selection was extended for aAction==eToEndOfLine in nsPlaintextEditor::DeleteSelection(), and aAction set to eNext, before entering WillDoAction(). So, nsHTMLEditRules::WillDeleteSelection() received such condition on entry in the previous code, which caused the whole "if (bCollapsed) ..." block to be skipped. Thus, re-checking bCollapse after ExtendSelectionForDelete() in the new code simply brought back the same execution steps.

Comment 30

[Peter Van der Beken [:peterv]]

Comment on attachment 353781 [details] [diff] [review]
updated patch, with bCollapsed re-checked

Ok, I see what you mean now.

On OS X ctrl-k works. You could try to use an iframe in designMode for the test. Or you could try to use a textarea and get its editor through .editor (probably need some privileges). Then you should be able to call deleteSelection(nsIEditor::eToEndOfLine) on the editor.

Comment 31

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

I think we can land this without waiting for the extra to-end-of-line test.

Comment 33

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I landed this on mozilla-central:
http://hg.mozilla.org/mozilla-central/rev/9b832d90d637

Not sure whether to mark fixed yet given the request for more tests...

Comment 34

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

And I had to back it out:
http://hg.mozilla.org/mozilla-central/rev/f5436f305f3a
http://hg.mozilla.org/mozilla-central/rev/58b777f8849a
due to test failures.

The tests passed on Mac.

On Windows, there was one failure:

*** 42583 ERROR TEST-UNEXPECTED-FAIL | /tests/layout/generic/test/test_backspace_delete.html | Delete next word broken in "text." - got [object Text], expected [object HTMLDivElement]

On Linux, there were 7 failures:

*** 42526 ERROR TEST-UNEXPECTED-FAIL | /tests/layout/generic/test/test_backspace_delete.html | Delete previous word broken in "You should not see this text." - got 29, expected 24
*** 42528 ERROR TEST-UNEXPECTED-FAIL | /tests/layout/generic/test/test_backspace_delete.html | Delete previous word broken in "You should not see this text." - got 29, expected 19
*** 42530 ERROR TEST-UNEXPECTED-FAIL | /tests/layout/generic/test/test_backspace_delete.html | Delete previous word broken in "You should not see this text." - got 29, expected 15
*** 42532 ERROR TEST-UNEXPECTED-FAIL | /tests/layout/generic/test/test_backspace_delete.html | Delete previous word broken in "You should not see this text." - got 29, expected 11
*** 42534 ERROR TEST-UNEXPECTED-FAIL | /tests/layout/generic/test/test_backspace_delete.html | Delete previous word broken in "You should not see this text." - got 29, expected 4
*** 42535 ERROR TEST-UNEXPECTED-FAIL | /tests/layout/generic/test/test_backspace_delete.html | Delete previous word broken in "You should not see this text." - got [object Text], expected [object HTMLDivElement]
*** 42536 ERROR TEST-UNEXPECTED-FAIL | /tests/layout/generic/test/test_backspace_delete.html | Delete previous word broken in "You should not see this text." - got 29, expected 0

Comment 35

[Theppitak Karoonboonyanan]

The tests pass so far on my system (Debian unstable, amd64), with direct build from trunk fresh checkout today. (My previous builds were lost with the crashed harddisk. So, I'm forced to start from scratch.)

Comment 36

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

Those tests pass on my Linux VM with the patch. :-(

Comment 37

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

In Linux we respect the native keybindings. Maybe the keybindings are defined strangely on the Tinderbox VM so that ctrl-backspace doesn't work?

Comment 38

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

Attachment: fix

With this patch, I'm changing the test to use editor commands directly without going through key events. That should prevent any problems with key bindings. I also found a problem with testDeleteNextWord which I'm fixing in the patch, hence the need for more review.

-- In test_selection_move_commands.xul, runTest isn't really needed, it's left over from some copy-paste job

-- In nsHTMLEditRules.cpp, we shouldn't do "if (aAction == nsIEditor::eNone) return NS_OK;" after ExtendSelectionForDelete as we did in the last iteration of the patch. ExtendSelectionForDelete will set aAction to eNone if it successfully extended the selection and no more work needs to be done by the caller to extend the selection. We still need to *delete* the selection, though. In this case I think we can just ignore any changes to aAction by ExtendSelectionForDelete, since we're going to check bCollapsed again. This fixes the deleteNextWord tests, which I think wasn't being tested properly at all with the current tests, because we weren't checking editor.textContent in testDeleteNextWord.

-- Move test_backspace_delete to a XUL file so we can access the commandDispatcher. The tests themselves run in an IFRAME. I originally designed this setup for test_selection_move_commands.xul. The tests are basically the same except I added tests of editor.textContent in testDeletePrevWord and testDeleteNextWord. The tests for testDeleteNextWord need to specify that the editor makes the first space a  .

Comment 39

[Theppitak Karoonboonyanan]

(In reply to comment #38)
> -- In nsHTMLEditRules.cpp, we shouldn't do "if (aAction == nsIEditor::eNone)
> return NS_OK;" after ExtendSelectionForDelete as we did in the last iteration
> of the patch. ExtendSelectionForDelete will set aAction to eNone if it
> successfully extended the selection and no more work needs to be done by the
> caller to extend the selection. We still need to *delete* the selection,
> though. In this case I think we can just ignore any changes to aAction by
> ExtendSelectionForDelete, since we're going to check bCollapsed again. This
> fixes the deleteNextWord tests, which I think wasn't being tested properly at
> all with the current tests, because we weren't checking editor.textContent in
> testDeleteNextWord.

Just for clarification of my patch, the selection is actually deleted later by DeleteSelectionImpl() call in nsPlaintextEditor::DeleteSelection(), after nsHTMLEditRules::WillDoAction() returns. And in fact, the test still passes even when adding editor.textContent check to testDeleteNextWord on my machine.

BTW, I'll try your patch soon. It looks cleaner not to rely on cross-function workflow. I was just too reluctant to try different execution paths.

Comment 41

[Peter Van der Beken [:peterv]]

(In reply to comment #39)
> Just for clarification of my patch, the selection is actually deleted later by
> DeleteSelectionImpl() call in nsPlaintextEditor::DeleteSelection(), after
> nsHTMLEditRules::WillDoAction() returns.

Yes, that's how I understood the patch. What exactly was broken by doing this?

Comment 42

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

I'm not sure, I didn't know it should still work via DeleteSelectionImpl. But the tests were failing. I guess I'll fire up the VM again...

Comment 43

[Peter Van der Beken [:peterv]]

Comment on attachment 357604 [details] [diff] [review]
fix

I'll + this, it'd be nice to know the answer to comment 41, but if it works let's just take it.

Comment 44

[Axel Hecht [:Pike]]

This landed, http://hg.mozilla.org/mozilla-central/rev/10dfe13222aa, and I suspect it made mochitest hang, so I'm backing this out. I don't get precise error information, the last thing in the logs are tests that come shortly before the libeditor ones.

Backout is http://hg.mozilla.org/mozilla-central/rev/df6affd823b8.

Comment 45

[Axel Hecht [:Pike]]

FWIW, we're green again after that backout now. There were other test failures landing on top, and things went green slower than I thought, so I can't totally rule out that the mochitest timeout was an artifact of system load or anything. Someone with better understanding of those mechanics would need to look.

Comment 46

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

It certainly does look like this caused the timeouts, although it makes no sense to be mostly hanging in postMessage tests --- I'm pretty sure that we don't do any buffering that would cause us to misreport the hang position.

Comment 47

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

Needless to say, when I apply the patch, mochitests don't hang on my machine :-(.

Comment 48

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

Attachment: Theppitak's fix with changed tests

This is Theppitak's code, with my XUL test conversion. The tests failed on my Mac, which sent me off bug-hunting which led to my additional code changes, but the real cause of the failures was that when my test deletes the last word via  testDeletePrevWord(editor,  0, ""), the editor inserts a bogus empty node, and the setting of .innerHTML in the next test isn't noticed by the editor so it thinks the content is still empty and bad things happen from there. So I worked around that bug again by toggling designMode after setting .innerHTML. Now the tests all pass.

I did notice, though, that the testDeleteNextWord tests are actually producing the wrong content output. They should have a leading  , not a leading normal space, so that a space is visible at the start of the line when you delete the first word (on Mac you need this at least, to match other Mac apps). My patch above actually fixed this bug. But for now, the minimal code change is probably best.

Since the last checkin of Theppitak's patch did not seem to cause mochitest hangs, I think we should try relanding this version and see what happens.

Comment 49

[Peter Van der Beken [:peterv]]

I haven't checked this, I see a hang in editor/libeditor/base/tests/test_selection_move_commands.xul but haven't been able to debug it yet.

Comment 50

[Peter Van der Beken [:peterv]]

The problem with the hanging test was the removal of the runTest function without removing the yields in the rest of the code. Removing both runTest and the yields made some tests fail, so I've just removed the changes to test_selection_move_commands.xul. They were orthogonal to this bug anyway.

Landed on trunk, and mochitests pass.

http://hg.mozilla.org/mozilla-central/rev/11e029835944

Comment 51

[Peter Van der Beken [:peterv]]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/0226bdfe6b36

(In reply to comment #48)
> I did notice, though, that the testDeleteNextWord tests are actually producing
> the wrong content output. They should have a leading  , not a leading
> normal space, so that a space is visible at the start of the line when you
> delete the first word (on Mac you need this at least, to match other Mac apps).

We should file a bug on this, right?

Comment 52

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

(In reply to comment #50)
> The problem with the hanging test was the removal of the runTest function
> without removing the yields in the rest of the code. Removing both runTest and
> the yields made some tests fail, so I've just removed the changes to
> test_selection_move_commands.xul. They were orthogonal to this bug anyway.

Sorry! Too bad that didn't fail on my machine :-(

> Landed on trunk, and mochitests pass.
> 
> http://hg.mozilla.org/mozilla-central/rev/11e029835944

Yay!!! Thanks!

(In reply to comment #51)
> (In reply to comment #48)
> > I did notice, though, that the testDeleteNextWord tests are actually producing
> > the wrong content output. They should have a leading  , not a leading
> > normal space, so that a space is visible at the start of the line when you
> > delete the first word (on Mac you need this at least, to match other Mac apps).
> 
> We should file a bug on this, right?

Yes.

Comment 55

[Dusan Halicky]

Can somebody please confirm it is the same bug as described in this minimal testcase:

http://komiks.xf.cz/bug/cannot_delete.html

It is still present in Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.2a1pre) Gecko/20090425 Minefield/3.6a1pre

Or should i file a new bug?

Comment 56

[Karl Tomlinson (:karlt)]

Nice testcase, thanks Dusan.

It seems that must be a different bug, as this bug should be fixed in that version (and the discription sounds a little different).

Can you file a new bug, please?

Comment 57

[Dusan Halicky]

>> Can you file a new bug, please?

Done: #490554