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)
Tracking
()
RESOLVED
FIXED
mozilla1.3beta
People
(Reporter: dlord, Assigned: bryner)
References
Details
(Keywords: topembed+, Whiteboard: [eapp])
Attachments
(3 files)
(deleted),
text/html
|
Details | |
(deleted),
patch
|
john
:
review+
hyatt
:
superreview+
|
Details | Diff | Splinter Review |
(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>
Comment 2•24 years ago
|
||
Comment 3•24 years ago
|
||
setting bug status to New. Tested with 110104 mozilla trunk build on NT
Status: UNCONFIRMED → NEW
Ever confirmed: true
Comment 4•24 years ago
|
||
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
Comment 5•24 years ago
|
||
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
Comment 7•24 years ago
|
||
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
Comment 8•24 years ago
|
||
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
Updated•24 years ago
|
Target Milestone: mozilla0.9.1 → mozilla0.9.2
Comment 10•23 years ago
|
||
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
Comment 12•23 years ago
|
||
*** Bug 85894 has been marked as a duplicate of this bug. ***
Comment 13•23 years ago
|
||
*** Bug 99349 has been marked as a duplicate of this bug. ***
Comment 14•23 years ago
|
||
*** Bug 104423 has been marked as a duplicate of this bug. ***
Comment 15•23 years ago
|
||
*** Bug 103370 has been marked as a duplicate of this bug. ***
Comment 16•23 years ago
|
||
*** Bug 107689 has been marked as a duplicate of this bug. ***
Comment 17•23 years ago
|
||
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
Comment 18•23 years ago
|
||
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?
Comment 19•23 years ago
|
||
*** Bug 122212 has been marked as a duplicate of this bug. ***
Updated•23 years ago
|
Whiteboard: [eapp]
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.
Comment 22•23 years ago
|
||
*** Bug 126437 has been marked as a duplicate of this bug. ***
Comment 23•23 years ago
|
||
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
Comment 24•23 years ago
|
||
Added topembed as this bug has affected at least 1 popular commerce site we know of.
Keywords: topembed
Updated•23 years ago
|
Updated•23 years ago
|
QA Contact: madhur → rakeshmishra
Updated•22 years ago
|
Keywords: regression
Comment 26•22 years ago
|
||
Looks like we have missed the train for 1.0.1. Can we retarget this for 1.2?
Comment 27•22 years ago
|
||
This bug is so hugely freaking ugly, I can't believe it hasn't gotten any
attention.
Comment 28•22 years ago
|
||
acutally this depends on the new event dispatch that is about to land (again).
Once that is in, we can fix this
Updated•22 years ago
|
QA Contact: rakeshmishra → trix
Updated•22 years ago
|
Target Milestone: mozilla1.0 → ---
Comment 29•22 years ago
|
||
->bryner, another bug that we should now be ready to fix with the new event dispatch
Assignee: joki → bryner
Status: ASSIGNED → NEW
Comment 30•22 years ago
|
||
Please please please get this one for 1.3. Pretty please.
Assignee | ||
Updated•22 years ago
|
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla1.3beta
Assignee | ||
Comment 31•22 years ago
|
||
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.
Assignee | ||
Comment 32•22 years ago
|
||
cc'ing hyatt for xbl insight
Assignee | ||
Updated•22 years ago
|
Attachment #113863 -
Flags: review?(jkeiser)
Comment 33•22 years ago
|
||
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 34•22 years ago
|
||
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+
Assignee | ||
Updated•22 years ago
|
Attachment #113863 -
Flags: superreview?(hyatt)
Comment 35•22 years ago
|
||
Comment on attachment 113863 [details] [diff] [review]
something like this
sr=hyatt
Attachment #113863 -
Flags: superreview?(hyatt) → superreview+
Assignee | ||
Comment 36•22 years ago
|
||
cc'ing some editor people, just to make sure i'm not off in the weeds here.
Assignee | ||
Comment 37•22 years ago
|
||
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.
Assignee | ||
Updated•22 years ago
|
Attachment #117309 -
Flags: superreview?(kin)
Attachment #117309 -
Flags: review?(aaronl)
Comment 38•22 years ago
|
||
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 39•22 years ago
|
||
Comment on attachment 117309 [details] [diff] [review]
follow-up patch to typeaheadfind
r=aaronl
Attachment #117309 -
Flags: review?(aaronl) → review+
Comment 40•22 years ago
|
||
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 41•22 years ago
|
||
Comment on attachment 117309 [details] [diff] [review]
follow-up patch to typeaheadfind
sr=kin@netscape.com
Attachment #117309 -
Flags: superreview?(kin) → superreview+
Assignee | ||
Comment 42•22 years ago
|
||
patch has been checked in.
Status: ASSIGNED → RESOLVED
Closed: 22 years ago
Resolution: --- → FIXED
Comment 43•22 years ago
|
||
I just filed bug 199103 about Ctrl-Enter not working in mail composer anymore.
Could this be related?
Comment 44•22 years ago
|
||
yes, caused a regression, see bug #198976 and bug #199068
Assignee | ||
Comment 45•22 years ago
|
||
*** Bug 179974 has been marked as a duplicate of this bug. ***
Comment 46•21 years ago
|
||
This caused two more regressions: bug 200439 and bug 212487. Both were fixed by
changing an onkeypress to an oninput.
Comment 47•21 years ago
|
||
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?
Assignee | ||
Comment 48•21 years ago
|
||
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?
Comment 49•21 years ago
|
||
This bug's fix may have regressed bug 211507 (tabbing in mail compose window)
Updated•6 years ago
|
Component: Event Handling → User events and focus handling
You need to log in
before you can comment on or make changes to this bug.
Description
•