#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 8 years ago by
Owner: | changed from bill to Nick Fenwick |
---|---|
Status: | new → pending |
comment:2 Changed 8 years ago by
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 8 years ago by
Milestone: | tbd → 1.9 |
---|---|
Owner: | changed from Nick Fenwick to bill |
Status: | pending → assigned |
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")); } })
comment:5 Changed 8 years ago by
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 8 years ago by
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 8 years ago by
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 8 years ago by
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 8 years ago by
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 8 years ago by
Oh I misunderstood, I meant that I didn't add a constructor as suggested in comment:2.
I think what you are proposing will break all the existing widgets that declare cssStateNodes in their prototypes, for example:
Do you know a way to avoid breaking all the existing widgets while avoiding this fragile code that you brought up?