Fix declarative margin reading in AbstractOrderedLayout (#18229)

Change-Id: Ia212d83568e4f0c891ec1a248b6d8567c0cf0095

5 files changed, 185 insertions, 186 deletions

diff --git a/server/src/com/vaadin/ui/AbstractLayout.java b/server/src/com/vaadin/ui/AbstractLayout.java
index 9cdb0a326a..0770670680 100644
--- a/server/src/com/vaadin/ui/AbstractLayout.java
+++ b/server/src/com/vaadin/ui/AbstractLayout.java
@@ -16,7 +16,12 @@
 
 package com.vaadin.ui;
 
+import org.jsoup.nodes.Element;
+
 import com.vaadin.shared.ui.AbstractLayoutState;
+import com.vaadin.shared.ui.MarginInfo;
+import com.vaadin.ui.declarative.DesignAttributeHandler;
+import com.vaadin.ui.declarative.DesignContext;
 
 /**
  * An abstract class that defines default implementation for the {@link Layout}
@@ -33,4 +38,90 @@ public abstract class AbstractLayout extends AbstractComponentContainer
         return (AbstractLayoutState) super.getState();
     }
 
+    /**
+     * Reads margin attributes from a design into a MarginInfo object. This
+     * helper method should be called from the
+     * {@link #readDesign(Element, DesignContext) readDesign} method of layouts
+     * that implement {@link MarginHandler}.
+     * 
+     * @since 7.5
+     * 
+     * @param design
+     *            the design from which to read
+     * @param defMargin
+     *            the default margin state for edges that are not set in the
+     *            design
+     * @param context
+     *            the DesignContext instance used for parsing the design
+     * @return the margin info
+     */
+    protected MarginInfo readMargin(Element design, MarginInfo defMargin,
+            DesignContext context) {
+
+        if (design.hasAttr("margin")) {
+            boolean margin = DesignAttributeHandler.readAttribute("margin",
+                    design.attributes(), boolean.class);
+            return new MarginInfo(margin);
+        } else {
+            boolean left = DesignAttributeHandler.readAttribute("margin-left",
+                    design.attributes(), defMargin.hasLeft(), boolean.class);
+
+            boolean right = DesignAttributeHandler.readAttribute(
+                    "margin-right", design.attributes(), defMargin.hasRight(),
+                    boolean.class);
+
+            boolean top = DesignAttributeHandler.readAttribute("margin-top",
+                    design.attributes(), defMargin.hasTop(), boolean.class);
+
+            boolean bottom = DesignAttributeHandler.readAttribute(
+                    "margin-bottom", design.attributes(),
+                    defMargin.hasBottom(), boolean.class);
+
+            return new MarginInfo(top, right, bottom, left);
+        }
+    }
+
+    /**
+     * Writes margin attributes from a MarginInfo object to a design. This
+     * helper method should be called from the
+     * {@link #readDesign(Element, DesignContext) writeDesign} method of layouts
+     * that implement {@link MarginHandler}.
+     * 
+     * 
+     * @since 7.5
+     * 
+     * @param design
+     *            the design to write to
+     * @param margin
+     *            the margin state to write
+     * @param defMargin
+     *            the default margin state to compare against
+     * @param context
+     *            the DesignContext instance used for parsing the design
+     */
+    protected void writeMargin(Element design, MarginInfo margin,
+            MarginInfo defMargin, DesignContext context) {
+        if (margin.hasAll()) {
+            DesignAttributeHandler.writeAttribute("margin",
+                    design.attributes(), margin.hasAll(), defMargin.hasAll(),
+                    boolean.class);
+        } else {
+
+            DesignAttributeHandler.writeAttribute("margin-left",
+                    design.attributes(), margin.hasLeft(), defMargin.hasLeft(),
+                    boolean.class);
+
+            DesignAttributeHandler.writeAttribute("margin-right",
+                    design.attributes(), margin.hasRight(),
+                    defMargin.hasRight(), boolean.class);
+
+            DesignAttributeHandler.writeAttribute("margin-top",
+                    design.attributes(), margin.hasTop(), defMargin.hasTop(),
+                    boolean.class);
+
+            DesignAttributeHandler.writeAttribute("margin-bottom",
+                    design.attributes(), margin.hasBottom(),
+                    defMargin.hasBottom(), boolean.class);
+        }
+    }
 }
diff --git a/server/src/com/vaadin/ui/AbstractOrderedLayout.java b/server/src/com/vaadin/ui/AbstractOrderedLayout.java
index 0214ff4be1..afe4717212 100644
--- a/server/src/com/vaadin/ui/AbstractOrderedLayout.java
+++ b/server/src/com/vaadin/ui/AbstractOrderedLayout.java
@@ -478,30 +478,7 @@ public abstract class AbstractOrderedLayout extends AbstractLayout implements
         // process default attributes
         super.readDesign(design, designContext);
 
-        // handle margins
-        if (design.hasAttr("margin")) {
-            setMargin(DesignAttributeHandler.readAttribute("margin",
-                    design.attributes(), Boolean.class));
-        } else {
-            boolean marginLeft = DesignAttributeHandler.readAttribute(
-                    "margin-left", design.attributes(), getMargin().hasLeft(),
-                    Boolean.class);
-
-            boolean marginRight = DesignAttributeHandler.readAttribute(
-                    "margin-right", design.attributes(),
-                    getMargin().hasRight(), Boolean.class);
-
-            boolean marginTop = DesignAttributeHandler.readAttribute(
-                    "margin-top", design.attributes(), getMargin().hasTop(),
-                    Boolean.class);
-
-            boolean marginBottom = DesignAttributeHandler.readAttribute(
-                    "margin-bottom", design.attributes(), getMargin()
-                            .hasBottom(), Boolean.class);
-
-            setMargin(new MarginInfo(marginTop, marginBottom, marginLeft,
-                    marginRight));
-        }
+        setMargin(readMargin(design, getMargin(), designContext));
 
         // handle children
         for (Element childComponent : design.children()) {
@@ -557,31 +534,7 @@ public abstract class AbstractOrderedLayout extends AbstractLayout implements
         AbstractOrderedLayout def = (AbstractOrderedLayout) designContext
                 .getDefaultInstance(this);
 
-        // handle margin
-        MarginInfo marginInfo = getMargin();
-
-        if (marginInfo.hasAll()) {
-            DesignAttributeHandler.writeAttribute("margin",
-                    design.attributes(), marginInfo.hasAll(), def.getMargin()
-                            .hasAll(), Boolean.class);
-        } else {
-
-            DesignAttributeHandler.writeAttribute("margin-left", design
-                    .attributes(), marginInfo.hasLeft(), def.getMargin()
-                    .hasLeft(), Boolean.class);
-
-            DesignAttributeHandler.writeAttribute("margin-right", design
-                    .attributes(), marginInfo.hasRight(), def.getMargin()
-                    .hasRight(), Boolean.class);
-
-            DesignAttributeHandler.writeAttribute("margin-top", design
-                    .attributes(), marginInfo.hasTop(), def.getMargin()
-                    .hasTop(), Boolean.class);
-
-            DesignAttributeHandler.writeAttribute("margin-bottom", design
-                    .attributes(), marginInfo.hasBottom(), def.getMargin()
-                    .hasBottom(), Boolean.class);
-        }
+        writeMargin(design, getMargin(), def.getMargin(), designContext);
 
         // handle children
         if (!designContext.shouldWriteChildren(this, def)) {
diff --git a/server/tests/src/com/vaadin/tests/server/component/AbstractLayoutDeclarativeMarginTest.java b/server/tests/src/com/vaadin/tests/server/component/AbstractLayoutDeclarativeMarginTest.java
deleted file mode 100644
index 70855f67dc..0000000000
--- a/server/tests/src/com/vaadin/tests/server/component/AbstractLayoutDeclarativeMarginTest.java
+++ /dev/null
@@ -1,104 +0,0 @@
-/*
- * Copyright 2000-2014 Vaadin Ltd.
- * 
- * Licensed under the Apache License, Version 2.0 (the "License"); you may not
- * use this file except in compliance with the License. You may obtain a copy of
- * the License at
- * 
- * http://www.apache.org/licenses/LICENSE-2.0
- * 
- * Unless required by applicable law or agreed to in writing, software
- * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
- * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
- * License for the specific language governing permissions and limitations under
- * the License.
- */
-package com.vaadin.tests.server.component;
-
-import org.junit.Test;
-
-import com.vaadin.shared.ui.MarginInfo;
-import com.vaadin.tests.design.DeclarativeTestBase;
-import com.vaadin.ui.AbstractLayout;
-import com.vaadin.ui.VerticalLayout;
-
-public class AbstractLayoutDeclarativeMarginTest extends
-        DeclarativeTestBase<AbstractLayout> {
-
-    @Test
-    public void testMarginInfo() {
-        VerticalLayout vl = new VerticalLayout();
-
-        String left = getMarginTag(true, false, false, false);
-        MarginInfo leftInfo = getMarginInfo(true, false, false, false);
-
-        String right = getMarginTag(false, true, false, false);
-        MarginInfo rightInfo = getMarginInfo(false, true, false, false);
-
-        String top = getMarginTag(false, false, true, false);
-        MarginInfo topInfo = getMarginInfo(false, false, true, false);
-
-        String bottom = getMarginTag(false, false, false, true);
-        MarginInfo bottomInfo = getMarginInfo(false, false, false, true);
-
-        String topLeft = getMarginTag(true, false, true, false);
-        MarginInfo topLeftInfo = getMarginInfo(true, false, true, false);
-
-        String topRight = getMarginTag(false, true, true, false);
-        MarginInfo topRightInfo = getMarginInfo(false, true, true, false);
-
-        String bottomLeft = getMarginTag(true, false, false, true);
-        MarginInfo bottomLeftInfo = getMarginInfo(true, false, false, true);
-
-        String bottomRight = getMarginTag(false, true, false, true);
-        MarginInfo bottomRightInfo = getMarginInfo(false, true, false, true);
-
-        testRW(vl, left, leftInfo);
-        testRW(vl, right, rightInfo);
-        testRW(vl, top, topInfo);
-        testRW(vl, bottom, bottomInfo);
-
-        testRW(vl, topLeft, topLeftInfo);
-        testRW(vl, topRight, topRightInfo);
-        testRW(vl, bottomLeft, bottomLeftInfo);
-        testRW(vl, bottomRight, bottomRightInfo);
-
-        // Test special case of all edges margin'ed
-        testRW(vl, getMarginTag(true, true, true, true), new MarginInfo(true));
-    }
-
-    private void testRW(VerticalLayout vl, String design, MarginInfo margin) {
-        vl.setMargin(margin);
-        testWrite(design, vl);
-        testRead(design, vl);
-    }
-
-    private String getMarginTag(boolean left, boolean right, boolean top,
-            boolean bottom) {
-        String s = "<v-vertical-layout ";
-
-        if (left && right && top && bottom) {
-            s += "margin='true'";
-        } else {
-            if (left) {
-                s += "margin-left='true' ";
-            }
-            if (right) {
-                s += "margin-right='true' ";
-            }
-            if (top) {
-                s += "margin-top='true' ";
-            }
-            if (bottom) {
-                s += "margin-bottom='true' ";
-            }
-        }
-        return s + " />";
-    }
-
-    private MarginInfo getMarginInfo(boolean left, boolean right, boolean top,
-            boolean bottom) {
-        return new MarginInfo(top, right, bottom, left);
-    }
-
-}
diff --git a/server/tests/src/com/vaadin/tests/server/component/DeclarativeMarginTest.java b/server/tests/src/com/vaadin/tests/server/component/DeclarativeMarginTest.java
new file mode 100644
index 0000000000..d31a93a68b
--- /dev/null
+++ b/server/tests/src/com/vaadin/tests/server/component/DeclarativeMarginTest.java
@@ -0,0 +1,76 @@
+/*
+ * Copyright 2000-2014 Vaadin Ltd.
+ * 
+ * Licensed under the Apache License, Version 2.0 (the "License"); you may not
+ * use this file except in compliance with the License. You may obtain a copy of
+ * the License at
+ * 
+ * http://www.apache.org/licenses/LICENSE-2.0
+ * 
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+ * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+ * License for the specific language governing permissions and limitations under
+ * the License.
+ */
+package com.vaadin.tests.server.component;
+
+import org.junit.Assert;
+import org.junit.Ignore;
+import org.junit.Test;
+
+import com.vaadin.shared.ui.MarginInfo;
+import com.vaadin.tests.design.DeclarativeTestBase;
+import com.vaadin.ui.Layout;
+import com.vaadin.ui.Layout.MarginHandler;
+
+@Ignore
+public abstract class DeclarativeMarginTest<L extends Layout & MarginHandler>
+        extends DeclarativeTestBase<L> {
+
+    @Test
+    public void testMargins() {
+
+        for (int i = 0; i < 16; ++i) {
+            boolean top = (i & 1) == 1;
+            boolean right = (i & 2) == 2;
+            boolean bottom = (i & 4) == 4;
+            boolean left = (i & 8) == 8;
+
+            MarginInfo m = new MarginInfo(top, right, bottom, left);
+
+            String design = getMarginTag(top, right, bottom, left);
+
+            // 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(m, layout.getMargin());
+
+            testWrite(design, layout);
+        }
+    }
+
+    private String getMarginTag(boolean top, boolean right, boolean bottom,
+            boolean left) {
+        String s = "<v-vertical-layout ";
+
+        if (left && right && top && bottom) {
+            s += "margin='true'";
+        } else {
+            if (left) {
+                s += "margin-left='true' ";
+            }
+            if (right) {
+                s += "margin-right='true' ";
+            }
+            if (top) {
+                s += "margin-top='true' ";
+            }
+            if (bottom) {
+                s += "margin-bottom='true' ";
+            }
+        }
+        return s + " />";
+    }
+}
diff --git a/server/tests/src/com/vaadin/tests/server/component/abstractorderedlayout/AbstractOrderedLayoutDeclarativeTest.java b/server/tests/src/com/vaadin/tests/server/component/abstractorderedlayout/AbstractOrderedLayoutDeclarativeTest.java
index 8ccd41f797..38bd68e6e1 100644
--- a/server/tests/src/com/vaadin/tests/server/component/abstractorderedlayout/AbstractOrderedLayoutDeclarativeTest.java
+++ b/server/tests/src/com/vaadin/tests/server/component/abstractorderedlayout/AbstractOrderedLayoutDeclarativeTest.java
@@ -21,7 +21,7 @@ import java.util.List;
 import org.junit.Test;
 
 import com.vaadin.shared.ui.label.ContentMode;
-import com.vaadin.tests.design.DeclarativeTestBase;
+import com.vaadin.tests.server.component.DeclarativeMarginTest;
 import com.vaadin.ui.AbstractOrderedLayout;
 import com.vaadin.ui.Alignment;
 import com.vaadin.ui.Button;
@@ -35,58 +35,42 @@ import com.vaadin.ui.VerticalLayout;
  * @author Vaadin Ltd
  */
 public class AbstractOrderedLayoutDeclarativeTest extends
-        DeclarativeTestBase<AbstractOrderedLayout> {
+        DeclarativeMarginTest<AbstractOrderedLayout> {
 
     private List<String> defaultAlignments = Arrays.asList(new String[] {
             ":top", ":left" });
 
     @Test
-    public void testMargin() {
-        String design = getDesign(0, true);
-        AbstractOrderedLayout layout = getLayout(0, true, null);
-        testRead(design, layout);
-        testWrite(design, layout);
-        design = getDesign(0, false);
-        layout = getLayout(0, false, null);
-        testRead(design, layout);
-        testWrite(design, layout);
-    }
-
-    @Test
     public void testExpandRatio() {
-        String design = getDesign(1, false);
-        AbstractOrderedLayout layout = getLayout(1, false, null);
+        String design = getDesign(1);
+        AbstractOrderedLayout layout = getLayout(1, null);
         testRead(design, layout);
         testWrite(design, layout);
-        design = getDesign(0.25f, false);
-        layout = getLayout(0.25f, false, null);
+        design = getDesign(0.25f);
+        layout = getLayout(0.25f, null);
         testRead(design, layout);
         testWrite(design, layout);
     }
 
     @Test
     public void testAlignment() {
-        String design = getDesign(0, false, ":top", ":left");
-        AbstractOrderedLayout layout = getLayout(0, false, Alignment.TOP_LEFT);
+        String design = getDesign(0, ":top", ":left");
+        AbstractOrderedLayout layout = getLayout(0, Alignment.TOP_LEFT);
         testRead(design, layout);
         testWrite(design, layout);
-        design = getDesign(0, false, ":middle", ":center");
-        layout = getLayout(0, false, Alignment.MIDDLE_CENTER);
+        design = getDesign(0, ":middle", ":center");
+        layout = getLayout(0, Alignment.MIDDLE_CENTER);
         testRead(design, layout);
         testWrite(design, layout);
-        design = getDesign(0, false, ":bottom", ":right");
-        layout = getLayout(0, false, Alignment.BOTTOM_RIGHT);
+        design = getDesign(0, ":bottom", ":right");
+        layout = getLayout(0, Alignment.BOTTOM_RIGHT);
         testRead(design, layout);
         testWrite(design, layout);
     }
 
-    private String getDesign(float expandRatio, boolean margin,
-            String... alignments) {
-        String result = "<v-vertical-layout caption=test-layout";
-        if (margin) {
-            result += " margin=true";
-        }
-        result += "><v-label caption=test-label ";
+    private String getDesign(float expandRatio, String... alignments) {
+        String result = "<v-vertical-layout caption=test-layout>";
+        result += "<v-label caption=test-label ";
         String ratioString = expandRatio == 1.0f ? "\"\"" : String
                 .valueOf(expandRatio);
         if (expandRatio != 0) {
@@ -110,10 +94,9 @@ public class AbstractOrderedLayoutDeclarativeTest extends
         return result;
     }
 
-    private AbstractOrderedLayout getLayout(float expandRatio, boolean margin,
+    private AbstractOrderedLayout getLayout(float expandRatio,
             Alignment alignment) {
         VerticalLayout layout = new VerticalLayout();
-        layout.setMargin(margin);
         layout.setCaption("test-layout");
         Label l = new Label();
         l.setCaption("test-label");