Auto merge of #8526 - frewsxcv:parse-length, r=eefriedman

Fix parse_length 0 values, implement <hr> 'width'

Follow-up to https://github.com/servo/servo/issues/8424

<!-- Reviewable:start -->
[<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/8526)
<!-- Reviewable:end -->

components/script/dom/element.rs

@@ -390,6 +390,9 @@ impl LayoutElementHelpers for LayoutJS<Element> {
             this.get_width()
         } else if let Some(this) = self.downcast::<HTMLTableCellElement>() {
             this.get_width()
+        } else if let Some(this) = self.downcast::<HTMLHRElement>() {
+            // https://html.spec.whatwg.org/multipage/#the-hr-element-2:attr-hr-width
+            this.get_width()
         } else {
             LengthOrPercentageOrAuto::Auto
         };

components/script/dom/htmlhrelement.rs

@@ -13,7 +13,7 @@ use dom::htmlelement::HTMLElement;
 use dom::node::Node;
 use dom::virtualmethods::VirtualMethods;
 use string_cache::Atom;
-use util::str::DOMString;
+use util::str::{DOMString, LengthOrPercentageOrAuto};
 
 #[dom_struct]
 pub struct HTMLHRElement {
@@ -42,10 +42,17 @@ impl HTMLHRElementMethods for HTMLHRElement {
 
     // https://html.spec.whatwg.org/multipage/#dom-hr-color
     make_legacy_color_setter!(SetColor, "color");
+
+    // https://html.spec.whatwg.org/multipage/#dom-hr-width
+    make_getter!(Width);
+
+    // https://html.spec.whatwg.org/multipage/#dom-hr-width
+    make_dimension_setter!(SetWidth, "width");
 }
 
 pub trait HTMLHRLayoutHelpers {
     fn get_color(&self) -> Option<RGBA>;
+    fn get_width(&self) -> LengthOrPercentageOrAuto;
 }
 
 impl HTMLHRLayoutHelpers for LayoutJS<HTMLHRElement> {
@@ -58,6 +65,17 @@ impl HTMLHRLayoutHelpers for LayoutJS<HTMLHRElement> {
                 .cloned()
         }
     }
+
+    #[allow(unsafe_code)]
+    fn get_width(&self) -> LengthOrPercentageOrAuto {
+        unsafe {
+            (&*self.upcast::<Element>().unsafe_get())
+                .get_attr_for_layout(&ns!(""), &atom!("width"))
+                .map(AttrValue::as_dimension)
+                .cloned()
+                .unwrap_or(LengthOrPercentageOrAuto::Auto)
+        }
+    }
 }
 
 
@@ -69,6 +87,7 @@ impl VirtualMethods for HTMLHRElement {
     fn parse_plain_attribute(&self, name: &Atom, value: DOMString) -> AttrValue {
         match name {
             &atom!("color") => AttrValue::from_legacy_color(value),
+            &atom!("width") => AttrValue::from_dimension(value),
             _ => self.super_type().unwrap().parse_plain_attribute(name, value),
         }
     }

components/script/dom/webidls/HTMLHRElement.webidl

@@ -14,5 +14,5 @@ partial interface HTMLHRElement {
   attribute DOMString color;
   //         attribute boolean noShade;
   //         attribute DOMString size;
-  //         attribute DOMString width;
+  attribute DOMString width;
 };

components/servo/Cargo.lock

@@ -1923,6 +1923,7 @@ dependencies = [
 name = "util_tests"
 version = "0.0.1"
 dependencies = [
+ "app_units 0.1.3 (registry+https://github.com/rust-lang/crates.io-index)",
  "euclid 0.3.2 (registry+https://github.com/rust-lang/crates.io-index)",
  "libc 0.1.10 (registry+https://github.com/rust-lang/crates.io-index)",
  "plugins 0.0.1",

components/util/str.rs

@@ -303,20 +303,39 @@ pub enum LengthOrPercentageOrAuto {
     Length(Au),
 }
 
-/// Parses a length per HTML5 § 2.4.4.4. If unparseable, `Auto` is returned.
+/// TODO: this function can be rewritten to return Result<LengthOrPercentage, _>
+/// Parses a dimension value per HTML5 § 2.4.4.4. If unparseable, `Auto` is
+/// returned.
+/// https://html.spec.whatwg.org/multipage/#rules-for-parsing-dimension-values
 pub fn parse_length(mut value: &str) -> LengthOrPercentageOrAuto {
+    // Steps 1 & 2 are not relevant
+
+    // Step 3
     value = value.trim_left_matches(WHITESPACE);
-    if value.is_empty() {
+
-        return LengthOrPercentageOrAuto::Auto
+    // Step 4
-    }
-    if value.starts_with("+") {
-        value = &value[1..]
-    }
-    value = value.trim_left_matches('0');
     if value.is_empty() {
         return LengthOrPercentageOrAuto::Auto
     }
 
+    // Step 5
+    if value.starts_with("+") {
+        value = &value[1..]
+    }
+
+    // Steps 6 & 7
+    match value.chars().nth(0) {
+        Some('0'...'9') => {},
+        _ => return LengthOrPercentageOrAuto::Auto,
+    }
+
+    // Steps 8 to 13
+    // We trim the string length to the minimum of:
+    // 1. the end of the string
+    // 2. the first occurence of a '%' (U+0025 PERCENT SIGN)
+    // 3. the second occurrence of a '.' (U+002E FULL STOP)
+    // 4. the occurrence of a character that is neither a digit nor '%' nor '.'
+    // Note: Step 10 is directly subsumed by FromStr::from_str
     let mut end_index = value.len();
     let (mut found_full_stop, mut found_percent) = (false, false);
     for (i, ch) in value.chars().enumerate() {
@@ -348,7 +367,7 @@ pub fn parse_length(mut value: &str) -> LengthOrPercentageOrAuto {
     }
 
     match FromStr::from_str(value) {
-        Ok(number) => LengthOrPercentageOrAuto::Length(Au::from_px(number)),
+        Ok(number) => LengthOrPercentageOrAuto::Length(Au::from_f64_px(number)),
         Err(_) => LengthOrPercentageOrAuto::Auto,
     }
 }

tests/unit/util/Cargo.toml

@@ -16,6 +16,7 @@ path = "../../../components/util"
 path = "../../../components/plugins"
 
 [dependencies]
+app_units = {version = "0.1", features = ["plugins"]}
 libc = "0.1"
 euclid = {version = "0.3", features = ["plugins"]}
 

tests/unit/util/lib.rs

@@ -7,6 +7,7 @@
 #![feature(alloc)]
 
 extern crate alloc;
+extern crate app_units;
 extern crate euclid;
 extern crate libc;
 extern crate util;

tests/unit/util/str.rs

@@ -2,9 +2,26 @@
  * License, v. 2.0. If a copy of the MPL was not distributed with this
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
-use util::str::{search_index, split_html_space_chars, str_join};
+use app_units::Au;
+use util::str::LengthOrPercentageOrAuto;
+use util::str::{parse_length, search_index, split_html_space_chars, str_join};
 
 
+#[test]
+pub fn test_parse_length() {
+    fn check(input: &str, expected: LengthOrPercentageOrAuto) {
+        let parsed = parse_length(input);
+        assert_eq!(parsed, expected);
+    }
+
+    check("0", LengthOrPercentageOrAuto::Length(Au::from_px(0)));
+    check("0.000%", LengthOrPercentageOrAuto::Percentage(0.0));
+    check("+5.82%", LengthOrPercentageOrAuto::Percentage(0.0582));
+    check("5.82", LengthOrPercentageOrAuto::Length(Au::from_f64_px(5.82)));
+    check("invalid", LengthOrPercentageOrAuto::Auto);
+    check("12 followed by invalid", LengthOrPercentageOrAuto::Length(Au::from_px(12)));
+}
+
 #[test]
 pub fn split_html_space_chars_whitespace() {
     assert!(split_html_space_chars("").collect::<Vec<_>>().is_empty());

tests/wpt/metadata/MANIFEST.json

@@ -4495,6 +4495,16 @@
         ],
         "url": "/html/rendering/non-replaced-elements/the-hr-element-0/color.html"
       },
+      {
+        "path": "html/rendering/non-replaced-elements/the-hr-element-0/width.html",
+        "references": [
+          [
+            "/html/rendering/non-replaced-elements/the-hr-element-0/width-ref.html",
+            "=="
+          ]
+        ],
+        "url": "/html/rendering/non-replaced-elements/the-hr-element-0/width.html"
+      },
       {
         "path": "html/rendering/non-replaced-elements/the-page/body_text_00ffff.xhtml",
         "references": [

tests/wpt/metadata/html/dom/interfaces.html.ini

@@ -2313,9 +2313,6 @@
   [HTMLHRElement interface: attribute size]
     expected: FAIL
 
-  [HTMLHRElement interface: attribute width]
-    expected: FAIL
-
   [HTMLHRElement interface: document.createElement("hr") must inherit property "align" with the proper type (0)]
     expected: FAIL
 
@@ -2325,9 +2322,6 @@
   [HTMLHRElement interface: document.createElement("hr") must inherit property "size" with the proper type (3)]
     expected: FAIL
 
-  [HTMLHRElement interface: document.createElement("hr") must inherit property "width" with the proper type (4)]
-    expected: FAIL
-
   [HTMLPreElement interface: existence and properties of interface object]
     expected: FAIL
 

tests/wpt/metadata/html/dom/reflection-grouping.html.ini

@@ -1686,135 +1686,6 @@
   [hr.size: IDL set to object "test-valueOf" followed by IDL get]
     expected: FAIL
 
-  [hr.width: typeof IDL attribute]
-    expected: FAIL
-
-  [hr.width: IDL get with DOM attribute unset]
-    expected: FAIL
-
-  [hr.width: setAttribute() to "" followed by IDL get]
-    expected: FAIL
-
-  [hr.width: setAttribute() to " \\0\\x01\\x02\\x03\\x04\\x05\\x06\\x07 \\b\\t\\n\\v\\f\\r\\x0e\\x0f \\x10\\x11\\x12\\x13\\x14\\x15\\x16\\x17 \\x18\\x19\\x1a\\x1b\\x1c\\x1d\\x1e\\x1f  foo " followed by IDL get]
-    expected: FAIL
-
-  [hr.width: setAttribute() to undefined followed by IDL get]
-    expected: FAIL
-
-  [hr.width: setAttribute() to 7 followed by IDL get]
-    expected: FAIL
-
-  [hr.width: setAttribute() to 1.5 followed by IDL get]
-    expected: FAIL
-
-  [hr.width: setAttribute() to true followed by IDL get]
-    expected: FAIL
-
-  [hr.width: setAttribute() to false followed by IDL get]
-    expected: FAIL
-
-  [hr.width: setAttribute() to object "[object Object\]" followed by IDL get]
-    expected: FAIL
-
-  [hr.width: setAttribute() to NaN followed by IDL get]
-    expected: FAIL
-
-  [hr.width: setAttribute() to Infinity followed by IDL get]
-    expected: FAIL
-
-  [hr.width: setAttribute() to -Infinity followed by IDL get]
-    expected: FAIL
-
-  [hr.width: setAttribute() to "\\0" followed by IDL get]
-    expected: FAIL
-
-  [hr.width: setAttribute() to null followed by IDL get]
-    expected: FAIL
-
-  [hr.width: setAttribute() to object "test-toString" followed by IDL get]
-    expected: FAIL
-
-  [hr.width: setAttribute() to object "test-valueOf" followed by IDL get]
-    expected: FAIL
-
-  [hr.width: IDL set to "" followed by getAttribute()]
-    expected: FAIL
-
-  [hr.width: IDL set to " \\0\\x01\\x02\\x03\\x04\\x05\\x06\\x07 \\b\\t\\n\\v\\f\\r\\x0e\\x0f \\x10\\x11\\x12\\x13\\x14\\x15\\x16\\x17 \\x18\\x19\\x1a\\x1b\\x1c\\x1d\\x1e\\x1f  foo " followed by getAttribute()]
-    expected: FAIL
-
-  [hr.width: IDL set to undefined followed by getAttribute()]
-    expected: FAIL
-
-  [hr.width: IDL set to undefined followed by IDL get]
-    expected: FAIL
-
-  [hr.width: IDL set to 7 followed by getAttribute()]
-    expected: FAIL
-
-  [hr.width: IDL set to 7 followed by IDL get]
-    expected: FAIL
-
-  [hr.width: IDL set to 1.5 followed by getAttribute()]
-    expected: FAIL
-
-  [hr.width: IDL set to 1.5 followed by IDL get]
-    expected: FAIL
-
-  [hr.width: IDL set to true followed by getAttribute()]
-    expected: FAIL
-
-  [hr.width: IDL set to true followed by IDL get]
-    expected: FAIL
-
-  [hr.width: IDL set to false followed by getAttribute()]
-    expected: FAIL
-
-  [hr.width: IDL set to false followed by IDL get]
-    expected: FAIL
-
-  [hr.width: IDL set to object "[object Object\]" followed by getAttribute()]
-    expected: FAIL
-
-  [hr.width: IDL set to object "[object Object\]" followed by IDL get]
-    expected: FAIL
-
-  [hr.width: IDL set to NaN followed by getAttribute()]
-    expected: FAIL
-
-  [hr.width: IDL set to NaN followed by IDL get]
-    expected: FAIL
-
-  [hr.width: IDL set to Infinity followed by getAttribute()]
-    expected: FAIL
-
-  [hr.width: IDL set to Infinity followed by IDL get]
-    expected: FAIL
-
-  [hr.width: IDL set to -Infinity followed by getAttribute()]
-    expected: FAIL
-
-  [hr.width: IDL set to -Infinity followed by IDL get]
-    expected: FAIL
-
-  [hr.width: IDL set to "\\0" followed by getAttribute()]
-    expected: FAIL
-
-  [hr.width: IDL set to null followed by getAttribute()]
-    expected: FAIL
-
-  [hr.width: IDL set to null followed by IDL get]
-    expected: FAIL
-
-  [hr.width: IDL set to object "test-toString" followed by getAttribute()]
-    expected: FAIL
-
-  [hr.width: IDL set to object "test-toString" followed by IDL get]
-    expected: FAIL
-
-  [hr.width: IDL set to object "test-valueOf" followed by IDL get]
-    expected: FAIL
-
   [hr.itemScope: typeof IDL attribute]
     expected: FAIL
 

tests/wpt/web-platform-tests/html/rendering/non-replaced-elements/the-hr-element-0/width-ref.html

@@ -0,0 +1,19 @@
+<style>
+.hr {
+  color: gray;
+  border-style: inset;
+  border-width: 1px;
+  margin: 0.5em auto;
+}
+</style>
+<div class=hr></div>
+<div class=hr style="width: 50%"></div>
+<div class=hr style="width: 100px"></div>
+<div class=hr style="width: 100px"></div>
+<div class=hr style="width: 100px"></div>
+<div class=hr style="width: 100.99px"></div>
+<div class=hr style="width: 0%"></div>
+<div class=hr style="width: 0%"></div>
+<div class=hr style="width: 0%"></div>
+<div class=hr style="width: 0%"></div>
+<div class=hr></div>

tests/wpt/web-platform-tests/html/rendering/non-replaced-elements/the-hr-element-0/width.html

@@ -0,0 +1,15 @@
+<!doctype html>
+<meta charset="utf-8">
+<title></title>
+<link rel="match" href="width-ref.html">
+<hr>
+<hr width='50%'>
+<hr width='100'>
+<hr width='100foo'>
+<hr width='  100    '>
+<hr width='100.99'>
+<hr width='0'>
+<hr width='00'>
+<hr width='+0'>
+<hr width='+00'>
+<hr width='++0'>