fix: Toggling visibility of an svg feature did nothing

We were setting the style visibility value, not the attribute visibility
value.

Author: manthey

src/svg/object.js

@@ -23,6 +23,7 @@ var svg_object = function (arg) {
   var m_id = 'svg-' + uniqueID(),
       s_exit = this._exit,
       m_this = this,
+      s_visible = this.visible,
       s_draw = this.draw;
 
   this._svgid = function () {
@@ -59,6 +60,26 @@ var svg_object = function (arg) {
     s_exit();
   };
 
+  /**
+   * Get/Set the visibility of the feature.
+   *
+   * @param {boolean} [val] A boolean to change the visibility, or `undefined`
+   *    to return the visibility.
+   * @param {boolean} [direct] If `true`, when getting the visibility,
+   *    disregard the visibility of the parent layer, and when setting, refresh
+   *    the state regardless of whether it has changed or not.  Otherwise, the
+   *    functional visibility is returned, where both the feature and the layer
+   *    must be visible for a `true` result.
+   * @returns {boolean|this} Either the visibility (if getting) or the feature
+   *    (if setting).
+   */
+  this.visible = function (val, direct) {
+    if (val !== undefined && val !== s_visible(undefined, true) && m_this.renderer()) {
+      m_this.renderer()._scheduleUpdate();
+    }
+    return s_visible(val, direct);
+  };
+
   return this;
 };
 

src/svg/svgRenderer.js

@@ -548,9 +548,16 @@ var svgRenderer = function (arg) {
     return m_this;
   };
 
+  /**
+   * Schedule an update in the next render frame.
+   */
+  this._scheduleUpdate = function () {
+    m_this.layer().map().scheduleAnimationFrame(m_this._renderFrame);
+  };
+
   /**
    * Render all features that are marked as needing an update.  This should
-   * only be called duration an animation frame.
+   * only be called during an animation frame.
    */
   this._renderFrame = function () {
     var id;
@@ -566,6 +573,11 @@ var svgRenderer = function (arg) {
         m_this._renderFeature(id);
       }
     }
+    for (id in m_features) {
+      if (m_features.hasOwnProperty(id) && ids[id] === undefined) {
+        m_this.select(id).attr('visibility', !m_features[id].visible || m_features[id].visible() ? 'visible' : 'hidden');
+      }
+    }
   };
 
   /**
@@ -580,6 +592,7 @@ var svgRenderer = function (arg) {
     if (!m_features[id]) {
       return m_this;
     }
+    m_this.select(id).remove();
     var data = m_features[id].data,
         index = m_features[id].index,
         style = m_features[id].style,
@@ -598,7 +611,7 @@ var svgRenderer = function (arg) {
     rendersel.attr('class', classes.concat([id]).join(' '));
     setStyles(rendersel, style);
     if (visible) {
-      rendersel.style('visibility', visible() ? 'visible' : 'hidden');
+      rendersel.attr('visibility', visible() ? 'visible' : 'hidden');
     }
     if (entries.size() && m_features[id].sortByZ) {
       selection.sort(function (a, b) {

tests/cases/feature.js

@@ -201,7 +201,7 @@ describe('geo.feature', function () {
       var modTime = feat.timestamp();
       expect(feat.visible(false)).toBe(feat);
       expect(feat.visible()).toBe(false);
-      expect(feat.timestamp()).toBeGreaterThan(modTime);
+      expect(feat.timestamp()).toBe(modTime);
 
       expect(feat.visible(true)).toBe(feat);
       var depFeat = geo.feature({layer: layer, renderer: layer.renderer()});
@@ -210,7 +210,7 @@ describe('geo.feature', function () {
       expect(feat.visible(false)).toBe(feat);
       expect(feat.visible()).toBe(false);
       expect(depFeat.visible()).toBe(false);
-      expect(depFeat.timestamp()).toBeGreaterThan(modTime);
+      expect(depFeat.timestamp()).toBe(modTime);
       feat.dependentFeatures([]);
       expect(feat.visible(true)).toBe(feat);
       expect(depFeat.visible()).toBe(false);

tests/cases/lineFeature.js

@@ -344,6 +344,11 @@ describe('geo.lineFeature', function () {
       expect(paths.eq(1).css('stroke-linejoin')).toBe('bevel');
       expect(paths.eq(2).css('stroke-linejoin')).toBe('round');
       expect(paths.eq(0).css('stroke-miterlimit')).toBe('5');
+      line.visible(false);
+      line.draw();
+      stepAnimationFrame();
+      expect(layer.node().find('path').length).toBe(9);
+      expect(layer.node().find('path').attr('visibility')).toBe('hidden');
       unmockAnimationFrame();
     });
   });