Opened 10 years ago

Closed 10 years ago

#9409 closed defect (fixed)

Make sure that dojo.mixin() works correctly with non-enumerable properties on IE.

Reported by: Eugene Lazutkin Owned by: Eugene Lazutkin
Priority: high Milestone: 1.4
Component: Core Version: 1.3.0
Keywords: Cc: James Burke, alex, bill
Blocked By: Blocking:

Description

dojo.mixin() and its real implementation dojo._mixin() do not handle well non-enumerable properties on IE.

Background: IE always skips certain names assuming them non-enumerable. Here is the list:

  • hasOwnProperty
  • valueOf
  • isPrototypeOf
  • propertyIsEnumerable
  • toLocaleString
  • toString

dojo._mixin() handles explicitly only one property: toString making redefining these properties futile on IE.

To compound the problem there are other facilities, which duplicate the mix-in functionality without handling the aforementioned properties on IE. The most notable example is dojo.declare().

In order to fix the situation we need to:

  • Make the list of these properties public to share between different mix-in functions.
  • Make sure that the list is checked explicitly on IE in dojo._mixin() and dojo.declare().

Attachments (1)

mixin.patch (1.4 KB) - added by Nathan Toone 10 years ago.
Patch which fixes this issue - also contains an update to the test case.

Download all attachments as: .zip

Change History (22)

comment:1 Changed 10 years ago by Eugene Lazutkin

Cc: James Burke alex added
Owner: changed from anonymous to Eugene Lazutkin

comment:2 Changed 10 years ago by Eugene Lazutkin

Status: newassigned

One possible solution is shown in http://bugs.dojotoolkit.org/browser/dojox/trunk/lang/oo/declare.js, which fixes several problems including the correct mix-in behavior.

comment:3 Changed 10 years ago by Eugene Lazutkin

Hmm. Fixed in [19481].

comment:4 Changed 10 years ago by Eugene Lazutkin

(In [19482]) lang: changing to accommodate extra names, !strict, refs #9409.

comment:5 Changed 10 years ago by Eugene Lazutkin

Resolution: fixed
Status: assignedclosed

comment:6 Changed 10 years ago by Eugene Lazutkin

Milestone: tbd1.4

comment:7 Changed 10 years ago by Nathan Toone

Resolution: fixed
Status: closedreopened

This seems to avoid copying in explicitly-set undefined values...which previously worked. i.e.

dojo.mixin({a: 1}, {a: undefined})

Previously would end up with an object

{a: undefined}

But with this current change you end up with

{a:1}

Suggest changing line 267 to:

if(empty[name] === undefined || (s !== empty[name] && s !== target[name])){

Changed 10 years ago by Nathan Toone

Attachment: mixin.patch added

Patch which fixes this issue - also contains an update to the test case.

comment:8 Changed 10 years ago by Eugene Lazutkin

Resolution: fixed
Status: reopenedclosed

(In [19942]) Simplification of dojo._mixin() + more unit tests, !strict, fixes #9409.

comment:9 Changed 10 years ago by Douglas Hays

This change breaks dijit.form widgets. References #9775

comment:10 Changed 10 years ago by Douglas Hays

Cc: bill added
Resolution: fixed
Status: closedreopened

[19942] needs to be rolled back. Using IE, there are 2 different altered mixin behaviors resulting from that change:
1) prototype methods in the source are now overriding user methods
2) attributes with undefined values are being added to the target and they used to not (not sure if this is bad or good, but its different and slightly less performant)

var t = {toString: function(){alert('toString');}};
dojo.mixin(t, {name:undefined});
t.toString();
alert('name in t = ' + ('name' in t));

now shows 1 alert displaying "name in t = true", it used to show 2 alerts, the first being "toString" and the 2nd alert displaying "name in t = false".

comment:11 in reply to:  10 Changed 10 years ago by Eugene Lazutkin

Replying to doughays:

2) attributes with undefined values are being added to the target and they used to not (not sure if this is bad or good, but its different and slightly less performant)

Copying of undefined properties were added separately on request. See toontown's comments above. Actually he reopened this ticket specifically for that. In my opinion copying undefined properties is more consistent. At least I would expect that properties are copied regardless their value.

comment:12 in reply to:  10 ; Changed 10 years ago by Eugene Lazutkin

Replying to doughays:

1) prototype methods in the source are now overriding user methods

I suspect that you mean that methods available in the Object.prototype from the source can override custom methods in the target. It is a valid point. I'll fix it ASAP.

comment:13 Changed 10 years ago by Eugene Lazutkin

(In [19978]) Making sure that properties available on the empty object do not override custom properties on the target, !strict, refs #9409.

comment:14 in reply to:  12 Changed 10 years ago by Eugene Lazutkin

Replying to elazutkin:

Replying to doughays:

1) prototype methods in the source are now overriding user methods

I suspect that you mean that methods available in the Object.prototype from the source can override custom methods in the target. It is a valid point. I'll fix it ASAP.

Should be fixed now. Please retest whatever was failing on your side. If it works as expected, please close this ticket. Otherwise let me know what's still broken.

comment:15 Changed 10 years ago by Douglas Hays

Resolution: fixed
Status: reopenedclosed

comment:16 Changed 10 years ago by Eugene Lazutkin

(In [20176]) "constructor" was missing from the list of "invisible" properties (for IE), !strict, refs #9409.

comment:17 Changed 10 years ago by haysmark

Priority: normalhigh
Resolution: fixed
Status: closedreopened

I went revision by revision and starting with [20176] I get a JavaScript? error in IE8 when running the DataGrid? automated test:

http://archive.dojotoolkit.org/dojo-2009-09-21/dojotoolkit/dojox/grid/tests/robot/DataGrid_mouse.html

The error is: "'bases' is null or not an object" and points to the first line of findInherited in dojo._base.declare:

	function findInherited(self, caller, name){
		var meta = self.constructor._meta, bases = meta.bases, // ERROR

comment:18 in reply to:  17 Changed 10 years ago by Eugene Lazutkin

Replying to haysmark:

The test in question uses different frames to load code => IE uses different JavaScript interpreters for different frames. As a precaution the test uses dojo.global. prefix for all objects, save for params (the first constructor's argument, which is a bag of properties for widgets, see dijit._Widget.postscript() and related methods). Because of this params has default yet distinct constructor from an empty object:

var x = new Object();
var y = new dojo.global.Object();
console.log(x.constructor === y.constructor);
// prints "false" instead of "true"

I modified the test. The old code was:

setUp: function(){
  this.grid = new dojo.global.dojox.grid.DataGrid(
    {id: 'prog_grid_1', structure: dojo.global.structure1},
    dojo.byId('prog_grid_1'));
  this.grid.startup();
},

I re-wrote it like this:

setUp: function(){
  var params = new dojo.global.Object();
  params.id = 'prog_grid_1';
  params.structure = dojo.global.structure1;
  this.grid = new dojo.global.dojox.grid.DataGrid(params, dojo.byId('prog_grid_1'));
  this.grid.startup();
},

Now the test works.

The true underlying problem (besides IE's obvious craziness) is that dijit._Widget.create() contains following code:

create: function(/*Object?*/params, /*DomNode|String?*/srcNodeRef){
  // ...
  if(params){
    this.params = params;
    dojo.mixin(this, params);
  }
  // ...
}

It mixes in the first argument to its own object potentially overriding anything, including the constructor. Probably instead of dojo.mixin() we should use dijit.mixin() (or any other name) that will do two things:

  1. Protect vital methods from accidental overriding by skipping them while copying.
  2. Optionally add dojo.declare()-compatible meta information to copied methods so we don't have any surprises here.

Thoughts? If it is worth considering, please open a separate ticket for that.

But coming back to the problem at hand: please re-test and close this ticket, if everything is fine. Otherwise let me know if anything else is broken.

comment:19 Changed 10 years ago by Eugene Lazutkin

(In [20517]) Making sure that all objects that are mixed in are of the same interpreter (IE-specific quirk), !strict, refs #9409.

comment:20 Changed 10 years ago by bill

Too bad it's such a hassle to create widgets across frames. Thanks for fixing the test.

I don't think we need to add a dijit.mixin() just yet, probably for 2.0 we'll revamp how parameters to widgets work and get rid of that dojo.mixin() call altogether, replacing it with a list of expected parameters for the widget (and something about registering callbacks like onClick).

comment:21 Changed 10 years ago by Eugene Lazutkin

Resolution: fixed
Status: reopenedclosed
Note: See TracTickets for help on using tickets.