id attribute values are not escaped before being used as jquery selectors

[dominicgamble]

Affected: selectmenu_v1.1.0-11 and all previous versions.

See: http://docs.jquery.com/Frequently_Asked_Questions#How_do_I_select_an_element_by_an_ID_that_has_characters_used_in_CSS_notation.3F

"Because jQuery uses CSS syntax for selecting elements, some characters are interpreted as CSS notation. For example, ID attributes, after an initial letter (a-z or A-Z), may also use periods and colons, in addition to letters, numbers, hyphens, and underscores (see W3C Basic HTML Data Types). The colon (":") and period (".") are problematic within the context of a jQuery selector because they indicate a pseudo-class and class, respectively.

In order to tell jQuery to treat these characters literally rather than as CSS notation, they must be "escaped" by placing two backslashes in front of them."

On line 41, and line 413 id attribute values are used to generate jquery selectors, and in my case if the id contains a colon (:) then it causes an exception within jquery when trying to use it as a selector. Escaping the colon with a double slash () fixes the problem.

[fnagel]

Im not sure if I get this right. What do you want me to do? Escape the incoming ID atribute?
The gudelines say that you need to escape the elements IDs so jQuery wont crash upon them.

Im confused. :-)

[dominicgamble]

Yes, you must escape the incoming attribute id before using it to create a jquery selector.

My destroy function now looks like this:

destroy: function() {
    this.element.removeData(this.widgetName)
        .removeClass(this.widgetBaseClass + '-disabled' + ' ' + this.namespace + '-state-disabled')
        .removeAttr('aria-disabled')
        .unbind("click");

    //escape colon characters for jquery selectors
    var newElementId = this.newelement.attr('id').replace(':', '\\:');        
    //unbind click on label, reset its for attr
    $('label[for='+newElementId+']')
        .attr('for',this.element.attr('id').replace(':', '\\:'))
        .unbind('click');
    this.newelement.remove();
    this.wrapper.remove();
    this.element.show();    

    // call widget destroy function
    $.Widget.prototype.destroy.apply(this, arguments);
},

As you can see, I've escaped the ":" character - this was a quick and dirty fix to get it to work for us, but a proper solution would escape the full range of characters that have meaning in jquery selectors, such as these []=.:,'"

I suggest a regular expression replace would be the easiest way to achieve this.

[fnagel]

Ive never seen someting like that in any other jQuery UI widget and I understand the guideline in a way YOU should escape the id in your markup. Am I wrong?

[dominicgamble]

Hi Felix, not sure what you mean here, but this is your code, and you are calling element.attr('id') and then using that to build a selector. This is your code where you are building a selector.

$('label[for='+newElementId+']')

That is a selector and if you want to use the id attribute from the new element (newElementId) then you need to escape it. There's not much more I think I can say here.

Up to you whether you want to fix it or not.

[fnagel]

If have no clue what I was thinking in my last post. You are right, the id should be escaped. I will implement this as soon as the main work for the new version is done (see #140).

Thanks for your contribution and sorry for my inconvenience :-)

[dominicgamble]

No problems Felix. Thanks for your efforts, it is a popular widget and your work is very much appreciated.

[fnagel]

Should be fixed in latest commit, please test and report!

[thg2k]

This is something I don't understand, why do you escape in _create() function while you should only escape when composing the selector string? i.e. if selectmenuId a.k.a. this.ids[*] contains the escaped version, you will generate a wrong tag because of this code:
$( 'label[for="' + selectmenuId + '"]' )
.attr( 'for', this.ids[1] )
.bind( 'click.selectmenu', function() {
self.newelement[0].focus();
return false;
});

so if your markup contains [select id="hello:world"] and [label for="hello:world"], your generated code will be [label for="hello:world-button"] which i don't even think is a valid ID anymore.

I suggest keeping in your variables only the clean version, "hello:world", and escaping it on the fly (maybe with an helper method) only when you are composing your selector:

    $( 'label[for="' + this.ids[0].replace(':','\\:') + '"]' )
        .attr( 'for', this.ids[1] )
        .bind( 'click.selectmenu', function() {
            self.newelement[0].focus();
            return false;
        });

(and of course same in the other use case similar)

[fnagel]

I get your point. This is a problem atm.

Why not using somethign like this:
$( 'label[for="' + this.element.attr( 'id' ) + '"]' )
.attr( 'for', this.ids[1] )
.bind( 'click.selectmenu', function() {
self.newelement[0].focus();
return false;
});

This needs to bed fixed in destroy method, too.

[fnagel]

Ive updated the branch with a backport of the new version of Selectmenu (#140). There is no need to update the label as we forward the label click event to the new element.

What do you think?

[fnagel]

Any feedback on this issue?

[thg2k]

I didn't have time to look into that, but I took a look at the commit and IIRC I wasn't to enthusiast about that, especially for the multiple-bind (passing object as argument), when was this introduced in jquery?
is it a jqueryui standard practice? i think you should use the same style of .bind().bind().bind() instead, at least for consistency inside your plugin

[fnagel]

No problem. Just my usual "clean up" to get rid of dead issues.

Mhh, what is IIRC?
Im not quite sure what your talking about here. Can you give me a code example?

[thg2k]

Next week i'll have some time and will give some decent feedback :)
IIRC = If I Recall Correctly

[fnagel]

Just a gentle reminder...

[fnagel]

Any feedback on this issue?

[fnagel]

Please open a new pull request if needed.