Selects in 2.3.17 reset after onchange event #1667

Closed
crisward opened this Issue Mar 10, 2016 · 11 comments

Projects

None yet

3 participants

@crisward
Member
  1. Describe your issue:
    Selects in 2.3.17 work different from 2.3.15 - when each is used for option.
    The value gets reset to nothing after the change event has fired.
  2. Can you reproduce the issue?
    yes
    http://plnkr.co/edit/YNlNhC7JD4m4JoxPTyyP?p=preview
  3. On which browser/OS does the issue appear?
    Chrome, Firefox, Safari (tests fail in CI on all tested browsers)
  4. Which version of Riot does it affect?
    2.3.17
  5. How would you tag this issue?
    • Question
    • Bug
    • Discussion
    • Feature request
    • Tip
    • Enhancement
    • Performance
@GianlucaGuarini GianlucaGuarini added the bug label Mar 10, 2016
@GianlucaGuarini
Member

I am not sure this is really a bug. Can you check my version?
Either you select the selected option manually or you disable the updates using e.preventUpdate = true

@GianlucaGuarini
Member

I think this is also part of this discussion

@crisward
Member

This may not be a bug, but is a breaking change as the behaviour has modified since the previous riot version(s). What was the reason for the change, does it fix another issue?

I can confirm your's works, but unfortunately the update isn't that straight forward as the select is inside a much used component, which is scattered amongst various projects, not all of which have the same amount of test coverage as the one I'm working on. So I'm not sure if I'll break stuff elsewhere by introducing this change.

Let me know your plans regarding this. If you're happy with it's current behaviour, and no one else has this issue I'll modify my component to fall inline with this. If you're going to change it back in the next release, again let me know and I'll hold back updating riot until 2.3.18.

My current project has 100% test coverage, so if a fly farts on the other side of the planet I get a failed test. Hopefully I won't bother you too much with fly flatulence failed test issues.

@aMarCruz
Member

@crisward , this behavior was introduced by the fix for #1374 - with a non-multiple select, if you set the value via option.selected in Firefox, the current option is not deselected.
The solution was to use selectedIndex which works as expected, but we cannot set it before the select is complete (indexes are unknown) so the fix forces selected to the expression value.
The resulting behavior was unexpected and not consistent with the rest. It should be fixed of course. I'm working on this.

@crisward
Member

Cool thanks.

@aMarCruz
Member

@crisward , @GianlucaGuarini , if you have the time, please play with this plunker.
It is using a patched version of riot+compiler
I think this have now the spected behavior, and works ok in various browsers, including IE9 and FF.
Seems #1642 go in this direction as well.

@crisward
Member

Had a mess and copied your dev code to my plunker and it worked fine.

@GianlucaGuarini
Member

@aMarCruz cool please make a pull request

@aMarCruz aMarCruz added a commit to aMarCruz/riot that referenced this issue Mar 20, 2016
@aMarCruz aMarCruz fix #1667 d32452a
@aMarCruz aMarCruz referenced this issue Mar 20, 2016
Merged

fix #1667 #1696

@aMarCruz
Member
@GianlucaGuarini
Member

@aMarCruz merged thank you soooo much! 😄

@GianlucaGuarini GianlucaGuarini added fixed and removed to verify labels Mar 20, 2016
@crisward
Member
crisward commented Apr 3, 2016

Thanks for all the hard work, just updated from 2.3.15 to 2.3.18 and all my 337 tests passed successfully.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment