Opened 7 years ago

Closed 7 years ago

Last modified 5 years ago

#15892 closed defect (fixed)

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

Reported by: Adrian Vasiliu Owned by: Evan
Priority: high Milestone: 1.7.7
Component: Dojox Version: 1.8.0
Keywords: Cc: bill, cjolif, ben hockey
Blocked By: Blocking:

Description (last modified by cjolif)

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.

Attachments (3)

15892.html (901 bytes) - added by Evan 7 years ago.
The missed test case for reproducing the issue in Andriod2.3/4.0
15892.patch (542 bytes) - added by Evan 7 years ago.
A new version for the fix patch
15892-v2.patch (494 bytes) - added by Evan 7 years ago.
A simpler fix version since the evt is already delegated, so we can modified its properties freely

Download all attachments as: .zip

Change History (31)

comment:1 Changed 7 years ago by cjolif

Owner: changed from Adam Peller to Evan
Priority: undecidedhigh
Status: newassigned

comment:2 Changed 7 years ago by cjolif

Description: modified (diff)

comment:3 Changed 7 years ago by 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.

comment:4 Changed 7 years ago by Evan

Milestone: tbd1.8.1
Summary: dojox/gesture: "Uncaught TypeError: Illegal invocation" on Android devices[cla][patch] dojox/gesture: "Uncaught TypeError: Illegal invocation" on Android devices

comment:5 Changed 7 years ago by Evan

Cc: bill cjolif added

comment:6 Changed 7 years ago by 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(); };

Changed 7 years ago by Evan

Attachment: 15892.html added

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

comment:7 in reply to:  6 Changed 7 years ago by 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...

Last edited 7 years ago by Evan (previous) (diff)

Changed 7 years ago by Evan

Attachment: 15892.patch added

A new version for the fix patch

comment:8 Changed 7 years ago by bill

I checked your updated patch, but it seems like

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

still won't work on desktop?

Last edited 7 years ago by bill (previous) (diff)

comment:9 in reply to:  8 ; Changed 7 years ago by 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 the15892.html

comment:10 in reply to:  9 Changed 7 years ago by 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 the15892.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

comment:11 Changed 7 years ago by 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(); }
})); 

comment:12 Changed 7 years ago by bill

Or

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

comment:13 Changed 7 years ago by 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);
		}
	});
};

Last edited 7 years ago by Evan (previous) (diff)

Changed 7 years ago by Evan

Attachment: 15892-v2.patch added

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

comment:14 Changed 7 years ago by 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.

comment:15 in reply to:  14 Changed 7 years ago by 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.

comment:16 Changed 7 years ago by 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);

comment:17 in reply to:  16 Changed 7 years ago by 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
}));

comment:18 Changed 7 years ago by Evan

Resolution: fixed
Status: assignedclosed

In [29638]:

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

comment:19 Changed 7 years ago by Colin Snover

Resolution: fixed
Status: closedreopened

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).

Last edited 7 years ago by Colin Snover (previous) (diff)

comment:20 in reply to:  19 Changed 7 years ago by 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)

Last edited 7 years ago by Evan (previous) (diff)

comment:21 Changed 7 years ago by ben hockey

Cc: ben hockey added

comment:22 Changed 7 years ago by 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).

comment:23 Changed 7 years ago by 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(); }
})); 

comment:24 in reply to:  22 Changed 7 years ago by 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

comment:25 in reply to:  23 ; Changed 7 years ago by 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

Last edited 7 years ago by Evan (previous) (diff)

comment:26 in reply to:  25 Changed 7 years ago by Evan

Last edited 7 years ago by Evan (previous) (diff)

comment:27 Changed 7 years ago by Evan

Resolution: fixed
Status: reopenedclosed

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

comment:28 Changed 5 years ago by bill

Milestone: 1.8.11.7.7

Backported to 1.7 in e7403082d02faba2e47cb645d2532dd2920f60b8.

Note: See TracTickets for help on using tickets.