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

RESOLVED FIXED in mozilla5

Status

()

Toolkit
Form Manager
--
minor
RESOLVED FIXED
8 years ago
3 years ago

People

(Reporter: Jaroslav Benc, Assigned: mounir)

Tracking

Trunk
mozilla5
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(status2.0 wontfix)

Details

Attachments

(2 attachments)

(Reporter)

Description

8 years ago
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.
(Reporter)

Comment 1

8 years ago
Created attachment 511263 [details]
A test case, press up/down keys and notice no event is fired on the document
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!
Status: UNCONFIRMED → NEW
Component: Event Handling → Form Manager
Ever confirmed: true
OS: Windows 7 → All
Product: Core → Toolkit
QA Contact: events → form.manager
Hardware: x86 → All
Summary: document.keypress is not fired when an input element is focused and up/down pressed → document.keypress bubbling handlers are not fired when an input element is focused and up/down pressed
(Reporter)

Comment 3

8 years ago
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..
The autofill code calls both, actually.
(Assignee)

Comment 5

8 years ago
Created attachment 511347 [details] [diff] [review]
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.
Assignee: nobody → mounir.lamouri
Status: NEW → ASSIGNED
Attachment #511347 - Flags: review?(dolske)
(Assignee)

Updated

8 years ago
Version: unspecified → Trunk
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.
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?
status2.0: --- → wontfix
> 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.
(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.
(Assignee)

Comment 10

8 years ago
So if I followed correctly, the patch is actually fine?
Whiteboard: [post-2.0]
(Assignee)

Updated

7 years ago
Whiteboard: [post-2.0] → [post-2.0][passed-try]
Comment on attachment 511347 [details] [diff] [review]
Patch v1

r+ (for landing after Firefox 4 branches).
Attachment #511347 - Flags: review?(dolske) → review+
(Assignee)

Updated

7 years ago
Whiteboard: [post-2.0][passed-try] → [can land][post-2.0][passed-try]
(Assignee)

Comment 12

7 years ago
Pushed:
http://hg.mozilla.org/mozilla-central/rev/f01a7a6670db
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [can land][post-2.0][passed-try]
Target Milestone: --- → mozilla2.2
You need to log in before you can comment on or make changes to this bug.