gmac / backbone.epoxy

Declarative data binding and computed models for Backbone
http://epoxyjs.org
MIT License
615 stars 89 forks source link

Epoxy doesn't seem to respect the initial display state of elements with bound collections. #54

Closed aaronchilcott closed 10 years ago

aaronchilcott commented 10 years ago

If a value of an bound object is changed, epoxy seems to reset the associated view's bindings, which then triggers the hide()/show() calls for (collection) bound elements. This eventually causes the elements to become visible, when perhaps they should have stayed hidden.

Following, are the line numbers which I believe to be causing this behaviour.

  1. On: https://github.com/gmac/backbone.epoxy/blob/master/backbone.epoxy.js#L633
                    // Hide element before manipulating:
                    $element.hide();

Does not first check the display state of the element before hiding.

  1. On: https://github.com/gmac/backbone.epoxy/blob/master/backbone.epoxy.js#L652
                    // Show element after manipulating:
                    $element.show();

Does not check if the element was initially hidden before binding.

I believe something like the following could be a viable solution, if this behaviour is unintended: https://github.com/gmac/backbone.epoxy/blob/master/backbone.epoxy.js#L623

<+>UTF-8
===================================================================
--- mobile/j/backbone.epoxy.js  (revision )
+++ mobile/j/backbone.epoxy.js  (revision )
@@ -631,7 +631,7 @@

+                     var isAlreadyHidden = $element.isHidden();

-                    // Hide element before manipulating:
+                    // Hide element before manipulating, if not already:
                     if (!isAlreadyHidden) {
                         $element.hide();
                     }
@@ -652,7 +652,7 @@
                         });
                     }

-                    // Show element after manipulating:
+                    // Show element after manipulating, if it wasn't initially hidden:
                     if (!isAlreadyHidden) {
                         $element.show();
                     }
gmac commented 10 years ago

Great catch. That was actually a poor workaround for avoiding DOM reflows. I'll change that to use a document fragment swap.

gmac commented 10 years ago

Cool, this should be fixed in commit 570dbb9. The original methodology was using the display of the parent element to avoid document reflows, which led to the issue you reported. That was definitely not the best solution to that problem. I've change the methodology to use a document fragment for the view reshuffling, and no longer touch the parent element's display.

aaronchilcott commented 10 years ago

Nice work, thanks!