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)

Product:
SeaMonkey
Component:
Preferences
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
VERIFIED FIXED
Milestone:
Future

People

(Reporter: target, Assigned: raccettura)

References

(
URL
)

Details

Attachments

(1 file, 11 obsolete files)

Fix 1r2
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
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
Doron, 
Feel free to take this if you have the cycles adn interest in this.
Target Milestone: --- → Future
Depends on: oncontextmenu
OS: Windows 98 → All
Hardware: PC → All
*** Bug 236643 has been marked as a duplicate of this bug. ***
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
URL: http://can't remember
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
Summary: UI Pref to prevent sites from taking over content menu → UI Pref to prevent sites from taking over context menu
Attached patch Seamonkey - Patch v1 (obsolete) — — Splinter Review 
My first attempt at a seamonkey patch.
Assignee: ben.bucksch → robert
Status: NEW → ASSIGNED
Attached patch Seamonkey - Patch v2 (obsolete) — — Splinter Review 
A more complete patch.
Attachment #143157 - Attachment is obsolete: true
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.
Attached patch Seamonkey - Patch v3 (obsolete) — — Splinter Review 
Updated per comment above
Attachment #143159 - Attachment is obsolete: true
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.
Attached patch Seamonkey - Patch v4 (obsolete) — — Splinter Review 
Address comment by caillon
Attachment #143162 - Attachment is obsolete: true
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".
Attached patch Seamonkey - Patch v5 (obsolete) — — Splinter Review 
I hope bugzilla isn't running short on disk space
Attachment #143192 - Attachment is obsolete: true
Who can review this?

I'm normally playing with Thunderbird.  So I'm rather clueless here. :-/
I would prefer "override" to "disable" :) Oh, and do we reallly need more UI?
(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. :-)
(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.
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!
(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.
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)
>+    prefinverse="true" prefstring="dom.event.contextmenu.enabled"/>

Maybe I've misunderstood something but shouldn't that be prefinverse="false" in
this case?
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+
Attached patch Seamonkey - Patch v6 (obsolete) — — Splinter Review 
Address Neil's comments
Attachment #143206 - Attachment is obsolete: true
Attachment #143377 - Flags: superreview?(blizzard)
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-
that is for firefox I think, neil reigns seamonkey ui land
Looking at lxr, I think this code works for both no?

I can't find any other instance of this menu.
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 on attachment 143377 [details] [diff] [review]
Seamonkey - Patch v6

r=jst
Attachment #143377 - Flags: review?(jst) → review+
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
Attached patch Patch v7 (obsolete) — — Splinter Review 
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 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 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".
checked in by mconnor
Status: ASSIGNED → RESOLVED
Closed: 20 years ago
Resolution: --- → FIXED
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.
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 → ---
Attached patch A fix of previous patch's mixup (obsolete) — — Splinter Review 
Here's a fix... I can't check in, though.
Attachment #143501 - Flags: review?(neil.parkwaycc.co.uk)
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+
Attached patch s/Override/Disable/ (obsolete) — — Splinter Review 

    
  

        Attachment #143503 -
        Flags: superreview?(mkaply)

    
    

    
  
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

    
    

    
  
(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.

    
    

    
  
Adding a URL for testing.
URL: http://filesharing.megacodecpack.com/

    
    

    
  
How about: "Allow scripts to:"..."Disable or Replace Context Menus"?

Prog.

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Fix 1 (obsolete)
        — 
          — Splinter Review
      

    


  
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.

    
  

        Attachment #143457 -
        Attachment is obsolete: true

    
    

    
  
(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].

    
    

    
  
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.

    
    

    
  
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.

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Fix 1r2
        — 
          — Splinter Review
      

    


  
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

    
  
Alias: patch-soap-opera

    
    

    
  
Comment on attachment 143522 [details] [diff] [review]
Fix 1r2

r=me if you need it.

    
    

    
  
(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?

    
    

    
  
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 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?

    
  

        Attachment #143503 -
        Attachment is obsolete: true

        Attachment #143503 -
        Flags: superreview?(mkaply)

    
    

    
  
I can't commit, so upon sr and driver approval for 1.7b, would someone checkin?

    
  

        Attachment #143501 -
        Attachment is obsolete: true

    
    

    
  
Sorry for more bugspam:

This should pass by ben as well, since he sr'd conditionally in comment 28 about
the wording.

    
    

    
  
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+

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Firefox Patch v1 (obsolete)
        — 
          — Splinter Review
      

    


  
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.

    
  

        Attachment #143744 -
        Flags: superreview?(bugs)

        Attachment #143744 -
        Flags: review?(jst)

    
    

    
  
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)

    
    

    
  
I already attached a patch for Firefox in Bug 236666 last week, do you want me
to remove that one?

    
    

    
  
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 on attachment 143744 [details] [diff] [review]
Firefox Patch v1

ok. sr=me

        Attachment #143744 -
        Flags: superreview?(bugs) → superreview+

    
    

    
  
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!

    
    

    
  
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

    
    

    
  
Verified in:
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7b) Gecko/20040316

Perfecto
Status: RESOLVED → VERIFIED

    
  
Flags: blocking1.7b?

    
    

    
  
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.

    
    

    
  
(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 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

    
  

        Attachment #143377 -
        Flags: superreview?(bugs)

    
  

        Attachment #143522 -
        Flags: superreview?(bugs)

    
    

    
  
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.
Product: Browser → Seamonkey

          You need to log in
          before you can comment on or make changes to this bug.