From 072ff20140746cb543f7e04cf4cdd0e5274becfe Mon Sep 17 00:00:00 2001 From: Tetsuharu OHZEKI Date: Tue, 12 Nov 2013 21:11:52 +0900 Subject: [PATCH 1/3] Use 'match' instead of 'if'. --- src/components/script/dom/element.rs | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/src/components/script/dom/element.rs b/src/components/script/dom/element.rs index 3a33948de15..324c034bbb7 100644 --- a/src/components/script/dom/element.rs +++ b/src/components/script/dom/element.rs @@ -252,8 +252,11 @@ impl<'self> Element { local_name: DOMString, value: DOMString) { - if "style" == local_name && *namespace == namespace::Null { - self.style_attribute = Some(style::parse_style_attribute(value)); + match local_name.as_slice() { + "style" if *namespace == namespace::Null => { + self.style_attribute = Some(style::parse_style_attribute(value)) + } + _ => () } // TODO: update owner document's id hashmap for `document.getElementById()` From aa72c413ed9e1dab8e3e2dfcb8be95b108a551e8 Mon Sep 17 00:00:00 2001 From: Tetsuharu OHZEKI Date: Tue, 12 Nov 2013 23:04:05 +0900 Subject: [PATCH 2/3] Enable to update id attribute for Document.getElementById(). (#740) --- src/components/script/dom/document.rs | 22 +++++++++++++++++++ src/components/script/dom/element.rs | 15 ++++++++----- .../content/test_document_getElementById.html | 17 +++++++++++++- 3 files changed, 48 insertions(+), 6 deletions(-) diff --git a/src/components/script/dom/document.rs b/src/components/script/dom/document.rs index eb30f110533..4df11e57754 100644 --- a/src/components/script/dom/document.rs +++ b/src/components/script/dom/document.rs @@ -340,6 +340,28 @@ impl Document { self.idmap.pop(id); }); } + + pub fn update_idmap(&mut self, + abstract_self: AbstractNode, + new_id: DOMString, + old_id: Option) { + // remove old ids if the old ones are not same as the new one. + match old_id { + Some(ref old_id) if new_id != *old_id => { + self.idmap.remove(old_id); + } + _ => () + } + + // TODO: support the case if multiple elements which haves same id are in the same document. + self.idmap.mangle(new_id, abstract_self, + |_, new_node: AbstractNode| -> AbstractNode { + new_node + }, + |_, old_node: &mut AbstractNode, new_node: AbstractNode| { + *old_node = new_node; + }); + } } #[inline(always)] diff --git a/src/components/script/dom/element.rs b/src/components/script/dom/element.rs index 324c034bbb7..9ca8257eed5 100644 --- a/src/components/script/dom/element.rs +++ b/src/components/script/dom/element.rs @@ -216,6 +216,7 @@ impl<'self> Element { let win = self.node.owner_doc().document().window; let new_attr = Attr::new_ns(win, local_name.clone(), value.clone(), name.clone(), namespace.clone(), prefix); + let mut old_raw_value: Option = None; self.attrs.mangle(local_name.clone(), new_attr, |new_name: &~str, new_value: @mut Attr| { // register to the ordered list. @@ -229,6 +230,7 @@ impl<'self> Element { for attr in old_value.mut_iter() { if eq_slice(attr.local_name, *name) && attr.namespace == new_value.namespace { + old_raw_value = Some(attr.Value()); *attr = new_value; found = true; break; @@ -242,7 +244,7 @@ impl<'self> Element { } }); - self.after_set_attr(abstract_self, &namespace, local_name, value); + self.after_set_attr(abstract_self, &namespace, local_name, value, old_raw_value); Ok(()) } @@ -250,18 +252,21 @@ impl<'self> Element { abstract_self: AbstractNode, namespace: &Namespace, local_name: DOMString, - value: DOMString) { + value: DOMString, + old_value: Option) { match local_name.as_slice() { "style" if *namespace == namespace::Null => { self.style_attribute = Some(style::parse_style_attribute(value)) } + "id" => { + let doc = self.node.owner_doc(); + let doc = doc.mut_document(); + doc.update_idmap(abstract_self, value.clone(), old_value); + } _ => () } - // TODO: update owner document's id hashmap for `document.getElementById()` - // if `name` == "id". - //XXXjdm We really need something like a vtable so we can call AfterSetAttr. // This hardcoding is awful. match abstract_self.type_id() { diff --git a/src/test/html/content/test_document_getElementById.html b/src/test/html/content/test_document_getElementById.html index 370ec77fde0..069db0ee4b7 100644 --- a/src/test/html/content/test_document_getElementById.html +++ b/src/test/html/content/test_document_getElementById.html @@ -43,8 +43,23 @@ is(removed, null, "test2-3, removed element"); } - // TODO: // test3: update `id` attribute + { + // setup fixtures. + let TEST_ID = "test3"; + let test = document.createElement("div"); + test.setAttribute("id", TEST_ID); + gBody.appendChild(test); + + // update id + let UPDATED_ID = "test3-updated"; + test.setAttribute("id", UPDATED_ID); + let e = document.getElementById(UPDATED_ID); + is(e, test, "test3-0, update 'id' attribute."); + + let old = document.getElementById(TEST_ID); + is(old, null, "test3-1, the method shouldn't get the element by the old id."); + } // TODO: // test4: "in tree order, within the context object's tree" From 4e4eb10ffc768a51fb68d2a5b79f9266f024a72b Mon Sep 17 00:00:00 2001 From: Tetsuharu OHZEKI Date: Sat, 23 Nov 2013 12:14:00 +0900 Subject: [PATCH 3/3] Use 'DOMString' to define the type insteand of '~str'. At now, 'DOMString' is equal to '~str'. https://github.com/mozilla/servo/blob/155befe10dc56cfb2dfbf0cca7b652293dba9753/src/components/script/dom/bindings/utils.rs#L119 --- src/components/script/dom/document.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/components/script/dom/document.rs b/src/components/script/dom/document.rs index 4df11e57754..08a0ee45763 100644 --- a/src/components/script/dom/document.rs +++ b/src/components/script/dom/document.rs @@ -92,7 +92,7 @@ pub struct Document { window: @mut Window, doctype: DocumentType, title: ~str, - idmap: HashMap<~str, AbstractNode> + idmap: HashMap> } impl Document { @@ -326,7 +326,7 @@ impl Document { } pub fn register_nodes_with_id(&mut self, root: &AbstractNode) { - foreach_ided_elements(root, |id: &~str, abstract_node: &AbstractNode| { + foreach_ided_elements(root, |id: &DOMString, abstract_node: &AbstractNode| { // TODO: "in tree order, within the context object's tree" // http://dom.spec.whatwg.org/#dom-document-getelementbyid. self.idmap.find_or_insert(id.clone(), *abstract_node); @@ -334,7 +334,7 @@ impl Document { } pub fn unregister_nodes_with_id(&mut self, root: &AbstractNode) { - foreach_ided_elements(root, |id: &~str, _| { + foreach_ided_elements(root, |id: &DOMString, _| { // TODO: "in tree order, within the context object's tree" // http://dom.spec.whatwg.org/#dom-document-getelementbyid. self.idmap.pop(id); @@ -366,7 +366,7 @@ impl Document { #[inline(always)] fn foreach_ided_elements(root: &AbstractNode, - callback: &fn(&~str, &AbstractNode)) { + callback: &fn(&DOMString, &AbstractNode)) { for node in root.traverse_preorder() { if !node.is_element() { continue;