Closed Bug 54035 Opened 24 years ago Closed 22 years ago

key handlers set by JS fire *after* character inserted in text field

Categories

(Core :: DOM: UI Events & Focus Handling, defect, P3)

Product:
Core
Component:
DOM: UI Events & Focus Handling
Platform:
x86
Windows 2000
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla1.3beta

People

(Reporter: dlord, Assigned: bryner)

References

Details

(Keywords: topembed+, Whiteboard: [eapp])

Attachments

(3 files)

attachment of testcase
(deleted), text/html
Details
something like this
(deleted), patch
john
: review+
hyatt
: superreview+
Details | Diff | Splinter Review
follow-up patch to typeaheadfind
(deleted), patch
aaronlev
: review+
kinmoz
: superreview+
Details | Diff | Splinter Review
From Bugzilla Helper: User-Agent: Mozilla/4.0 (compatible; MSIE 5.01; Windows NT 5.0) BuildID: 20000092505 When a function is set from javascript for onkeypress, the return value is ignored. The attached HTML works on IE and Netscape 4.7. The simple case that I attached to 53582 now works but this more complex case (the one I actually use) is still broken. Reproducible: Always Steps to Reproduce: 1.Run attached html 2. 3. Actual Results: First 2 cases work, last two do not. Expected Results: Input should be suspressed in all 4 cases. <html> <head> <script> function OnKey(event){ return false; } </script> </head> <body onload= 'document.forms["IATeleform"].elements["Constr_3"].onkeypress=OnKey; document.forms["IATeleform"].elements["Constr_4"].onkeypress=new Function ("return false")'> <p> Input should not be accepted </p> <form name="IATeleform" onsubmit="return checkAll(this)"> <div>"1) return false - works"</div> <input type="text" name="Constr_1" onkeypress="return false" maxlength="13" /> <div>"2) return OnKey(event) - works"</div> <input type="text" name="Constr_2" onkeypress="return OnKey(event)" maxlength="13" /> <div>3) script onkeypress=OnKey - works for IE and Netscape 4.7</div> <input type="text" name="Constr_3" maxlength="13" /> <div>4) script onkeypress=new Function("return false") - works for IE & Netscape 4.7</div> <input type="text" name="Constr_4" maxlength="13" /> </form> </body> </html>
Updating QA Contact.
QA Contact: janc → lorca
Attached file attachment of testcase (deleted) —
setting bug status to New. Tested with 110104 mozilla trunk build on NT
Status: UNCONFIRMED → NEW
Ever confirmed: true
After some testing I think I know the problem. JS listeners are being registered after the browser registers its listeners on the text field. I'm going to assign this a milestone but if it proves to be more difficult than I'm hoping I may move it to Future.
Status: NEW → ASSIGNED
Summary: return value of onkeypress ignored when function is set via javascript → key handlers set by JS fire *after* character inserted in text field
Target Milestone: --- → mozilla0.9
Reassigning QA Contact for all open and unverified bugs previously under Lorca's care to Gerardo as per phone conversation this morning.
QA Contact: lorca → gerardok
Setting milestone.
Target Milestone: mozilla0.9 → mozilla0.9.1
Do you have any idea what it would take to fix this? We have a customer that really needs this, and we'd like to look into fixing it. Thanks
Just pasting in some email discussion about this bug that's pertinent so it gets archived. "The main problem you're seeing is based on the fact that in 4.x, script listeners always fired before the browser did anything. So whenever a script listener looked at a key event it was always before the key had been entered so it could cancel it. In 6.0, many of the internal listeners are using the same event model as the script listeners and, as a result, get intermixed in ordering with the script listeners. In this case, the browser key listener gets registered at page load when the text field is created. The markup based listeners on your test are read in before that so they work. The script based listeners on your test are added after the page has loaded so they are registered after the browser's key listeners. The correct fix to this is something we're planning but it won't be in the 6.5 release. It involves separating out the browser and script based listeners into two separate dispatch queues. The script stuff would fire first, then all the browser stuff would fire. But the size of this work prohibits our doing it right now. "So short term I see a couple of options. The easiest is if your client is willing to look at workarounds. A simple one would be to have the text areas in question have a simple 'onkeypress=""' in them (or possibly fill the function with 'void(0)', I'd have to test to be sure). After that they could register their listener from script and it would overwrite the blank function which had been registered in the correct order. I know this will work, though it might require the fixes currently in my tree which will be checked in in a couple of days to work properly. "The second option would be a partial fix that would solve the case you're seeing, though not all the cases in general. We could possibly reorder the event listeners so that attribute based event listeners (setting the value of onkeypress equal to something, either from script or markup) always fired first. There can only be one of these per Node per event since the attribute can only have one value. I talked to some other people and they don't think the subtle reordering would break anything internally, though we'd have to test that to be sure. It's only a partial fix since anyone using the new standard AddEventListener method to register listeners would still be registered in the order they occurred. That and anyone using an event listener which listened to the event higher in the content tree as it bubbled upward would have long since lost their chance to cancel it. But it would solve your test case. Event listener registration is handled in mozilla/content/src/nsEventListenerManager.cpp
*** Bug 69825 has been marked as a duplicate of this bug. ***
Target Milestone: mozilla0.9.1 → mozilla0.9.2
Given the current timeframe and that we have a plan to fix this properly in Moz1.0 I'm marking this for that milestone.
Target Milestone: mozilla0.9.2 → mozilla1.0
QA contact updated
QA Contact: gerardok → madhur
*** Bug 85894 has been marked as a duplicate of this bug. ***
*** Bug 99349 has been marked as a duplicate of this bug. ***
*** Bug 104423 has been marked as a duplicate of this bug. ***
*** Bug 103370 has been marked as a duplicate of this bug. ***
*** Bug 107689 has been marked as a duplicate of this bug. ***
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)
Target Milestone: mozilla1.0 → mozilla1.0.1
This bug desperately needs to be fixed for Mozilla 1.0. This is a MAJOR broken functionality. Is there still a plan to fix this?
*** Bug 122212 has been marked as a duplicate of this bug. ***
Whiteboard: [eapp]
Depends on: 124990
Target Milestone: mozilla1.0.1 → mozilla0.9.9
nsbeta1+ per ADT triage
Keywords: nsbeta1+
Major corporations depend on eapp bugs, and need them to be fixed before they can recommend Mozilla-based products to their customers. Adding nsbeta1+ keyword and making sure the bugs get re-evaluated if they are targeted beyond 1.0.
*** Bug 126437 has been marked as a duplicate of this bug. ***
124990 and its associated bugs is not quite ready for checkin for 0.9.9. Maintaining high priority and moving to 1.0 for completion and further testing.
Target Milestone: mozilla0.9.9 → mozilla1.0
Added topembed as this bug has affected at least 1 popular commerce site we know of.
Keywords: topembed
Keywords: topembed → topembed+
QA Contact: madhur → rakeshmishra
Keywords: regression
Sorry, wrong bug...
Keywords: regression
Looks like we have missed the train for 1.0.1. Can we retarget this for 1.2?
This bug is so hugely freaking ugly, I can't believe it hasn't gotten any attention.
acutally this depends on the new event dispatch that is about to land (again). Once that is in, we can fix this
QA Contact: rakeshmishra → trix
Target Milestone: mozilla1.0 → ---
->bryner, another bug that we should now be ready to fix with the new event dispatch
Assignee: joki → bryner
Status: ASSIGNED → NEW
Please please please get this one for 1.3. Pretty please.
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla1.3beta
Attached patch something like this (deleted) — — Splinter Review
I think this is the basic idea of what needs to happen to fix this bug. I made editor only type characters for key events during the system event pass, so that content listeners get first crack at cancelling the event. I made all XUL/XBL command handlers be part of the system event group so that they aren't calling preventDefault on events before editor sees them. I can definitely imagine that we may have to move additional event listeners into the system event group to make things work properly. Ideally, I think any listener that's intrinsically part of Gecko, XUL, or chrome should probably be in the system event group, but that's a ways off.
cc'ing hyatt for xbl insight
Attachment #113863 - Flags: review?(jkeiser)
Comment on attachment 113863 [details] [diff] [review] something like this Doesn't this mean that userspace XBL handlers will run in the same pass as chrome?
Comment on attachment 113863 [details] [diff] [review] something like this OK, per our conversation on IRC, this may cause user-defined <command>s to be in the system pass, but it can be argued that that's what they are for. r=me. It surely makes the situation better. Please get sr=kin or another editor person, though.
Attachment #113863 - Flags: review?(jkeiser) → review+
Attachment #113863 - Flags: superreview?(hyatt)
Comment on attachment 113863 [details] [diff] [review] something like this sr=hyatt
Attachment #113863 - Flags: superreview?(hyatt) → superreview+
cc'ing some editor people, just to make sure i'm not off in the weeds here.
I found that with the Midas demo (http://www.mozilla.org/editor/midasdemo/), typeahead find was getting triggered when I typed in the editor area with the above patch applied. I'm not sure why this isn't an issue for text inputs. Anyway, this patch fixes it.
Attachment #117309 - Flags: superreview?(kin)
Attachment #117309 - Flags: review?(aaronl)
I don't know about the DOM3 stuff at all. Maybe it's the right thing to do, I'll have to learn more. In any case Typeaheadfind should never trigger when in Midas mode. That's a fix I'm definitely comfortable with. I have a check for Midas mode here: http://lxr.mozilla.org/seamonkey/source/extensions/typeaheadfind/src/nsTypeAheadFind.cpp#2841 That check should be turned into a helper method, and should also be used in nsTypeAheadFind::GetAutoStart()
Comment on attachment 117309 [details] [diff] [review] follow-up patch to typeaheadfind r=aaronl
Attachment #117309 - Flags: review?(aaronl) → review+
My build is still in progress, but I'm wondering if the block of code that Aaron points to in comment 38 is not being triggered or because somewhere else we aren't making the right checks (for IsCommandEnabled?). Perhaps that code is checking the wrong dom document for designMode?
Comment on attachment 117309 [details] [diff] [review] follow-up patch to typeaheadfind sr=kin@netscape.com
Attachment #117309 - Flags: superreview?(kin) → superreview+
patch has been checked in.
Status: ASSIGNED → RESOLVED
Closed: 22 years ago
Resolution: --- → FIXED
I just filed bug 199103 about Ctrl-Enter not working in mail composer anymore. Could this be related?
*** Bug 179974 has been marked as a duplicate of this bug. ***
This caused two more regressions: bug 200439 and bug 212487. Both were fixed by changing an onkeypress to an oninput.
hmm, there is still a problem when the event listener is added with .addEventListener. I created an attachment for bug 163435 a while ago, which still doesn't work entirely. It's located at: http://bugzilla.mozilla.org/attachment.cgi?id=97653&action=view The event gets sent to the event listener, but after the character is inserted (tested by adding an alert to the function). I don't know about the useCapture parameter, but changing it to true has no effect. I think this bug should be reopened?
I think that's something else. In that case, it looks like returning false from the event handler does not have the same effect as calling event.preventDefault(). Does anyone know if this is intentional? jst? jkeiser?
Blocks: 211507
This bug's fix may have regressed bug 211507 (tabbing in mail compose window)
Component: Event Handling → User events and focus handling
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: