From 2b0e77945d7c6a8ee4e3d42e5ebc93ac0ed99525 Mon Sep 17 00:00:00 2001 From: Tetsuharu OHZEKI Date: Thu, 10 Apr 2014 23:52:00 +0900 Subject: [PATCH 1/4] Add testcase #2025 for document.getElementById(). --- src/test/content/test_document_getElementById.html | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/src/test/content/test_document_getElementById.html b/src/test/content/test_document_getElementById.html index 97b506f58af..382c07574a7 100644 --- a/src/test/content/test_document_getElementById.html +++ b/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(); From a74397432ee8d9311e5c7558cfcea2ec5c843143 Mon Sep 17 00:00:00 2001 From: Tetsuharu OHZEKI Date: Fri, 11 Apr 2014 00:04:31 +0900 Subject: [PATCH 2/4] Make Attr::SetValue uses the internal setter. --- src/components/script/dom/attr.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/components/script/dom/attr.rs b/src/components/script/dom/attr.rs index 40da74cfbd2..07c22fa019d 100644 --- a/src/components/script/dom/attr.rs +++ b/src/components/script/dom/attr.rs @@ -69,7 +69,7 @@ impl Attr { } pub fn SetValue(&mut self, value: DOMString) { - self.value = value; + self.set_value(value); } pub fn Name(&self) -> DOMString { From b2fa6fa2215cb2914617f2cdb6e092626614b59e Mon Sep 17 00:00:00 2001 From: Tetsuharu OHZEKI Date: Tue, 15 Apr 2014 14:52:16 +0900 Subject: [PATCH 3/4] Make Attr::SetValue to reflect attribute change. --- src/components/script/dom/attr.rs | 30 +++++++++++++++++++++++----- src/components/script/dom/element.rs | 15 ++++++-------- src/components/script/dom/node.rs | 6 +++++- 3 files changed, 36 insertions(+), 15 deletions(-) diff --git a/src/components/script/dom/attr.rs b/src/components/script/dom/attr.rs index 07c22fa019d..dd289ff3b88 100644 --- a/src/components/script/dom/attr.rs +++ b/src/components/script/dom/attr.rs @@ -3,9 +3,14 @@ * 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; @@ -16,7 +21,10 @@ pub struct Attr { value: DOMString, name: DOMString, namespace: Namespace, - prefix: Option + prefix: Option, + + /// the element that owns this attribute. + owner: JS, } impl Reflectable for Attr { @@ -32,26 +40,38 @@ impl Reflectable for Attr { impl Attr { fn new_inherited(local_name: DOMString, value: DOMString, name: DOMString, namespace: Namespace, - prefix: Option) -> Attr { + prefix: Option, owner: JS) -> 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, local_name: DOMString, value: DOMString, name: DOMString, namespace: Namespace, - prefix: Option) -> JS { - let attr = Attr::new_inherited(local_name, value, name, namespace, prefix); + prefix: Option, owner: JS) -> JS { + 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) { + let node: JS = NodeCast::from(&self.owner); + let namespace_is_null = self.namespace == namespace::Null; + + if namespace_is_null { + vtable_for(&node).before_remove_attr(self.local_name.clone(), self.value.clone()); + } + 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 { diff --git a/src/components/script/dom/element.rs b/src/components/script/dom/element.rs index ace3f68bccc..2796b7aa870 100644 --- a/src/components/script/dom/element.rs +++ b/src/components/script/dom/element.rs @@ -265,23 +265,20 @@ impl AttributeHandlers for JS { let idx = self.get().attrs.iter().position(cb); 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()); } None => { let doc = node.get().owner_doc().get(); let new_attr = Attr::new(&doc.window, local_name.clone(), value.clone(), - name, namespace.clone(), prefix); + name, namespace.clone(), prefix, self.clone()); self.get_mut().attrs.push(new_attr); - } - } - if namespace == namespace::Null { - vtable_for(&node).after_set_attr(local_name, value); + // FIXME: This part should be handled in `Attr`. + if namespace == namespace::Null { + vtable_for(&node).after_set_attr(local_name, value); + } + } } } diff --git a/src/components/script/dom/node.rs b/src/components/script/dom/node.rs index 86137fef714..04910555552 100644 --- a/src/components/script/dom/node.rs +++ b/src/components/script/dom/node.rs @@ -1365,6 +1365,10 @@ impl Node { let node_elem: JS = ElementCast::to(node).unwrap(); let node_elem = node_elem.get(); let mut copy_elem: JS = ElementCast::to(©).unwrap(); + + // XXX: to avoid double borrowing compile error. we might be able to fix this after #1854 + let copy_elem_alias: JS = 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())); } }, _ => () From cdbe179acbefc878d6594c8631c5604988451e9a Mon Sep 17 00:00:00 2001 From: Tetsuharu OHZEKI Date: Fri, 18 Apr 2014 23:53:00 +0900 Subject: [PATCH 4/4] Make Attr handles all setting value. --- src/components/script/dom/attr.rs | 18 ++++++++++++++---- src/components/script/dom/element.rs | 21 ++++++++++----------- 2 files changed, 24 insertions(+), 15 deletions(-) diff --git a/src/components/script/dom/attr.rs b/src/components/script/dom/attr.rs index dd289ff3b88..237806c07f8 100644 --- a/src/components/script/dom/attr.rs +++ b/src/components/script/dom/attr.rs @@ -14,6 +14,11 @@ 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, @@ -59,12 +64,17 @@ impl Attr { 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 = NodeCast::from(&self.owner); let namespace_is_null = self.namespace == namespace::Null; - if namespace_is_null { - vtable_for(&node).before_remove_attr(self.local_name.clone(), self.value.clone()); + 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; @@ -89,7 +99,7 @@ impl Attr { } pub fn SetValue(&mut self, value: DOMString) { - self.set_value(value); + self.set_value(ReplacedAttr, value); } pub fn Name(&self) -> DOMString { diff --git a/src/components/script/dom/element.rs b/src/components/script/dom/element.rs index 2796b7aa870..d60181f5349 100644 --- a/src/components/script/dom/element.rs +++ b/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,23 +263,22 @@ impl AttributeHandlers for JS { prefix: Option, cb: |&JS| -> bool) { let node: JS = NodeCast::from(self); let idx = self.get().attrs.iter().position(cb); - match idx { + let (mut attr, set_type): (JS, AttrSettingType) = match idx { Some(idx) => { - 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(), + let attr = Attr::new(&doc.window, local_name.clone(), value.clone(), name, namespace.clone(), prefix, self.clone()); - self.get_mut().attrs.push(new_attr); - - // FIXME: This part should be handled in `Attr`. - if namespace == namespace::Null { - vtable_for(&node).after_set_attr(local_name, value); - } + self.get_mut().attrs.push(attr.clone()); + (attr, FirstSetAttr) } - } + }; + + attr.get_mut().set_value(set_type, value); } // http://dom.spec.whatwg.org/#dom-element-setattribute