[cla][patch] dojox/gesture: "Uncaught TypeError: Illegal invocation" on Android devices

[Adrian Vasiliu]

Loading on Android devices the following test: dojox/gesture/tests/test_gesture.html the console shows many times: Uncaught TypeError?: Illegal invocation <path>/dojox/gesture/Base.js:267

The error is raised by a call to e.preventDefault()" in Base._process.

Exactly the same error hurts in demos/touch/demo.html (which uses dojox/gesture) as soon as you try to rotate the pie chart using the multi-touch gesture. Apparently as a consequence of this error, it is not possible to rotate the chart by the gesture on any Android device which is supposed to support it, such as Samsung Galaxy SIII Android 4.0.4, HTC One X Android 4.0.3.

The same exception is also raised on Android 2.2 devices, such as HTC Desire Android 2.2 even tough mutli-touch is not supported.

[Evan]

This issue only occurs when using touch.move(node, callback) on Andriod, it's caused by the customized event in touch.move handle. The fix patch works well for me(on Andriod2.3 and 4.0 - the demos/touch also works for muti-touch on Andriod 4.0).

But need Bill's review in case the fix breaks anything newly added in dojo/touch module.

[bill]

Hmm well I thought there was some browser, probably IE, where you couldn't modify the event object, which is why the current code has the {} in it.

I do remember seeing some other code that works around the problem you are describing by doing something like

var newEvt = lang.mixin(...
newEvt.preventDefault = function(){ evt.preventDefault(); };

[Evan]

The missed test case for reproducing the issue in Andriod2.3/4.0

[Evan]

Replying to bill:

Hmm well I thought there was some browser, probably IE, where you couldn't modify the event object, which is why the current code has the {} in it.

I do remember seeing some other code that works around the problem you are describing by doing something like

var newEvt = lang.mixin(...
newEvt.preventDefault = function(){ evt.preventDefault(); };

Oh yes, evt is not allowed to be modified on desktop browsers(at least FF, Chrome, IE...). so we need the mixin({}, evt, ...) for !hasTouch, and may use the fix when hasTouch == true.

We do added some tricky steps in dojo/on from line 458 - when on touch devices we tried to modified the native evt by

delete originalEvent.type;

So the result is in iOS browsers, the native evt cann't be modified, so we goes to this condition for an event delegation

if(originalEvent.type){ ... }

while in Android browsers, the evt can be modified directly as

    }else{
         // deletion worked, use property as is
	event = originalEvent;
	event.type = type;
    }

I think that's the reason why the native evt.preventDefault() can be called after mixin({}, evt, ...) on android

Also added an updated patch version...

[Evan]

A new version for the fix patch

[bill]

I checked your updated patch, but it seems like

on(node, touch.move, function(evt){
   evt.preventDefault();
});

still won't work on desktop?

[Evan]

Replying to bill:

I checked your updated patch, but it seems like

on(node, touch.move, function(evt){
   evt.preventDefault();
});

still won't work on desktop?

Just had a quick try, seems working well for me on Chrome, FF and IE9 with the​15892.html

[Evan]

Replying to Evan:

Replying to bill:

I checked your updated patch, but it seems like

on(node, touch.move, function(evt){
   evt.preventDefault();
});

still won't work on desktop?

Just had a quick try, seems working well for me on Chrome, FF and IE9 with the​15892.html

Found one regression - touch.move can't be stopped with event.stop() on touch devices, but that's a different issue, so created one new ticket to track with - #15946

[bill]

Ok, well sounds like the solution to both problems is

listener.call(this, lang.mixin({}, evt, { 
   target: hoveredNode,
   preventDefault: function(){ evt.preventDefault(); },
   stopPropagation: function(){ evt.stopPropagation(); }
})); 

[bill]

Or

listener.call(this, lang.delegate(evt, { 
   target: hoveredNode,
   preventDefault: function(){ evt.preventDefault(); },
   stopPropagation: function(){ evt.stopPropagation(); }
})); 

[Evan]

Sorry I missed one point, noticed

move: hasTouch ? touchmove :_handle("mousemove"),

which means

touchmove = function(node, listener){
     return on(win.doc, "touchmove", function(evt){
		if(node === win.doc || dom.isDescendant(hoveredNode, node)){
                    ...
             ...

is only applied to touch devices, so I think(also verified) following way shall work(​15892-v2.patch), since here the evt is already a delegated event by dojo/on, so various properties can be modified.

touchmove = function(node, listener){
	return on(win.doc, "touchmove", function(evt){
		if(node === win.doc || dom.isDescendant(hoveredNode, node)){
			evt.target = hoveredNode;
			listener.call(this, evt);
		}
	});
};

[Evan]

A simpler fix version since the evt is already delegated, so we can modified its properties freely

[bill]

What do you mean by "delegated"? It's a plain on() call.

Anyway it looks dangerous to modify the event directly. You said that you can't modify the event on desktop chrome, but you can modify it on mobile chrome? It's hard to believe. And I wonder whether you'll be allowed to modify the event on mobile IE10.

[Evan]

Replying to bill:

What do you mean by "delegated"? It's a plain on() call.

on(n, 'touchmove', function(evt){ 
   // -- 1
})

Well, when we reach #1, the evt is already customized in dojo/on(line 458+ e.g. var fixTouchListener = ...), and my observation is in iOS an event delegation is needed so after that we can normalize the various event properties:

//dojo/on(line 458+

var type = originalEvent.type;
try{
	delete originalEvent.type; // on some JS engines (android), deleting properties make them mutable
}catch(e){} 

if(originalEvent.type){
	// deleting properties doesn't work (older iOS), have to use delegation
	Event.prototype = originalEvent;
	var event = new Event;
	// have to delegate methods to make them work
	event.preventDefault = function(){
		originalEvent.preventDefault();
	};
	event.stopPropagation = function(){
		originalEvent.stopPropagation();
	};

But in Android, the event can be modifired directly, so don't need to delegate the event:

}else{
	// deletion worked, use property as is
	event = originalEvent;
	event.type = type;
}

Either way we can modify the "evt" when reaching the above #1 - at least tested in iOS and Android.

Anyway it looks dangerous to modify the event directly. You said that you can't modify the event on desktop chrome, but you can modify it on mobile chrome? It's hard to believe. And I wonder whether you'll be allowed to modify the event on mobile IE10.

In desktop chrome, my observation is changing the native event properties(e.g. target) won't throw any errors, the original one just kept unchanged

Not sure, haven't got chance to verify this on mobile IE yet.

[bill]

I see, makes sense. Well, I guess your second patch is good. My only worry is if modifying the original event object will affect other handlers setup on win.doc listening to "touchmove" directly. I.e. will the evt passed to func (below) be corrupted?

on(n, touch.move, ...);
on(document, "touchmove", func);

[Evan]

Replying to bill:

I see, makes sense. Well, I guess your second patch is good. My only worry is if modifying the original event object will affect other handlers setup on win.doc listening to "touchmove" directly. I.e. will the evt passed to func (below) be corrupted?

on(n, touch.move, ...);  ---- 1
on(document, "touchmove", func);  ----2

Yep, I see your point, just had a quick try, the result is quite interesting, the evt passed to func won't be corrupted. I guess the reason is only the raw event will be bubbled to parent - which means evt customization will only be effective to the current node.

But I agree the following way shall be more safe, going to get it in

listener.call(this, lang.delegate(evt, {
	target: hoveredNode
}));

[Evan]

In [29638]:

Fixes #15892 !strict, using lang.delegate to change evt.target. also back ported to 1.8.x branch

[Colin Snover]

You cannot use lang.delegate on an Event object to change a property which already exists. The rules of [[CanPut]] in ES5 make it so the property will not be changed. The only way this works is if you use Object.defineProperty on the new delegate object. As this is an ES5 + Web IDL spec requirement, future mobile browsers will work the same way unless the specs are changed (unlikely).

[Evan]

Replying to csnover:

You cannot use lang.delegate on an Event object to change a property which already exists. The rules of [[CanPut]] in ES5 make it so the property will not be changed. The only way this works is if you use Object.defineProperty on the new delegate object. As this is an ES5 + Web IDL spec requirement, future mobile browsers will work the same way unless the specs are changed (unlikely).

So this is different from a real case unless I missed anything:

As mentioned above, in dojo/on(line 458+) we are able to change the already existed properties of raw event with two approaches:

Way 1 - use an event delegation on iOS

//dojo/on - line 458+

var Event = function(){};
...
var type = originalEvent.type;
try{
	delete originalEvent.type; // on some JS engines (android), deleting properties make them mutable
}catch(e){} 

if(originalEvent.type){
	// deleting properties doesn't work (older iOS), have to use delegation
	Event.prototype = originalEvent;
	var event = new Event;
	// have to delegate methods to make them work
	event.preventDefault = function(){
		originalEvent.preventDefault();
	};
	event.stopPropagation = function(){
		originalEvent.stopPropagation();
	};

Way 2 - on Android, changing the original properties directly

}else{
	// deletion worked, use property as is
	event = originalEvent;
	event.type = type;
}

Based on that, on touch devices(at least iOS + Android), it's possible to change the already existed properties in event(which is raw touch event on Android and delegated event on iOS). a quick test can be:

var inner = dom.byId("inner");
var outer = dom.byId("outer");
on(inner, touch.move, function(evt){
    //log 1 - "e.target = inner"
    console.log('e.target = ' + evt.target.id); 
    
    evt.target = outer;
    
    //log 2 - "after changed, e.target = outer"
    console.log('after changed, e.target = ' + evt.target.id);
});

When moving touch on the "inner" div, evt.target will be changed to "outer" on iOS5 and Android(2.3 + 4.0)

[Colin Snover]

I suspect that the dojo/on code will stop working at some point in the future too, so it would be a good idea to avoid adding more places where things will break…

Using the test case at ​http://jsfiddle.net/BVKPf/ gives the following:

Firefox 13-: no error, type not updated
Firefox 14-15: error
Chrome 22-: type not updated
Safari 6-: type updated
IE 10-: error
Opera 12: type updated
Android 4.1: type not updated

As such I don’t really feel like delegation is safe here. Furthermore, calls to preventDefault and stopPropagation will fail on some platforms with delegation even if property setters work because |this| is modified at call time to point to the delegate object, not the original event object, and many engines cannot handle this (some silently pretend to work but do not actually cancel the event, others throw errors).

[bill]

Good catch, too bad that lang.delegate() has that issue. So I revert to my original suggestion

listener.call(this, lang.mixin({}, evt, { 
   target: hoveredNode,
   preventDefault: function(){ evt.preventDefault(); },
   stopPropagation: function(){ evt.stopPropagation(); }
})); 

[Evan]

Replying to csnover:

I suspect that the dojo/on code will stop working at some point in the future too, so it would be a good idea to avoid adding more places where things will break…

Using the test case at ​http://jsfiddle.net/BVKPf/ gives the following:

Firefox 13-: no error, type not updated
Firefox 14-15: error
Chrome 22-: type not updated
Safari 6-: type updated
IE 10-: error
Opera 12: type updated
Android 4.1: type not updated

Yep, dojo/on was only targeting touch devices to make the raw touch events changeable, not related with any desktops'

btw, the tricky way similarly used in dojo/on shall make the above event.type modifiable on Android. ​http://jsfiddle.net/BVKPf/6/

As such I don’t really feel like delegation is safe here. Furthermore, calls to preventDefault and stopPropagation will fail on some platforms with delegation even if property setters work because |this| is modified at call time to point to the delegate object, not the original event object, and many engines cannot handle this (some silently pretend to work but do not actually cancel the event, others throw errors).

OK, I agree with your point, though it's possible to make the raw touch event modifiable, it's a bit tricky and might not be officially expected.

So in the future if the tricky way stopped working and we still have to normalize touch events across devices, we may have to create a new event obj

[Evan]

Replying to bill:

Good catch, too bad that lang.delegate() has that issue. So I revert to my original suggestion

listener.call(this, lang.mixin({}, evt, { 
   target: hoveredNode,
   preventDefault: function(){ evt.preventDefault(); },
   stopPropagation: function(){ evt.stopPropagation(); }
})); 

Thanks, Bill, this works nicely.

BTW, I was previously wondering if this will bring performance affect since many new event objs are continuously created and mixin-ed when user keep moving touch points, but I don't think there are any better ways except the above deprecated tricky way.

Actually this also fixes #15946

[Evan]

In [29646]:

Fixes #15892 !strict, create new event obj by mixin rather than lang.delegate for changing evt.target, also back ported to 1.8.x branch

[bill]

Backported to 1.7 in e7403082d02faba2e47cb645d2532dd2920f60b8.