setEntry allows title to be html
abzainuddin opened this issue · 5 comments
First of all, thanks for this great plugin!
From the api reference it's not 100% clear that the title
argument can also be html (see https://jsfiddle.net/asn01r0m/). This may lead to unexpected behavior that a user might inject JS if not properly escaped:
We can either change updateTitle
to escape title
, or update the docs to explicitly state that you can inject html and that you have to escape properly. Do you have any other options?
Thanks for contributing.
There seems to be another problem: setEntry
seems only to replace the part before the <kbd>
element.
$(document).contextmenu({
menu: [
{title: "Copy <kbd>Ctrl+C</kbd>", cmd: "copy", uiIcon: "ui-icon-copy"},
],
beforeOpen: function(event, ui) {
$(document)
.contextmenu("setEntry", "copy", "<b>test2</b> <i>adding</i> <b>html</b>");
}
});
Allowing to set html content, would only make sense if the whole previous html wold be replaced.
But then it may be useful.
So I guess HTML should be accepted, and Iconsider this to be a bug.
Anyway feel free to improve (or suggest improvement) the current docs to make this more clear.
Contextmenu adds the uiIcon by appending a span to your title inside of the container. So, after you have created the item ... there is no clear separation between your title and the icon HTML if you were to allow arbitrary HTML in the title. Indeed, contextmenu explicitly assumes that title is text in setEntry -- as it uses that assumption to define what to replace.
So -- either:
-
HTML should be disallowed (or stripped), or
-
HTML allowed with the caveat that setEntry will no longer work, or
-
contextmenu would need to be re-architected to add wrappers to separate the title and the icon so that each can be independently addressed, even if the title is arbitrary html.
If you want to replace more than the title, I guess it's better to use the menu definition object
version:
.contextmenu("setEntry", "copy", {title: "New title <kbd>Ctrl+C</kbd>", uiIcon: "ui-icon-clipboard"})
Following from that I would expect that:
.contextmenu("setEntry", "copy", "New title <kbd>Ctrl+C</kbd>")
would work the same as:
.contextmenu("setEntry", "copy", {title: "New title <kbd>Ctrl+C</kbd>"})
but currently it does something like this:
.contextmenu("setEntry", "copy", {title: "New title"})
Should I make a PR which does this?
Hi,
I haven't looked into it yet, but if you have a solution, a PR would be helpful.
Note that it should work with different jQuery Ui versions (1.10, 1.11, 1.12) which have different markups.
setTitle(string)
should accept html in order to allow <kbd>
annotations and other modifications. I will make this more clear in the documentation.
I also just fixed a bug that caused setEntry(string)
to not replace icons or other html elements in the existing title.
There is still a problem with setTitle({object})
and jQuery 1.12: #110