Opened 7 years ago

Closed 4 years ago

#15045 closed enhancement (patchwelcome)

dijit.form._FormSelectWidget.js __fillContent attribute handling

Reported by: Jonathan Owned by:
Priority: undecided Milestone: 1.13
Component: Dijit - Form Version: 1.7.2
Keywords: Cc:
Blocked By: Blocking:

Description (last modified by bill)

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

Change History (12)

comment:1 Changed 7 years ago by bill

Description: modified (diff)

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

comment:2 Changed 7 years ago by 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?

comment:3 Changed 7 years ago by 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").

Last edited 7 years ago by bill (previous) (diff)

comment:4 Changed 7 years ago by 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"?

comment:5 Changed 7 years ago by 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".

comment:6 Changed 7 years ago by Jonathan

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

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

comment:7 Changed 7 years ago by bill

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

comment:8 Changed 7 years ago by 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?

comment:9 in reply to:  8 Changed 7 years ago by 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.

Last edited 7 years ago by bill (previous) (diff)

comment:10 Changed 6 years ago by Douglas Hays

Owner: Douglas Hays deleted
Status: newassigned

comment:11 Changed 6 years ago by Douglas Hays

Status: assignedopen

comment:12 Changed 4 years ago by dylan

Milestone: tbd1.12
Resolution: patchwelcome
Status: openclosed

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

Note: See TracTickets for help on using tickets.