426246 - cmd_deleteToBeginningOfLine should delete to the beginning of the line, not the whole line

Status: RESOLVED FIXED

(Core :: DOM: Editor, defect)

Description

[Martin Stubenschrott]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9b5pre) Gecko/2008032404 Minefield/3.0b5pre
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9b5pre) Gecko/2008032404 Minefield/3.0b5pre

Starting with Firefox3, when I issue the cmd_deleteToBeginningOfLine with a script in a textarea, it should just do that... and not delete the whole line.

Reproducible: Always

Steps to Reproduce:
1. Write abc|def in a textbox (| denotes the cursor)
2. Execute the following javascript code in a setTimeout, so that you can give focus to a textfield:

var controller = window.document.commandDispatcher.focusedElement.controllers.getControllerForCommand("deleteToBeginningOfLine");
controller.doCommand("deleteToBeginningOfLine");

Actual Results:  
The whole line is erased.

Expected Results:  
Only abc is erased, textbox reads "def" now.

Comment 1

[Martin Stubenschrott]

Sorry, something must have gone wrong in the "Steps to Reproduce", that should read: cmd_deleteToBeginningOfLine instead of 
deleteToBeginningOfLine (twice).

Comment 2

[ce]

This doesn't look like it's going anywhere, but i guess i'll second this, because it's an issue for the Mac as well (you could even call it a platform-integration bug) — ⌘Backspace is supposed to behave exactly as described by Martin (delete from cursor to beginning, not clear the whole line). This is one of a handful of standard Cocoa bindings that aren't implemented in platformHTMLBindings.xml, and probably should be, but the point is somewhat moot because of this bug (wouldn't behave as intended anyway, obviously).

I think there should be a separation between the current behaviour and the desired behaviour. cmd_deleteToBeginningOfLine should work like ⌘Backspace (or ^U in readline), and the current behaviour should be moved to a new command like cmd_deleteLine.

Comment 3

[ce]

I've just grabbed the source for 3.5RC3 and i think i found the problem. If you look at nsPlaintextEditor.cpp around line 695 it says:

      case eToBeginningOfLine:
        selCont->IntraLineMove(PR_TRUE, PR_FALSE);          // try to move to end
        result = selCont->IntraLineMove(PR_FALSE, PR_TRUE); // select to beginning
        *aAction = eNone;
        break;

I assume all that needs to be done to correct the behaviour then is to remove the 'try to move to end' part here? So, if you did that and added a new case for clearing the line i guess you'd have something like this:

      case eToBeginningOfLine:
        result = selCont->IntraLineMove(PR_FALSE, PR_TRUE); // select to beginning
        *aAction = eNone;
        break;
      case eEntireLine:
        selCont->IntraLineMove(PR_TRUE, PR_FALSE);          // try to move to end
        result = selCont->IntraLineMove(PR_FALSE, PR_TRUE); // select to beginning
        *aAction = eNone;
        break;

And then you would have add the appropriate references in nsEditorCommands.cpp and a few other places, i suppose. It seems like relatively easy stuff just looking at it, but i'm not a programmer at all, so that's the best i can do. :)

Comment 4

[ce]

Attachment: patch to change cmd_deleteToBeginningOfLine and move previous behaviour to cmd_deleteEntireLine

This patch alters cmd_deleteToBeginningOfLine so that it deletes back from the cursor rather than clearing the entire line. The latter/previous behaviour is moved to a new command, cmd_deleteEntireLine. Tested on trunk, works OK on Mac -- however you will have to alter your platformHTMLBindings.xml after building in order to test both functions (since there is no built-in reference to the new command obviously). The only thing that concerns me is a reference to these delete commands under /widget/src/gtk2/nsNativeKeyBindings.cpp -- this is a GTK thing, which i don't know much about and have no way of testing. :)

Comment 5

[Peter Van der Beken [:peterv]]

Comment on attachment 385490 [details] [diff] [review]
patch to change cmd_deleteToBeginningOfLine and move previous behaviour to cmd_deleteEntireLine

(In reply to comment #0)
> Starting with Firefox3, when I issue the cmd_deleteToBeginningOfLine with a
> script in a textarea, it should just do that... and not delete the whole line.

This code hasn't really changed since 2001, so if this a regression in FF3 I'd like to know what patch regressed this before we start changing it. Are you sure this worked fine in FF2?

This looks ok to me, but I wouldn't add eEntireLine in this patch. If someone thinks we need that they should file a new bug. Looking at the GTK code I don't think we need any changes there. Is there a bug about ⌘Backspace? We also need a mochitest for this.

Comment 6

[ce]

> This looks ok to me, but I wouldn't add eEntireLine in this patch. If someone
thinks we need that they should file a new bug.

I don't personally see the need for it, but i figured someone must have made it work like that for a reason, so i thought i'd try to be thorough. :) If an 'eEntireLine' feature is not required then i guess the change is even easier.

> Is there a bug about ⌘Backspace?

You could say there is. Firefox for Mac is missing (or has incorrectly implemented) several of the standard Cocoa key bindings, of which ⌘Backspace is one. Its behaviour should be equal to the desired behaviour of cmd_deleteToBeginningOfLine here (the same as ^U in readline). Should i create a new bug for the missing/incorrect bindings?

> We also need a mochitest for this.

I'm not sure if this is directed at me, but if it is i'm afraid i may not be knowledgeable enough to do this. :(

Comment 7

[Nochum Sossonko [:Natch]]

(In reply to comment #6)
> > We also need a mochitest for this.
> 
> I'm not sure if this is directed at me, but if it is i'm afraid i may not be
> knowledgeable enough to do this. :(

Mochitests are outlined on this page: https://developer.mozilla.org/en/Mochitest and https://developer.mozilla.org/en/Mochitest#Writing_tests explains how to make them. It's an automated test system, so that this doesn't regress in the future.

Comment 8

[Markus Stange [:mstange]]

Attachment: Bug 426246 - cmd_deleteToBeginningOfLine should delete to the beginning of the line, not the whole line.

Review commit: https://reviewboard.mozilla.org/r/250552/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/250552/

Comment 9

[Markus Stange [:mstange]]

The incorrect behavior was present when this code was first introduced in bug 66290.
https://searchfox.org/mozilla-central/diff/fb60392b419394f600b392bffa4fa2a0b07e97ae/editor/libeditor/text/nsPlaintextEditor.cpp#931

Comment 10

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

Comment on attachment 8984696 [details]
Bug 426246 - cmd_deleteToBeginningOfLine should delete to the beginning of the line, not the whole line.

https://reviewboard.mozilla.org/r/250552/#review256862

::: editor/libeditor/tests/test_bug426246.html:19
(Diff revision 1)
> +<div contenteditable="true" id="contenteditable1">
> +  <p>first line</p>
> +  <p>this is the second line</p>
> +</div>
> +<pre id="test">
> +
> +<script class="testbody" type="application/javascript">
> +SimpleTest.waitForExplicitFinish();
> +SimpleTest.waitForFocus(function() {
> +  let elm = document.getElementById("contenteditable1");
> +  elm.focus();
> +  window.getSelection().collapse(elm.lastElementChild.firstChild, "this is the ".length);
> +
> +  SpecialPowers.doCommand(window, "cmd_deleteToBeginningOfLine");
> +
> +  is(elm.firstElementChild.textContent, "first line", "the first line should stay untouched");
> +  is(elm.lastElementChild.textContent, "second line", "the characters after the caret should remain");

Could you add some other tests?
1. To test how it behaves when the caret is after a <br> element.
2. To test how it behaves when the caret is in second <li> element in a list.
3. To test how it behaves when the caret is after a line break in <pre>.

Comment 11

[Markus Stange [:mstange]]

Comment on attachment 8984696 [details]
Bug 426246 - cmd_deleteToBeginningOfLine should delete to the beginning of the line, not the whole line.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/250552/diff/1-2/

Comment 12

[Markus Stange [:mstange]]

I've added the test. I didn't add a lot of variation though, they're all basically the same test. For example I'm not testing what happens when the caret is at the start of the line.

Comment 13

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

(In reply to Markus Stange [:mstange] from comment #12)
> I've added the test. I didn't add a lot of variation though, they're all
> basically the same test. For example I'm not testing what happens when the
> caret is at the start of the line.

The code path of line handling in various elements are different, unfortunately :-(

Comment 14

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

Comment on attachment 8984696 [details]
Bug 426246 - cmd_deleteToBeginningOfLine should delete to the beginning of the line, not the whole line.

https://reviewboard.mozilla.org/r/250552/#review257450

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2887cf2355af
cmd_deleteToBeginningOfLine should delete to the beginning of the line, not the whole line. r=masayuki

Comment 16

[Arthur Iakab [arthur_iakab]]

https://hg.mozilla.org/mozilla-central/rev/2887cf2355af