633058 - document.keypress bubbling handlers are not fired when an input element is focused and up/down pressed

Status: RESOLVED FIXED

(Toolkit :: Form Manager, defect)

Description

[Jaroslav Benc]

User-Agent:       Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b10) Gecko/20100101 Firefox/4.0b10
Build Identifier: 4.0b10

As far as I understand, keypress event is not fired when pressing up/down keys in form fields as it is considered as "modifier" key. 

However, it should bubble this event so we can still handle them on non-form elements such as top document element. 

There is an example attached in the bug report showing that document.keypress event is not fired when an input element is focused, however it is fired when the field is not active. 

Reproducible: Always

Steps to Reproduce:
1. Open the example and press up/down keys
2. Click outside of the input field and press up/down again
Actual Results:  
No keypress event is fired when the field is focused

Expected Results:  
keypress event should bubble but not fire on the field input.

Comment 1

[Jaroslav Benc]

Attachment: A test case, press up/down keys and notice no event is fired on the document

Comment 2

[Boris Zbarsky [:bzbarsky]]

A keypress event is fired in the testcase attached.  It's just that your listener doesn't see it, because there's a listener on the text input itself that calls stopPropagation on the event.  You can tell the difference between that and the event not being fired by either using a listener on the input itself (which will get the event in this case), or using a capturing listener on the document.

The stopPropagation call is made by the code in nsFormFillController::KeyPress.  I have no idea why it needs to do that, not just preventDefault.  But certainly the core event handling here is correct.  Welcome to the mess that is DOM events if you're not the only one defining listeners!

Comment 3

[Jaroslav Benc]

Thanks for explaining that. There is an easy workaround for us so it's not a problem. I'd personally vote for using preventDefault instead of stopPropagation..

Comment 4

[Boris Zbarsky [:bzbarsky]]

The autofill code calls both, actually.

Comment 5

[Mounir Lamouri (:mounir)]

Attachment: Patch v1

Let's try to fix that.

AFAICT, only Opera have UP/DOWN bubbling out of the input field. We do have that too for MacOS in some situations, see:
https://mxr.mozilla.org/mozilla-central/source/toolkit/components/autocomplete/src/nsAutoCompleteController.cpp#449

So this change _shouldn't_ break any web site but I guess we can push it after Gecko 2.0.

Comment 6

[Boris Zbarsky [:bzbarsky]]

A few comments:

1)  Are we sure that's what we want?  The cancel stuff for arrow keys just wants to prevent default, but did you look at the other things that set cancel to true and why they do that?

2)  There is no way this should be landing before 2.0.  I fully expect it to break at least some sites.

Comment 7

[Justin Dolske [:Dolske]]

This code dates back to the original satchel landing before Firefox 1.0, so I agree we shouldn't try to fix it this late in Firefox 4.

I'm not sure what this line of code was trying to do. I'd _guess_ that it's trying to hide events from the page when interacting with the autocomplete dropdown. But it has no effect on capturing listeners, so that's clearly not very successful.

We probably should suppress events when the autocomplete popup is open, but to work correctly I think that would need to be done differently (and earlier) than how it's attempted here.

Hmm. Now that I look further, that's mostly what this is already being used for. The various mController methods that return |cancel| mostly base it on if the popup is open or not. But there are also a few special cases... Eg, nsAutoCompleteController.cpp's HandleKeyNavigation() suggests that the arrow key cancellation is to "prevent the input from handling up/down events, as it may move the cursor to home/end on some systems".

And then there's the question of if key events that trigger the autocomplete should be hidden or not. EG, if I'm in a text field and press down-arrow to open the dropdown, should content get that?

Comment 8

[Boris Zbarsky [:bzbarsky]]

> prevent the input from handling up/down events, as it may move the cursor to
> home/end on some systems

Right, that's what the comments say.  But if the autocomplete popup is not open, and up/down won't open it (e.g. the user has autocomplete off), then we _want_ this behavior, no?  

> EG, if I'm in a text field and press down-arrow to open the dropdown, should
> content get that?

Imo, yes.  And the listener that opens the dropdown should be in the default action phase (or system event group), not the normal bubbling phase.  And if content calls preventDefault we shouldn't open the dropdown.  Just like any other default action.

Comment 9

[Justin Dolske [:Dolske]]

(In reply to comment #8)
> But if the autocomplete popup is not
> open, and up/down won't open it (e.g. the user has autocomplete off), then we
> _want_ this behavior, no?

Agreed.

> > EG, if I'm in a text field and press down-arrow to open the dropdown, should
> > content get that?
> 
> Imo, yes. [...]

Ok, that seems reasonable.

Comment 10

[Mounir Lamouri (:mounir)]

So if I followed correctly, the patch is actually fine?

Comment 11

[Justin Dolske [:Dolske]]

Comment on attachment 511347 [details] [diff] [review]
Patch v1

r+ (for landing after Firefox 4 branches).

Comment 12

[Mounir Lamouri (:mounir)]

Pushed:
http://hg.mozilla.org/mozilla-central/rev/f01a7a6670db