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)
Created attachment 153890 [details] Patch
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.
Created attachment 153892 [details] Patch
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.
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)
Created attachment 153933 [details] Patch
(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.
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.
Created attachment 154286 [details] Patch
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.
Any comments on the last patch which addresses Riosuke's feedback?
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.
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,
(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 :)
Created attachment 156139 [details] Patch
Thank you very much Riosuke and Mike. True, old patch was using duplicated logic.
(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 :)
(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.
Comment on attachment 156139 [details] Patch Can we add a WebKit API test for this?
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?
(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.
Created attachment 156220 [details] Patch
(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.
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.
Created attachment 156225 [details] Patch
(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
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.
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
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).
Created attachment 156269 [details] Patch
(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
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".
Created attachment 156271 [details] Patch
(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!
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.
Created attachment 156279 [details] Patch
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.
Created attachment 156282 [details] Patch
Comment on attachment 156282 [details] Patch Clearing flags on attachment: 156282 Committed r124586: <http://trac.webkit.org/changeset/124586>
All reviewed patches have been landed. Closing bug.