Fix incorrect color value handling in ColorPicker (#10812)

10 files changed, 139 insertions, 18 deletions

diff --git a/client/src/main/java/com/vaadin/client/ui/VColorPicker.java b/client/src/main/java/com/vaadin/client/ui/VColorPicker.java
index ddae5bdc72..c525eaabab 100644
--- a/client/src/main/java/com/vaadin/client/ui/VColorPicker.java
+++ b/client/src/main/java/com/vaadin/client/ui/VColorPicker.java
@@ -48,6 +48,16 @@ public class VColorPicker extends VButton {
     }
 
     /**
+     * Gets the color.
+     *
+     * @since
+     * @return the color
+     */
+    public String getColor() {
+        return color;
+    }
+
+    /**
      * Mark the popup opened/closed.
      *
      * @param open
diff --git a/client/src/main/java/com/vaadin/client/ui/VColorPickerArea.java b/client/src/main/java/com/vaadin/client/ui/VColorPickerArea.java
index e10f49d00c..a1b6a8de34 100644
--- a/client/src/main/java/com/vaadin/client/ui/VColorPickerArea.java
+++ b/client/src/main/java/com/vaadin/client/ui/VColorPickerArea.java
@@ -77,6 +77,7 @@ public class VColorPickerArea extends Widget
     @Override
     public void onClick(ClickEvent event) {
         setOpen(!isOpen);
+        refreshColor();
     }
 
     @Override
@@ -164,6 +165,16 @@ public class VColorPickerArea extends Widget
     }
 
     /**
+     * Gets the color.
+     *
+     * @since
+     * @return the color
+     */
+    public String getColor() {
+        return color;
+    }
+
+    /**
      * Update the color area with the currently set color.
      */
     public void refreshColor() {
diff --git a/client/src/main/java/com/vaadin/client/ui/colorpicker/ColorPickerConnector.java b/client/src/main/java/com/vaadin/client/ui/colorpicker/ColorPickerConnector.java
index 9a25e5da2d..6b3f21111e 100644
--- a/client/src/main/java/com/vaadin/client/ui/colorpicker/ColorPickerConnector.java
+++ b/client/src/main/java/com/vaadin/client/ui/colorpicker/ColorPickerConnector.java
@@ -66,6 +66,7 @@ public class ColorPickerConnector extends AbstractColorPickerConnector {
     @Override
     protected void refreshColor() {
         getWidget().refreshColor();
+        rpc.changeColor(getWidget().getColor());
     }
 
     @Override
diff --git a/client/src/main/java/com/vaadin/client/ui/colorpicker/VColorPickerGradient.java b/client/src/main/java/com/vaadin/client/ui/colorpicker/VColorPickerGradient.java
index 3113849e10..d3d0e84e93 100644
--- a/client/src/main/java/com/vaadin/client/ui/colorpicker/VColorPickerGradient.java
+++ b/client/src/main/java/com/vaadin/client/ui/colorpicker/VColorPickerGradient.java
@@ -176,8 +176,9 @@ public class VColorPickerGradient extends FocusPanel implements
         }
         if (y >= 0) {
             lowercross.getElement().getStyle().setHeight(height - y, Unit.PX);
+        } else {
+            lowercross.getElement().getStyle().setHeight(Math.abs(y), Unit.PX);
         }
-
         if (x >= 0) {
             highercross.getElement().getStyle().setWidth(width - x, Unit.PX);
         }
@@ -186,6 +187,8 @@ public class VColorPickerGradient extends FocusPanel implements
         }
         if (y >= 0) {
             highercross.getElement().getStyle().setHeight(y, Unit.PX);
+        } else {
+            highercross.getElement().getStyle().setHeight(height + y, Unit.PX);
         }
     }
 
diff --git a/server/src/main/java/com/vaadin/ui/AbstractColorPicker.java b/server/src/main/java/com/vaadin/ui/AbstractColorPicker.java
index c2eafbfdc2..8cba9af5df 100644
--- a/server/src/main/java/com/vaadin/ui/AbstractColorPicker.java
+++ b/server/src/main/java/com/vaadin/ui/AbstractColorPicker.java
@@ -88,8 +88,6 @@ public abstract class AbstractColorPicker extends AbstractField<Color> {
         }
     }
 
-    private ColorPickerServerRpc rpc = this::showPopup;
-
     protected static final String STYLENAME_DEFAULT = "v-colorpicker";
     protected static final String STYLENAME_BUTTON = "v-button";
     protected static final String STYLENAME_AREA = "v-colorpicker-area";
@@ -113,6 +111,21 @@ public abstract class AbstractColorPicker extends AbstractField<Color> {
     protected boolean historyVisible = true;
     protected boolean textfieldVisible = true;
 
+    private ColorPickerServerRpc rpc = new ColorPickerServerRpc() {
+        @Override
+        public void openPopup(boolean openPopup) {
+            showPopup(openPopup);
+        }
+
+        @Override
+        public void changeColor(String col) {
+            Color valueC = new Color(
+                    Integer.parseInt(col.substring(1, col.length()), 16));
+            color = valueC;
+            setValue(color,true);
+        }
+    };
+
     /**
      * Instantiates a new color picker.
      */
@@ -170,6 +183,9 @@ public abstract class AbstractColorPicker extends AbstractField<Color> {
     public void setValue(Color color) {
         Objects.requireNonNull(color, "color cannot be null");
         super.setValue(color);
+        if (window != null) {
+            window.setValue(color);
+        }
     }
 
     /**
@@ -446,13 +462,15 @@ public abstract class AbstractColorPicker extends AbstractField<Color> {
 
                 window.addCloseListener(
                         event -> getState().popupVisible = false);
-                window.addValueChangeListener(
-                        event -> setValue(event.getValue()));
-
+                window.addValueChangeListener(event -> {
+                    setValue(event.getValue());
+                    rpc.changeColor(event.getValue().getCSS());
+                });
                 window.getHistory().setValue(color);
                 window.setPositionX(positionX);
                 window.setPositionY(positionY);
                 window.setVisible(true);
+                window.setValue(color);
 
                 parent.addWindow(window);
                 window.focus();
@@ -472,6 +490,7 @@ public abstract class AbstractColorPicker extends AbstractField<Color> {
                 parent.addWindow(window);
                 window.focus();
             }
+            window.setValue(color);
 
         } else if (window != null) {
             window.setVisible(false);
diff --git a/server/src/main/java/com/vaadin/ui/components/colorpicker/ColorPickerPopup.java b/server/src/main/java/com/vaadin/ui/components/colorpicker/ColorPickerPopup.java
index dc2bc920b2..c2ca3d012f 100644
--- a/server/src/main/java/com/vaadin/ui/components/colorpicker/ColorPickerPopup.java
+++ b/server/src/main/java/com/vaadin/ui/components/colorpicker/ColorPickerPopup.java
@@ -28,19 +28,19 @@ import com.vaadin.data.HasValue;
 import com.vaadin.shared.Registration;
 import com.vaadin.shared.ui.MarginInfo;
 import com.vaadin.shared.ui.colorpicker.Color;
-import com.vaadin.ui.AbstractColorPicker.Coordinates2Color;
+import com.vaadin.ui.TabSheet;
+import com.vaadin.ui.Window;
+import com.vaadin.ui.VerticalLayout;
+import com.vaadin.ui.HorizontalLayout;
+import com.vaadin.ui.Component;
+import com.vaadin.ui.Slider;
+import com.vaadin.ui.Layout;
 import com.vaadin.ui.Alignment;
 import com.vaadin.ui.Button;
-import com.vaadin.ui.Button.ClickEvent;
-import com.vaadin.ui.Component;
 import com.vaadin.ui.HasComponents;
-import com.vaadin.ui.HorizontalLayout;
-import com.vaadin.ui.Layout;
-import com.vaadin.ui.Slider;
+import com.vaadin.ui.AbstractColorPicker.Coordinates2Color;
+import com.vaadin.ui.Button.ClickEvent;
 import com.vaadin.ui.Slider.ValueOutOfBoundsException;
-import com.vaadin.ui.TabSheet;
-import com.vaadin.ui.VerticalLayout;
-import com.vaadin.ui.Window;
 
 /**
  * A component that represents color selection popup within a color picker.
@@ -160,8 +160,8 @@ public class ColorPickerPopup extends Window implements HasValue<Color> {
      */
     public ColorPickerPopup(Color initialColor) {
         this();
-        selectedColor = initialColor;
         initContents();
+        setValue(initialColor);
     }
 
     private void initContents() {
@@ -248,6 +248,7 @@ public class ColorPickerPopup extends Window implements HasValue<Color> {
         buttons.setComponentAlignment(ok, Alignment.MIDDLE_CENTER);
         buttons.setComponentAlignment(cancel, Alignment.MIDDLE_CENTER);
         layout.addComponent(buttons);
+        setRgbSliderValues(selectedColor);
     }
 
     /**
@@ -451,6 +452,9 @@ public class ColorPickerPopup extends Window implements HasValue<Color> {
 
     private void okButtonClick(ClickEvent event) {
         fireEvent(new ValueChangeEvent<>(this, previouslySelectedColor, true));
+        rgbPreview.setValue(getValue());
+        hsvPreview.setValue(getValue());
+        selPreview.setValue(getValue());
         close();
     }
 
@@ -516,6 +520,7 @@ public class ColorPickerPopup extends Window implements HasValue<Color> {
     }
 
     private void colorChanged(ValueChangeEvent<Color> event) {
+        event.getSource().setValue(event.getValue());
         setValue(event.getValue());
 
         updatingColors = true;
diff --git a/shared/src/main/java/com/vaadin/shared/ui/colorpicker/ColorPickerServerRpc.java b/shared/src/main/java/com/vaadin/shared/ui/colorpicker/ColorPickerServerRpc.java
index 0ba4357c4b..de6c4e30ae 100644
--- a/shared/src/main/java/com/vaadin/shared/ui/colorpicker/ColorPickerServerRpc.java
+++ b/shared/src/main/java/com/vaadin/shared/ui/colorpicker/ColorPickerServerRpc.java
@@ -33,4 +33,12 @@ public interface ColorPickerServerRpc extends ServerRpc {
      */
     public void openPopup(boolean openPopup);
 
+    /**
+     * ColorPicker's selected color is changed
+     *
+     * @since
+     * @param color
+     */
+    public void changeColor(String color);
+
 }
diff --git a/uitest/src/main/java/com/vaadin/tests/components/colorpicker/ColorPickerTestUI.java b/uitest/src/main/java/com/vaadin/tests/components/colorpicker/ColorPickerTestUI.java
index 121ddf8ddb..65a7d2ece0 100644
--- a/uitest/src/main/java/com/vaadin/tests/components/colorpicker/ColorPickerTestUI.java
+++ b/uitest/src/main/java/com/vaadin/tests/components/colorpicker/ColorPickerTestUI.java
@@ -17,7 +17,7 @@ import com.vaadin.server.StreamResource;
 import com.vaadin.server.VaadinRequest;
 import com.vaadin.shared.ui.ContentMode;
 import com.vaadin.shared.ui.colorpicker.Color;
-import com.vaadin.tests.components.AbstractReindeerTestUI;
+import com.vaadin.tests.components.AbstractTestUI;
 import com.vaadin.ui.AbstractColorPicker;
 import com.vaadin.ui.Alignment;
 import com.vaadin.ui.CheckBox;
@@ -31,7 +31,7 @@ import com.vaadin.ui.Panel;
 import com.vaadin.ui.VerticalLayout;
 
 @Widgetset("com.vaadin.DefaultWidgetSet")
-public class ColorPickerTestUI extends AbstractReindeerTestUI {
+public class ColorPickerTestUI extends AbstractTestUI {
 
     @Override
     public String getTestDescription() {
diff --git a/uitest/src/main/java/com/vaadin/tests/components/colorpicker/ColorPickerValueTest.java b/uitest/src/main/java/com/vaadin/tests/components/colorpicker/ColorPickerValueTest.java
new file mode 100644
index 0000000000..7a1fc5bcd9
--- /dev/null
+++ b/uitest/src/main/java/com/vaadin/tests/components/colorpicker/ColorPickerValueTest.java
@@ -0,0 +1,31 @@
+package com.vaadin.tests.components.colorpicker;
+
+import com.vaadin.annotations.Widgetset;
+
+import com.vaadin.server.VaadinRequest;
+import com.vaadin.shared.ui.colorpicker.Color;
+import com.vaadin.tests.components.AbstractTestUI;
+import com.vaadin.ui.AbstractColorPicker;
+
+import com.vaadin.ui.Button;
+import com.vaadin.ui.ColorPicker;
+
+@Widgetset("com.vaadin.DefaultWidgetSet")
+public class ColorPickerValueTest extends AbstractTestUI {
+
+    @Override
+    protected void setup(VaadinRequest request) {
+        AbstractColorPicker colorpicker = new ColorPicker("ColorPicker",
+                Color.GREEN);
+        colorpicker.setPosition(250, 0);
+        colorpicker.setId("clp");
+        colorpicker.setRGBVisibility(true);
+        colorpicker.setHSVVisibility(true);
+        colorpicker.setTextfieldVisibility(true);
+        colorpicker.setSwatchesVisibility(true);
+        addComponent(colorpicker);
+        addComponent(new Button("Change Color", e -> {
+            colorpicker.setValue(Color.YELLOW);
+        }));
+    }
+}
diff --git a/uitest/src/test/java/com/vaadin/tests/components/colorpicker/ColorPickerValueTestTest.java b/uitest/src/test/java/com/vaadin/tests/components/colorpicker/ColorPickerValueTestTest.java
new file mode 100644
index 0000000000..35487a0773
--- /dev/null
+++ b/uitest/src/test/java/com/vaadin/tests/components/colorpicker/ColorPickerValueTestTest.java
@@ -0,0 +1,33 @@
+package com.vaadin.tests.components.colorpicker;
+
+import com.vaadin.testbench.elements.ButtonElement;
+import com.vaadin.testbench.elements.ColorPickerElement;
+import com.vaadin.tests.tb3.MultiBrowserTest;
+import org.junit.Test;
+import org.openqa.selenium.By;
+import org.openqa.selenium.Keys;
+import org.openqa.selenium.WebElement;
+
+import static org.junit.Assert.assertEquals;
+
+public class ColorPickerValueTestTest extends MultiBrowserTest {
+
+    @Test
+    public void testValue() throws Exception {
+        openTestURL();
+
+        // Open ColorPicker
+        findElement(By.id("clp")).click();
+        // Click Button to change color
+        ButtonElement button = $(ButtonElement.class).first();
+        button.click();
+        Thread.sleep(300);
+        assertEquals("#ffff00", getColorpickerValue());
+    }
+
+    private String getColorpickerValue() {
+        WebElement field = findElement(
+                By.className("v-colorpicker-preview-textfield"));
+        return field.getAttribute("value");
+    }
+}