Problem with looped <option> selected attribute in Firefox #1374

Closed
yuretz opened this Issue Nov 21, 2015 · 19 comments

Projects

None yet

4 participants

@yuretz
yuretz commented Nov 21, 2015

I've got an issue with looped <option> selected attribute inside <select> element in Firefox.
Please see the demo here: http://jsfiddle.net/yuretz/8zpnwwLe/
It seems like the new loop rendering method is somehow able to provoke some kind of weird thing in Firefox, where the option element that has selected attribute set is actually not shown as selected in the actual select control. Adding no-reorder attribute fixes this.
I have a suspicion that this is because at a specific stage of loop rendering there might exist multiple <option> elements which are selected, and that's what causes all the confusion.

BTW, what is your policy about fixing browser-specific bugs?

@GianlucaGuarini
Member

@yuretz it seems to be a firefox bug and you are right there might exist multiple <option> elements which are selected, and that's what causes all the confusion.

BTW, what is your policy about fixing browser-specific bugs?

we run our unit test across several browsers and if someone find a bug like this we fix it for all the next releases on all the platforms adding a test

@GianlucaGuarini GianlucaGuarini added bug and removed question labels Nov 24, 2015
@GianlucaGuarini
Member

I made a quick example to check whether the issue is really in firefox, but it seems to be in riot http://jsfiddle.net/8zpnwwLe/7/

@GianlucaGuarini GianlucaGuarini added this to the 2.4.0 milestone Nov 27, 2015
@wellguimaraes
Contributor

@GianlucaGuarini The bug happens in Firefox when we set the selected attribute to an option before removing the attr from the one previously selected. Here is the normal and the buggy version in Firefox: http://jsfiddle.net/arkvgffw/5/

I'll take a look at riot.

@GianlucaGuarini
Member

@wellguimaraes ok thanks. Make sure to pull request on the dev branch ;)

@GianlucaGuarini GianlucaGuarini removed this from the 2.4.0 milestone Mar 3, 2016
@GianlucaGuarini
Member

it seems that this issue is not fixed yet I will have a look at it

@GianlucaGuarini GianlucaGuarini removed the fixed label Mar 3, 2016
@wellguimaraes
Contributor

Working fine here:

https://riot-wellguimaraes.c9users.io/test/tags.html (first top-left corner tag with "select next" button)

It loops the sequence 0, 1, 2.

@GianlucaGuarini
Member

yes but why the example on posted by @yuretz is not working yet?

@wellguimaraes
Contributor

Really! Intriguing...

I'll take a look here and test with yuretz example, ok?

@wellguimaraes
Contributor

Solving the problem in Firefox is about setting the <select> value instead of just changing the <option> selected attribute... Investigating why not working with the user example, saw that dom is not giving the correct parent for an <option> element so we can set its value.

@aMarCruz
Member
aMarCruz commented Mar 4, 2016

he

@aMarCruz
Member
aMarCruz commented Mar 4, 2016

well, this is a pretty block:

  if (attrName === 'selected' && dom.nodeName === 'OPTION' && parent) {
    parent.value = dom.value
  }

but doesn't works.
First, dom.value can be an expression. Example: {option.value}.
Second, the parent inside a loop is a <div>, not the final select.

Also, looking for attrName=='value' here (in update) doesn't work because selected can be setted in a non-evaluated option. So, it needs another aproximation, maybe in each(). I will work in it asap.

@aMarCruz
Member
aMarCruz commented Mar 4, 2016

The fix for this issue was not easy, too many gotchas between the server and browsers, and more code to the riot core, but now it is working... almost.

Remains an issue with Safari 5.1 not selecting the value when the options are not in a loop.
Also, for non-looped options there's no easy way for clear the select, but it works for selects with the multiple attribute set.

@wellguimaraes
Contributor

\o/

@aMarCruz
Member
aMarCruz commented Mar 4, 2016

Please see the test in this plunker

@wellguimaraes
Contributor

Who cares about Safari 5.1? :P

@yuretz
yuretz commented Mar 4, 2016

That's totally awesome. Thank you guys!

@GianlucaGuarini
Member

@aMarCruz can we mark this issue as fixed?

@aMarCruz
Member
aMarCruz commented Mar 6, 2016

@GianlucaGuarini yes.
The only gotcha is we cannot clear a non-multiline select with riot (i.e. set selectedIndex to -1) because almost all browsers set it to 0 in else root.insertBefore(tag.root, tags[i].root) of lib/browser/tag/each.js.
The only browser that respects selectedIndex -1 is IE9 !!!

btw: maybe we need drop support for Safari 5.1. ...he, ...is riot supporting it?

@GianlucaGuarini
Member

@aMarCruz we do not support ancient Safari releases anyway.. thanks for the info

@GianlucaGuarini GianlucaGuarini pushed a commit that closed this issue Mar 9, 2016
@aMarCruz aMarCruz Closes #1374
For select with non-looped options, `selectedIndex` cannot be set to -1 and Safari 5.1 fails setting the selected value.
58cc0d1
@aMarCruz aMarCruz referenced this issue Mar 13, 2016
Closed

Selects in 2.3.17 reset after onchange event #1667

1 of 7 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment