setting attribute value to blank in HTML doc now renders as blank

previously was rendered as a boolean attribute. also allow for boolean attributes by setting value to `nil`. Fixes sparklemotion#1800.
raotkind · Dec 2, 2018 · 3c5847e · 3c5847e
1 parent 01664a1
commit 3c5847e
Show file tree

Hide file tree

Showing 3 changed files with 51 additions and 23 deletions.
diff --git a/ext/java/nokogiri/XmlAttr.java b/ext/java/nokogiri/XmlAttr.java
@@ -130,7 +130,9 @@ public IRubyObject content(ThreadContext context) {
     @JRubyMethod(name = {"value=", "content="})
     public IRubyObject value_set(ThreadContext context, IRubyObject content){
         Attr attr = (Attr) node;
-        attr.setValue(rubyStringToString(XmlNode.encode_special_chars(context, content)));
+        if (content != null && !content.isNil()) {
+            attr.setValue(rubyStringToString(XmlNode.encode_special_chars(context, content)));
+        }
         setContent(content);
         return content;
     }

diff --git a/ext/nokogiri/xml_attr.c b/ext/nokogiri/xml_attr.c
@@ -4,37 +4,39 @@
  * call-seq:
  *  value=(content)
  *
- * Set the value for this Attr to +content+
+ * Set the value for this Attr to +content+. Use `nil` to remove the value
+ * (e.g., a HTML boolean attribute).
  */
 static VALUE set_value(VALUE self, VALUE content)
 {
   xmlAttrPtr attr;
-  Data_Get_Struct(self, xmlAttr, attr);
+  xmlChar *value;
 
-  if (attr->children) { xmlFreeNodeList(attr->children); }
+  Data_Get_Struct(self, xmlAttr, attr);
 
+  if (attr->children) {
+    xmlFreeNodeList(attr->children);
+  }
   attr->children = attr->last = NULL;
 
-  if (content) {
-    xmlChar *buffer;
-    xmlNode *tmp;
-
-    /* Encode our content */
-    buffer = xmlEncodeEntitiesReentrant(attr->doc, (unsigned char *)StringValueCStr(content));
+  if (content == Qnil) {
+    return content;
+  }
 
-    attr->children = xmlStringGetNodeList(attr->doc, buffer);
-    attr->last = NULL;
-    tmp = attr->children;
+  value = xmlEncodeEntitiesReentrant(attr->doc, (unsigned char *)StringValueCStr(content));
+  if (xmlStrlen(value) == 0) {
+    attr->children = xmlNewDocText(attr->doc, value);
+  } else {
+    attr->children = xmlStringGetNodeList(attr->doc, value);
+  }
+  xmlFree(value);
 
-    /* Loop through the children */
-    for(tmp = attr->children; tmp; tmp = tmp->next) {
-      tmp->parent = (xmlNode *)attr;
-      tmp->doc = attr->doc;
-      if (tmp->next == NULL) { attr->last = tmp; }
+  for (xmlNode *cur = attr->children; cur; cur = cur->next) {
+    cur->parent = (xmlNode *)attr;
+    cur->doc = attr->doc;
+    if (cur->next == NULL) {
+      attr->last = cur;
     }
-
-    /* Free up memory */
-    xmlFree(buffer);
   }
 
   return content;
@@ -74,7 +76,9 @@ static VALUE new(int argc, VALUE *argv, VALUE klass)
   rb_node = Nokogiri_wrap_xml_node(klass, (xmlNodePtr)node);
   rb_obj_call_init(rb_node, argc, argv);
 
-  if (rb_block_given_p()) { rb_yield(rb_node); }
+  if (rb_block_given_p()) {
+    rb_yield(rb_node);
+  }
 
   return rb_node;
 }

diff --git a/test/xml/test_attr.rb b/test/xml/test_attr.rb
@@ -26,12 +26,34 @@ def test_content=
         assert_equal "Y&ent1;", street.value
       end
 
-      def test_value=
+      def test_set_value
         xml = Nokogiri::XML.parse(File.read(XML_FILE), XML_FILE)
         address = xml.xpath('//address')[3]
         street = address.attributes['street']
         street.value = "Y&ent1;"
         assert_equal "Y&ent1;", street.value
+        assert_includes %Q{ street="Y&amp;ent1;"}, street.to_xml
+      end
+
+      def test_set_value_with_entity_string_in_html_file
+        html = Nokogiri::HTML("<html><body><div foo='asdf'>")
+        foo = html.at_css("div").attributes["foo"]
+        foo.value = "Y&ent1;"
+        assert_includes %Q{ foo="Y&amp;ent1;"}, foo.to_html
+      end
+
+      def test_set_value_with_blank_string_in_html_file
+        html = Nokogiri::HTML("<html><body><div foo='asdf'>")
+        foo = html.at_css("div").attributes["foo"]
+        foo.value = ""
+        assert_includes %Q{ foo=""}, foo.to_html
+      end
+
+      def test_set_value_of_boolean_attr_with_nil_in_html_file
+        html = Nokogiri::HTML("<html><body><div disabled='asdf'>")
+        disabled = html.at_css("div").attributes["disabled"]
+        disabled.value = nil
+        assert_includes %Q{ disabled}, disabled.to_html
       end
 
       def test_unlink # aliased as :remove