_CssStateMixin cssStateNodes is shared, causes problems when set programatically

[Nick Fenwick]

Not sure if this is a bug, per se. I write to this.cssStateNodes in a loop, because the nodes I want _CssStateMixin to manage are not known in advance, and this breaks other dijits that are instantiated, because _CssStateMixin.js contains:

        cssStateNodes: {},

This is fine for simple dijits that set that attribute directly, e.g.

    declare(_CssStateMixin, {
        cssStateNodes: { foo: 'bar' }
    })

That hides the prototype instance for that dijit. However, if you write to it in a loop,

    declare([_WidgetBase, _CssStateMixin], {
        buildRendering: function() {
            this.inherited(arguments);
            query('.something', this.domNode).forEach(lang.hitch(this, function(node) {
                this.cssStateNodes[node.id] = node.id; /* DANGER Will Robinson! */
            })
        }
    })

This pollutes the _CssStateMixin.prototype.cssStateNodes and the next dijit you instantiate that derives from _CssStateMixin but does _not_ set this.cssStateNodes will have this.cssStateNodes left over from above, and fail horribly.

This happened to me with new TabContainer which goes into TabController, creates a MenuItem, and falls over while constructing it.

Should this be considered buggy, and we change _CssStateMixin?

        cssStateNodes: null,

        constructor: function() {
                this.cssStateNodes = {}
        },

This fixed my situation. A nicer workaround in my case is to set this.cssStateNodes = {} in my code, before writing to it, so we are no longer polluting the prototype instance.

[bill]

I think what you are proposing will break all the existing widgets that declare cssStateNodes in their prototypes, for example:

declare("dijit.Calendar",
	[CalendarLite, _Widget, _CssStateMixin], // _Widget for deprecated methods like setAttribute()
	{
	...
	// Set node classes for various mouse events, see dijit._CssStateMixin for more details
	cssStateNodes: {
		"decrementMonth": "dijitCalendarArrow",
		"incrementMonth": "dijitCalendarArrow",
		"previousYearLabelNode": "dijitCalendarPreviousYear",
		"nextYearLabelNode": "dijitCalendarNextYear"
	},
...

Do you know a way to avoid breaking all the existing widgets while avoiding this fragile code that you brought up?

[ben hockey]

i suppose you could at least set cssStateNodes to null in _CssStateMixin so that people don't accidentally use it and then either leave it to users to add this themselves in the constructors of their subclasses or add it to the constructor of _CssStateMixin

this.cssStateNodes = this.cssStateNodes || {};

i think that should keep the existing code working.

[bill]

Agreed.

I don't see much point to setting up a per-instance cssStateNodes Object in _CssStateMixin's constructor. As I said, generally widgets set cssStateNodes in their prototype (see for example all of the widgets in dijit), and for the example given in this ticket, it would be better written to use _trackMouseState() rather than modifying cssStateNodes dynamically:

    declare([_WidgetBase, _CssStateMixin], {
        buildRendering: function() {
            this.inherited(arguments);
            query('.something', this.domNode).forEach(lang.hitch(this, "_trackMouseState"));
        }
    })

[bill]

In [30215]:

Don't define cssStateNodes Object in _CssStateMixin's prototype, since it may lead to programming errors in subclasses that try to modify this shared object. Fixes #16486 !strict.

[Nick Fenwick]

Balls, you beat me to it, I just finished preparing a patch :) since you seem to have committed, I'll quit while you're ahead.

[freddefisk]

Didn't you forget to set cssStateNodes to null instead of {}? I don't see how the current change will have any effect, or am I missing something?

[Nick Fenwick]

He simply ommitted (commented out) its definition. This makes the line that logical or's it with {} return the empty object, when this.cssStateNodes doesn't exist. I've been running with a monkey patched, but otherwise identical, version all day without trouble, it fixes the original problem.

[bill]

Actually, I didn't put in a line that logical or's it with {} either.

The effect of my change is simply that it will be more obvious to user code when they've made an error. "Error" meaning that they are trying to use _cssStateNodes without defining it first. In other words, a line like

this._cssStateNodes[foo] = bar;

will fail immediately, with an exception, because this._cssStateNodes isn't defined

I also updated the comments to hopefully make it clear that user code needs to define this._cssStateNodes itself if it wants to use it.

[Nick Fenwick]

Actually, I didn't put in a line that logical or's it with {} either.

Eh? Then what is this?

for(var ap in this.cssStateNodes || {}){ 

The || {} is a logical OR operator followed by an empty object. It's a bit terse, my patch was going to wrap the for loop in if (this.cssStateNodes) to be a little more obvious and probably save a bit of work too.

I did think it was unsafe to iterate objects like that without a .hasOwnProperty(idx) test to weed out Object prototype members, but apparently that's not a problem here, though I'm not sure why :)

[bill]

Oh I misunderstood, I meant that I didn't add a constructor as suggested in comment:2.