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

[WIP] Fix select change handler #521

Merged
merged 4 commits into from
Apr 25, 2017
Merged

[WIP] Fix select change handler #521

merged 4 commits into from
Apr 25, 2017

Conversation

Conduitry
Copy link
Member

I'm marking this [WIP] because I'm a little unsure of my change here still, but also because the unit test is still failing for some reason. I see some other unit tests involving <select> have been disabled, apparently because of limitations or bugs in jsdom.

The specific test cases (REPL, REPL) seem to work with this change when they're compiled and manipulated manually - and no other unit tests break.

The idea in the PR is to move all of the attribute handling of <select> elements (attaching event handlers, etc) after the code for handling its children - but within that code generated by visiting the attributes to keep everything in the same relative order as it is for other types of elements. I can't see what any unpleasant side-effects of this might be, but there could still be some. As a bonus, the generation code in Element.js is slightly tidier.

cc @saibotsivad @TehShrike

@Rich-Harris
Copy link
Member

This looks like a good fix to me — fairly sure it's just the same JSDOM quirks as before causing the test to fail, so we can probably just skip it.

Maybe we should use a proper headless browser instead. Would probably make the tests run a lot slower though, especially in a CI context where you have to install a lot more. I know @PaulBGD has been getting his hands dirty with this stuff recently for svelte-bench.

@Rich-Harris Rich-Harris merged commit 09b1635 into master Apr 25, 2017
@Rich-Harris Rich-Harris deleted the select-change-handler branch April 25, 2017 18:58
@Zirro
Copy link
Contributor

Zirro commented Apr 25, 2017

@Rich-Harris I've recently taken an interest in jsdom and have submitted a few PRs. Since you've extracted several neat testcases in this repo and specifically marked them as failing in jsdom, I'd like to see if I can resolve some of these issues when I have time over the coming weeks.

@Rich-Harris
Copy link
Member

@Zirro that'd be awesome, thanks — have been meaning to get round to it myself but there aren't enough hours in the day!

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

Successfully merging this pull request may close these issues.

4 participants