Opened 8 years ago

Closed 8 years ago

Last modified 8 years ago

#12278 closed enhancement (fixed)

_WidgetBase: fix inheritance issues with attributeMap

Reported by: bill Owned by: billl
Priority: high Milestone: 1.7
Component: Dijit Version: 1.6.0b1
Keywords: 1.7-mobile Cc:
Blocked By: Blocking:

Description (last modified by bill)

attributeMap has issues with inheritance, in that it's hard for a mixin to add/override an attribute in an already existing attributeMap. In particular, the dojo.delegate() call hardcodes the name of the superclass, but mixins can't know the superclass' name.

This can be solved by using custom setters for everything, rather than attributeMap. However, that makes the code longer. So, one solution is to provide a custom setter generator as part of _WidgetBase. Same basic syntax as a single key/value entry in attributeMap. For example:

attributeMap:
dojo.delegate(dijit.form._FormValueWidget.prototype.attributeMap, {
   maxLength: "focusNode"
})

can instead be written as:

_setMaxLengthAttr: dijit.das("maxLength", "focusNode")

The downside of this approach is that we lose the meta-data, which would be a problem if (like in cujo) attributeMap could run in reverse, reflecting DOMNode changes (like clicking a checkbox or typing into an input) back into the widget javascript object.

Doug's suggestion was to change:

attributeMap: {foo: "", bar: "focusNode"}

into something like:

_fooAttrMap: "",
_barAttrMap: "focusNode"

Attachments (1)

generatedSetter2.patch (3.1 KB) - added by bill 8 years ago.
patch from before _Widget was refactored into _WidgetBase and _Widget

Download all attachments as: .zip

Change History (9)

Changed 8 years ago by bill

Attachment: generatedSetter2.patch added

patch from before _Widget was refactored into _WidgetBase and _Widget

comment:1 Changed 8 years ago by bill

Description: modified (diff)
Summary: _WidgetBase: custom setter generator_WidgetBase: fix inheritance issues with attributeMap

comment:2 Changed 8 years ago by bill

(In [23941]) Support for _mapXXXAttr attributes that specifying a mapping from a widget's XXX attribute to a DOMNode attribute/style/innerHTML etc.

Each _mapXXXAttr attribute/value is equivalent to a single key/value pair from attributeMap. The _mapXXXAttr API supercedes attributeMap, which is deprecated and will be removed in 2.0.

For backwards-compatibility, the widget attributes with attributeMap entries are processed first, followed by the attributes with _mapXXXAttr objects and _setXXXAttr() methods. The _mapXXXAttr objects / _setXXXAttr() methods are called according to the order the attributes are declared in the widget.

Refs #12278 !strict.

comment:3 Changed 8 years ago by bill

Resolution: fixed
Status: newclosed

(In [23943]) Fix for _mapXXXAttr attributes support, and conversion of dijit widgets from attributeMap to that format.

Fixes #12278 !strict.

comment:4 Changed 8 years ago by bill

(In [23946]) Rather than having _mapXXXAttr: { ... } to replace attributeMap, allow _setXXXAttr to be defined as a hash/string/array value, in addition to being defined as a method. This avoids confusion when a widget has both a _mapXXXAttr and a _setXXXAttr.

There was some weirdness with "class" and "style" where the _setClassAttr() and _setStyleAttr() methods depended on _mapClassAttr/attributeMapclass? and _mapStyleAttr/attributeMapstyle? that I had to work around. Note: it seems like "style" is always applied to the widget root node.

Refs #12278 !strict.

comment:5 in reply to:  4 Changed 8 years ago by ben hockey

just a thought... if this relationship was bi-directional, ie a property of a node mapped back to a widget property then using _setXXXAttr for this mapping might not be a good choice. for example, if you added support for _getXXXAttr to do the reverse of _setXXXAttr then you would need to repeat this map in _getXXXAttr. i don't happen to do this at all but just looking towards future possibilities.

Replying to bill:

(In [23946]) Rather than having _mapXXXAttr: { ... } to replace attributeMap, allow _setXXXAttr to be defined as a hash/string/array value, in addition to being defined as a method. This avoids confusion when a widget has both a _mapXXXAttr and a _setXXXAttr.

There was some weirdness with "class" and "style" where the _setClassAttr() and _setStyleAttr() methods depended on _mapClassAttr/attributeMapclass? and _mapStyleAttr/attributeMapstyle? that I had to work around. Note: it seems like "style" is always applied to the widget root node.

Refs #12278 !strict.

comment:6 Changed 8 years ago by bill

I am thinking about bi-directional support, although note that "bi-directional support" can mean multiple things.

Also, note that this.set("foo", bar) also sets this.foo = bar, so you don't need a _getFooAttr() method at all.

Anyway, I agree that _setXXXAttr() probably isn't an ideal name, but I needed to maintain backwards-compatibility and didn't want to hit that templatePath/templateString duality confusion where widgets have two variables that conflict with each other.

comment:7 Changed 8 years ago by bill

Keywords: 1.7-mobile added

comment:8 Changed 8 years ago by bill

(In [24281]) clean up test case, refs #12278 !strict.

Note: See TracTickets for help on using tickets.