117532 - (patch-soap-opera) UI Pref to prevent sites from taking over context menu

Status: VERIFIED FIXED

(SeaMonkey :: Preferences, enhancement)

Description

[Target]

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

[Boris Zbarsky [:bzbarsky]]

bug 70135 is the back end for this.  See also bug 64737.

Comment 2

[Samir Gehani]

Doron, 
Feel free to take this if you have the cycles adn interest in this.

Comment 3

[Constantine A. Murenin]

*** Bug 236643 has been marked as a duplicate of this bug. ***

Comment 4

[Ben Bucksch (:BenB)]

This was a dup of bug 86193, where the backend has been done now.
Making this bug UI-only.
Taking over.

Comment 5

[Robert Accettura [:raccettura]]

Attachment: Seamonkey - Patch v1

My first attempt at a seamonkey patch.

Comment 6

[Robert Accettura [:raccettura]]

Attachment: Seamonkey - Patch v2

A more complete patch.

Comment 7

[Vaclav Dvorak]

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.

Comment 8

[Robert Accettura [:raccettura]]

Attachment: Seamonkey - Patch v3

Updated per comment above

Comment 9

[Christopher Aillon (sabbatical, not receiving bugmail)]

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.

Comment 10

[Robert Accettura [:raccettura]]

Attachment: Seamonkey - Patch v4

Address comment by caillon

Comment 11

[Alex Bishop]

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

Comment 12

[Robert Accettura [:raccettura]]

Attachment: Seamonkey - Patch v5

I hope bugzilla isn't running short on disk space

Comment 13

[Robert Accettura [:raccettura]]

Who can review this?

I'm normally playing with Thunderbird.  So I'm rather clueless here. :-/

Comment 14

[Doron Rosenberg (IBM)]

I would prefer "override" to "disable" :) Oh, and do we reallly need more UI?

Comment 15

[Robert Accettura [:raccettura]]

(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

[Prognathous]

(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

[Vaclav Dvorak]

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

[Prognathous]

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

Comment 19

[Robert Accettura [:raccettura]]

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.

Comment 20

[PikeUK]

>+    prefinverse="true" prefstring="dom.event.contextmenu.enabled"/>

Maybe I've misunderstood something but shouldn't that be prefinverse="false" in
this case?

Comment 21

[[e-mail]]

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

Comment 22

[Robert Accettura [:raccettura]]

Attachment: Seamonkey - Patch v6

Address Neil's comments

Comment 23

[Christopher Blizzard (: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.

Comment 24

[Doron Rosenberg (IBM)]

that is for firefox I think, neil reigns seamonkey ui land

Comment 25

[Robert Accettura [:raccettura]]

Looking at lxr, I think this code works for both no?

I can't find any other instance of this menu.

Comment 26

[Robert Accettura [:raccettura]]

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.

Comment 27

[Johnny Stenback (:jst)]

Comment on attachment 143377 [details] [diff] [review]
Seamonkey - Patch v6

r=jst

Comment 28

[Ben Goodger (use ben at mozilla dot org for email)]

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=[e-mail]

Comment 29

[Robert Accettura [:raccettura]]

Attachment: Patch v7

Final patch, addresses Ben's comment.

I promise next time, a patch this simple won't go through so many tiny
revisions.

Comment 30

[Alex Bishop]

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

[Alex Bishop]

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 32

[Robert Accettura [:raccettura]]

checked in by mconnor

Comment 33

[Prognathous]

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

[[e-mail]]

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?

Comment 35

[Vaclav Dvorak]

Attachment: A fix of previous patch's mixup

Here's a fix... I can't check in, though.

Comment 36

[[e-mail]]

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

Comment 37

[Vaclav Dvorak]

Attachment: s/Override/Disable/

Comment 38

[Mike Kaply [: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

Comment 39

[Robert Accettura [:raccettura]]

(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

[Jacek Piskozub]

Adding a URL for testing.

Comment 41

[Prognathous]

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

Prog.

Comment 42

[Robert Accettura [:raccettura]]

Attachment: Fix 1

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.

Comment 43

[Vaclav Dvorak]

(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

[Zook Valem]

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

[Mike Kaply [:mkaply]]

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.

Comment 46

[Robert Accettura [:raccettura]]

Attachment: Fix 1r2

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.

Comment 47

[[e-mail]]

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

r=me if you need it.

Comment 48

[Robert Accettura [:raccettura]]

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

Comment 49

[Zook Valem]

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

[Vaclav Dvorak]

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.

Comment 51

[Robert Accettura [:raccettura]]

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

Comment 52

[Robert Accettura [:raccettura]]

Sorry for more bugspam:

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

Comment 53

[Mike Kaply [:mkaply]]

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

a=mkaply

Need to get ben to sr this.

Comment 54

[Robert Accettura [:raccettura]]

Attachment: Firefox Patch v1

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.

Comment 55

[Johnny Stenback (: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.

Comment 56

[PikeUK]

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

Comment 57

[Vaclav Dvorak]

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

[Ben Goodger (use ben at mozilla dot org for email)]

Comment on attachment 143744 [details] [diff] [review]
Firefox Patch v1

ok. sr=me

Comment 59

[Vaclav Dvorak]

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!

Comment 60

[Robert Accettura [:raccettura]]

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 [e-mail] r=neil sr=ben a=mkaply

Comment 61

[Robert Accettura [:raccettura]]

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

Perfecto

Comment 62

[Jeremy M. Dolan]

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.

Comment 63

[Robert Accettura [:raccettura]]

(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

[Steffen Wilberg]

Comment on attachment 143744 [details] [diff] [review]
Firefox Patch v1

This wasn't checked in and is now obsolete because of bug 236666.

Comment 65

[Daniel Wang]

CC documentation people
btw, for UI changes please CC at least one of [e-mail] ,
[e-mail] , and [e-mail] so they are documented in Help.