When using the backspace key in the cookies dialog opened from about:preferences#privacy, the selected cookies get removed, but the event's default action isn't prevented, so it also triggers the 'back' navigation, which removes the dialog.

This is the same issue as bug 1342472. I'm really not sure why bug 1294989 deleted those. Prathiksha, would you like to own this one? It's really easy (just add event.preventDefault to https://searchfox.org/mozilla-central/rev/79f625ac56d936ef5d17ebe13a123b25efee4241/browser/components/preferences/cookies.js#735), but we would ideally want to uplift this to beta soon, which is why I'm hesitant to make it a good first bug.

I'm not sure why you have listed bug 1294989 as causing this. The patch from that bug didn't change behavior for onCookieKeyPress: https://hg.mozilla.org/mozilla-central/rev/90324e0a64e7#l3.188 onCookieKeyPress never[1][2][3] called event.preventDefault(), though I agree that it should. [1] http://searchfox.org/mozilla-central/rev/80f663c70857bdbdb03bf59d01a1f8f30ce55314/browser/components/preferences/cookies.js#721 [2] http://searchfox.org/mozilla-central/rev/797f4c4fe4ac878b1f8be8c4aebef1a7b20dd157/browser/components/preferences/cookies.js#765 [3] http://searchfox.org/mozilla-central/rev/79f625ac56d936ef5d17ebe13a123b25efee4241/browser/components/preferences/cookies.js#734

Oh, sorry. Bug 1342472 marks bug 1294989 as the regression for a very similar issue and I saw that the latest blame here was from your patch. That's where I stopped checking. Thanks for correcting that! We might still uplift this, the patch should be pretty simple.

I'll do it. :)

Great, can you please also add a test for this? I think https://searchfox.org/mozilla-central/source/browser/components/preferences/in-content/tests/browser_cookies_exceptions.js is the right test file but you should check with jaws in case of doubt.

ok :)

Yes, you can add to browser_cookies_exceptions.js. Thanks!

Comment on attachment 8887123 [details] Bug 1365957 - Pressing backspace on the cookies dialog to delete a cookie no longer removes the dialog. https://reviewboard.mozilla.org/r/157874/#review163458 This looks good, just a couple of small issues. Not sure whether it's necessary to put this test in browser/components/preferences/in-content/ as well, maybe jaws knows more about the state of that directory. ::: browser/components/preferences/cookies.js:737 (Diff revision 2) > this.deleteCookie(); > + aEvent.preventDefault(); > } else if (AppConstants.platform == "macosx" && > aEvent.keyCode == KeyEvent.DOM_VK_BACK_SPACE) { > this.deleteCookie(); > + aEvent.preventDefault(); I don't really understand why this is not a single if condition instead of two branches, since the two branches are exactly the same. Can you transform this into a single if, please? I know it's not your code, but that would be much cleaner. ::: browser/components/preferences/in-content-new/tests/browser.ini:59 (Diff revision 2) > [browser_searchsuggestions.js] > [browser_security.js] > [browser_siteData.js] > [browser_siteData2.js] > [browser_site_login_exceptions.js] > +[browser_cookies_dialog_test.js] I don't think it's necessary to append _test to your test file, it's pretty obvious that it's a test ;) ::: browser/components/preferences/in-content-new/tests/browser_cookies_dialog_test.js:5 (Diff revision 2) > +"use strict"; > + > +Components.utils.import("resource://gre/modules/Services.jsm"); > +var cookieService = Components.classes["@mozilla.org/cookieService;1"] > + .getService(Components.interfaces.nsICookieService); I think there's Services.cookies, which you can use instead. ::: browser/components/preferences/in-content-new/tests/browser_cookies_dialog_test.js:9 (Diff revision 2) > +var cookieService = Components.classes["@mozilla.org/cookieService;1"] > + .getService(Components.interfaces.nsICookieService); > + > +const COOKIES_URL = "chrome://browser/content/preferences/cookies.xul"; > + > +var uri = Services.io.newURI("http://www.example.com"); nit: const URI ::: browser/components/preferences/in-content-new/tests/browser_cookies_dialog_test.js:26 (Diff revision 2) > + }); > + > + cookiesDialog = await dialogOpened; > +}); > + > +add_task(async function TestACookie() { Nit: might want to give this a different name (testDeleteCookie?) ::: browser/components/preferences/in-content-new/tests/browser_cookies_dialog_test.js:48 (Diff revision 2) > + await waitForCondition(() => tree.view.rowCount == 0); > + > + is_element_visible(content.gSubDialog._dialogs[0]._box, > + "Subdialog is visible after deleting an element"); > + > + gBrowser.removeCurrentTab(); You should probably do this in a separate task named "cleanup" to not mess with anyone adding additional tests to this file.

Comment on attachment 8887123 [details] Bug 1365957 - Pressing backspace on the cookies dialog to delete a cookie no longer removes the dialog. https://reviewboard.mozilla.org/r/157874/#review164014 ::: browser/components/preferences/cookies.js:732 (Diff revision 3) > this.focusFilterBox(); > }, > > onCookieKeyPress(aEvent) { > - if (aEvent.keyCode == KeyEvent.DOM_VK_DELETE) { > - this.deleteCookie(); > + if (aEvent.keyCode == KeyEvent.DOM_VK_DELETE || > + AppConstants.platform == "macosx" && You need to put the latter two conditions in parentheses, I think. ::: browser/components/preferences/in-content-new/tests/browser_cookies_dialog.js:7 (Diff revision 3) > + > +Components.utils.import("resource://gre/modules/Services.jsm"); > + > +const COOKIES_URL = "chrome://browser/content/preferences/cookies.xul"; > + > +const uri = Services.io.newURI("http://www.example.com"); Nit: please uppercase URI

Comment on attachment 8887123 [details] Bug 1365957 - Pressing backspace on the cookies dialog to delete a cookie no longer removes the dialog. https://reviewboard.mozilla.org/r/157874/#review164014 > You need to put the latter two conditions in parentheses, I think. No, I checked the precedence. It should work.

Comment on attachment 8887123 [details] Bug 1365957 - Pressing backspace on the cookies dialog to delete a cookie no longer removes the dialog. https://reviewboard.mozilla.org/r/157874/#review164128 You are right, of course. Thank you for including the parentheses anyway, for folks like me who are less familiar with the JS operator precedence table. :)

Pushed by cbook@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/e1f21848af1a Pressing backspace on the cookies dialog to delete a cookie no longer removes the dialog. r=johannh

I managed to reproduce this bug using Nightly from 2017-05-18 on macOS 10.12. Pressing "Backspace" from keyboard actually triggered the "back" navigation option. I also tested it using latest Nightly (2017-08-16), Firefox RC 55.0.2 and beta 56.0b3, but the bug is not reproducing here anymore. I also tested it using the previously mentioned builds on Windows 10 x64 and Ubuntu 16.04. This problem didn't appear there, but I noticed that both on Windows and Ubuntu you can't delete the content from the first file you access. And on the latest Nightly (2017-08-16) using Ubuntu, pressing "Backspace" doesn't do anything at all. Is this something that you are aware about and maybe tracked in some other bug/s?

(In reply to Bogdan Maris, QA [:bogdan_maris] from comment #21) > I managed to reproduce this bug using Nightly from 2017-05-18 on macOS > 10.12. Pressing "Backspace" from keyboard actually triggered the "back" > navigation option. I also tested it using latest Nightly (2017-08-16), > Firefox RC 55.0.2 and beta 56.0b3, but the bug is not reproducing here > anymore. > > I also tested it using the previously mentioned builds on Windows 10 x64 and > Ubuntu 16.04. This problem didn't appear there, but I noticed that both on > Windows and Ubuntu you can't delete the content from the first file you > access. And on the latest Nightly (2017-08-16) using Ubuntu, pressing > "Backspace" doesn't do anything at all. > Is this something that you are aware about and maybe tracked in some other > bug/s? Yes, I think this is expected. Pressing backspace should only delete on OSX, for deleting on the other platforms you should use the delete key. At the very least it wasn't regressed in this bug because that's what the code did before as well.

Marking this issue as VERIFIED FIXED based on comment 22.