dijit.form._FormSelectWidget.js __fillContent attribute handling

[Jonathan]

Since at least Dojo 1.5, the lines 490+ in dijit.form._FormSelectWidget::_fillContent() say:

// FIXME: disabled and selected are not valid on complex markup children (which is why we're
// looking for data-dojo-value above.  perhaps we should data-dojo-props="" this whole thing?)
// decide before 1.6

Moving to data-dojo-props would not only solve the "disabled" and "selected" issues but would also developers to pass data from markup to JS without having to override _fillContent().

[bill]

It sounds like a lot of data to squeeze into data-dojo-props though... what if it was a list of 50 values?

[Jonathan]

Good point.

The reason I found this comment in the code was having to pass some of my custom "data-" attributes to my widget. I ended up overriding the entire _fillContent() method which defeats encapsulation and DRY principals, but had no other choice (that I could see).

What would you suggest?

[bill]

I would leave the attributes on the <option> tags, and since the code right above looks for "data-dojo-value" (in addition to "value"), I think it should also look for "data-dojo-disabled" and "data-dojo-selected" (in addition to "disabled" and "selected").

[Jonathan]

Unless I'm misunderstanding or missing something, when you "leave" the attributes on the <option> tag, they get removed when the widget replaces the <select> with a <table>. That's why I had to overwrite dijit.form._FormSelectWidget::_fillContent().

Couldn't that method ~pass/parse~ _all_ "data-" attributes into the dijit.form.__SelectOption object? Or is that what you mean by "leave the attributes"?

[bill]

Right, I meant to make _fillContent() parse children of this.srcNodeRef like <div data-dojo-disabled=true ...> in addition to <option disabled>. Shouldn't have used the word "leave".

[Jonathan]

Sounds good. Just to confirm though... :)

Would custom attributes, e.g. <option data-my-custom-attribute ...> also be parsed?

[bill]

No, not with the current (pattern of) code, it just looks for the list of attributes specified in the code.

[Jonathan]

Is there a good reason not to change that?

At the very least, could the code that ~builds~ the dijit.form.__SelectOption not be refactored/extracted into its own method (eg. _mapOption())?

_fillContent: function() {
        ...
        // We've extracted the method applied in map() to its own method _mapOption().
        // This makes it easier to override cleanly.
        opts = this.options = this.srcNodeRef ? dojo.query(">",
            this.srcNodeRef).map(this._mapOption, this) : [];
        ...
},
_mapOption: function(node){
        if(node.getAttribute("type") === "separator"){
            return { value: "", label: "", selected: false, disabled: false };
        }
        return {
            value: (node.getAttribute("data-" + kernel._scopeName + "-value") || node.getAttribute("value")),
            label: String(node.innerHTML),
            selected: node.getAttribute("selected") || false,
            disabled: node.getAttribute("disabled") || false
        };
}

Doing so would allow developers to have a very clean override that calls this.inherited and ~extends~ the dijit.form._SelectOption with the attributes they need.

Would a patch be useful? If so, what are the steps for submitting a patch to Dojo?

[bill]

Replying to jonathan.dh:

Is there a good reason not to change that?

ISTM it would be adding a bunch of parsing code for a corner case.

At the very least, could the code that ~builds~ the dijit.form.__SelectOption not be refactored/extracted into its own method (eg. _mapOption())?

That seems reasonable to me.

Would a patch be useful? If so, what are the steps for submitting a patch to Dojo?

If you've filed a ​CLA you can just attach a ​patch file to this ticket.

[dylan]

Given that no one has shown interest in creating a patch in the past 3+ years, I'm closing this as patchwelcome.