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.

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

accesskey adds html as text #252

Closed
UziTech opened this issue Apr 27, 2015 · 12 comments
Closed

accesskey adds html as text #252

UziTech opened this issue Apr 27, 2015 · 12 comments

Comments

@UziTech
Copy link

UziTech commented Apr 27, 2015

setting the accesskey option adds a span around the letter as text.

UziTech added a commit to UziTech/jQuery-contextMenu that referenced this issue Apr 27, 2015
change .text to .html when adding item.name to the item
UziTech added a commit to UziTech/jQuery-contextMenu that referenced this issue May 15, 2015
accesskey adds span as text. fixes swisnl#252
@bbrala bbrala closed this as completed in 83e37fb Aug 21, 2015
bbrala added a commit that referenced this issue Aug 21, 2015
…-text

change .text to .html when adding item.name to the item fixes #252
@arai-a
Copy link
Contributor

arai-a commented Oct 25, 2015

This causes following issue again:
f3137f2

@UziTech
Copy link
Author

UziTech commented Oct 25, 2015

The item name should be set by the programmer so that shouldn't be an issue. If the item name is dynamic then the programmer needs to scrub it before adding it to the context menu

@arai-a
Copy link
Contributor

arai-a commented Oct 25, 2015

yeah, the risk is low, but this is breaking change, and I think the accesskey's span element should be inserted with different way, instead of modifying the name property to HTML.
I'll make a patch shortly.

@arai-a
Copy link
Contributor

arai-a commented Oct 25, 2015

opened #299

@bbrala
Copy link
Member

bbrala commented Oct 26, 2015

Nice catch @arai-a, i didn't realize the reason there.

@UziTech
Copy link
Author

UziTech commented Oct 26, 2015

I disagree that the item name needs to be added as text.

I think it is more likely that the programmer would want to add html to allow styles then the programmer allowing the item name to be set by a users input.

@arai-a
Copy link
Contributor

arai-a commented Oct 26, 2015

there's 'html' type for that purpose, iiuc.

$.contextMenu({
    selector: '#foo',
    trigger: 'left',
    items: [{ type: "html", html: "<b>Hello</b>", }],
});

@bbrala
Copy link
Member

bbrala commented Oct 26, 2015

Hmm, what i did notice though is that callbacks are not supported on a HTML type, which makes it a bit harder to use.

@arai-a
Copy link
Contributor

arai-a commented Oct 26, 2015

I guess, there should be another way to create selectable menuitem that parses HTML, that also works in form label and sub menu. The API should explicitly tell it's parsed as HTML (like, property name is html, or something like that) .name property name with HTML support is a pitfall.

Also, current .name way doesn't interact nicely with accesskey if .name contains HTML.
e.g. if .name contains "<b>" and accesskey is "b", it results in corrupted HTML.
so, there should be some way to suppress that replacement but keep accesskey still working.

Both issues could be fixed at once, couldn't they?

@bbrala
Copy link
Member

bbrala commented Oct 26, 2015

I agree with @arai-a here, we do need to have a look on how we can support using html in a different way. I'll be merging his fix.

@UziTech
Copy link
Author

UziTech commented Oct 26, 2015

I agree.
be careful though, parsing html is an extremely difficult game of cat and mouse as people find different strings that don't parse correctly. (unicode chars, etc.)

@arai-a
Copy link
Contributor

arai-a commented Oct 26, 2015

ah, I meant .html(...) with "parses HTML", as currently .name does.

@bbrala bbrala mentioned this issue Mar 13, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants