Closed Bug 1455087 Opened 6 years ago Closed 6 years ago

Deleting via keyboard doesn't work.

Categories

(Toolkit :: Form Autofill, defect, P3)

defect

Tracking

()

VERIFIED FIXED
mozilla61
Tracking Status
firefox61 --- verified
firefox62 --- verified

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.
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
Mentor: MattN+bmo
Keywords: good-first-bug
Priority: -- → P3
Whiteboard: [lang=js]
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?
Attached patch manageDialog.js (obsolete) — Splinter Review
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!
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)
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)
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)
Yep, just did that and it fixed it! Amazing how streamlined this review workflow is.
Flags: needinfo?(mbkupfer)
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+
Attachment #8969181 - Attachment is obsolete: true
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 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)
Assignee: MattN+bmo → mbkupfer
Attachment #8969474 - Attachment is obsolete: true
Attachment #8969475 - Attachment is obsolete: true
Flags: qe-verify+
Pushed by mozilla@noorenberghe.ca:
https://hg.mozilla.org/integration/autoland/rev/962aeddcff62
Support deleting autofill records with the delete key. r=MattN
Hey Matt, once this bug is finished could you suggest another bug I could work on? 

Thanks,
Maxim
Flags: needinfo?(MattN+bmo)
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)
https://hg.mozilla.org/mozilla-central/rev/962aeddcff62
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
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.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.