Closed
Bug 1256724
Opened 8 years ago
Closed 6 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)
SeaMonkey
Passwords & Permissions
Tracking
(seamonkey2.49esr fixed, seamonkey2.54 wontfix, seamonkey2.55 fixed, seamonkey2.56 fixed, seamonkey2.53+ fixed)
RESOLVED
FIXED
seamonkey2.56
People
(Reporter: philip.chee, Assigned: frg, Mentored)
References
Details
(Whiteboard: [good first bug][gfb][lang=js])
Attachments
(1 file, 2 obsolete files)
8.10 KB,
patch
|
stefanh
:
review+
iannbugzilla
:
approval-comm-beta+
iannbugzilla
:
approval-comm-esr52+
|
Details | Diff | Splinter Review |
(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
Reporter | ||
Updated•8 years ago
|
Mentor: philip.chee
Comment 1•8 years ago
|
||
Hi. I've created the attached patch for this. Is it alright or do I need to change something?
Reporter | ||
Comment 2•8 years ago
|
||
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)
Assignee | ||
Comment 3•8 years ago
|
||
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+
Assignee | ||
Comment 4•8 years ago
|
||
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)
Comment 5•8 years ago
|
||
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)
Assignee | ||
Comment 6•8 years ago
|
||
>> 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)
Assignee | ||
Comment 7•8 years ago
|
||
Sorry for the confusion. Replace ESC with BACKSPACE in my comments. Blush...
Comment 8•8 years ago
|
||
(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?
Comment 9•8 years ago
|
||
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 ;-)
Assignee | ||
Comment 10•8 years ago
|
||
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.
Updated•8 years ago
|
status-firefox48:
affected → ---
Comment 11•8 years ago
|
||
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)
Assignee | ||
Comment 12•8 years ago
|
||
>> 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.
Assignee | ||
Comment 13•6 years ago
|
||
Using Backspace as Delete does actually only makes sense under OSX.
Attachment #8787700 -
Attachment is obsolete: true
Assignee | ||
Comment 14•6 years ago
|
||
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 15•6 years ago
|
||
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+
Comment 16•6 years ago
|
||
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: 6 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 17•6 years ago
|
||
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?
Assignee | ||
Updated•6 years ago
|
status-seamonkey2.49esr:
--- → affected
status-seamonkey2.53:
--- → affected
status-seamonkey2.54:
--- → wontfix
status-seamonkey2.55:
--- → affected
status-seamonkey2.56:
--- → fixed
Target Milestone: --- → Seamonkey2.56
Comment 18•6 years ago
|
||
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+
Assignee | ||
Comment 19•6 years ago
|
||
https://hg.mozilla.org/releases/comm-beta/rev/741471a1f3246daeac9a88666cad7868daedcd6b https://hg.mozilla.org/releases/comm-esr52/rev/bbc00efc7636d3662ac387a014e1c857c5594461
Assignee | ||
Comment 20•4 years ago
|
||
Target 2.53.1
https://gitlab.com/seamonkey-project/seamonkey-2.53-comm/-/commit/6ee467afcbadaa7d6fa313ae139cd6959416fd0c
tracking-seamonkey2.53:
--- → +
You need to log in
before you can comment on or make changes to this bug.
Description
•