Correctly write margin info for FormLayout into design files (#8804)

Fixes #8800

4 files changed, 85 insertions, 26 deletions

diff --git a/server/src/main/java/com/vaadin/ui/AbstractLayout.java b/server/src/main/java/com/vaadin/ui/AbstractLayout.java
index fe073eb687..7e62176e7c 100644
--- a/server/src/main/java/com/vaadin/ui/AbstractLayout.java
+++ b/server/src/main/java/com/vaadin/ui/AbstractLayout.java
@@ -105,10 +105,16 @@ public abstract class AbstractLayout extends AbstractComponentContainer
      */
     protected void writeMargin(Element design, MarginInfo margin,
             MarginInfo defMargin, DesignContext context) {
-        if (margin.hasAll() || margin.hasNone()) {
+        if (defMargin.getBitMask() == margin.getBitMask()) {
+            // Default, no need to write
+        } else if (margin.hasNone()) {
+            // Write "margin='false'"
             DesignAttributeHandler.writeAttribute("margin", design.attributes(),
-                    margin.hasAll(), defMargin.hasAll(), boolean.class,
-                    context);
+                    false, true, boolean.class, context);
+        } else if (margin.hasAll()) {
+            // Write "margin"
+            DesignAttributeHandler.writeAttribute("margin", design.attributes(),
+                    true, false, boolean.class, context);
         } else {
 
             DesignAttributeHandler.writeAttribute("margin-left",
diff --git a/server/src/test/java/com/vaadin/tests/server/component/DeclarativeMarginTestBase.java b/server/src/test/java/com/vaadin/tests/server/component/DeclarativeMarginTestBase.java
index b333c89fc8..66f6076f7b 100644
--- a/server/src/test/java/com/vaadin/tests/server/component/DeclarativeMarginTestBase.java
+++ b/server/src/test/java/com/vaadin/tests/server/component/DeclarativeMarginTestBase.java
@@ -21,11 +21,34 @@ import com.vaadin.shared.ui.MarginInfo;
 import com.vaadin.tests.design.DeclarativeTestBase;
 import com.vaadin.ui.Layout;
 import com.vaadin.ui.Layout.MarginHandler;
+import com.vaadin.ui.Layout.SpacingHandler;
 
-public abstract class DeclarativeMarginTestBase<L extends Layout & MarginHandler>
+public abstract class DeclarativeMarginTestBase<L extends Layout & MarginHandler & SpacingHandler>
         extends DeclarativeTestBase<L> {
 
-    protected void testMargins(String componentTag, boolean defaultMargin) {
+    protected void testSpacing(String componentTag, boolean defaultSpacing) {
+        // Spacing on
+        String design = "<" + componentTag;
+        if (!defaultSpacing) {
+            design += " spacing";
+        }
+        design += " />";
+        L layout = read(design);
+        Assert.assertTrue(layout.isSpacing());
+        testWrite(design, layout);
+
+        // Spacing off
+        design = "<" + componentTag;
+        if (defaultSpacing) {
+            design += " spacing='false'";
+        }
+        design += " />";
+        layout = read(design);
+        Assert.assertFalse(layout.isSpacing());
+        testWrite(design, layout);
+    }
+
+    protected void testMargins(String componentTag, MarginInfo defaultMargin) {
 
         for (int i = 0; i < 16; ++i) {
             boolean top = (i & 1) == 1;
@@ -33,44 +56,52 @@ public abstract class DeclarativeMarginTestBase<L extends Layout & MarginHandler
             boolean bottom = (i & 4) == 4;
             boolean left = (i & 8) == 8;
 
-            MarginInfo m = new MarginInfo(top, right, bottom, left);
+            MarginInfo marginToTest = new MarginInfo(top, right, bottom, left);
 
-            String design = getMarginTag(componentTag, defaultMargin, top,
-                    right, bottom, left);
+            String design = getMarginTag(componentTag, defaultMargin,
+                    marginToTest);
 
             // The assertEquals machinery in DeclarativeTestBase uses bean
             // introspection and MarginInfo is not a proper bean. It ends up
             // considering *all* MarginInfo objects equal... (#18229)
             L layout = read(design);
-            Assert.assertEquals("For tag: " + design, m, layout.getMargin());
+            Assert.assertEquals("For tag: " + design, marginToTest,
+                    layout.getMargin());
 
             testWrite(design, layout);
         }
     }
 
-    private String getMarginTag(String componentTag, boolean defaultMargin,
-            boolean top, boolean right, boolean bottom, boolean left) {
+    private String getMarginTag(String componentTag, MarginInfo defaultMargin,
+            MarginInfo marginToTest) {
         String s = "<" + componentTag + " ";
-        String suffix = defaultMargin ? "=false " : " ";
 
-        if (top == left && top == right && top == bottom) {
-            if (top != defaultMargin) {
-                s += "margin" + suffix;
+        if (marginToTest.hasAll()) {
+            if (!defaultMargin.hasAll()) {
+                s += "margin ";
+            }
+        } else if (marginToTest.hasNone()) {
+            if (!defaultMargin.hasNone()) {
+                s += "margin=false ";
             }
         } else {
-            if (left != defaultMargin) {
-                s += "margin-left" + suffix;
+            if (marginToTest.hasLeft() != defaultMargin.hasLeft()) {
+                s += marginValue("margin-left", defaultMargin.hasLeft());
             }
-            if (right != defaultMargin) {
-                s += "margin-right" + suffix;
+            if (marginToTest.hasRight() != defaultMargin.hasRight()) {
+                s += marginValue("margin-right", defaultMargin.hasRight());
             }
-            if (top != defaultMargin) {
-                s += "margin-top" + suffix;
+            if (marginToTest.hasTop() != defaultMargin.hasTop()) {
+                s += marginValue("margin-top", defaultMargin.hasTop());
             }
-            if (bottom != defaultMargin) {
-                s += "margin-bottom" + suffix;
+            if (marginToTest.hasBottom() != defaultMargin.hasBottom()) {
+                s += marginValue("margin-bottom", defaultMargin.hasBottom());
             }
         }
         return s + " />";
     }
+
+    private String marginValue(String prefix, boolean defaultOn) {
+        return prefix + (defaultOn ? "=false " : " ");
+    }
 }
diff --git a/server/src/test/java/com/vaadin/tests/server/component/abstractorderedlayout/AbstractOrderedLayoutDeclarativeTest.java b/server/src/test/java/com/vaadin/tests/server/component/abstractorderedlayout/AbstractOrderedLayoutDeclarativeTest.java
index ec8db9e461..8db29d72ab 100644
--- a/server/src/test/java/com/vaadin/tests/server/component/abstractorderedlayout/AbstractOrderedLayoutDeclarativeTest.java
+++ b/server/src/test/java/com/vaadin/tests/server/component/abstractorderedlayout/AbstractOrderedLayoutDeclarativeTest.java
@@ -21,6 +21,7 @@ import java.util.List;
 import org.junit.Test;
 
 import com.vaadin.shared.ui.ContentMode;
+import com.vaadin.shared.ui.MarginInfo;
 import com.vaadin.tests.server.component.DeclarativeMarginTestBase;
 import com.vaadin.ui.AbstractOrderedLayout;
 import com.vaadin.ui.Alignment;
@@ -41,12 +42,32 @@ public class AbstractOrderedLayoutDeclarativeTest
 
     @Test
     public void testMarginsVertical() {
-        testMargins("vaadin-vertical-layout", true);
+        testMargins("vaadin-vertical-layout", new MarginInfo(true));
     }
 
     @Test
     public void testMarginsHorizontal() {
-        testMargins("vaadin-horizontal-layout", false);
+        testMargins("vaadin-horizontal-layout", new MarginInfo(false));
+    }
+
+    @Test
+    public void testMarginsForm() {
+        testMargins("vaadin-form-layout", new MarginInfo(true, false));
+    }
+
+    @Test
+    public void testSpacingVertical() {
+        testSpacing("vaadin-vertical-layout", true);
+    }
+
+    @Test
+    public void testSpacingHorizontal() {
+        testSpacing("vaadin-horizontal-layout", true);
+    }
+
+    @Test
+    public void testSpacingForm() {
+        testSpacing("vaadin-form-layout", true);
     }
 
     @Test
diff --git a/server/src/test/java/com/vaadin/tests/server/component/gridlayout/GridLayoutDeclarativeTest.java b/server/src/test/java/com/vaadin/tests/server/component/gridlayout/GridLayoutDeclarativeTest.java
index 0c1b4ce162..146810d4bf 100644
--- a/server/src/test/java/com/vaadin/tests/server/component/gridlayout/GridLayoutDeclarativeTest.java
+++ b/server/src/test/java/com/vaadin/tests/server/component/gridlayout/GridLayoutDeclarativeTest.java
@@ -23,6 +23,7 @@ import org.junit.Assert;
 import org.junit.Test;
 
 import com.vaadin.shared.ui.ContentMode;
+import com.vaadin.shared.ui.MarginInfo;
 import com.vaadin.tests.server.component.DeclarativeMarginTestBase;
 import com.vaadin.ui.Alignment;
 import com.vaadin.ui.Button;
@@ -37,7 +38,7 @@ public class GridLayoutDeclarativeTest
 
     @Test
     public void testMargins() {
-        testMargins("vaadin-grid-layout", false);
+        testMargins("vaadin-grid-layout", new MarginInfo(false));
     }
 
     @Test