Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Escape the optgroup labels during initialization step. #969

Closed
wants to merge 1 commit into from
Closed

Escape the optgroup labels during initialization step. #969

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 22 additions & 1 deletion chosen/chosen.jquery.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@
this.parsed.push({
array_index: group_position,
group: true,
label: group.label,
label: AbstractChosen.escapeExpression(group.label),
children: 0,
disabled: group.disabled
});
Expand Down Expand Up @@ -281,6 +281,27 @@ Copyright (c) 2011 by Harvest
return newchar = chars.substring(rand, rand + 1);
};

AbstractChosen.escapeExpression = function(text) {
var map, unsafe_chars;
if (!(text != null) || text === false) {
return "";
}
if (!/[\&\<\>\"\'\`]/.test(text)) {
return text;
}
map = {
"<": "&lt;",
">": "&gt;",
'"': "&quot;",
"'": "&#x27;",
"`": "&#x60;"
};
unsafe_chars = /&(?!\w+;)|[\<\>\"\'\`]/g;
return text.replace(unsafe_chars, function(chr) {
return map[chr] || "&amp;";
});
};

return AbstractChosen;

})();
Expand Down
2 changes: 1 addition & 1 deletion chosen/chosen.jquery.min.js
View file
Open in desktop

Large diffs are not rendered by default.

23 changes: 22 additions & 1 deletion chosen/chosen.proto.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@
this.parsed.push({
array_index: group_position,
group: true,
label: group.label,
label: AbstractChosen.escapeExpression(group.label),
children: 0,
disabled: group.disabled
});
Expand Down Expand Up @@ -281,6 +281,27 @@ Copyright (c) 2011 by Harvest
return newchar = chars.substring(rand, rand + 1);
};

AbstractChosen.escapeExpression = function(text) {
var map, unsafe_chars;
if (!(text != null) || text === false) {
return "";
}
if (!/[\&\<\>\"\'\`]/.test(text)) {
return text;
}
map = {
"<": "&lt;",
">": "&gt;",
'"': "&quot;",
"'": "&#x27;",
"`": "&#x60;"
};
unsafe_chars = /&(?!\w+;)|[\<\>\"\'\`]/g;
return text.replace(unsafe_chars, function(chr) {
return map[chr] || "&amp;";
});
};

return AbstractChosen;

})();
Expand Down
2 changes: 1 addition & 1 deletion chosen/chosen.proto.min.js
View file
Open in desktop

Large diffs are not rendered by default.

19 changes: 17 additions & 2 deletions coffee/lib/abstract-chosen.coffee
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ class AbstractChosen
setTimeout (=> this.container_mousedown()), 50 unless @active_field
else
@activate_field() unless @active_field

input_blur: (evt) ->
if not @mouse_on_container
@active_field = false
Expand Down Expand Up @@ -118,10 +118,25 @@ class AbstractChosen
new_id = this.generate_random_id()
@form_field.id = new_id
new_id

generate_random_char: ->
chars = "0123456789ABCDEFGHIJKLMNOPQRSTUVWXYZ"
rand = Math.floor(Math.random() * chars.length)
newchar = chars.substring rand, rand+1

@escapeExpression: (text) ->
if not text? or text is false
return ""
unless /[\&\<\>\"\'\`]/.test(text)
return text
map =
"<": "&lt;"
">": "&gt;"
'"': "&quot;"
"'": "&#x27;"
"`": "&#x60;"
unsafe_chars = /&(?!\w+;)|[\<\>\"\'\`]/g
text.replace unsafe_chars, (chr) ->
map[chr] || "&amp;"

root.AbstractChosen = AbstractChosen
4 changes: 2 additions & 2 deletions coffee/lib/select-parser.coffee
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
class SelectParser

constructor: ->
@options_index = 0
@parsed = []
Expand All @@ -15,7 +15,7 @@ class SelectParser
@parsed.push
array_index: group_position
group: true
label: group.label
label: AbstractChosen.escapeExpression(group.label)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Making SelectParser call AbstractChosen for the escaping looks weird to me. Why not defining the method in the SelectParser instead ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with @stof. I think this makes sense in the SelectParser class. Mind moving it @zsombor?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay so assume that you need to escape from somewhere else as well. We certainly do escaping in the prototype & jQuery specific classes. Now will that helper function be named SelectParser.escapeExpression or AbstractChosen.escapeExpression, I don't like neither but would find the second one to be better. Perhaps Chosen.escapeExpression would be the best.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

currently, Chosen extends AbstractChosen and uses SelectParser. So IMO, SelectParser should nto depend on Chosen or AbstractChosen.

Thus, defining it directly in chosen is even worse than AbstractChosen as it would require duplicating it

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Whether it winds up in SelectParser or AbstractChosen, I find it a little awkward. Between the two, I prefer it live in SelectParser. I wonder if we should consider introducing a third option instead - something like ChosenUtils. How does that sit with you @zsombor?

children: 0
disabled: group.disabled
this.add_option( option, group_position, group.disabled ) for option in group.childNodes
Expand Down