Opened 13 years ago

Closed 13 years ago

Last modified 12 years ago

#7244 closed enhancement (fixed)

[patch][cla] attr() for Dijit — at Version 51

Reported by: alex Owned by: bill
Priority: high Milestone: 1.2
Component: Dijit Version: 1.1.1
Keywords: Cc: alex
Blocked By: Blocking:

Description (last modified by bill)

Support Widget.attr() as canonical method to get/set all widget attributes. Ex:

myWidget.attr('value', 35)   --> set value to 35

or

myWidget.attr('disabled') --> returns true if widget is disabled

Can also pass in a hash of values to set:

myWidget.attr({ value: 35, disabled: true} )

Internally, attr() will set the widget attribute (aka property) using data in attributeMap. attributeMap has been enhanced to support changes to DOM node innerHTML or class, in addition to DOM node attributes.

Developers of widgets can also specify custom getters/setters using a naming convention of _setFooAttr(), _getFooAttr()

Change History (52)

comment:1 Changed 13 years ago by bill

Thanks for the patch. I think you are missing a file though? Where is dijit.attr() defined? I don't see it in the patch (or the existing code in SVN).

Also, what exactly does injectAttributes do? There's no comment on the flag. I looked at the test but I'd rather have it spelled out explicitly.

About the new test in manager.html (which BTW isn't really a test for manager, it should be in test_Templated or somewhere else)... isn't it meaningless to have two separate nodes with dojoAttachPoint='fooNode' ?

comment:2 Changed 13 years ago by bill

BTW, I like the general idea of this. I thought we could expand setAttribute() to do all this work, but I guess attr() is a better name since it matches dojo.attr() and since it can be a getter or setter.

comment:3 Changed 13 years ago by alex

/me slaps forehead.

Updated patch attached.

Changed 13 years ago by alex

Attachment: attr.patch added

comment:4 Changed 13 years ago by alex

regarding injectAttributes:

The in many places, the ${propertyName} replacement syntax is desireable for populating the content of a node, like this:

<div dojoAttachPoint="fooNode">${foo}</div>

This, however, is sub-optimal in that it requires regex evaluation and string stubstitution on the template. It is usually much faster for the constructor chain to do something like:

this.fooNode.innerHTML = this.foo;

So the new injectAttributes allows you to specify a template like this:

<div dojoAttachPoint="fooNode"></div>

Which automatically populates that node with the contents of the property with the name "foo". It also avoids the regex/replace cost (at some up-front scanning price). It's just another codification of a pattern and practice that we use often but which attr() can help us speed up. Note that injectAttributes is off by default, allowing for full backwards compatibility (e.g., no surprises if your template had a fooNode and foo property on 1.1 but didn't use them in the way outlined above).

comment:5 Changed 13 years ago by bill

OK, that's much better :-).

How about widget attributes that map to DomNode attributes, like for example if I did myWidget.attr("disabled", true) ? I don't see code in there for that, although possibly I missed it.

comment:6 Changed 13 years ago by alex

I wasn't really sure what to do with stuff in the attributes array. It seemed to me that the semantic of attr() was to manage attributes of the instance, but clearly there's overlap with DOM attribute state. I can look into implementing something for that, but I don't know that it has back-compat issues that can/should block landing this patch as-is.

comment:7 Changed 13 years ago by bill

Yah, please look into that. ISTM the main purpose of Widget.attr() is to present a standard interface for getting/setting all the attributes of the widget. It doesn't make much sense if it only handles 20% of them.

comment:8 Changed 13 years ago by alex

hrm, I don't think I've been clear: the current impl handles every property of a widget BUT things in this.attributes, but you can plug in w/ a defaultSetter/defaultGetter to handle that right now. The only thing that isn't wired up for you is an agreement on the convention about what happens if the widget has a property with the name of something in attributes. That can be handled later. The patch should land now, as-is.

comment:9 Changed 13 years ago by bill

The entries in this.attributeMap (not this.attribute) make up 80% of the attributes for the widgets (excluding methods).

I think this is a trivial change, something like

if(attr in this.attributeMap){
   this.setAttribute(attr, value);
}

defaultSetter/defaultGetter won't do the trick since they would call setAttribute() in too many cases, like to adjust "duration".

comment:10 Changed 13 years ago by bill

Status: newassigned

I'm going to check this in with the changes for handling DOM node attributes in addition to things like title and duration.

comment:11 Changed 13 years ago by bill

Resolution: fixed
Status: assignedclosed

(In [14641]) Widget.attr() method to function like dojo.attr(), setting/getting all properties of a widget, regardless of whether they map to:

  • innerHTML,(ex: TitlePane?'s title)
  • a DOM node attribute (ex: TextBox?'s disabled)
  • or just a property in "this" (ex: duration).

Patch from Alex (thanks!) with some modifications from me. Fixes #7244 !strict.

comment:12 Changed 13 years ago by alex

hrm, seems to have missed the manager changes? Or is the intent not to support a dijit.attr(widget, prop, value); syntax?

comment:13 Changed 13 years ago by alex

also, regarding the change to handle attributeMap, what about in the getter branch? Or is that handled?

comment:14 Changed 13 years ago by bill

Right, I didn't add dijit.attr() because I only wanted to support one API (widget.attr()).

As for attributeMap, the getter corresponding to widget.setAttribute("foo", ...) is simply widget.foo, so that's handled automatically by existing code paths.

comment:15 Changed 13 years ago by bill

(In [14679]) Fix Widget.attr(hash), and some of the line errors in Widget.js. Refs #7244 !strict

comment:16 Changed 13 years ago by bill

(In [14707]) Change deprecation warnings to recommend attr() rather than setAttribute(). Refs #7244 !strict

comment:17 Changed 13 years ago by bill

(In [14708]) Change deprecation warnings to recommend attr() rather than setAttribute(). Refs #7244 !strict

comment:18 Changed 13 years ago by bill

(In [14709]) Update tests to use attr() rather than setAttribute(). Refs #7244.

comment:19 Changed 13 years ago by bill

(In [14711]) use _attrSetFoo() rather than setFoo() to register special handlers for setting/getting attributes, so that setFoo() doesn't show up in the online doc and to avoid triggering spurious deprecation warnings from setDisabled() being called by attr("disabled", bool), etc. Will be followed by many checkins updating each widget so that attr() works on all attributes. Refs #7244 !strict

comment:20 Changed 13 years ago by bill

(In [14713]) Deprecate setContent()/setHref() in favor of attr(). Had to change attr() to allow custom return values; in this case attr('href', ...) returns a Deprecated. Refs #7244 !strict

comment:21 Changed 13 years ago by bill

(In [14714]) ContentPane? programmatic test/example. Refs #7244 !strict

comment:22 Changed 13 years ago by bill

(In [14715]) Make attr('title', ...) on TitlePane? work. Had to fix some issues in _Templated and _Widget because title is *both* an attribute on TitlePane?.domNode and the innerHTML of TitlePane?.titleNode. Actually perhaps that's more of a workaround than a fix, as there's code in ContentPane?.postCreate() to remove the title attribute from this.domNode. Refs #7244 !strict

comment:23 Changed 13 years ago by bill

(In [14716]) Oops, checked in this line by mistake; reverting. Refs #7244 !strict

comment:24 Changed 13 years ago by bill

(In [14717]) attr() for InlineEditBox?. Refs #7244 !strict

comment:25 Changed 13 years ago by bill

(In [14718]) setDisabled() --> attr('disabled', ...) Refs #7244 !strict

comment:26 Changed 13 years ago by bill

(In [14719]) setDisabled() --> attr('disabled', ...) Refs #7244 !strict

comment:27 Changed 13 years ago by bill

(In [14720]) setLabel() --> attr('label', ...) Refs #7244 !strict

comment:28 Changed 13 years ago by bill

(In [14721]) setValue() --> attr('value', ...) for Calendar and TimePicker? Refs #7244 !strict

comment:29 Changed 13 years ago by bill

(In [14725]) Refactor how reset() works in preparation for getting attr('value', ...) to work for form widgets. Refs #7244 !strict

comment:30 Changed 13 years ago by Bryan Forbes

(In [14734]) refs #7244 !strict

comment:31 Changed 13 years ago by bill

(In [14740]) Support attr('value') and deprecate myWidget.getValue()/myWidget.value for form widgets. Refs #7244 !strict

comment:32 Changed 13 years ago by bill

(In [14742]) Support attr('value') and deprecate myWidget.getValue()/myWidget.value for form widgets. Refs #7244 !strict

comment:33 Changed 13 years ago by bill

(In [14748]) displayedValue:

  • support displayedValue as initialization parameter
  • support attr('displayedValue') and deprecate getDisplayedValue()
  • support attr('displayedValue', val) and deprecate setDisplayedValue()

Refs #7244 !strict

comment:34 Changed 13 years ago by bill

(In [14749]) InlineEditBox? fixes related to attr('displayedValue'). Refs #7244 !strict

comment:35 Changed 13 years ago by bill

(In [14762]) attr('checked', ...) for CheckedMenuItem?. Unclear whether or not attr('checked', ...) as opposed to an actual click should trigger onChange() (it doesn't, after my change), but I don't think it's very important either way. Refs #7244 !strict

comment:36 Changed 13 years ago by bill

(In [14796]) Call attr('label', ...) during initialization to setup label/title (regardless of whether it comes from label initialization parameter, or srcNodeRef.innerHTML. Refs #7244 !strict

comment:37 Changed 13 years ago by bill

(In [14798]) Fix checkbox as per previous change to Button. Refs #7244 !strict

comment:38 Changed 13 years ago by bill

(In [14799]) Put verb first, as per coding standards. To register a handler for setting value attribute, call it _setValueAttr(). Refs #7244 !strict

comment:39 Changed 13 years ago by bill

(In [14801]) Make tests use attr('value', ...) instead of setValue(). Refs #7244.

comment:40 Changed 13 years ago by bill

(In [14802]) Start replacing calls to setAttribute() with attr(). Refs #7244 !strict

comment:41 Changed 13 years ago by bill

(In [14803]) setAttribute() related changes:

  • Deprecate setAttribute().
  • Move all special processing code (like for setting wai state when a widget is disabled) into attr()-callback setters like _setDisabledAttr().
  • Code to set DOM attributes according to attributeMap moved from setAttribute() to new _attrToDom() method.

Refs #7244 !strict

comment:42 Changed 13 years ago by bill

(In [14821]) Make MappedTextBox? do it's magic as part of buildRendering() rather than postCreate(). It's adding another node to the DOM so it really _should_ be part of buildRendering().

This fixes the FilteringSelect? initialization issue although it's still debatable whether all the attr() setter calls should happen before or after postCreate(), or inside _Widget.postCreate().

Also moved the attr() setter calls into a private method, in case that needs to be overridden.

Refs #7244, fixes #7414. !strict

comment:43 Changed 13 years ago by bill

(In [14822]) Changes to disabled state should be reflected to hidden <input> as well.

Refs #7244, fixes #7414. !strict

comment:44 Changed 13 years ago by bill

(In [14823]) Hopefully cleaner fix for CheckBox? initialization vis-a-vis "checked" and "value" parameters.

Fixes #7413, refs #7244 !strict.

comment:45 Changed 13 years ago by bill

(In [14825]) Use this.attr() to set 'required' during initialization. Also, there is no required attribute on DOM nodes; just the wai role. Refs #7244 !strict.

comment:46 Changed 13 years ago by bill

(In [14826]) Remove unneeded code since this.attr('disabled', this.disabled) is called during widget creation automatically. Refs #7244 !strict.

comment:47 Changed 13 years ago by bill

(In [14827]) setValue() --> attr('value', ...) Refs #7244 !strict.

comment:48 Changed 13 years ago by bill

(In [14828]) Remove injectAttributes feature and instead extend attributeMap to handle map to a DOM node's innerHTML or class (in addition to DOM node attributes). Refs #7244 !strict.

comment:49 Changed 13 years ago by bill

(In [14836]) Update comments, little lint fixes. Refs #7244 !strict.

comment:50 Changed 13 years ago by bill

(In [14860]) Remove search for default getter/setter (called get() and set()) since that feature is unused and causes problems with grid (naming conflict). Refs #7244 !strict.

comment:51 Changed 13 years ago by bill

Description: modified (diff)

Make description more end-user friendly

Note: See TracTickets for help on using tickets.