Opened 8 years ago

Closed 7 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 bill)

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)

textboxConnect.patch (1.2 KB) - added by bill 8 years ago.
14755.patch (4.2 KB) - added by Douglas Hays 8 years ago.
possible fix

Download all attachments as: .zip

Change History (13)

comment:1 Changed 8 years ago by bill

Description: modified (diff)

comment:2 Changed 8 years ago by bill

Description: modified (diff)

Changed 8 years ago by bill

Attachment: textboxConnect.patch added

comment:3 Changed 8 years ago by bill

Description: modified (diff)

comment:4 Changed 8 years ago by Douglas Hays

Why won't this suffer the same IE event misfortune?

comment:5 Changed 8 years ago by bill

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 Douglas Hays

Milestone: tbd1.8
Status: newassigned

comment:7 Changed 8 years ago by Douglas Hays

bill, I attached a patch that does a shallow copy of the event. Please review.

Changed 8 years ago by Douglas Hays

Attachment: 14755.patch added

possible fix

comment:8 Changed 8 years ago by bill

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:9 Changed 7 years ago by Douglas Hays

Resolution: fixed
Status: assignedclosed

In [27954]:

Fixes #14755. Replace mixin(..., evt) calls with a simple event shallow copy and skip layerX/Y attributes to avoid webkit warnings. Change TextBox?'s _onInput to use on(...) instead of this.connect for input events.

comment:10 Changed 7 years ago by bill

Resolution: fixed
Status: closedreopened

Slider_mouse.html is failing after this, in addition to Spinner_a11.html.

comment:11 Changed 7 years ago by Douglas Hays

Resolution: fixed
Status: reopenedclosed

In [27962]:

Fixes #14755. Ensure faux event object is fully initialized before calling typematic callback.

Note: See TracTickets for help on using tickets.