164249 - button attribute on handler element not filtering properly

Status: RESOLVED FIXED

(Core :: XBL, defect)

Description

[Jonathan Watt [:jwatt]]

For handler tags with the event attribute set to 'mousedown' or 'mouseup', when
I place a button attribute on the tag the event is never matched if there are no
other attribute set on the tag - even if the button depressed is the button
specified in the button attribute. However, add say modifiers="alt" and, as long
as the alt key is depressed, the button attribute suddenly starts filtering
properly and the event is matched. 

Additionally, if the event attribute is set to 'mousemove' then even with the
modifiers attribute set to something the event is never matched.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Jonathan, could you possibly create a testcase that could be used to reproduce
this bug?

Comment 2

[[e-mail]]

bz: no testcase needed, just try dragging tree column headers with any modifier
pressed.

Comment 3

[Hixie (not reading bugmail)]

Per draft spec, attributes on <handler> that are not present should not be 
checked. So if modifier="" is not present then any modifier should be allowed.
   http://www.hixie.ch/specs/xbl/xbl

Comment 4

[[e-mail]]

Attachment: Proposed patch

OK, so by making mKeyMask signed I can put -1 in it to indicate that modifiers
should not be tested. I also optimized the key event test - mMisc can't be set
unless mDetail is set.

Comment 5

[[e-mail]]

Comment on attachment 137110 [details] [diff] [review]
Proposed patch

I've just discovered a large issue with this patch: most of our current key
bindings omit modifiers expecting them to default to none :-(

Comment 6

[Brian Ryner (not reading)]

Comment on attachment 137110 [details] [diff] [review]
Proposed patch

in that case... (just to get this out of my queue)

Comment 7

[[e-mail]]

Attachment: Tweaked to never ignore modifiers on specific key presses

OK, this version makes mouse events follow the spec. However key events still
work in two modes: 1) no key or code specified, match any key 2) key or code
specified, match key/code and modifiers, even when no modifiers are specified.

Comment 8

[David Hyatt]

The intent of the spec has always been that if any filter is imposed on an event, you must match what is 
specified exactly.

That means, for example, that if the author specifies a button but no modifiers, that the event will only 
be matched if no modifiers were pressed and if the precises button was pressed.

For key events, if the user specifies a key code but no modifiers, the event should only be matched if no 
modifiers are pressed.

Comment 9

[Hixie (not reading bugmail)]

would probably be useful to have a modifiers="any", then.

Comment 10

[[e-mail]]

Most native widgets ignore modifiers on mouse events... does this means we need
to fix up all our mouse xbl to test for event.button in script?

Comment 11

[Hixie (not reading bugmail)]

neil: no, it means you would have

   button="1" modifiers="any"

...or some such. I'm thinking maybe modifiers should be a comma separated list 
of space separated lists, so you could say:

   button="1" modifiers="ctrl, alt ctrl, shift ctrl"

...to match when either ctrl is down, alt and ctrl are down, or shift and ctrl 
are down. Or maybe:

   button="1" modifiers="ctrl any, alt any"

...to mean when ctrl is down or alt is down (and any other modifiers, they don't 
matter) but not when neither is down.

I don't know how useful that would be though. Maybe simpler is better.

Comment 12

[[e-mail]]

Attachment: Another go

OK, so this version supports the keyword "any" in the modifiers which indicates
that the state of the preceeding modifiers is irrelevent, thus if you specify
modifiers="control any, shift" it requires shift but not alt or meta.

Comment 13

[[e-mail]]

Comment on attachment 147720 [details] [diff] [review]
Another go

The reason I'm doing this is I want to be able to tweak tree.xml to say (e.g.)
<handler event="keypress" keycode="vk_down" modifiers="control any, shift"> ...
this.view.selection.rangedSelect(-1, c + 1, event.ctrlKey); [subject to changes
to syntax of modifiers attribute]

Comment 14

[Jan Varga [:janv]]

Comment on attachment 147720 [details] [diff] [review]
Another go

what a nice enhancement!
r=varga

Comment 15

[Brian Ryner (not reading)]

Comment on attachment 147720 [details] [diff] [review]
Another go

Just wanted to make sure you tested shift+space, which I think is the main
reason for the special handling of shift. Looks fine otherwise.

Comment 16

[[e-mail]]

Fix checked in.

Comment 17

[Jonathan Watt [:jwatt]]

Neil, I think your changes are great, but I'm not sure about the word "any" for
what it does, or that the ignored modifiers should come first. How about putting
the required modifiers first and using the word "ignore"? E.g. modifiers="shift,
ignore control". That seems more intuative, and it leaves the use of "any" for
what hixie describes in c11 if it turns out to be desirable to add that
functionality in the future. 

Comment 18

[[e-mail]]

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