hiroshi / knockoutFire

KnockoutFire.observable(firebase, map) WARNING: Abandoned
MIT License
29 stars 4 forks source link

firefox problem multiple select-fields and value binding #9

Open ayurmedia opened 11 years ago

ayurmedia commented 11 years ago

I found the bug:

if (selectWasPreviouslyEmpty && ('value' in allBindings)) { // Ensure consistency between model value and selected option. // If the dropdown is being populated for the first time here (or was otherwise previously empty), // the dropdown selection state is meaningless, so we preserve the model value. // ===> ensureDropdownSelectionIsConsistentWithModelValue(element, ko.utils.peekObservable(allBindings['value']), /* preferModelValue */ true); }

its in knockout.js itself and not in the knockoutfire.js.

seems this workaround for IE6 ensureDropdown... breaks in Firefox if you have more than one select field, as the timing is wrong, in my case this "workaround" is called while still in the rendering of the template.

if i comment out the ensure... line, it works also fine with my example.

another info, if i use selectedOptions, it will create an array in my firebase-object and i cannot map it so nicely to a input field.

seems best solution is to fix the ensureDropdown for nonIE browsers. which should be easy to catch with a browser-detect.

hiroshi commented 11 years ago

Sorry, but I don't understand what the problem is. Please show me reproduction steps of the problem.

ayurmedia commented 11 years ago

it try to make a jsfiddle which shows the problem. but the bug is in knockout.js itself and not knockoutfire.js, so i probably should report the bug there.

ayurmedia commented 11 years ago

I have an example with the bug: its a bit tricky to understand but its reproducable:

http://www.youtube.com/watch?v=Uldf-WIf5RM

Version with bug: (using original knockout.js) with faulty behaviour: http://www.holomind.de/todos/index.2.html Firebug-Console: give some errors:

Use of attributes' specified attribute is deprecated. It always returns true.
[Break On This Error]   
return !val || val.specified ? elem.value : elem.text;

Version without the "IE -Fix" working perfectly. http://www.holomind.de/todos/index.1.html

Normal behavior is in index.1.html, index.2.html will cause the error and stop rendering arround the select-field, as its a kind of callback with timer it can happen somewhere else. the bug causes the value of the select-field to be empty. which then will show in index.1.html that the values are all gone.

screen shot 2013-05-03 at 17 20 33

maybe its even a side-effect with jquery (oder versions of jquery have same behaviour)

The example shows same behavior, bug in Firefox, Safari and Chrome . in IE8 on Windows-XP the example does not work at all and breaks with other errors, so the IE-Fix is useless anyways.

screen shot 2013-05-03 at 17 27 30

http://stackoverflow.com/questions/156696/which-web-browsers-natively-support-array-foreach

finally a bug in knockoutfire.js ;) IE8 does not support item.foreach, which breaks the code anyways.

...

i guess that's why its called BETA, it only works with simple examples. all i did was correct usage of knockout.js and knockoutfire.js, but the library has bugs.

Reading (codereview) the code of knockout.js i just want to puke, its horrible code. it could be written using jquery in probably 20% of the whole code, no wonder there are some many bugs there. also why the hate of jquery and build all your own, jquery is basically standard now, and i wish it was just part of ecma-script and built into browsers. the idea of knockout.js is very nice, this automatic mapping , listeners etc. but the code is not well written.

hiroshi commented 11 years ago

Thanks for your details explanation. I think I got what going on.

There is an issue in knockout itself. The author of knockout suggests a workaround;

<!-- ko if: availableProjects().length -->

I thought others may have same problem and I googled with keywords knockoutjs select delayed options, then I found http://stackoverflow.com/a/16064996/338986. The answer suggests providing a option which matched against the initial value.

I thought if there is a way to workaround the issue in the layer of knockoutFire, but no idea yet.

Also, I learnt that Array.forEach() is not implemented in IE8 or under. Both KnockoutJs and Firebase support IE8, so knockoutFire should support IE8. I agreed. I'll try to replace all usages of Array.forEach with a compatible function.