30878 - [ESM/CSS] Right-click != Left-click

Status: VERIFIED FIXED

(Core :: XUL, defect, P2)

Description

[Henrik Gemal]

(Almost) all over Mozilla you can right-click on a Ok or Cancel or other buttons 
and the button gets activated.

EX: go into preferences. Right-click on Ok and your prefs get saved!

This should NOT be the case! Right-clicking on a button is not the same a 
left-clicking!

Expected:
The right-click should be ignored!

Comment 1

[Peter Trudelle]

reassigning to evaughan as p2 for m16, since this could plausibly have
unintended bad consequences.

Comment 2

[Peter Trudelle]

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

Comment 3

[Eric Vaughan]

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

Comment 4

[John Morrison]

Above, it says that "bug #20020 has been as a duplicate" but on bug #20020
the corresponding comment/resolution-change has not been made. 

They could certainly be marked as duplicates, but evaughan was accepting 20020
at the same time that this duplicate annotation was added here. (?)

(cc: terry if he wants to investigate how this happened). 

Comment 5

[Eric Vaughan]

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

Comment 6

[Peter Trudelle]

Mass moving M17 bugs to M18

Comment 7

[Eric Vaughan]

Tom,

How do we handle this? A button moves to the active state whether or not you 
press the right or left button. CSS shows a depressed look when :active. How 
can we fix this?

Comment 8

[Eric Vaughan]

I think we need some kind of new pseudo styles. Right now we have :active but we 
really need 

:active1 - active only when the left button is pressed
:active2 - active only when right button is pressed

and then :active would be left or right.

Comment 9

[Peter Trudelle]

mass-moving all bugs to m21 that are not dofood+, or nsbeta2+

Comment 10

[Eric Vaughan]

Tom how do we fix this? It probably happens in html as well.

Comment 11

[John Morrison]

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

Comment 12

[joki (gone)]

Adding [ESM/CSS] prefix to bugs in EventStateManager with its generated events 
or CSS pseudo-class management.

Comment 13

[Blake Ross]

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

Comment 14

[ckritzer (gone)]

This bug has been marked "future" because the original netscape engineer working 
on this is over-burdened. If you feel this is an error, that you or another
known resource will be working on this bug,or if it blocks your work in some way 
-- please attach your concern to the bug for reconsideration.

Comment 15

[Heikki Toivonen (remove -bugzilla when emailing directly)]

I thought this was buttons only, but it does not seem to be. If I have onclick 
handler on some HTML element (tested input text field and textarea), the onclick 
handler gets called! IE5 gets it right.

Reassigning to me. Removing Future milestone and nominating for nsbeta3.

Comment 16

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Triage with nisheeth & ekrock: nsbeta3+.

Comment 17

[Heikki Toivonen (remove -bugzilla when emailing directly)]

There is a related bug about onclick handler and context menu, it is bug . From 
that bug we could shamelessly copy the proposal of [e-mail] to 
prevent us from processing all but left clicks on dialog buttons quite easily. 
First, we should remove the onclick handlers in platformOverlay.xul:

<button class="exit-dialog" id="cancel" value="&cancelButton.label;" 
onclick="doCancelButton()"/>
xxxxxxxxxxxxxxxxxxxxxxxxxx

Then in dialogOverlay.js, for each button in the dialog we could create an event 
handler function:

function cancelClickHandler(e) {
  if(e.button == 1)
    doCancelButton();
}

Finally attach the event handlers to the buttons, in doSetOKCancel 
(dialogOverlay.js):

  var cancelButton = document.getElementById("cancel");
  if (cancelButton) {
    okButton.addEventListener("click",clickHandler,false);
  }

Comment 18

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Doh, the related bug 28604.

Comment 19

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Attachment: Fix dialog default buttons to act on left-click only

Comment 20

[Heikki Toivonen (remove -bugzilla when emailing directly)]

I made a patch if anyone wants to give this a try. 

There is still a problem with the Macs, though, because they do not use onclick 
handlers. Also, this patch does nothing with the style, eg. you will get pressed 
down look for the button even with a right click.

Comment 21

[Heikki Toivonen (remove -bugzilla when emailing directly)]

There is also a simpler way that does not concern the Macs I believe. In 
platformDialogOverlay.xul we could change the onclick handlers to look like 
this:

onclick="if(event.button==1)doOKButton()"
         +++++++++++++++++++

The problem seems to be that the whole GUI is full of onclick handlers. Fixing 
the default buttons is not much work, but most other things also respond to 
right click. There are also exceptions that get it right.

Comment 22

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Ben, what are your thoughts about going through the GUI and changing controls so 
that they act on left-click only?

Comment 23

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Quick grep for onlick in XUL files on Windows gave 422 instances. Should we add:

if (event.button==1)

to all of them, or some selected like Ok & Cancel, or just future this bug? Of 
course each use would need to be checked first if we want to fix those things...

About onclick getting called for HTML: I guess that is correct after all, you 
can get the button that was pressed from event.button. There are problems with 
context menus which are addressed in bug bug 28604.

Comment 24

[timeless]

imo The fix should be done in c++ code not xul.
ERM
do we have onCommand for buttons? if so does it only use primary mouse button? 
If so maybe that is the solution.

Hyatt do you do anything xul/xbl/button related?

Comment 25

[Heikki Toivonen (remove -bugzilla when emailing directly)]

oncommand seems to work for buttons as well. Still there are things that are not 
affected by this, for example checkboxes generally don't seem to have onclick 
handlers so they would still respond to right-click...

I did a blind search-replace for onlick in XUL & JS chrome, and that left at 
least the sidebar crippled (couldn't change tabs).

Comment 26

[David Hyatt]

This has to do with the nsButtonBoxFrame.cpp's MouseClicked handler.  Most XUL 
widgets don't use onclick, but instead use oncommand.  I need to not fire that 
on a right click.  (Right now I do.)

Comment 27

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Actually, it is not nsButtonBoxFrame.cpp's MouseClicked. That creates an 
"command" event only on mouse left click. But it is already too late in the game 
anyway, the script handlers have been called already.

It seems XBL can also help here. At least for checkboxes we have, in 
xulBindings.xml, a line that says:

<handler type="click" value="this.checked = !this.checked;"/>

If I add the attribute button="1" or change type from "click" to "command"
then nsXBLEventHandler::MouseEventMatched(nsIDOMMouseEvent* aMouseEvent) will 
filter all but left-clicks. Now XBL is totally uncharted terrotory for me, so I 
am not sure if this would cover all the XUL (checkboxes, radiobuttons, normal 
buttons, menus, drop-down lists and whatever). I guess it could.

This also brings up a couple of issues. First, the documentation at

http://www.mozilla.org/xpfe/xptoolkit/xbl.html

is a wrong at least in regard with the button attribute. The docs say you can 
use values left, middle and right, but code checks for numbers.

Also, there are 2 xulBindinds.xml files in the source tree, in 
xpfe/global/resources/content and ... skin.

Is anyone willing to take this stuff since this doesn't seem to be event stuff, 
or should I go and learn XBL?

Comment 28

[Heikki Toivonen (remove -bugzilla when emailing directly)]

In my opinion this is not an event system bug. It could be "fixed" in event 
code, perhaps, but I think the correct way is to fix the XBL and XUL files. 
Therefore, reassigning to Trudelle for further triage...

Comment 29

[Peter Trudelle]

->hyatt, based on his last comment. clearing nsbeta3+ for triage by current owners

Comment 30

[David Hyatt]

I am reassigning this to ben, but this problem is huge and really can't be 
covered by one bug.

The big problem is that most people still use onclick for buttons and they don't 
check the button type.  I designed oncommand intending for people to always use 
it instead of onclick for buttons, since oncommand filters to the left button.

Ben, you can add button="1" to checkbox and radio bindings to fix those, but 
still, anyone who puts an onclick handler on these without checking the button 
is going to get in trouble.

We need to have a mass onclick-->oncommand spree with the XUL.

Comment 31

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

I've fixed this in the simple case for checkboxes and radiobuttons by checking 
event.button checks. The button problem remains. Removing nsbeta3+ and 
reassigning back to hyatt. 

Comment 32

[Peter Trudelle]

Ben, why exactly did you give this back to hyatt?  Is there a remaining problem
with button, or just with the incorrect use of onclick in buttons?  

Comment 33

[timeless]

fwiw, I have patches for all of the onclicks I found using lxr.  I sent them to 
hyatt. Full diffs are available in the url. I think I need module owner reviews 
or something like that.

Comment 34

[Peter Trudelle]

Why Hyatt?  Who owns all of those onclicks?

Comment 35

[Peter Trudelle]

->ben

Comment 36

[Brendan Eich [:brendan]]

Timeless: please attach your patch.  Ben: please give it an r= if you can, or
say who else should review it to get it in.  Thanks,

/be

Comment 37

[johng]

nav triage team:
nsbeta3- not a show stopper

Comment 38

[Henrik Gemal]

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

Comment 39

[Blake Ross]

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

Comment 40

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

=> hyatt. oncommand should fire on left click only. 

(I have a feeling this has been bounced about before ;)

Comment 41

[David Hyatt]

Oncommand does fire on left click only.  This bug is about people using onclick
handlers when they should use oncommand.

Comment 42

[Henrik Gemal]

Will we see a checkin soon?

Comment 43

[Blake Ross]

I just filed bug 62256, but I guess it's really a dup of this, so I'll take 
this.  I don't understand the attached patch; why use onclick and then filter 
to button 1 (which will also break the case that I filed 62256 for) rather than 
just using oncommand?  I'll attach a patch soon with a generic 
s/onclick/oncommand for buttons.  I can't think of any case where a button 
would want onclick, but I'll be careful as I go...

Comment 44

[Blake Ross]

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

Comment 45

[timeless]

One problem, as i was going through this patch (a while ago), i found a few 
objects that did not fire oncommand events.  Also, by now my patch has 
certainly rotted.

Clearing keywords and nominating for 0.9 and 1.0.

fwiw, the diffs are still available in the url if anyone really wants to try 
them.

Comment 46

[Blake Ross]

Attachment: [patch][1] some XPApps overlays

Comment 47

[Peter "jag" Annema]

r=jag

Comment 48

[Blake Ross]

cc alec for sr

[anyone getting notifications: I'm going to be attaching a number of patches in 
different areas so the appropriate people can review each.  please feel free to 
remove yourself if you don't want the spam.]

Comment 49

[Alec Flett]

sr=alecf

Comment 50

[Blake Ross]

Attachment: [patch][2] editor files

Comment 51

[timeless]

RCS file: /cvsroot/mozilla/editor/ui/dialogs/content/EdImageMap.xul,v
you didn't add ; after ().

Comment 52

[Blake Ross]

r=kin via e-mail, cc simon for sr

Comment 53

[Simon Fraser [no longer active]]

sr=sfraser. You should get r= from cmanske on the editor changes.

Comment 54

[Blake Ross]

Charley already looked them over.  Thanks Simon.

Comment 55

[Blake Ross]

Attachment: [patch][3] mail/news files

Comment 56

[(not reading, please use [e-mail] instead)]

the tree has shifted under blake.  blake, can you attach a new mailnews patch?

Comment 57

[(not reading, please use [e-mail] instead)]

ok, blake emailed me the update.  

r=sspitzer

blake, you'll should attach it for the super reviewer.

Comment 58

[Alec Flett]

sr=alecf

Comment 59

[Blake Ross]

Attachment: [patch][4] cookie/wallet files

Comment 60

[Alec Flett]

as before, sr=alecf as long as each button has been tested

Comment 61

[Stephen P. Morse]

WalletEditor.xul is no longer being used, but the fix there is certainly 
harmless.

However, I think you missed a file.  How about SignonViewer.xul?

r=morse for the files presented.  And the r= applies to the SignonViewer file as 
well if that will be the same sort of change.   

Comment 62

[Blake Ross]

Attachment: [patch][5] chatzilla file

Comment 63

[Robert Ginda]

r=rginda for patch 5

Comment 64

[Blake Ross]

Marking fixed. Pretty much all cases have been fixed.

Comment 65

[Henrik Gemal]

what about right clicking on the "Get Msgs" and "Print" button in the Mail&News?
also the entire addressbook

Comment 66

[Blake Ross]

Umm...I don't know what you're talking about, but file new bugs to appropriate
components.

Comment 67

[Henrik Gemal]

What I meant was:
If you right click on the "Get Msgs" and "Print" button in the Mail&News the
button seems to get clicked. It doesn't do anything but the button seems to get
pressed down. It only happens to those two button because they both have some
sort of dropdown function. It doesn't happen on the "File" button which also
have some dropdown function.

should I file new bugs or if so what component is that?

Comment 68

[Asa Dotzler [:asa]]

Bugs targeted at mozilla1.0 without the mozilla1.0 keyword moved to mozilla1.0.1 
(you can query for this string to delete spam or retrieve the list of bugs I've 
moved)

Comment 69

[Blake Ross]

That has nothing to do with this bug.

Comment 70

[Henrik Gemal]

v 20020221
will file new bugs about weird rightclick != leftclick stuff