Fix URL attributes

URL attributes should always use AttrValue::Url, and the input should be
resolved against the document's base URL at setting time always.

components/script/dom/element.rs

@@ -1302,21 +1302,30 @@ impl Element {
 
     pub fn get_url_attribute(&self, local_name: &LocalName) -> DOMString {
         assert!(*local_name == local_name.to_ascii_lowercase());
-        if !self.has_attribute(local_name) {
+        let attr = match self.get_attribute(&ns!(), local_name) {
-            return DOMString::new();
+            Some(attr) => attr,
-        }
+            None => return DOMString::new(),
-        let url = self.get_string_attribute(local_name);
+        };
-        let doc = document_from_node(self);
+        let value = attr.value();
-        let base = doc.base_url();
+        match *value {
-        // https://html.spec.whatwg.org/multipage/#reflect
+            AttrValue::Url(ref value, _) => {
-        // XXXManishearth this doesn't handle `javascript:` urls properly
+                // XXXManishearth this doesn't handle `javascript:` urls properly
-        match base.join(&url) {
+                let base = document_from_node(self).base_url();
-            Ok(parsed) => DOMString::from(parsed.into_string()),
+                let value = base.join(value)
-            Err(_) => DOMString::from(""),
+                    .map(|parsed| parsed.into_string())
+                    .unwrap_or_else(|_| value.clone());
+                DOMString::from(value)
+            },
+            _ => panic!("attribute value should be AttrValue::Url(..)"),
         }
     }
+
     pub fn set_url_attribute(&self, local_name: &LocalName, value: DOMString) {
-        self.set_string_attribute(local_name, value);
+        let value = AttrValue::from_url(
+            document_from_node(self).base_url(),
+            value.into(),
+        );
+        self.set_attribute(local_name, value);
     }
 
     pub fn get_string_attribute(&self, local_name: &LocalName) -> DOMString {

components/script/dom/htmlbaseelement.rs

@@ -16,7 +16,6 @@ use dom::virtualmethods::VirtualMethods;
 use dom_struct::dom_struct;
 use html5ever::{LocalName, Prefix};
 use servo_url::ServoUrl;
-use style::attr::AttrValue;
 
 #[dom_struct]
 pub struct HTMLBaseElement {
@@ -67,28 +66,31 @@ impl HTMLBaseElement {
 impl HTMLBaseElementMethods for HTMLBaseElement {
     // https://html.spec.whatwg.org/multipage/#dom-base-href
     fn Href(&self) -> DOMString {
+        // Step 1.
         let document = document_from_node(self);
 
-        // Step 1.
-        if !self.upcast::<Element>().has_attribute(&local_name!("href")) {
-            return DOMString::from(document.base_url().as_str());
-        }
-
         // Step 2.
-        let fallback_base_url = document.fallback_base_url();
+        let attr = self.upcast::<Element>().get_attribute(&ns!(), &local_name!("href"));
+        let value = attr.as_ref().map(|attr| attr.value());
+        let url = value.as_ref().map_or("", |value| &**value);
 
         // Step 3.
-        let url = self.upcast::<Element>().get_url_attribute(&local_name!("href"));
+        let url_record = document.fallback_base_url().join(url);
 
-        // Step 4.
+        match url_record {
-        let url_record = fallback_base_url.join(&*url);
+            Err(_) => {
-
+                // Step 4.
-        // Step 5, 6.
+                url.into()
-        DOMString::from(url_record.as_ref().map(|url| url.as_str()).unwrap_or(""))
+            }
+            Ok(url_record) => {
+                // Step 5.
+                url_record.into_string().into()
+            },
+        }
     }
 
     // https://html.spec.whatwg.org/multipage/#dom-base-href
-    make_url_setter!(SetHref, "href");
+    make_setter!(SetHref, "href");
 }
 
 impl VirtualMethods for HTMLBaseElement {

components/script/dom/htmlbodyelement.rs

@@ -4,6 +4,7 @@
 
 use cssparser::RGBA;
 use dom::attr::Attr;
+use dom::bindings::codegen::Bindings::AttrBinding::AttrBinding::AttrMethods;
 use dom::bindings::codegen::Bindings::HTMLBodyElementBinding::{self, HTMLBodyElementMethods};
 use dom::bindings::codegen::Bindings::WindowBinding::WindowMethods;
 use dom::bindings::inheritance::Castable;
@@ -73,7 +74,12 @@ impl HTMLBodyElementMethods for HTMLBodyElement {
     make_legacy_color_setter!(SetText, "text");
 
     // https://html.spec.whatwg.org/multipage/#dom-body-background
-    make_getter!(Background, "background");
+    fn Background(&self) -> DOMString {
+        self.upcast::<Element>()
+            .get_attribute(&ns!(), &local_name!("background"))
+            .map(|attr| attr.Value())
+            .unwrap_or_default()
+    }
 
     // https://html.spec.whatwg.org/multipage/#dom-body-background
     make_url_setter!(SetBackground, "background");
@@ -154,7 +160,7 @@ impl VirtualMethods for HTMLBodyElement {
             local_name!("bgcolor") |
             local_name!("text") => AttrValue::from_legacy_color(value.into()),
             local_name!("background") => {
-                AttrValue::from_url(document_from_node(self).url(), value.into())
+                AttrValue::from_url(document_from_node(self).base_url(), value.into())
             },
             _ => self.super_type().unwrap().parse_plain_attribute(name, value),
         }

components/script/dom/htmliframeelement.rs

@@ -568,14 +568,10 @@ pub fn Navigate(iframe: &HTMLIFrameElement, direction: TraversalDirection) -> Er
 
 impl HTMLIFrameElementMethods for HTMLIFrameElement {
     // https://html.spec.whatwg.org/multipage/#dom-iframe-src
-    fn Src(&self) -> DOMString {
+    make_url_getter!(Src, "src");
-        self.upcast::<Element>().get_string_attribute(&local_name!("src"))
-    }
 
     // https://html.spec.whatwg.org/multipage/#dom-iframe-src
-    fn SetSrc(&self, src: DOMString) {
+    make_url_setter!(SetSrc, "src");
-        self.upcast::<Element>().set_url_attribute(&local_name!("src"), src)
-    }
 
     // https://html.spec.whatwg.org/multipage/#dom-iframe-sandbox
     fn Sandbox(&self) -> DomRoot<DOMTokenList> {
@@ -765,6 +761,7 @@ impl VirtualMethods for HTMLIFrameElement {
 
     fn parse_plain_attribute(&self, name: &LocalName, value: DOMString) -> AttrValue {
         match name {
+            &local_name!("src") => AttrValue::from_url(document_from_node(self).base_url(), value.into()),
             &local_name!("sandbox") => AttrValue::from_serialized_tokenlist(value.into()),
             &local_name!("width") => AttrValue::from_dimension(value.into()),
             &local_name!("height") => AttrValue::from_dimension(value.into()),

components/script/dom/htmlimageelement.rs

@@ -824,8 +824,9 @@ impl HTMLImageElementMethods for HTMLImageElement {
 
     // https://html.spec.whatwg.org/multipage/#dom-img-src
     make_url_getter!(Src, "src");
+
     // https://html.spec.whatwg.org/multipage/#dom-img-src
-    make_setter!(SetSrc, "src");
+    make_url_setter!(SetSrc, "src");
 
     // https://html.spec.whatwg.org/multipage/#dom-img-crossOrigin
     fn GetCrossOrigin(&self) -> Option<DOMString> {
@@ -980,6 +981,7 @@ impl VirtualMethods for HTMLImageElement {
 
     fn parse_plain_attribute(&self, name: &LocalName, value: DOMString) -> AttrValue {
         match name {
+            &local_name!("src") => AttrValue::from_url(document_from_node(self).base_url(), value.into()),
             &local_name!("name") => AttrValue::from_atomic(value.into()),
             &local_name!("width") | &local_name!("height") => AttrValue::from_dimension(value.into()),
             &local_name!("hspace") | &local_name!("vspace") => AttrValue::from_u32(value.into(), 0),

components/script/dom/htmlinputelement.rs

@@ -1056,6 +1056,7 @@ impl VirtualMethods for HTMLInputElement {
 
     fn parse_plain_attribute(&self, name: &LocalName, value: DOMString) -> AttrValue {
         match name {
+            &local_name!("src") => AttrValue::from_url(document_from_node(self).base_url(), value.into()),
             &local_name!("accept") => AttrValue::from_comma_separated_tokenlist(value.into()),
             &local_name!("name") => AttrValue::from_atomic(value.into()),
             &local_name!("size") => AttrValue::from_limited_u32(value.into(), DEFAULT_INPUT_SIZE),

components/script/dom/htmllinkelement.rs

@@ -208,6 +208,7 @@ impl VirtualMethods for HTMLLinkElement {
 
     fn parse_plain_attribute(&self, name: &LocalName, value: DOMString) -> AttrValue {
         match name {
+            &local_name!("href") => AttrValue::from_url(document_from_node(self).base_url(), value.into()),
             &local_name!("rel") => AttrValue::from_serialized_tokenlist(value.into()),
             _ => self.super_type().unwrap().parse_plain_attribute(name, value),
         }
@@ -373,7 +374,7 @@ impl HTMLLinkElementMethods for HTMLLinkElement {
     make_url_getter!(Href, "href");
 
     // https://html.spec.whatwg.org/multipage/#dom-link-href
-    make_setter!(SetHref, "href");
+    make_url_setter!(SetHref, "href");
 
     // https://html.spec.whatwg.org/multipage/#dom-link-rel
     make_getter!(Rel, "rel");

components/script/dom/htmlmediaelement.rs

@@ -46,6 +46,7 @@ use std::collections::VecDeque;
 use std::mem;
 use std::rc::Rc;
 use std::sync::{Arc, Mutex};
+use style::attr::AttrValue;
 use task_source::TaskSource;
 use time::{self, Timespec, Duration};
 
@@ -837,8 +838,9 @@ impl HTMLMediaElementMethods for HTMLMediaElement {
 
     // https://html.spec.whatwg.org/multipage/#dom-media-src
     make_url_getter!(Src, "src");
+
     // https://html.spec.whatwg.org/multipage/#dom-media-src
-    make_setter!(SetSrc, "src");
+    make_url_setter!(SetSrc, "src");
 
     // https://html.spec.whatwg.org/multipage/#dom-media-srcobject
     fn GetSrcObject(&self) -> Option<DomRoot<Blob>> {
@@ -913,6 +915,13 @@ impl VirtualMethods for HTMLMediaElement {
         Some(self.upcast::<HTMLElement>() as &VirtualMethods)
     }
 
+    fn parse_plain_attribute(&self, name: &LocalName, value: DOMString) -> AttrValue {
+        match name {
+            &local_name!("src") => AttrValue::from_url(document_from_node(self).base_url(), value.into()),
+            _ => self.super_type().unwrap().parse_plain_attribute(name, value),
+        }
+    }
+
     fn attribute_mutated(&self, attr: &Attr, mutation: AttributeMutation) {
         self.super_type().unwrap().attribute_mutated(attr, mutation);
 

components/script/dom/htmlscriptelement.rs

@@ -42,6 +42,7 @@ use std::io::{Read, Write};
 use std::path::PathBuf;
 use std::process::{Command, Stdio};
 use std::sync::{Arc, Mutex};
+use style::attr::AttrValue;
 use style::str::{HTML_SPACE_CHARACTERS, StaticStringVec};
 use uuid::Uuid;
 
@@ -653,6 +654,13 @@ impl VirtualMethods for HTMLScriptElement {
         Some(self.upcast::<HTMLElement>() as &VirtualMethods)
     }
 
+    fn parse_plain_attribute(&self, name: &LocalName, value: DOMString) -> AttrValue {
+        match name {
+            &local_name!("src") => AttrValue::from_url(document_from_node(self).base_url(), value.into()),
+            _ => self.super_type().unwrap().parse_plain_attribute(name, value),
+        }
+    }
+
     fn attribute_mutated(&self, attr: &Attr, mutation: AttributeMutation) {
         self.super_type().unwrap().attribute_mutated(attr, mutation);
         match *attr.local_name() {
@@ -702,8 +710,9 @@ impl VirtualMethods for HTMLScriptElement {
 impl HTMLScriptElementMethods for HTMLScriptElement {
     // https://html.spec.whatwg.org/multipage/#dom-script-src
     make_url_getter!(Src, "src");
+
     // https://html.spec.whatwg.org/multipage/#dom-script-src
-    make_setter!(SetSrc, "src");
+    make_url_setter!(SetSrc, "src");
 
     // https://html.spec.whatwg.org/multipage/#dom-script-type
     make_getter!(Type, "type");

components/script/dom/macros.rs

@@ -194,11 +194,8 @@ macro_rules! make_url_setter(
         fn $attr(&self, value: DOMString) {
             use dom::bindings::inheritance::Castable;
             use dom::element::Element;
-            use dom::node::document_from_node;
-            let value = AttrValue::from_url(document_from_node(self).url(),
-                                            value.into());
             let element = self.upcast::<Element>();
-            element.set_attribute(&local_name!($htmlname), value);
+            element.set_url_attribute(&local_name!($htmlname), value);
         }
     );
 );

tests/wpt/metadata/html/semantics/document-metadata/the-base-element/base_href_invalid.html.ini

@@ -1,5 +0,0 @@
-[base_href_invalid.html]
-  type: testharness
-  [base element with unparseable href should have .href getter return attr value]
-    expected: FAIL
-