Opened 11 years ago

Closed 11 years ago

Last modified 10 years ago

#7244 closed enhancement (fixed)

[patch][cla] attr() for Dijit

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()

Attachments (1)

attr.patch (10.1 KB) - added by alex 11 years ago.

Download all attachments as: .zip

Change History (76)

comment:1 Changed 11 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 11 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 11 years ago by alex

/me slaps forehead.

Updated patch attached.

Changed 11 years ago by alex

Attachment: attr.patch added

comment:4 Changed 11 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 11 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 11 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 11 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 11 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 11 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 11 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 11 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 11 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 11 years ago by alex

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

comment:14 Changed 11 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 11 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 11 years ago by bill

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

comment:17 Changed 11 years ago by bill

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

comment:18 Changed 11 years ago by bill

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

comment:19 Changed 11 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 11 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 11 years ago by bill

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

comment:22 Changed 11 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 11 years ago by bill

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

comment:24 Changed 11 years ago by bill

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

comment:25 Changed 11 years ago by bill

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

comment:26 Changed 11 years ago by bill

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

comment:27 Changed 11 years ago by bill

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

comment:28 Changed 11 years ago by bill

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

comment:29 Changed 11 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 11 years ago by Bryan Forbes

(In [14734]) refs #7244 !strict

comment:31 Changed 11 years ago by bill

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

comment:32 Changed 11 years ago by bill

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

comment:33 Changed 11 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 11 years ago by bill

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

comment:35 Changed 11 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 11 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 11 years ago by bill

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

comment:38 Changed 11 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 11 years ago by bill

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

comment:40 Changed 11 years ago by bill

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

comment:41 Changed 11 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 11 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 11 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 11 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 11 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 11 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 11 years ago by bill

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

comment:48 Changed 11 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 11 years ago by bill

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

comment:50 Changed 11 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 11 years ago by bill

Description: modified (diff)

Make description more end-user friendly

comment:52 Changed 11 years ago by Nathan Toone

(In [14870]) Refs #7244 - better documentation of dijit.TitlePane?.attr("open") and allow for non-typed comparison of values

comment:53 Changed 11 years ago by Nathan Toone

(In [14872]) Refs #7244 - call the _setXXXAttr functions during creation for any parameters passed in to the widget !strict

comment:54 Changed 11 years ago by Nathan Toone

(In [14873]) Refs #7244 - fix logic for determining which attributes are processed automatically !strict

comment:55 Changed 11 years ago by Nathan Toone

(In [14877]) Fixes #7456 Refs #7244 - add attr support to FilePicker? widget and fix layout issues with setting value in a hidden picker !strict

comment:56 Changed 11 years ago by Nathan Toone

(In [14903]) Refs #7244 - fix breakage in dijit.layout.ContentPane? and dijit.layout.LinkPane? due to moving _setXXXAttr prior to postCreate

comment:57 Changed 11 years ago by Nathan Toone

(In [14904]) Refs #7244 - move the regular expression searching of all functions into a registry so that it will be called only once per widget class - not once per instantiated widget !strict

comment:58 Changed 11 years ago by Nathan Toone

(In [14905]) Refs #7244 - Cut a little too deep on that last checkin...oops !strict

comment:59 Changed 11 years ago by Nathan Toone

(In [14914]) Refs #7244 - add in attr() hook to get the selected option values of the widget

comment:60 Changed 11 years ago by bill

(In [14920]) Make iconClass work through attributeMap so that attr('iconClass', 'foo') works. Refs #7244 !strict.

comment:61 Changed 11 years ago by bill

(In [14921]) attr() related cleanup for ComboButton?. Refs #7244 !strict.

comment:62 Changed 11 years ago by bill

(In [14922]) Use attr() for AccordionPane? title so that myPane.attr('title', foo) should work now, although I guess things are still tricky when an AccordionPane? wraps another widget. Refs #7244 !strict.

comment:63 Changed 11 years ago by Nathan Toone

(In [14923]) Refs #7244 - _setValueAttr getting called three times on instantiation of form value widgets. Update applyAttributes to only apply the function if there value does not exist in the attribute map - and also remove extra call in _FormValueWidget !strict

comment:64 Changed 11 years ago by Adam Peller

(In [14928]) Avoid global namespace pollution. Refs #7244 !strict

comment:65 Changed 11 years ago by bill

(In [14932]) Fix ContentPane? not to load content in hidden panes (unless preload=true). Refs #7244 !strict.

comment:66 Changed 11 years ago by bill

(In [14947]) Remove unneeded code from postCreate(). Widget.create() will call either _setDisplayedValueAttr() *or* _setValueAttr() during initialization, but not both, since it ignores null/empty values when looping through the attributes. (Thus there's no race condition w/value overwriting displayedValue or vice-versa.) Refs #7244 !strict.

comment:67 Changed 11 years ago by Nathan Toone

(In [14949]) Refs #7244 - since attr(value) will only be called if there *is* a value, we need to set the reset value if it is still undefined during postCreate. Otherwise, calling reset() on the widget does not actually reset it (since attr("value", undefined) will not actually set the value) !strict

comment:68 Changed 11 years ago by bill

(In [14981]) Prevent spurious call to onChange() during initialization when displayedValue parameter specified, and when using data store does XHR request, thus delaying initialization until after create() has finished running.

Refs #7244 !strict.

comment:69 Changed 11 years ago by Nathan Toone

(In [15002]) Refs #7244 - do not execute animations if they have not been set up yet

comment:70 Changed 11 years ago by bill

(In [15037]) Redo LinkPane? part of [14903]. It was causing the tab labels to be blank (in test_TabContainer.html, first test). LinkPane? is special in that the innerHTML represents the title.

Refs #7244

comment:71 Changed 11 years ago by bill

(In [15044]) Starting w/the change to ContentPane?.html in [14932], ContentPane?.html hangs in IE7/parallels for me, with the CPU pegged at 100%. This check in eliminates the hang, and also changes some mysterious code (why was pane4.domNode being placed inside the StackContainer?.domNode even though pane4 isn't a child of the StackContainer??). But perhaps this is all race conditions and I haven't really fixed anything. Refs #7244.

comment:72 Changed 11 years ago by bill

(In [15254]) Make attr('value') work for the Form widget (and derivative widgets like Dialog) Refs #7244 !strict

comment:73 Changed 10 years ago by bill

(In [16519]) Use attr('value') not getValue() in tests, and also remove strange thing that it was console.log()'ing the length of the content not the actual content. Refs #7244.

comment:74 Changed 10 years ago by bill

(In [20579]) Since we already have a reference to a widget instance, no reason to call getObject(). Fixes #10085, refs #7244 ([14904]) !strict.

comment:75 Changed 10 years ago by bill

(In [20580]) Fix comments, refs #10085, #7244 ([14904]) !strict.

Note: See TracTickets for help on using tickets.