Closed
Bug 1455087
Opened 6 years ago
Closed 6 years ago
Deleting via keyboard doesn't work.
Categories
(Toolkit :: Form Autofill, defect, P3)
Toolkit
Form Autofill
Tracking
()
VERIFIED
FIXED
mozilla61
People
(Reporter: mhoye, Assigned: mbkupfer, Mentored)
Details
(Keywords: good-first-bug, Whiteboard: [lang=js])
Attachments
(1 file, 3 obsolete files)
In about:preferences#privacy -> saved addresses, selecting from the list and hitting the "delete" key doesn't delete the selected entries. Hitting "remove" works, though. Current Nightly, current Win10.
Comment 1•6 years ago
|
||
We have a keypress listener to handle ESC so we can handle deletion in the two subclasses (address and credit card). Relevant code: https://dxr.mozilla.org/mozilla-central/rev/789e30ff2e3d6e1fcfce1a373c1e5635488d24da/browser/extensions/formautofill/content/manageDialog.js#204-206,231-240,276,298
Assignee | ||
Comment 2•6 years ago
|
||
Brand new to the, but I feel like I can give this one a try. I don't see anyone assigned, can I take it? Also, I navigated here via the "Mentored bugs". I don't see any title that mentions this is mentored though. Is it?
Assignee | ||
Comment 3•6 years ago
|
||
This seems to do the trick. Let me know if it looks fine. I was also thinking of adding a switch statement and take in different cases, but wasn't sure if it was necessary. Let me know what you think in those regards as well. Thanks!
Comment 4•6 years ago
|
||
Hello Maxim, it looks like you submitted the code file instead of submitting a patch/diff or a MozReview request. Can you please follow the instructions at https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Introduction#Step_4_Get_your_code_reviewed and request review? Thanks
Flags: needinfo?(mbkupfer)
Assignee | ||
Comment 5•6 years ago
|
||
Hi Matt. I followed those instructions (didn't use SSH or LDAP), but the push review doesn't seem to be doing what it is supposed to be doing. when I enter $hg push review it returns: pushing to https://reviewboard-hg.mozilla.org/autoreview searching for appropriate review repository redirecting push to https://reviewboard-hg.mozilla.org/gecko (ignoring public changeset a0c804993efc in review request) abort: no non-public changesets left to reivew (add or change the -r argument to include draft changesets) Any idea what it is I'm doing wrong. Also, I'm not sure whether I need to be in the mozilla-central top directory, or the directory where I made the change when running the hg push review comman. Note: the linux like terminal doesn't support copy paste. I simply retyped the messages so there might be a typo in there.
Flags: needinfo?(mbkupfer) → needinfo?(MattN+bmo)
Comment 6•6 years ago
|
||
Hi Maxim, Did you commit your changes first with `hg commit`? If you want live help #introduction on irc.mozilla.org will be faster.
Assignee: nobody → MattN+bmo
Status: NEW → ASSIGNED
Flags: needinfo?(MattN+bmo) → needinfo?(mbkupfer)
Comment hidden (mozreview-request) |
Assignee | ||
Comment 8•6 years ago
|
||
Yep, just did that and it fixed it! Amazing how streamlined this review workflow is.
Flags: needinfo?(mbkupfer)
Comment 9•6 years ago
|
||
mozreview-review |
Comment on attachment 8969403 [details] Bug 1455087 - Support deleting autofill records with the delete key. https://reviewboard.mozilla.org/r/238144/#review243878 ::: commit-message-a0c80:1 (Diff revision 1) > +Bug 1455087 Deleting via keyboard doesn't work r?MattN Hi Maxim, this is great. We like to describe the fix in the commit message, rather than describing the problem[1]. e.g. > Bug 1455087 - Support deleting autofill records with the delete key. r?MattN Also, generally there should be a hyphen (surrounded by spaces) after the bug number[2]. [1] https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Committing_Rules_and_Responsibilities#Checkin_comment [2] https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Committing_Rules_and_Responsibilities#Commit_message_restrictions ::: browser/extensions/formautofill/content/manageDialog.js:236 (Diff revision 1) > handleKeyPress(event) { > if (event.keyCode == KeyEvent.DOM_VK_ESCAPE) { > window.close(); > } > + if (event.keyCode == KeyEvent.DOM_VK_DELETE) { > + this.removeRecords(this._selectedOptions); Could you also add tests for this behaviour for both addresses and credit cards. You can put the test inside an another add_task of the two existing test files. addresses: browser/extensions/formautofill/test/browser/browser_manageAddressesDialog.js creditCards: browser/extensions/formautofill/test/browser/browser_manageCreditCardsDialog.js See `test_cancelManageCreditCardsDialogWithESC` along with `test_removingSingleAndMultipleCreditCards` and likewise for addresses. See also https://developer.mozilla.org/en-US/docs/Mozilla/Browser_chrome_tests
Attachment #8969403 -
Flags: review?(MattN+bmo) → review+
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Updated•6 years ago
|
Attachment #8969181 -
Attachment is obsolete: true
Comment 12•6 years ago
|
||
mozreview-review |
Comment on attachment 8969475 [details] Bug 1455087 - Support deleting autofill records with the delete key. https://reviewboard.mozilla.org/r/238230/#review243954 Please squash all 3 commits into 1 as we don't squash upon merging and like to review exactly how commits will land. You can use `hg histedit` and choose to "roll" the 2 additional commits into the first. Thanks
Attachment #8969475 -
Flags: review?(MattN+bmo)
Comment 13•6 years ago
|
||
mozreview-review |
Comment on attachment 8969474 [details] Bug 1455087 - Support deleting autofill records with the delete key. https://reviewboard.mozilla.org/r/238228/#review243956 Looks great! I'll push to try server one they are squashed/rolled together.
Attachment #8969474 -
Flags: review?(MattN+bmo)
Updated•6 years ago
|
Assignee: MattN+bmo → mbkupfer
Comment hidden (mozreview-request) |
Assignee | ||
Updated•6 years ago
|
Attachment #8969474 -
Attachment is obsolete: true
Assignee | ||
Updated•6 years ago
|
Attachment #8969475 -
Attachment is obsolete: true
Comment hidden (mozreview-request) |
Updated•6 years ago
|
Flags: qe-verify+
Comment 16•6 years ago
|
||
Pushed by mozilla@noorenberghe.ca: https://hg.mozilla.org/integration/autoland/rev/962aeddcff62 Support deleting autofill records with the delete key. r=MattN
Assignee | ||
Comment 17•6 years ago
|
||
Hey Matt, once this bug is finished could you suggest another bug I could work on? Thanks, Maxim
Flags: needinfo?(MattN+bmo)
Comment 18•6 years ago
|
||
https://bugzilla.mozilla.org/show_bug.cgi?id=1434480 is similar and would be great to have. The code for this bug landed on an integration branch so automated tests are being run on it and if there are no problems it will merge to mozilla-central by tomorrow.
Flags: needinfo?(MattN+bmo)
Comment 19•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/962aeddcff62
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox61:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Comment 20•6 years ago
|
||
I reproduced this issue using Fx 61.0a1 (20180418220025), on Windows 10 x64. I can confirm this issue is fixed, I verified using Fx 62.0a1 (20180509100510) and Fx 61.0b3 (I turned on the "extensions.formautofill.available" prefcode) on Windows 10 x64.
You need to log in
before you can comment on or make changes to this bug.
Description
•