Closed Bug 1365957 Opened 2 years ago Closed 2 years ago

Pressing backspace on the cookies dialog to delete a cookie also removes the dialog

Categories

(Firefox :: Preferences, defect)

51 Branch
defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 56
Tracking Status
firefox-esr52 --- affected
firefox53 --- wontfix
firefox54 --- wontfix
firefox55 --- wontfix
firefox56 --- verified

People

(Reporter: florian, Assigned: prathiksha, Mentored)

References

Details

Attachments

(1 file, 1 obsolete file)

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.
Blocks: 1294989
Mentor: jhofmann
Flags: needinfo?(prathikshaprasadsuman)
Keywords: regression
See Also: → 1342472
Version: unspecified → 51 Branch
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.
Flags: needinfo?(jhofmann)
I'll do it. :)
Flags: needinfo?(prathikshaprasadsuman)
Assignee: nobody → prathikshaprasadsuman
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 :)
Attachment #8870090 - Attachment is obsolete: true
Attachment #8870090 - Flags: review?(jhofmann)
Yes, you can add to browser_cookies_exceptions.js. Thanks!
Status: NEW → ASSIGNED
Flags: qe-verify+
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.
Attachment #8887123 - Flags: review?(jhofmann) → review-
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
Attachment #8887123 - Flags: review?(jhofmann) → review-
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. :)
Attachment #8887123 - Flags: review?(jhofmann) → review+
Keywords: checkin-needed
Keywords: checkin-needed
Keywords: checkin-needed
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
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/e1f21848af1a
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 56
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?
Flags: needinfo?(prathikshaprasadsuman)
(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.
Flags: needinfo?(prathikshaprasadsuman)
Marking this issue as VERIFIED FIXED based on comment 22.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.