Closed Bug 426246 Opened 12 years ago Closed Last year

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

Categories

(Core :: DOM: Editor, defect)

x86
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla62
Tracking Status
firefox62 --- fixed

People

(Reporter: stubenschrott, Assigned: mstange)

References

Details

Attachments

(1 file, 1 obsolete file)

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.
Sorry, something must have gone wrong in the "Steps to Reproduce", that should read: cmd_deleteToBeginningOfLine instead of 
deleteToBeginningOfLine (twice).
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.
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. :)
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. :)
Attachment #385490 - Flags: review?(peterv)
Status: UNCONFIRMED → NEW
Component: Keyboard Navigation → Editor
Ever confirmed: true
Product: Firefox → Core
QA Contact: keyboard.navigation → editor
Version: unspecified → Trunk
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.
> 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. :(
(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.
Attachment #385490 - Attachment is obsolete: true
Attachment #385490 - Flags: review?(peterv)
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>.
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.
(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 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
Attachment #8984696 - Flags: review?(masayuki) → review+
Pushed by mstange@themasta.com:
https://hg.mozilla.org/integration/autoland/rev/2887cf2355af
cmd_deleteToBeginningOfLine should delete to the beginning of the line, not the whole line. r=masayuki
https://hg.mozilla.org/mozilla-central/rev/2887cf2355af
Status: NEW → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
Assignee: nobody → mstange
Flags: in-testsuite+
You need to log in before you can comment on or make changes to this bug.