Bug 1562257 part 4. Fix style mapping of hspace and vspace attributes to match the spec. r=mccr8

Per spec, "hspace" and "vspace" are parsed as dimension attributes and are
supported on the following elements: embed, iframe, img, object,
<input type="image">, marquee.  Except no one implements this for iframe.
https://github.com/whatwg/html/issues/4742 tracks the spec changing accordingly.

As far as hspace/vpace on <table> go, Safari supports them in both quirks and
standards mode, while Chrome doesn't support them in either mode.  The HTML spec
doesn't have them supported at all, and neither does the quirks mode spec, so
I'm removing the quirks-only support we had to align with the specs and Chrome.

Differential Revision: https://phabricator.services.mozilla.com/D36375

--HG--
extra : moz-landing-system : lando

dom/html/HTMLImageElement.h

@@ -100,11 +100,15 @@ class HTMLImageElement final : public nsGenericHTMLElement,
   uint32_t NaturalWidth();
   uint32_t NaturalHeight();
   bool Complete();
-  uint32_t Hspace() { return GetUnsignedIntAttr(nsGkAtoms::hspace, 0); }
+  uint32_t Hspace() {
+    return GetDimensionAttrAsUnsignedInt(nsGkAtoms::hspace, 0);
+  }
   void SetHspace(uint32_t aHspace, ErrorResult& aError) {
     SetUnsignedIntAttr(nsGkAtoms::hspace, aHspace, 0, aError);
   }
-  uint32_t Vspace() { return GetUnsignedIntAttr(nsGkAtoms::vspace, 0); }
+  uint32_t Vspace() {
+    return GetDimensionAttrAsUnsignedInt(nsGkAtoms::vspace, 0);
+  }
   void SetVspace(uint32_t aVspace, ErrorResult& aError) {
     SetUnsignedIntAttr(nsGkAtoms::vspace, aVspace, 0, aError);
   }

dom/html/HTMLMarqueeElement.cpp

@@ -97,9 +97,8 @@ bool HTMLMarqueeElement::ParseAttribute(int32_t aNamespaceID,
       return aResult.ParseEnumValue(aValue, kDirectionTable, false,
                                     kDefaultDirection);
     }
-    if ((aAttribute == nsGkAtoms::hspace) ||
-        (aAttribute == nsGkAtoms::vspace)) {
-      return aResult.ParseIntWithBounds(aValue, 0);
+    if (aAttribute == nsGkAtoms::hspace || aAttribute == nsGkAtoms::vspace) {
+      return aResult.ParseHTMLDimension(aValue);
     }
 
     if (aAttribute == nsGkAtoms::loop) {

dom/html/HTMLMarqueeElement.h

@@ -48,7 +48,9 @@ class HTMLMarqueeElement final : public nsGenericHTMLElement {
   void SetHeight(const nsAString& aHeight, ErrorResult& aError) {
     SetHTMLAttr(nsGkAtoms::height, aHeight, aError);
   }
-  uint32_t Hspace() { return GetUnsignedIntAttr(nsGkAtoms::hspace, 0); }
+  uint32_t Hspace() {
+    return GetDimensionAttrAsUnsignedInt(nsGkAtoms::hspace, 0);
+  }
   void SetHspace(uint32_t aValue, ErrorResult& aError) {
     SetUnsignedIntAttr(nsGkAtoms::hspace, aValue, 0, aError);
   }
@@ -87,7 +89,9 @@ class HTMLMarqueeElement final : public nsGenericHTMLElement {
   void SetWidth(const nsAString& aWidth, ErrorResult& aError) {
     SetHTMLAttr(nsGkAtoms::width, aWidth, aError);
   }
-  uint32_t Vspace() { return GetUnsignedIntAttr(nsGkAtoms::vspace, 0); }
+  uint32_t Vspace() {
+    return GetDimensionAttrAsUnsignedInt(nsGkAtoms::vspace, 0);
+  }
   void SetVspace(uint32_t aValue, ErrorResult& aError) {
     SetUnsignedIntAttr(nsGkAtoms::vspace, aValue, 0, aError);
   }

dom/html/HTMLObjectElement.h

@@ -140,7 +140,9 @@ class HTMLObjectElement final : public nsGenericHTMLFormElement,
   void SetDeclare(bool aValue, ErrorResult& aRv) {
     SetHTMLBoolAttr(nsGkAtoms::declare, aValue, aRv);
   }
-  uint32_t Hspace() { return GetUnsignedIntAttr(nsGkAtoms::hspace, 0); }
+  uint32_t Hspace() {
+    return GetDimensionAttrAsUnsignedInt(nsGkAtoms::hspace, 0);
+  }
   void SetHspace(uint32_t aValue, ErrorResult& aRv) {
     SetUnsignedIntAttr(nsGkAtoms::hspace, aValue, 0, aRv);
   }
@@ -150,7 +152,9 @@ class HTMLObjectElement final : public nsGenericHTMLFormElement,
   void SetStandby(const nsAString& aValue, ErrorResult& aRv) {
     SetHTMLAttr(nsGkAtoms::standby, aValue, aRv);
   }
-  uint32_t Vspace() { return GetUnsignedIntAttr(nsGkAtoms::vspace, 0); }
+  uint32_t Vspace() {
+    return GetDimensionAttrAsUnsignedInt(nsGkAtoms::vspace, 0);
+  }
   void SetVspace(uint32_t aValue, ErrorResult& aRv) {
     SetUnsignedIntAttr(nsGkAtoms::vspace, aValue, 0, aRv);
   }

dom/html/HTMLTableElement.cpp

@@ -832,9 +832,6 @@ bool HTMLTableElement::ParseAttribute(int32_t aNamespaceID, nsAtom* aAttribute,
         aAttribute == nsGkAtoms::bordercolor) {
       return aResult.ParseColor(aValue);
     }
-    if (aAttribute == nsGkAtoms::hspace || aAttribute == nsGkAtoms::vspace) {
-      return aResult.ParseIntWithBounds(aValue, 0);
-    }
   }
 
   return nsGenericHTMLElement::ParseBackgroundAttribute(
@@ -855,8 +852,6 @@ void HTMLTableElement::MapAttributesIntoRule(
   // which *element* it's matching (style rules should not stop matching
   // when the display type is changed).
 
-  nsCompatibility mode = aDecls.Document()->GetCompatibilityMode();
-
   // cellspacing
   const nsAttrValue* value = aAttributes->GetAttr(nsGkAtoms::cellspacing);
   if (value && value->Type() == nsAttrValue::eInteger &&
@@ -875,28 +870,6 @@ void HTMLTableElement::MapAttributesIntoRule(
     }
   }
 
-  // hspace is mapped into left and right margin,
-  // vspace is mapped into top and bottom margins
-  // - *** Quirks Mode only ***
-  if (eCompatibility_NavQuirks == mode) {
-    value = aAttributes->GetAttr(nsGkAtoms::hspace);
-
-    if (value && value->Type() == nsAttrValue::eInteger) {
-      aDecls.SetPixelValueIfUnset(eCSSProperty_margin_left,
-                                  (float)value->GetIntegerValue());
-      aDecls.SetPixelValueIfUnset(eCSSProperty_margin_right,
-                                  (float)value->GetIntegerValue());
-    }
-
-    value = aAttributes->GetAttr(nsGkAtoms::vspace);
-
-    if (value && value->Type() == nsAttrValue::eInteger) {
-      aDecls.SetPixelValueIfUnset(eCSSProperty_margin_top,
-                                  (float)value->GetIntegerValue());
-      aDecls.SetPixelValueIfUnset(eCSSProperty_margin_bottom,
-                                  (float)value->GetIntegerValue());
-    }
-  }
   // bordercolor
   value = aAttributes->GetAttr(nsGkAtoms::bordercolor);
   nscolor color;
@@ -937,8 +910,7 @@ HTMLTableElement::IsAttributeMapped(const nsAtom* aAttribute) const {
   static const MappedAttributeEntry attributes[] = {
       {nsGkAtoms::cellpadding}, {nsGkAtoms::cellspacing},
       {nsGkAtoms::border},      {nsGkAtoms::width},
-      {nsGkAtoms::height},      {nsGkAtoms::hspace},
-      {nsGkAtoms::vspace},
+      {nsGkAtoms::height},
 
       {nsGkAtoms::bordercolor},
 

dom/html/nsGenericHTMLElement.cpp

@@ -1038,11 +1038,11 @@ bool nsGenericHTMLElement::ParseDivAlignValue(const nsAString& aString,
 bool nsGenericHTMLElement::ParseImageAttribute(nsAtom* aAttribute,
                                                const nsAString& aString,
                                                nsAttrValue& aResult) {
-  if ((aAttribute == nsGkAtoms::width) || (aAttribute == nsGkAtoms::height)) {
+  if (aAttribute == nsGkAtoms::width || aAttribute == nsGkAtoms::height ||
+      aAttribute == nsGkAtoms::hspace || aAttribute == nsGkAtoms::vspace) {
     return aResult.ParseHTMLDimension(aString);
   }
-  if ((aAttribute == nsGkAtoms::hspace) || (aAttribute == nsGkAtoms::vspace) ||
-      (aAttribute == nsGkAtoms::border)) {
+  if (aAttribute == nsGkAtoms::border) {
     return aResult.ParseIntWithBounds(aString, 0);
   }
   return false;
@@ -1247,33 +1247,15 @@ void nsGenericHTMLElement::MapImageMarginAttributeInto(
   // hspace: value
   value = aAttributes->GetAttr(nsGkAtoms::hspace);
   if (value) {
-    if (value->Type() == nsAttrValue::eInteger) {
-      aDecls.SetPixelValueIfUnset(eCSSProperty_margin_left,
-                                  (float)value->GetIntegerValue());
-      aDecls.SetPixelValueIfUnset(eCSSProperty_margin_right,
-                                  (float)value->GetIntegerValue());
-    } else if (value->Type() == nsAttrValue::ePercent) {
-      aDecls.SetPercentValueIfUnset(eCSSProperty_margin_left,
-                                    value->GetPercentValue());
-      aDecls.SetPercentValueIfUnset(eCSSProperty_margin_right,
-                                    value->GetPercentValue());
-    }
+    MapDimensionAttributeInto(aDecls, eCSSProperty_margin_left, *value);
+    MapDimensionAttributeInto(aDecls, eCSSProperty_margin_right, *value);
   }
 
   // vspace: value
   value = aAttributes->GetAttr(nsGkAtoms::vspace);
   if (value) {
-    if (value->Type() == nsAttrValue::eInteger) {
-      aDecls.SetPixelValueIfUnset(eCSSProperty_margin_top,
-                                  (float)value->GetIntegerValue());
-      aDecls.SetPixelValueIfUnset(eCSSProperty_margin_bottom,
-                                  (float)value->GetIntegerValue());
-    } else if (value->Type() == nsAttrValue::ePercent) {
-      aDecls.SetPercentValueIfUnset(eCSSProperty_margin_top,
-                                    value->GetPercentValue());
-      aDecls.SetPercentValueIfUnset(eCSSProperty_margin_bottom,
-                                    value->GetPercentValue());
-    }
+    MapDimensionAttributeInto(aDecls, eCSSProperty_margin_top, *value);
+    MapDimensionAttributeInto(aDecls, eCSSProperty_margin_bottom, *value);
   }
 }
 

testing/web-platform/meta/html/rendering/dimension-attributes.html.ini

@@ -142,3 +142,83 @@
   [<tr height="0px"> mapping to height]
     expected: FAIL
     bug: https://github.com/whatwg/html/issues/4716
+
+  [<embed hspace="200.%"> mapping to marginLeft]
+    expected: FAIL
+    bug: https://github.com/whatwg/html/issues/4736
+
+  [<embed hspace="200.%"> mapping to marginRight]
+    expected: FAIL
+    bug: https://github.com/whatwg/html/issues/4736
+
+  [<embed vspace="200.%"> mapping to marginTop]
+    expected: FAIL
+    bug: https://github.com/whatwg/html/issues/4736
+
+  [<embed vspace="200.%"> mapping to marginBottom]
+    expected: FAIL
+    bug: https://github.com/whatwg/html/issues/4736
+
+  [<img hspace="200.%"> mapping to marginLeft]
+    expected: FAIL
+    bug: https://github.com/whatwg/html/issues/4736
+
+  [<img hspace="200.%"> mapping to marginRight]
+    expected: FAIL
+    bug: https://github.com/whatwg/html/issues/4736
+
+  [<img vspace="200.%"> mapping to marginTop]
+    expected: FAIL
+    bug: https://github.com/whatwg/html/issues/4736
+
+  [<img vspace="200.%"> mapping to marginBottom]
+    expected: FAIL
+    bug: https://github.com/whatwg/html/issues/4736
+
+  [<object hspace="200.%"> mapping to marginLeft]
+    expected: FAIL
+    bug: https://github.com/whatwg/html/issues/4736
+
+  [<object hspace="200.%"> mapping to marginRight]
+    expected: FAIL
+    bug: https://github.com/whatwg/html/issues/4736
+
+  [<object vspace="200.%"> mapping to marginTop]
+    expected: FAIL
+    bug: https://github.com/whatwg/html/issues/4736
+
+  [<object vspace="200.%"> mapping to marginBottom]
+    expected: FAIL
+    bug: https://github.com/whatwg/html/issues/4736
+
+  [<input hspace="200.%"> mapping to marginLeft]
+    expected: FAIL
+    bug: https://github.com/whatwg/html/issues/4736
+
+  [<input hspace="200.%"> mapping to marginRight]
+    expected: FAIL
+    bug: https://github.com/whatwg/html/issues/4736
+
+  [<input vspace="200.%"> mapping to marginTop]
+    expected: FAIL
+    bug: https://github.com/whatwg/html/issues/4736
+
+  [<input vspace="200.%"> mapping to marginBottom]
+    expected: FAIL
+    bug: https://github.com/whatwg/html/issues/4736
+
+  [<marquee hspace="200.%"> mapping to marginLeft]
+    expected: FAIL
+    bug: https://github.com/whatwg/html/issues/4736
+
+  [<marquee hspace="200.%"> mapping to marginRight]
+    expected: FAIL
+    bug: https://github.com/whatwg/html/issues/4736
+
+  [<marquee vspace="200.%"> mapping to marginTop]
+    expected: FAIL
+    bug: https://github.com/whatwg/html/issues/4736
+
+  [<marquee vspace="200.%"> mapping to marginBottom]
+    expected: FAIL
+    bug: https://github.com/whatwg/html/issues/4736

testing/web-platform/meta/html/rendering/non-replaced-elements/tables/table-vspace-hspace.html.ini

@@ -1,4 +0,0 @@
-[table-vspace-hspace.html]
-  [table vspace hspace (quirks mode)]
-    expected: FAIL
-

testing/web-platform/meta/html/rendering/replaced-elements/images/space.html.ini

@@ -1,2 +0,0 @@
-[space.html]
-  expected: FAIL

testing/web-platform/tests/html/rendering/dimension-attributes.html

@@ -134,6 +134,26 @@ const tests = [
   // https://github.com/whatwg/html/issues/4717 tracks the fact that for the
   // <col width> case that "0 is valid" boolean should probably be true.
   [ newElem("col"), "width", "width", false ],
+  [ newElem("embed"), "hspace", "marginLeft", true ],
+  [ newElem("embed"), "hspace", "marginRight", true ],
+  [ newElem("embed"), "vspace", "marginTop", true ],
+  [ newElem("embed"), "vspace", "marginBottom", true ],
+  [ newElem("img"), "hspace", "marginLeft", true ],
+  [ newElem("img"), "hspace", "marginRight", true ],
+  [ newElem("img"), "vspace", "marginTop", true ],
+  [ newElem("img"), "vspace", "marginBottom", true ],
+  [ newElem("object"), "hspace", "marginLeft", true ],
+  [ newElem("object"), "hspace", "marginRight", true ],
+  [ newElem("object"), "vspace", "marginTop", true ],
+  [ newElem("object"), "vspace", "marginBottom", true ],
+  [ newImageInput(), "hspace", "marginLeft", true ],
+  [ newImageInput(), "hspace", "marginRight", true ],
+  [ newImageInput(), "vspace", "marginTop", true ],
+  [ newImageInput(), "vspace", "marginBottom", true ],
+  [ newElem("marquee"), "hspace", "marginLeft", true ],
+  [ newElem("marquee"), "hspace", "marginRight", true ],
+  [ newElem("marquee"), "vspace", "marginTop", true ],
+  [ newElem("marquee"), "vspace", "marginBottom", true ],
 ];
 
 

testing/web-platform/tests/html/rendering/unmapped-attributes.html

@@ -0,0 +1,68 @@
+<!doctype html>
+<meta charset=utf-8>
+<title>Test handling of attributes that should not be mapped into style, but
+  incorrectly were in some browsers</title>
+<script src=/resources/testharness.js></script>
+<script src=/resources/testharnessreport.js></script>
+<body>
+<div id="container" style="display: none"></div>
+<iframe></iframe>
+<script>
+ /*
+  * We wand to test both quirks and standards mode.  We can use the fact that
+  * our document is in standards mode and the about:blank iframe we have is in
+  * quirks mode.
+  */
+test(() => {
+  assert_equals(document.compatMode, "CSS1Compat")
+}, "We should be in standards mode");
+const container = document.getElementById("container");
+
+const frameDoc = document.querySelector("iframe").contentDocument;
+test(() => {
+  assert_equals(frameDoc.compatMode, "BackCompat")
+}, "Subframe should be in quirks mode");
+const frameContainer = frameDoc.createElement("div");
+frameContainer.style.display = "none";
+frameDoc.body.appendChild(frameContainer);
+
+function newElem(name) {
+  return (parent) =>
+    parent.appendChild(parent.ownerDocument.createElement(name));
+}
+
+ /*
+  * Array of tests.  Each test consists of the following information:
+  *
+  * 1) An element creation function, which takes a parent element as an
+  *    argument.
+  * 2) The name of the attribute to set
+  * 3) The name of the CSS property to get.
+  */
+const tests = [
+  [ newElem("table"), "hspace", "marginLeft" ],
+  [ newElem("table"), "hspace", "marginRight" ],
+  [ newElem("table"), "vspace", "marginTop" ],
+  [ newElem("table"), "vspace", "marginBottom" ],
+];
+
+function style(element) {
+  return element.ownerDocument.defaultView.getComputedStyle(element);
+}
+
+for (let [ctor, attr, prop] of tests) {
+  for (let parent of [container, frameContainer]) {
+    let elem = ctor(parent);
+    test(function() {
+      let default_elem = ctor(parent);
+      this.add_cleanup(() => {
+        elem.remove();
+        default_elem.remove();
+      });
+      elem.setAttribute(attr, "200");
+      assert_equals(elem.getAttribute(attr), "200");
+      assert_equals(style(elem)[prop], style(default_elem)[prop]);
+    }, `<${elem.localName} ${attr}> should not be mapped to style ${prop} in ${parent.ownerDocument.compatMode} mode`);
+  }
+}
+</script>