auto merge of #2110 : saneyuki/servo/attr, r=Ms2ger

- Fix: https://github.com/mozilla/servo/issues/2025
- Close: https://github.com/mozilla/servo/issues/2077

src/components/script/dom/attr.rs

@@ -3,12 +3,22 @@
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
 use dom::bindings::codegen::AttrBinding;
+use dom::bindings::codegen::InheritTypes::NodeCast;
 use dom::bindings::js::JS;
 use dom::bindings::utils::{Reflectable, Reflector, reflect_dom_object};
+use dom::element::Element;
+use dom::node::Node;
 use dom::window::Window;
+use dom::virtualmethods::vtable_for;
+use servo_util::namespace;
 use servo_util::namespace::Namespace;
 use servo_util::str::DOMString;
 
+pub enum AttrSettingType {
+    FirstSetAttr,
+    ReplacedAttr,
+}
+
 #[deriving(Encodable)]
 pub struct Attr {
     reflector_: Reflector,
@@ -16,7 +26,10 @@ pub struct Attr {
     value: DOMString,
     name: DOMString,
     namespace: Namespace,
-    prefix: Option<DOMString>
+    prefix: Option<DOMString>,
+
+    /// the element that owns this attribute.
+    owner: JS<Element>,
 }
 
 impl Reflectable for Attr {
@@ -32,26 +45,43 @@ impl Reflectable for Attr {
 impl Attr {
     fn new_inherited(local_name: DOMString, value: DOMString,
                      name: DOMString, namespace: Namespace,
-                     prefix: Option<DOMString>) -> Attr {
+                     prefix: Option<DOMString>, owner: JS<Element>) -> Attr {
         Attr {
             reflector_: Reflector::new(),
             local_name: local_name,
             value: value,
             name: name, //TODO: Intern attribute names
             namespace: namespace,
-            prefix: prefix
+            prefix: prefix,
+            owner: owner,
         }
     }
 
     pub fn new(window: &JS<Window>, local_name: DOMString, value: DOMString,
                name: DOMString, namespace: Namespace,
-               prefix: Option<DOMString>) -> JS<Attr> {
-        let attr = Attr::new_inherited(local_name, value, name, namespace, prefix);
+               prefix: Option<DOMString>, owner: JS<Element>) -> JS<Attr> {
+        let attr = Attr::new_inherited(local_name, value, name, namespace, prefix, owner);
         reflect_dom_object(~attr, window, AttrBinding::Wrap)
     }
 
-    pub fn set_value(&mut self, value: DOMString) {
+    pub fn set_value(&mut self, set_type: AttrSettingType, value: DOMString) {
+        let node: JS<Node> = NodeCast::from(&self.owner);
+        let namespace_is_null = self.namespace == namespace::Null;
+
+        match set_type {
+            ReplacedAttr => {
+                if namespace_is_null {
+                    vtable_for(&node).before_remove_attr(self.local_name.clone(), self.value.clone());
+                }
+            }
+            FirstSetAttr => {}
+        }
+
         self.value = value;
+
+        if namespace_is_null {
+            vtable_for(&node).after_set_attr(self.local_name.clone(), self.value.clone());
+        }
     }
 
     pub fn value_ref<'a>(&'a self) -> &'a str {
@@ -69,7 +99,7 @@ impl Attr {
     }
 
     pub fn SetValue(&mut self, value: DOMString) {
-        self.value = value;
+        self.set_value(ReplacedAttr, value);
     }
 
     pub fn Name(&self) -> DOMString {

src/components/script/dom/element.rs

@@ -4,7 +4,7 @@
 
 //! Element nodes.
 
-use dom::attr::Attr;
+use dom::attr::{Attr, AttrSettingType, ReplacedAttr, FirstSetAttr};
 use dom::attrlist::AttrList;
 use dom::bindings::codegen::ElementBinding;
 use dom::bindings::codegen::InheritTypes::{ElementDerived, NodeCast};
@@ -263,26 +263,22 @@ impl AttributeHandlers for JS<Element> {
                         prefix: Option<DOMString>, cb: |&JS<Attr>| -> bool) {
         let node: JS<Node> = NodeCast::from(self);
         let idx = self.get().attrs.iter().position(cb);
-        match idx {
+        let (mut attr, set_type): (JS<Attr>, AttrSettingType) = match idx {
             Some(idx) => {
-                if namespace == namespace::Null {
-                    let old_value = self.get().attrs[idx].get().Value();
-                    vtable_for(&node).before_remove_attr(local_name.clone(), old_value);
-                }
-                self.get_mut().attrs[idx].get_mut().set_value(value.clone());
+                let attr = self.get_mut().attrs[idx].clone();
+                (attr, ReplacedAttr)
             }
 
             None => {
                 let doc = node.get().owner_doc().get();
-                let new_attr = Attr::new(&doc.window, local_name.clone(), value.clone(),
-                                         name, namespace.clone(), prefix);
-                self.get_mut().attrs.push(new_attr);
+                let attr = Attr::new(&doc.window, local_name.clone(), value.clone(),
+                                         name, namespace.clone(), prefix, self.clone());
+                self.get_mut().attrs.push(attr.clone());
+                (attr, FirstSetAttr)
             }
-        }
+        };
 
-        if namespace == namespace::Null {
-            vtable_for(&node).after_set_attr(local_name, value);
-        }
+        attr.get_mut().set_value(set_type, value);
     }
 
     // http://dom.spec.whatwg.org/#dom-element-setattribute

src/components/script/dom/node.rs

@@ -1365,6 +1365,10 @@ impl Node {
                 let node_elem: JS<Element> = ElementCast::to(node).unwrap();
                 let node_elem = node_elem.get();
                 let mut copy_elem: JS<Element> = ElementCast::to(&copy).unwrap();
+
+                // XXX: to avoid double borrowing compile error. we might be able to fix this after #1854
+                let copy_elem_alias: JS<Element> = copy_elem.clone();
+
                 let copy_elem = copy_elem.get_mut();
                 // FIXME: https://github.com/mozilla/servo/issues/1737
                 copy_elem.namespace = node_elem.namespace.clone();
@@ -1373,7 +1377,7 @@ impl Node {
                     copy_elem.attrs.push(Attr::new(&document.get().window,
                                                    attr.local_name.clone(), attr.value.clone(),
                                                    attr.name.clone(), attr.namespace.clone(),
-                                                   attr.prefix.clone()));
+                                                   attr.prefix.clone(), copy_elem_alias.clone()));
                 }
             },
             _ => ()

src/test/content/test_document_getElementById.html

@@ -92,6 +92,20 @@
       is(document.getElementById("x"), null, "test 7-0");
     }
 
+    // test 8
+    {
+      let TEST_ID = "test-8"
+      let element = document.createElement("div");
+      element.setAttribute("id", TEST_ID);
+      document.body.appendChild(element);
+      let target = document.getElementById(TEST_ID);
+      is_not(target, null, "test 8-0, getElementById is correct before changing the value");
+
+      element.attributes[0].value = TEST_ID + "-updated";
+      let target2 = document.getElementById(TEST_ID);
+      is(target2, null, "test 8-1, should return null after updated id via Attr.value");
+    }
+
     finish();
   </script>
 </body>