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

Fix character escapes in combobox options #2972

Merged
merged 1 commit into from
Mar 25, 2021

Conversation

vidartf
Copy link
Member

@vidartf vidartf commented Sep 17, 2020

Fixes #2940. Independent of work in #2785 since these are limited to attribute values.

@vidartf vidartf changed the title Fix chcarcter escapes in combobox options Fix character escapes in combobox options Sep 17, 2020
@vidartf vidartf added this to the Minor release milestone Sep 17, 2020
@vidartf vidartf modified the milestones: Minor release, 8.0 Feb 18, 2021
@jasongrout
Copy link
Member

jasongrout commented Feb 20, 2021

Instead of splicing sanitized strings together and using innerHTML, what do you think of constructing the option elements, adding them to a document fragment, then adding that at the end? Something like:

    const opts = this.model.get('options') as string[];
    const optionFragment = document.createDocumentFragment();
    opts.forEach(v => {
      const o = document.createElement('option');
      o.value = v;
      optionFragment.appendChild(o);
    })
    this.datalist.appendChild(optionFragment);

@vidartf
Copy link
Member Author

vidartf commented Feb 23, 2021

When I originally wrote it, I found createElement loops to be less performant for large amounts of options (~10k+), but at the time I don't think I tried using document fragments. I've just ran some checks in chrome with 100k options and profiling and the performance seems to be at least same order of magnitude for the two, so I think your point about the code being much simpler should be the deciding factor here 👍

@jasongrout
Copy link
Member

jasongrout commented Feb 23, 2021

To be fair, I think we should be testing up to maybe 100 options. If someone is putting 10k options in, they have more serious problems and should probably be using a different filtering/selection strategy.

Edit: maybe up to 1000 options?

@jasongrout jasongrout merged commit 8cf38cf into jupyter-widgets:master Mar 25, 2021
@jasongrout
Copy link
Member

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Combobox does not work if options contain double quotes (")
2 participants