Opened 8 years ago
Closed 8 years ago
#14755 closed defect (fixed)
event normalization fixes for _TextBoxMixin
Reported by: | bill | Owned by: | Douglas Hays |
---|---|---|---|
Priority: | undecided | Milestone: | 1.8 |
Component: | Dijit - Form | Version: | 1.7.1 |
Keywords: | Cc: | ||
Blocked By: | Blocking: |
Description (last modified by )
There are two event related issues in _TextBoxMixin that should be fixed going forward:
1) it's generating scary warnings in the chrome console about layerX and layerY being undefined.
This can be worked around by generating the faux event using delegate() rather than mixin() when possible. I'm talking about this code:
var faux = lang.mixin({}, e, { charOrCode: charCode, wasConsumed: false, preventDefault: function(){ faux.wasConsumed = true; e.preventDefault(); }, stopPropagation: function(){ e.stopPropagation(); } });
And to replace it with something like:
var faux = lang.delegate(e, { charOrCode: charCode, wasConsumed: false, preventDefault: function(){ faux.wasConsumed = true; e.preventDefault(); }, stopPropagation: function(){ e.stopPropagation(); } });
See #14754 for details, although in this case it seems like we don't need a special branch for IE.
2) stop using connect()
Dojo.connect() and this.connect() are going away for 2.0, so before then you need to switch to using dojo/on instead. The basic connection code you have could be changed to:
array.map(["keydown", "keypress", "paste", "cut", "input" ], function(event){ on(this.textbox, event, lang.hitch(this, handleEvent)); }, this)
but then changes will be required in your handleEvent() routine, since you will no longer have the "normalize keypress and keydown to act like firefox" behavior of dojo.connect().
Attachments (2)
Change History (13)
comment:1 Changed 8 years ago by
Description: | modified (diff) |
---|
comment:2 Changed 8 years ago by
Description: | modified (diff) |
---|
Changed 8 years ago by
Attachment: | textboxConnect.patch added |
---|
comment:3 Changed 8 years ago by
Description: | modified (diff) |
---|
comment:4 Changed 8 years ago by
comment:5 Changed 8 years ago by
Good question. You are right, it has the same problem and I just didn't notice. Specifically that on IE after
var faux = lang.delegate(evt, props);
faux == props, inheriting nothing from evt. IE9 may get a different error but I don't think it works either.
Having said that, I don't fully understand the requirements for the "custom event" code that you have in _TextBoxMixin.js. I see how it collapses multiple events, so that onInput(evt) is only called once per keystroke (or other form of input), but since that notification could be the keydown, keypress, or input event type, I don't know how much information you are guaranteeing to be in the event object passed to onInput().
Anyway it seems like something like
var props = { charOrCode: charCode, wasConsumed: false, preventDefault: function(){ faux.wasConsumed = true; e.preventDefault(); }, stopPropagation: function(){ e.stopPropagation(); } }, faux = has("ie") ? lang.mixin({}, e, props) : lang.delegate(e, props);
should work.
comment:6 Changed 8 years ago by
Milestone: | tbd → 1.8 |
---|---|
Status: | new → assigned |
comment:7 Changed 8 years ago by
bill, I attached a patch that does a shallow copy of the event. Please review.
comment:8 Changed 8 years ago by
It seems good to me. The most important thing is to test on different browsers since keydown/keypress events are different on webkit & IE vs. firefox.
I'm happy to see us weening ourselves off of the keypress/keydown normalization code in dojo/_base/connect.js, especially since it seems silly to run that code in addition to your _TextBoxMixin "normalization" code to trim each keyboard action to a single event. I wish we didn't have the charOrCode attribute added to the event object but I guess we are stuck with it until 2.0, for backwards compatibility.
I'm not sure why you want to use mixin() on all browsers rather than the more efficient delegate() method. Perhaps just to avoid branching, which is understandable.
As a minor point, as an improvement to what I originally suggested is that:
array.map([ "keydown", "keypress", "paste", "cut", "input" ], function(event){ this._connects.push(on(this.textbox, event, lang.hitch(this, handleEvent))); }, this);
could be reduced to:
this._connects.push(on(this.textbox, "keydown, keypress, paste, cut, input" , lang.hitch(this, handleEvent)));
comment:10 Changed 8 years ago by
Resolution: | fixed |
---|---|
Status: | closed → reopened |
Slider_mouse.html is failing after this, in addition to Spinner_a11.html.
Why won't this suffer the same IE event misfortune?