Closed
Bug 117532
(patch-soap-opera)
Opened 22 years ago
Closed 20 years ago
UI Pref to prevent sites from taking over context menu
Categories
(SeaMonkey :: Preferences, enhancement)
SeaMonkey
Preferences
Tracking
(Not tracked)
VERIFIED
FIXED
Future
People
(Reporter: target, Assigned: raccettura)
References
()
Details
Attachments
(1 file, 11 obsolete files)
1.89 KB,
patch
|
vdvo
:
review+
|
Details | Diff | Splinter Review |
From Bugzilla Helper: User-Agent: Mozilla/5.0 (Windows; U; Win98; en-US; rv:0.9.7+) Gecko/20011231 BuildID: 2001123103 Pages can use javascript to change how a mouse button functions, eg: disable right-click context menu. I'd like to put in a request for a "Scripts & Windows" toggle to allow/disallow such behaviour. Reproducible: Always Steps to Reproduce: 1.Visit any site with context-click-disabling javascript 2.Try to use the right mouse button
![]() |
||
Comment 1•22 years ago
|
||
bug 70135 is the back end for this. See also bug 64737.
Assignee: asa → sgehani
Status: UNCONFIRMED → NEW
Component: Browser-General → Preferences
Depends on: 70135
Ever confirmed: true
QA Contact: doronr → sairuh
Comment 2•22 years ago
|
||
Doron, Feel free to take this if you have the cycles adn interest in this.
Target Milestone: --- → Future
Updated•21 years ago
|
Depends on: oncontextmenu
Comment 3•20 years ago
|
||
*** Bug 236643 has been marked as a duplicate of this bug. ***
Comment 4•20 years ago
|
||
This was a dup of bug 86193, where the backend has been done now. Making this bug UI-only. Taking over.
Assignee: samir_bugzilla → ben.bucksch
Summary: Feature request: Preference item for controlling wether a page can alter the behaviour of a mouse button → UI Pref to prevent sites from taking over content menu
Updated•20 years ago
|
Summary: UI Pref to prevent sites from taking over content menu → UI Pref to prevent sites from taking over context menu
Assignee | ||
Comment 5•20 years ago
|
||
My first attempt at a seamonkey patch.
Assignee: ben.bucksch → robert
Status: NEW → ASSIGNED
Assignee | ||
Comment 6•20 years ago
|
||
A more complete patch.
Attachment #143157 -
Attachment is obsolete: true
Comment 7•20 years ago
|
||
Robert: as the heading of the box is "Allow scripts to:", I suggest changing the text to "Disable context menus" (i.e., don't repeat "allow"). In fact, "Disable or change context menu" might be even better.
Assignee | ||
Comment 8•20 years ago
|
||
Updated per comment above
Attachment #143159 -
Attachment is obsolete: true
Comment 9•20 years ago
|
||
Why "change"? The event has nothing to do with changing the contextmenu. It is a disable/enable pref. Sites which disable the stock contextmenu can create their own completely different menu, but I don't believe that is pertinent to this pref.
Assignee | ||
Comment 10•20 years ago
|
||
Address comment by caillon
Assignee | ||
Updated•20 years ago
|
Attachment #143162 -
Attachment is obsolete: true
Comment 11•20 years ago
|
||
Comment on attachment 143192 [details] [diff] [review] Seamonkey - Patch v4 >+<!ENTITY allowContextmenuDisable.label "Disable context menu"> You either want "Disable the context menu" or "Disable context menus".
Assignee | ||
Comment 12•20 years ago
|
||
I hope bugzilla isn't running short on disk space
Attachment #143192 -
Attachment is obsolete: true
Assignee | ||
Comment 13•20 years ago
|
||
Who can review this? I'm normally playing with Thunderbird. So I'm rather clueless here. :-/
Comment 14•20 years ago
|
||
I would prefer "override" to "disable" :) Oh, and do we reallly need more UI?
Assignee | ||
Comment 15•20 years ago
|
||
(In reply to comment #14) > I would prefer "override" to "disable" :) Oh, and do we reallly need more UI? Asa apparantly thinks we need more UI: http://weblogs.mozillazine.org/asa/archives/005007.html Besides, that's an advanced button. Only people who need it go in there, it's not like yet another button in the toolbar. :-)
Comment 16•20 years ago
|
||
(In reply to comment #14) > Oh, and do we reallly need more UI? There's no need to add a more UI, nor is there any reason to give up functionality. See bug 86193, comment #114 for an alternative approach that doesn't suffer from these shortcomings. Prog.
Comment 17•20 years ago
|
||
I still fail to see how an additional checkbox in preferences->advanced->scripts&plugins->allow scripts to: is a shortcoming. That section of the preferences is _already_ there, and it's there for exactly this purpose. Come on, I can understand worries about making the UI too complex, but these preferences are one of the very things that make Mozilla stand out!
Comment 18•20 years ago
|
||
(In reply to comment #17) > I still fail to see how an additional checkbox in > preferences->advanced->scripts&plugins->allow scripts to: is a shortcoming. The suggested implementation (the checkbox and its backend) has two major shortcomings: 1. It is buried among dozens of other prefs, unlikely to be discovered by most users. 2. Only one context menu is available at a time - either Mozilla's or the website's. In both cases, functionality is missing. Why bother the user with prefs when a single solution can simultaneously answer both needs? Prog.
Assignee | ||
Comment 19•20 years ago
|
||
That location is the only place it will go. It's the obvious place, becauase that's where these types of preferences are located. All of the similar prefs are located in the same spot. It would be illogical to move. This is indeed a feature. A feature we should offer end users, since it's implemented, and most likely something they would want. Users will always be intimidated and confused in about:config. Hence a UI pref is needed. That's the argument for putting it in, and putting it where I propose.
Attachment #143206 -
Flags: review?(neil.parkwaycc.co.uk)
Comment 20•20 years ago
|
||
>+ prefinverse="true" prefstring="dom.event.contextmenu.enabled"/>
Maybe I've misunderstood something but shouldn't that be prefinverse="false" in
this case?
Comment 21•20 years ago
|
||
Comment on attachment 143206 [details] [diff] [review] Seamonkey - Patch v5 Note: patch was malformed. Rather than trying to stitch individual cvs diffs together you should cvs diff all three files at once from the mozilla/xpfe/components/prefwindow/resources directory i.e. [resources]$ cvs diff content/pref-scripts.js content/pref-scripts.xul locale/en-US/pref-scripts.dtd > ~/117532.txt >+ prefinverse="true" prefstring="dom.event.contextmenu.enabled"/> Unlike the other dom.disable_ prefs I think that this one doesn't need prefinverse, fix this and r=me
Attachment #143206 -
Flags: review?(neil.parkwaycc.co.uk) → review+
Assignee | ||
Comment 22•20 years ago
|
||
Address Neil's comments
Attachment #143206 -
Attachment is obsolete: true
Attachment #143377 -
Flags: superreview?(blizzard)
Comment 23•20 years ago
|
||
Comment on attachment 143377 [details] [diff] [review] Seamonkey - Patch v6 This needs to go through jst and ben since they still own the UI here.
Attachment #143377 -
Flags: superreview?(blizzard) → superreview-
Comment 24•20 years ago
|
||
that is for firefox I think, neil reigns seamonkey ui land
Assignee | ||
Comment 25•20 years ago
|
||
Looking at lxr, I think this code works for both no? I can't find any other instance of this menu.
Assignee | ||
Comment 26•20 years ago
|
||
Comment on attachment 143377 [details] [diff] [review] Seamonkey - Patch v6 Ok, now asking jst and ben for a review. I feel like a review slut today. Everyone gets a turn.
Attachment #143377 -
Flags: superreview?(bugs)
Attachment #143377 -
Flags: superreview-
Attachment #143377 -
Flags: review?(jst)
Comment 27•20 years ago
|
||
Comment on attachment 143377 [details] [diff] [review] Seamonkey - Patch v6 r=jst
Attachment #143377 -
Flags: review?(jst) → review+
Comment 28•20 years ago
|
||
Comment on attachment 143377 [details] [diff] [review] Seamonkey - Patch v6 The text is bad... the label preceding the listbox reads: "Allow scripts to:" If you complete that sentence with the checkbox label you provide, you get: "Allow scripts to: Allow Disabling Context Menus" Replace it with "Disable a Page's Context Menu" and sr=ben@mozilla.org
Assignee | ||
Comment 29•20 years ago
|
||
Final patch, addresses Ben's comment. I promise next time, a patch this simple won't go through so many tiny revisions.
Attachment #143377 -
Attachment is obsolete: true
Comment 30•20 years ago
|
||
Comment on attachment 143457 [details] [diff] [review] Patch v7 >+<!ENTITY allowContextmenuDisable.label "Disable a Page's Context Menu"> Case is inconsistent with existing options. Also, to me, "Disable a Page's Context Menu" makes it sound like each page has a different one, which almost makes the option seem to describe the inverse of what it actually does. I'd prefer something like "Disable the standard page context menu".
Comment 31•20 years ago
|
||
Comment on attachment 143457 [details] [diff] [review] Patch v7 >+<!ENTITY allowContextmenuDisable.label "Disable a Page's Context Menu"> Case is inconsistent with existing options. Also, to me, "Disable a Page's Context Menu" makes it sound like each page has a different one, which almost makes the option seem to describe the inverse of what it actually does. I'd prefer something like "Disable the standard page context menu".
Assignee | ||
Comment 32•20 years ago
|
||
checked in by mconnor
Status: ASSIGNED → RESOLVED
Closed: 20 years ago
Resolution: --- → FIXED
Comment 33•20 years ago
|
||
Comment on attachment 143457 [details] [diff] [review] Patch v7 >+<!ENTITY allowContextmenuDisable.label "Disable a Page's Context Menu"> I always thought that a non-living thing can't be a possessor, but granted, English is not my native language, so what do I know... Prog.
Comment 34•20 years ago
|
||
You did not remove prefinverse="true" as pointed out in comment 21 and in comment 20 (which I didn't see as I was reviewing the patch at the time). And why did you have to fiddle with the perfectly good text?
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment 35•20 years ago
|
||
Here's a fix... I can't check in, though.
Updated•20 years ago
|
Attachment #143501 -
Flags: review?(neil.parkwaycc.co.uk)
Comment 36•20 years ago
|
||
Comment on attachment 143501 [details] [diff] [review] A fix of previous patch's mixup >-<!ENTITY allowContextmenuDisable.label "Disable a Page's Context Menu"> >+<!ENTITY allowContextmenuDisable.label "Override context menus"> Actually I preferred disable... although this is all conditional on whether you can get mkaply to approve the wording change part of the patch...
Attachment #143501 -
Flags: review?(neil.parkwaycc.co.uk) → review+
Comment 37•20 years ago
|
||
Updated•20 years ago
|
Attachment #143503 -
Flags: superreview?(mkaply)
Comment 38•20 years ago
|
||
Is context menu the term for this on every platform? Incidentally, this has a down side. Some sites provide their own context menu and they won't work. Just a thought
Assignee | ||
Comment 39•20 years ago
|
||
(In reply to comment #38) > Is context menu the term for this on every platform? AFAIK, yes. Neil: Sorry, it slipped between Patch 6, and 7 somehow. In all honesty, I'm not sure how. Perhaps it would have been easier to just edit the patch file, rather than re-diff.
Comment 40•20 years ago
|
||
Adding a URL for testing.
Comment 41•20 years ago
|
||
How about: "Allow scripts to:"..."Disable or Replace Context Menus"? Prog.
Assignee | ||
Comment 42•20 years ago
|
||
I think this patch accomplishes it all (finally). The wording is about as clear as it can get, and addresses Ben's concern about grammar. It fixes my botched Patch 7 which regressed in addressing Neil's comments.
Assignee | ||
Updated•20 years ago
|
Attachment #143457 -
Attachment is obsolete: true
Comment 43•20 years ago
|
||
(In reply to comment #41) > How about: "Allow scripts to:"..."Disable or Replace Context Menus"? > > Prog. Actually, I proposed something similar in comment 7 and caillon didn't like it in comment 9. Let's not go on making more rounds of patches, please. (In reply to comment #38) > Is context menu the term for this on every platform? Yes, I believe it's a universal term. > Incidentally, this has a down side. Some sites provide their own context menu > and they won't work. I'd say that's the whole point. :-) Just as we have an option to disallow sites to change e.g. the status bar, we now have an option to disallow overriding the default context menu. In both cases there may be a loss of functionality, which is why it's not turned on by default, but it's still a good option to have because these things are often abused by websites. All that's left to do now is check in the trivial fix that 1) reverses the actual meaning of the checkbox in the prefs dialog, 2) improves the wording. The patch needs sr and, presumably, approval for checkin into the frozen tree, although I don't know whether that's necessary for a change as trivial as this. (In reply to comment #42) > I think this patch accomplishes it all (finally). The wording is about as > clear as it can get, and addresses Ben's concern about grammar. It fixes my > botched Patch 7 which regressed in addressing Neil's comments. Unfortunately, this new patch uses capitalization inconsistent with the other options in the dialog. Notice that all the other options have only one capital letter whereas the new one capitalises all words. I recommend to use attachment 143503 [details] [diff] [review].
Comment 44•20 years ago
|
||
Why not just check-in those patches. If nobody is happy with the wording then file a new bug to change that wording. That way, we have a working feature by then.
Comment 45•20 years ago
|
||
No. the wording has to be right. Localization if frozen after 1.7b so if it's not right for 1.7b it isn't getting changed for 1.7.
Assignee | ||
Comment 46•20 years ago
|
||
I agree, lets get it right. It's not like bugzilla is running out of space. Lets get the absolute clearest, perfect description in there. If we change it a later, it messes up documentation ppl will put together on the web. Just like marking junk mail in mailnews. My nomination with cAsE fix.
Attachment #143516 -
Attachment is obsolete: true
Updated•20 years ago
|
Alias: patch-soap-opera
Comment 47•20 years ago
|
||
Comment on attachment 143522 [details] [diff] [review] Fix 1r2 r=me if you need it.
Assignee | ||
Comment 48•20 years ago
|
||
(In reply to comment #47) > (From update of attachment 143522 [details] [diff] [review]) > r=me if you need it. > Thanks. And I think I can manage to keep the fix intact this time (still pissed about how I messed that up last night). What I we could use is a decision on the wording at this point. I agree 100% with mkaply that we should decide NOW, not file another bug over it. But what should it be? Nominate to block 1.7b, so that we can apply some preasure to those who need to make a decision. ;-)
Flags: blocking1.7b?
Comment 49•20 years ago
|
||
Well, I wasn't referring to 1.7 in comment #45, more like 1.8, 1.9, 2.0 or something which was what I meant. No hard feeling here. :-) I like the wording with the latest patch and it look good/satisfactory. Let's see what everyone else had to say about the wording, hopefully they would lead toward to agreeing with you on the latest wording this time.
Comment 50•20 years ago
|
||
Comment on attachment 143522 [details] [diff] [review] Fix 1r2 Marking as reviewed (by neil), requesting sr by mkaply and approval for 1.7b. I think this is as good as it gets.
Attachment #143522 -
Flags: superreview?(mkaply)
Attachment #143522 -
Flags: review+
Attachment #143522 -
Flags: approval1.7b?
Updated•20 years ago
|
Attachment #143503 -
Attachment is obsolete: true
Attachment #143503 -
Flags: superreview?(mkaply)
Assignee | ||
Comment 51•20 years ago
|
||
I can't commit, so upon sr and driver approval for 1.7b, would someone checkin?
Updated•20 years ago
|
Attachment #143501 -
Attachment is obsolete: true
Assignee | ||
Comment 52•20 years ago
|
||
Sorry for more bugspam: This should pass by ben as well, since he sr'd conditionally in comment 28 about the wording.
Comment 53•20 years ago
|
||
Comment on attachment 143522 [details] [diff] [review] Fix 1r2 a=mkaply Need to get ben to sr this.
Attachment #143522 -
Flags: superreview?(mkaply)
Attachment #143522 -
Flags: superreview?(bugs)
Attachment #143522 -
Flags: approval1.7b?
Attachment #143522 -
Flags: approval1.7b+
Assignee | ||
Comment 54•20 years ago
|
||
Took a stab at a Firefox patch. I've never really checked out the source for the pref manager, so this was more a learning experience for me, rather than a patch for everone. A bit different than the SeaMonkey prefs, but now that I played a bit, I think it makes more sense logically the way it was done.
Assignee | ||
Updated•20 years ago
|
Attachment #143744 -
Flags: superreview?(bugs)
Attachment #143744 -
Flags: review?(jst)
Comment 55•20 years ago
|
||
Comment on attachment 143744 [details] [diff] [review] Firefox Patch v1 I'll let Ben deal with this one, it's all FireFox code, so his review is enough.
Attachment #143744 -
Flags: review?(jst)
Comment 56•20 years ago
|
||
I already attached a patch for Firefox in Bug 236666 last week, do you want me to remove that one?
Comment 57•20 years ago
|
||
If this doesn't get checked in real soon, 1.7b is going to be released with the meaning of the pref reversed (the prefinverse="true" thing)! Ben, if you're receiving, could you have a look at this? Thanks!
Comment 58•20 years ago
|
||
Comment on attachment 143744 [details] [diff] [review] Firefox Patch v1 ok. sr=me
Attachment #143744 -
Flags: superreview?(bugs) → superreview+
Comment 59•20 years ago
|
||
Erm... Thanks, Ben, but I actually meant attachment 143522 [details] [diff] [review] for SeaMonkey, not attachment 143744 [details] [diff] [review] for FireFox. Anyway, the wording in both is identical and that's what the debate was about, so I think we can assume the patch has sr. Now, could someone check it in, please? Thanks!
Assignee | ||
Comment 60•20 years ago
|
||
Fixed. Checkin by Timeless: 03/15/2004 20:41 timeless%mozdev.org mozilla/ xpfe/ components/ prefwindow/ resources/ locale/ en-US/ pref-scripts.dtd 1.10 1/1 Bug 117532 UI Pref to prevent sites from taking over context menu patch by robert@accettura.com r=neil sr=ben a=mkaply
Status: REOPENED → RESOLVED
Closed: 20 years ago → 20 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 61•20 years ago
|
||
Verified in: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7b) Gecko/20040316 Perfecto
Status: RESOLVED → VERIFIED
Updated•20 years ago
|
Flags: blocking1.7b?
Comment 62•20 years ago
|
||
This is marked fixed and has been inactive, but it doesn't seem as if the Firefox patch was checked in. I don't see the UI in yesterday's build. Is there a seperate bug? Is there a Seamonkey porting tracking bug? Or someone actually paying attention? I grow more and more concerned valid features are getting lost in the shuffle.
Assignee | ||
Comment 63•20 years ago
|
||
(In reply to comment #62) > This is marked fixed and has been inactive, but it doesn't seem as if the > Firefox patch was checked in. I don't see the UI in yesterday's build. Is there > a seperate bug? Is there a Seamonkey porting tracking bug? Or someone actually > paying attention? I grow more and more concerned valid features are getting lost > in the shuffle. That became Bug 236666. We are still pending on that bug waiting for Ben to review.
Comment 64•20 years ago
|
||
Comment on attachment 143744 [details] [diff] [review] Firefox Patch v1 This wasn't checked in and is now obsolete because of bug 236666.
Attachment #143744 -
Attachment is obsolete: true
Updated•20 years ago
|
Attachment #143377 -
Flags: superreview?(bugs)
Updated•20 years ago
|
Attachment #143522 -
Flags: superreview?(bugs)
Comment 65•20 years ago
|
||
CC documentation people btw, for UI changes please CC at least one of robinf@formerly-netscape.com.tld , stolenclover@yahoo.com.tw , and rlk@mozdev.org so they are documented in Help.
Updated•19 years ago
|
Product: Browser → Seamonkey
You need to log in
before you can comment on or make changes to this bug.
Description
•