Opened 6 years ago

Closed 6 years ago

Last modified 6 years ago

#16486 closed defect (fixed)

_CssStateMixin cssStateNodes is shared, causes problems when set programatically

Reported by: Nick Fenwick Owned by: bill
Priority: undecided Milestone: 1.9
Component: Dijit Version: 1.8.1
Keywords: Cc:
Blocked By: Blocking:

Description

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.

Change History (10)

comment:1 Changed 6 years ago by bill

Owner: changed from bill to Nick Fenwick
Status: newpending

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?

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

comment:3 Changed 6 years ago by bill

Milestone: tbd1.9
Owner: changed from Nick Fenwick to bill
Status: pendingassigned

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"));
        }
    })
Last edited 6 years ago by bill (previous) (diff)

comment:4 Changed 6 years ago by bill

Resolution: fixed
Status: assignedclosed

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.

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

comment:6 Changed 6 years ago by 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?

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

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

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

comment:10 Changed 6 years ago by bill

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

Note: See TracTickets for help on using tickets.