Bug 92040 – Delete text from password does nothing

• Status: RESOLVED
• Resolution: FIXED
• Component: HTML Editing

[Carlos]

From Chromium bug: http://code.google.com/p/chromium/issues/detail?id=121547

When issuing a delete command to a password textfield does nothing.
Problem is the command is disabled because of enabledDelete in /trunk/Source/WebCore/editing/EditorCommand.cpp

enabledDelete calls for enabledCut

Problem is, when it is a password field we cannot cut and therefore enabledCut returns false.

Proposed change will now call enabledInEditableText (just to make sure we dont run into the same problem as WebKit issue 56771)

[Carlos]

Created attachment 153890 [details]
Patch

[WebKit Review Bot]

Attachment 153890 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebCore/ChangeLog:6:  Line contains tab character.  [whitespace/tab] [5]
Source/WebCore/ChangeLog:7:  Line contains tab character.  [whitespace/tab] [5]
Source/WebCore/ChangeLog:9:  Line contains tab character.  [whitespace/tab] [5]
Source/WebCore/ChangeLog:11:  Line contains tab character.  [whitespace/tab] [5]
Total errors found: 4 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Carlos]

Created attachment 153892 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 153892 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=153892&action=review

> Source/WebCore/editing/EditorCommand.cpp:1238
> -        return enabledCut(frame, event, source);
> +        return enabledInEditableText(frame, event, source);

If we're making this change, then we should just delete this function and replace the reference in createCommandMap by enabledInEditableText.
Now, according to http://trac.webkit.org/changeset/29300 which added this code, we'll end up enabling the delete in an empty text field by making this change
but that'll be a regression. r- because of this.

[Carlos]

Thanks Ryosuke, then if at the moment we cant issue a delete to a textfield via contextmenu, I believe the option should not appear as an enabled as it may decieve users. (Chromium shows as valid option)

[Carlos]

Created attachment 153933 [details]
Patch

[Ryosuke Niwa]

(In reply to comment #5)
> Thanks Ryosuke, then if at the moment we cant issue a delete to a textfield via contextmenu, I believe the option should not appear as an enabled as it may decieve users. (Chromium shows as valid option)

Enabling delete in password fields is okay. What I'm saying is that we shouldn't be enabling it when the password field is empty.

[Ryosuke Niwa]

Comment on attachment 153933 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=153933&action=review

> Source/WebKit/chromium/src/ContextMenuClientImpl.cpp:178
> -    if (m_webView->focusedWebCoreFrame()->editor()->canDelete())
> +    // Delete is not currently available for password fields: Webkit bug 92040.
> +    if (m_webView->focusedWebCoreFrame()->editor()->canDelete() && !selectedFrame->selection()->isInPasswordField())

I don't think this is the right fix. We should be able to delete text in password fields.

[Carlos]

Created attachment 154286 [details]
Patch

[Carlos]

Previous patches added regression, specifically for editor test "enabling-and-selection" and "enabling-and-selection2" because we were providing always or editable enabled on the field.

New patch provides desired functionality of allowing delete from context menu on password fields and I dont get any regression from it.

Check for password field makes sure we have a valid range, we have a root and we can edit it, test mentioned above now gets enabledWithEditableRange which is the expected result for that test.

[Carlos]

Any comments on the last patch which addresses Riosuke's feedback?

[Ryosuke Niwa]

Comment on attachment 154286 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=154286&action=review

> Source/WebCore/editing/EditorCommand.cpp:1239
> +        // Special handling for password fields: Webkit bug 92040
> +        if (frame->selection()->isInPasswordField())
> +            return frame->selection()->isRange() && enabledInEditableText(frame, event, source) && frame->selection()->isContentEditable();

We shouldn't be duplicating a code in enabledCut. I think all we need to do is to call Editor::canDelete() regardless of whether we're in a password field or not.

[Mike Fenton]

In the blackberry port, we encountered the same issue.

Our solution which has been working well is

diff --git a/Source/WebCore/editing/EditorCommand.cpp b/Source/WebCore/editing/EditorCommand.cpp
index a2f147e..bc637c8 100644
--- a/Source/WebCore/editing/EditorCommand.cpp
+++ b/Source/WebCore/editing/EditorCommand.cpp
@@ -1234,8 +1234,7 @@ static bool enabledDelete(Frame* frame, Event* event, EditorCommandSource source
 {
     switch (source) {
     case CommandFromMenuOrKeyBinding:
-        // "Delete" from menu only affects selected range, just like Cut but without affecting pasteboard
-        return enabledCut(frame, event, source);
+        return frame->editor()->canDelete();
     case CommandFromDOM:
     case CommandFromDOMWithUserInterface:
         // "Delete" from DOM is like delete/backspace keypress, affects selected range if non-empty,

[Ryosuke Niwa]

(In reply to comment #13)
> In the blackberry port, we encountered the same issue.
> 
> Our solution which has been working well is
> 
> diff --git a/Source/WebCore/editing/EditorCommand.cpp b/Source/WebCore/editing/EditorCommand.cpp
> index a2f147e..bc637c8 100644
> --- a/Source/WebCore/editing/EditorCommand.cpp
> +++ b/Source/WebCore/editing/EditorCommand.cpp
> @@ -1234,8 +1234,7 @@ static bool enabledDelete(Frame* frame, Event* event, EditorCommandSource source
>  {
>      switch (source) {
>      case CommandFromMenuOrKeyBinding:
> -        // "Delete" from menu only affects selected range, just like Cut but without affecting pasteboard
> -        return enabledCut(frame, event, source);
> +        return frame->editor()->canDelete();
>      case CommandFromDOM:
>      case CommandFromDOMWithUserInterface:
>          // "Delete" from DOM is like delete/backspace keypress, affects selected range if non-empty,

That's exactly what I've suggested in the comment #12 :)

[Carlos]

Created attachment 156139 [details]
Patch

[Carlos]

Thank you very much Riosuke and Mike.
True, old patch was using duplicated logic.

[Ryosuke Niwa]

(In reply to comment #16)
> Thank you very much Riosuke and Mike.

Note that I'm offended or anything, my name is spelled R*y*osuke :)

[Ryosuke Niwa]

(In reply to comment #17)
> (In reply to comment #16)
> > Thank you very much Riosuke and Mike.
> 
> Note that I'm offended or anything, my name is spelled R*y*osuke :)

Not* that I'm offended.

[Ryosuke Niwa]

Comment on attachment 156139 [details]
Patch

Can we add a WebKit API test for this?

[Carlos]

Thank you Ryosuke, I'll make sure I dont make that typo again hehehehe.

This is my first contrib, could you please explain a little about writing a WebKit API test?
Should I write a LayoutTest for this?

And one more thing: after giving the r+ to the patch, should I ask someone to commit it? or can you do it?

[Ryosuke Niwa]

(In reply to comment #20)
> Thank you Ryosuke, I'll make sure I dont make that typo again hehehehe.
> 
> This is my first contrib, could you please explain a little about writing a WebKit API test?

http://trac.webkit.org/browser/trunk/Tools/TestWebKitAPI

But it appears that we can test this from a layout test.

> Should I write a LayoutTest for this?

Yeah, you can call testRunner.execCommand("delete", false, null) (as opposed to document.execCommand("delete", false, null) after setting the focus on a password field).

> And one more thing: after giving the r+ to the patch, should I ask someone to commit it? or can you do it?

Yes.

[Carlos]

Created attachment 156220 [details]
Patch

[Carlos]

(In reply to comment #21)
> (In reply to comment #20)
> > Thank you Ryosuke, I'll make sure I dont make that typo again hehehehe.
> > 
> > This is my first contrib, could you please explain a little about writing a WebKit API test?
> 
> http://trac.webkit.org/browser/trunk/Tools/TestWebKitAPI
> 
> But it appears that we can test this from a layout test.
> 
> > Should I write a LayoutTest for this?
> 
> Yeah, you can call testRunner.execCommand("delete", false, null) (as opposed to document.execCommand("delete", false, null) after setting the focus on a password field).
> 
> > And one more thing: after giving the r+ to the patch, should I ask someone to commit it? or can you do it?
> 
> Yes.

Re-submitted same patch, but added test.

[Ryosuke Niwa]

Comment on attachment 156220 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=156220&action=review

> LayoutTests/editing/deleting/password-delete-contents-expected.txt:3
> +PASS textField.value == desiredString is true

This output doesn't tell us what we're testing.

> LayoutTests/editing/deleting/password-delete-contents.html:26
> +    textField.value = "helllo";
> +    textField.focus();
> +    textField.selectionStart = 3;
> +    textField.selectionEnd = 4;
> +
> +    testRunner.execCommand("Delete",false,null)
> +    shouldBeTrue('textField.value == desiredString');

It should have been better to include the test code in should* itself as in:
shouldBe("passwordField.value='helllo'; passwordField.setSelectionRange(3, 4); testRunner.execCommand('Delete', false, null); passwordField.value", "'hello'");
so that you can see the test code itself in the expected result.

[Carlos]

Created attachment 156225 [details]
Patch

[Carlos]

(In reply to comment #24)
> (From update of attachment 156220 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=156220&action=review
> 
> > LayoutTests/editing/deleting/password-delete-contents-expected.txt:3
> > +PASS textField.value == desiredString is true
> 
> This output doesn't tell us what we're testing.
> 
> > LayoutTests/editing/deleting/password-delete-contents.html:26
> > +    textField.value = "helllo";
> > +    textField.focus();
> > +    textField.selectionStart = 3;
> > +    textField.selectionEnd = 4;
> > +
> > +    testRunner.execCommand("Delete",false,null)
> > +    shouldBeTrue('textField.value == desiredString');
> 
> It should have been better to include the test code in should* itself as in:
> shouldBe("passwordField.value='helllo'; passwordField.setSelectionRange(3, 4); testRunner.execCommand('Delete', false, null); passwordField.value", "'hello'");
> so that you can see the test code itself in the expected result.
Thank you very much for your patience, surely I've learned a lot from this, resubmitted

[Ryosuke Niwa]

Comment on attachment 156225 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=156225&action=review

> Source/WebCore/ChangeLog:10
> +        

There's a whitespace on this line. I guess it's not a big deal though.
Please remove these whitespaces in the future.

[WebKit Review Bot]

Comment on attachment 156225 [details]
Patch

Rejecting attachment 156225 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 1

ERROR: /mnt/git/webkit-commit-queue/Source/WebCore/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive).

Full output: http://queues.webkit.org/results/13414940

[Ryosuke Niwa]

Comment on attachment 156225 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=156225&action=review

> Source/WebCore/ChangeLog:7
> +        Use Editor::canDelete() to determine if field is editable or not.
> +        Added password-delete-contents test.

Oh oops you've removed Reviewed by line :( You need to leave that line after the bug URL but before these descriptions (surrounded by blank lines).

[Carlos]

Created attachment 156269 [details]
Patch

[Carlos]

(In reply to comment #29)
> (From update of attachment 156225 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=156225&action=review
> 
> > Source/WebCore/ChangeLog:7
> > +        Use Editor::canDelete() to determine if field is editable or not.
> > +        Added password-delete-contents test.
> 
> Oh oops you've removed Reviewed by line :( You need to leave that line after the bug URL but before these descriptions (surrounded by blank lines).

Fixed whitespace and added Reviewed by line on CHANGELOG

[Ryosuke Niwa]

Comment on attachment 156269 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=156269&action=review

> Source/WebCore/ChangeLog:6
> +        Reviewed by Ryosuke Niwa.

In the future, you can leave "Reviewed by NOBODY (OOPS!)".
The commit queue is going to replace that by reviewer's name automatically.
In addition, if you've manually filled in reviewer's name, you can just set cq? instead of both r? and cq?.

> LayoutTests/editing/deleting/password-delete-contents.html:1
> +<!DOCTYPE>

It appears that you're missing a change log entry for LayoutTests.
You can run Tools/Scripts/prepare-ChangeLogs LayoutTests to generate one.
Add a short description like "Added a regression test".

[Carlos]

Created attachment 156271 [details]
Patch

[Carlos]

(In reply to comment #32)
> (From update of attachment 156269 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=156269&action=review
> 
> > Source/WebCore/ChangeLog:6
> > +        Reviewed by Ryosuke Niwa.
> 
> In the future, you can leave "Reviewed by NOBODY (OOPS!)".
> The commit queue is going to replace that by reviewer's name automatically.
> In addition, if you've manually filled in reviewer's name, you can just set cq? instead of both r? and cq?.
> 
> > LayoutTests/editing/deleting/password-delete-contents.html:1
> > +<!DOCTYPE>
> 
> It appears that you're missing a change log entry for LayoutTests.
> You can run Tools/Scripts/prepare-ChangeLogs LayoutTests to generate one.
> Add a short description like "Added a regression test".

LayoutTest changelog entry added. Thanks!

[Ryosuke Niwa]

Comment on attachment 156271 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=156271&action=review

> LayoutTests/ChangeLog:3
> +        Added regression test for password field content delete.

This should appear after Reviewed by Ryosuke Niwa (on a separate line after a blank line).
And this line should be the bug title as in Source/WebCore/ChangeLog.

[Carlos]

Created attachment 156279 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 156279 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=156279&action=review

> Source/WebCore/ChangeLog:3
> +        Improve handling of delete command, specifically for password fields.

Oops, I didn't realize but this is different from the bug title :(
It should be "Delete text from password does nothing".

> LayoutTests/ChangeLog:3
> +        Improve handling of delete command, specifically for password fields.

Ditto.

[Ryosuke Niwa]

Comment on attachment 156279 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=156279&action=review

> Source/WebCore/ChangeLog:3
> +        Improve handling of delete command, specifically for password fields.

Oops, I didn't realize but this is different from the bug title :(
It should be "Delete text from password does nothing".

> LayoutTests/ChangeLog:3
> +        Improve handling of delete command, specifically for password fields.

Ditto.

[Carlos]

Created attachment 156282 [details]
Patch

[WebKit Review Bot]

Comment on attachment 156282 [details]
Patch

Clearing flags on attachment: 156282

Committed r124586: <http://trac.webkit.org/changeset/124586>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.