mar10/jquery-ui-contextmenu

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:

image

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?

mar10 commented

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:

  1. HTML should be disallowed (or stripped), or

  2. HTML allowed with the caveat that setEntry will no longer work, or

  3. 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?

mar10 commented

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.

mar10 commented

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