gvas / knockout-jqueryui

Knockout bindings for the jQuery UI widgets.
http://gvas.github.com/knockout-jqueryui/
MIT License
103 stars 38 forks source link

Button binding within buttonset binding breaks on destroy domNodeDisposal #25

Open haydenc opened 10 years ago

haydenc commented 10 years ago

Minor issue - jQuery buttonset destroy takes care of destroying the buttons within it, but when

$(element)[widgetName]('destroy');

is called for the button binding itself jquery will throw the following error "cannot call methods on button prior to initialization; attempted to call method 'destroy'"

gvas commented 10 years ago

@haydenc: Could you create a fiddle demonstrating the bug? Here's my version (which doesn't throw any error): http://jsfiddle.net/gvas/urLfb/ Thanks!

vgaiduk commented 9 years ago

Please see http://jsfiddle.net/urLfb/2/

Sometimes there is a need to define bindings for both buttons (to set the options) and the enclosing buttonset (to visually unite the buttons)

gvas commented 9 years ago

Aha, now I see the problem, thanks.

I have to decide what should I do with this/should I take any actions. Meanwhile, if the only reason to use the buttonset widget is to modify the buttons' visual style, you can apply the ui-buttonset css class to the enclosing element: http://jsfiddle.net/urLfb/3/

vgaiduk commented 9 years ago

Thank you for a nice workaround - it does most of the task. However, not all of it (e.g. it rounds the corners of all buttons instead of doing it for external buttons only) but we can live without it.

May be it is worth fixing it to be on a clean side? It shall be somewhere in the knockout-jquery logic, since jqueryui works by itself without throwing exceptions, if you apply button and buttonset in JavaScript instead of data-bind, please see: http://jsfiddle.net/urLfb/4/

bago commented 9 years ago

Same issue here, but not "minor": my buttonset+button bindings are inside an "if" binding so they are added/disposed frequently and this bug break it.

Applying the classes manually doesn't work too unless I also apply the corner classes (and I can't do that.. it's easier to patch ko-jqui).

jQuery doc states that "buttonset destroy" also destry inner buttons.

Until you find a better (cleaner) options, I guess this would be an improvement:

try {
  $(element)[widgetName]('destroy');
} catch (e) {  } 

If you want to narrow it further it could be catched only for "widgetName == 'button'"

A "cleaner" option would be to have buttonset store something in the bindingContext so that inner buttons can detect they are nested in a buttonset and skip calling the destroy method, but I'm not sure it worth it (the catch is much easier).