Closed Bug 1256724 Opened 4 years ago Closed 2 years ago

Update The Data Manager, and the Cookie Viewer so as to treat backspace and delete equivalently on Mac OS X.

Categories

(SeaMonkey :: Passwords & Permissions, defect)

defect
Not set

Tracking

(seamonkey2.49esr fixed, seamonkey2.53 affected, seamonkey2.54 wontfix, seamonkey2.55 fixed, seamonkey2.56 fixed)

RESOLVED FIXED
seamonkey2.56
Tracking Status
seamonkey2.49esr --- fixed
seamonkey2.53 --- affected
seamonkey2.54 --- wontfix
seamonkey2.55 --- fixed
seamonkey2.56 --- fixed

People

(Reporter: philip.chee, Assigned: frg, Mentored)

References

Details

(Whiteboard: [good first bug][gfb][lang=js])

Attachments

(1 file, 2 obsolete files)

(In Bug 431901 comment #0)

> In Firefox's Cookies dialog, the selected cookie can be deleted by pressing
> the Remove Cookie button or the Delete Forward key (Del on platforms other
> than the Mac). On Mac laptops, which don't have a dedicated DelFwd key,
> Fn+Delete is used -- not a very discoverable shortcut. So in addition to
> DelFwd, the dialog should allow the Delete (Backspace) key to remove the
> cookie.

It seems that in most places in SeaMonkey we already treat both DOM_VK_DELETE and DOM_VK_BACK_SPACE as delete.

The main places that need to be updated are the Data Manager and the Cookie Viewer.
> http://mxr.mozilla.org/comm-central/source/suite/common/permissions/cookieViewer.js
> http://mxr.mozilla.org/comm-central/source/suite/common/dataman/dataman.js?force=1
> http://mxr.mozilla.org/comm-central/source/suite/common/dataman/dataman.xml?force=1
Mentor: philip.chee
Attached patch bug-1256724-fix.patch (obsolete) — Splinter Review
Hi. I've created the attached patch for this. Is it alright or do I need to change something?
Comment on attachment 8774230 [details] [diff] [review]
bug-1256724-fix.patch

(In reply to Prashant Baisla from comment #1)
> Created attachment 8774230 [details] [diff] [review]
> bug-1256724-fix.patch
> 
> Hi. I've created the attached patch for this. Is it alright or do I need to
> change something?

This is great! Thank you.

> do I need to  change something?
Yes you need to request a review from a SeaMonkey reviewer. To request a review you should set the review flag to "?" which I have done this for you.

Next you wait for the review.
Attachment #8774230 - Flags: review?(frgrahl)
Comment on attachment 8774230 [details] [diff] [review]
bug-1256724-fix.patch

Prashant, 

this fix works well but has one problem. In the Cookieviewer if I delete the cookie with the backspace key the cookie is deleted but I am also taken back to the last visited page. This is because the backspace key is also used as the Back key if browser.backspace_action is set to 0 which is the default. This is handled differently in the Data Manager. Could you add a e.StopPropagation(); after the DeleteCookie(); and retest it on OSX. This works for me on Windows. 

f+ for now. If it works please upload a new patch and I will give it an r+

>> function HandleCookieKeyPress(e) {
>> -  if (e.keyCode == KeyEvent.DOM_VK_DELETE) {
>> +  if (e.keyCode == KeyEvent.DOM_VK_DELETE || e.keyCode == KeyEvent.DOM_VK_BACK_SPACE) {
>>     DeleteCookie();
>>   }
>> }
Attachment #8774230 - Flags: review?(frgrahl)
Attachment #8774230 - Flags: review-
Attachment #8774230 - Flags: feedback+
Attached patch 1256724-backspace-V2.patch (obsolete) — Splinter Review
Stefan,

it looks like Prashant is no longer around. Adjusted the patch for bitrot and put the stopPropagation in. Could you check if this works on OSX and give it an r+ if its ok. Works fine on Windows.
Assignee: nobody → frgrahl
Attachment #8774230 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #8787700 - Flags: review?(stefanh)
 function HandleCookieKeyPress(e) {
-  if (e.keyCode == KeyEvent.DOM_VK_DELETE) {
+  if (e.keyCode == KeyEvent.DOM_VK_DELETE || e.keyCode == KeyEvent.DOM_VK_BACK_SPACE) {
     DeleteCookie();
+    e.StopPropagation();
   }

During what circumstances is this ('e.StopPropagation();') supposed to work? Afaik I can only edit cookies in the data manager...
Flags: needinfo?(frgrahl)
>> During what circumstances is this ('e.StopPropagation();') supposed to work? Afaik I can only edit cookies in the data manager...

This is the old cookie manager from:

chrome://communicator/content/permissions/cookieViewer.xul

Under Windows ESC closes the dialog there which is somewhat contraproductive if you just want to delete a cookie whit this key. 

FRG
Flags: needinfo?(frgrahl)
Sorry for the confusion. Replace ESC with BACKSPACE in my comments. Blush...
(In reply to Frank-Rainer Grahl from comment #7)
> Sorry for the confusion. Replace ESC with BACKSPACE in my comments. Blush...

OK. But I still don't get it. I mean, of course I can load chrome://communicator/content/permissions/cookieViewer.xul in a browser window but then Backspace will both delete and move 1 step back in history... (regardless of the StopPropagation() at least for me). It will only work if you load the chrome url in a new window/tab, because then you don't have any history - but then the StopPropagation() wouldn't be needed?
frg,

+    e.StopPropagation();

This line doesn't make any difference. If you open the old cookie manager in a new tab or a new window, backspace won't take you back (since there's nothing to move back to). If you open the old cookie manager in an existing window which has the back button enabled, you'll move back in history even with the above line.

So, just push the patch without that line ;-)
Stefan,

I didn't have much time yesterday. Want to look into this again probably next weekend. When I tested it first in Windows it would delete the cookie and then go back to the last web page in one step if you open the xul in a tab. With the line it would just delete the cookie and not go back.
Comment on attachment 8787700 [details] [diff] [review]
1256724-backspace-V2.patch

Cancelling the request, I think you want the other patch.
Attachment #8787700 - Flags: review?(stefanh)
>> Cancelling the request, I think you want the other patch.

Didn't have much time to look at it. Briefly checked and the stoppropagation definitely makes a difference under Windows. Will try to sort it out and put up a new patch when I am sure which would be best.
Using Backspace as Delete does actually only makes sense under OSX.
Attachment #8787700 - Attachment is obsolete: true
Comment on attachment 8931893 [details] [diff] [review]
1256724-backspace-V3.patch

Stefan at long last :) Looks good to me now under Windows. Didn't test it in OSX yet.
Attachment #8931893 - Flags: review?(stefanh)
Comment on attachment 8931893 [details] [diff] [review]
1256724-backspace-V3.patch

Looks good to me. You'll need to open the old cookie manager in a new tab/window without history, but I don't think many people use it.
Attachment #8931893 - Flags: review?(stefanh) → review+
Pushed by frgrahl@gmx.net:
https://hg.mozilla.org/comm-central/rev/0954b13949a2
Treat both DOM_VK_DELETE and DOM_VK_BACK_SPACE in OSX as Delete. r=stefanh
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Comment on attachment 8931893 [details] [diff] [review]
1256724-backspace-V3.patch

[Approval Request Comment]
Regression caused by (bug #): --
User impact if declined: backspace on OSX can not be used to delete items.
Testing completed (on m-c, etc.): c-r 2.53 
Risk to taking this patch (and alternatives if risky): none trivial
String changes made by this patch: --

IanN in retrospect. Does it make sense to put the 3 lines in the dataman.js ifs into a separate funktion? I first thought not worth it.
Attachment #8931893 - Flags: approval-comm-esr52?
Attachment #8931893 - Flags: approval-comm-beta?
Target Milestone: --- → Seamonkey2.56
Comment on attachment 8931893 [details] [diff] [review]
1256724-backspace-V3.patch

a=me
Attachment #8931893 - Flags: approval-comm-esr52?
Attachment #8931893 - Flags: approval-comm-esr52+
Attachment #8931893 - Flags: approval-comm-beta?
Attachment #8931893 - Flags: approval-comm-beta+
You need to log in before you can comment on or make changes to this bug.