Fix for handling selectors with both width and height ranges (#13587)

This fix splits the used regular expressions into a more manageable
parts while fixing the handling of a case where both width-range and
height-range are defined. Also refactored some duplicate code into
a helper function.

Change-Id: I98295b8b0772b33a9985c0630e065eea2281d11d

4 files changed, 164 insertions, 44 deletions

diff --git a/WebContent/VAADIN/themes/tests-responsive/styles.css b/WebContent/VAADIN/themes/tests-responsive/styles.css
index 4bfc6dd768..db92a2a2fc 100644
--- a/WebContent/VAADIN/themes/tests-responsive/styles.css
+++ b/WebContent/VAADIN/themes/tests-responsive/styles.css
@@ -90,3 +90,12 @@
 .description[width-range="701px-"] {
     font-size: 30px;
 }
+
+/* Styles for ResponsiveWidthAndHeight test. */
+.v-csslayout-width-and-height[width-range~="0-599px"][height-range~="0-499px"] {
+	background: blue;
+	color: #fff;
+}
+.v-csslayout-width-and-height[height-range~="500px-"][width-range~="600px-"] {
+	background: red;
+}
\ No newline at end of file
diff --git a/client/src/com/vaadin/client/extensions/ResponsiveConnector.java b/client/src/com/vaadin/client/extensions/ResponsiveConnector.java
index 1392a1a49a..62913400db 100644
--- a/client/src/com/vaadin/client/extensions/ResponsiveConnector.java
+++ b/client/src/com/vaadin/client/extensions/ResponsiveConnector.java
@@ -208,58 +208,55 @@ public class ResponsiveConnector extends AbstractExtensionConnector implements
             } else if(rule.type == 1 || !rule.type) {
                 // Regular selector rule
 
-                // IE parses CSS like .class[attr="val"] into [attr="val"].class so we need to check for both
-
-                // Pattern for matching [width-range] selectors
-                var widths = IE? /\[width-range~?=["|'](.*)-(.*)["|']\]([\.|#]\S+)/i : /([\.|#]\S+)\[width-range~?=["|'](.*)-(.*)["|']\]/i;
-
-                // Patter for matching [height-range] selectors
-                var heights = IE? /\[height-range~?=["|'](.*)-(.*)["|']\]([\.|#]\S+)/i : /([\.|#]\S+)\[height-range~?=["|'](.*)-(.*)["|']\]/i;
+                // Helper function
+                var pushToCache = function(ranges, selector, min, max) {
+                    // Avoid adding duplicates
+                    var duplicate = false;
+                    for(var l = 0, len3 = ranges.length; l < len3; l++) {
+                        var bp = ranges[l];
+                        if (selector == bp[0] && min == bp[1] && max == bp[2]) {
+                            duplicate = true;
+                            break;
+                        }
+                    }
+                    if (!duplicate) {
+                        ranges.push([selector, min, max]);
+                    }
+                };
 
                 // Array of all of the separate selectors in this ruleset
                 var haystack = rule.selectorText.split(",");
 
+                // IE parses CSS like .class[attr="val"] into [attr="val"].class so we need to check for both
+                var selectorRegEx = IE ? /\[.*\]([\.|#]\S+)/ : /([\.|#]\S+?)\[.*\]/;
+
                 // Loop all the selectors in this ruleset
                 for(var k = 0, len2 = haystack.length; k < len2; k++) {
-                    var result;
-
-                    // Check for width-range matches
-                    if(result = haystack[k].match(widths)) {
-                        var selector = IE? result[3] : result[1]
-                        var min = IE? result[1] : result[2];
-                        var max = IE? result[2] : result[3];
-
-                        // Avoid adding duplicates
-                        var duplicate = false;
-                        for(var l = 0, len3 = widthRanges.length; l < len3; l++) {
-                            var bp = widthRanges[l];
-                            if(selector == bp[0] && min == bp[1] && max == bp[2]) {
-                                duplicate = true;
-                                break;
-                            }
-                        }
-                        if(!duplicate) {
-                            widthRanges.push([selector, min, max]);
+                    
+                    // Split the haystack into parts.
+                    var widthRange = haystack[k].match(/\[width-range.*?\]/);
+                    var heightRange = haystack[k].match(/\[height-range.*?\]/);
+                    var selector = haystack[k].match(selectorRegEx);
+
+                    if (selector != null) {
+                        selector = selector[1];
+                        
+                        // Check for width-ranges.
+                        if (widthRange != null) {
+                            var minMax = widthRange[0].match(/\[width-range~?=["|'](.*?)-(.*?)["|']\]/i);
+                            var min = minMax[1];
+                            var max = minMax[2];
+                            
+                            pushToCache(widthRanges, selector, min, max);
                         }
-                    }
 
-                    // Check for height-range matches
-                    if(result = haystack[k].match(heights)) {
-                        var selector = IE? result[3] : result[1]
-                        var min = IE? result[1] : result[2];
-                        var max = IE? result[2] : result[3];
-
-                        // Avoid adding duplicates
-                        var duplicate = false;
-                        for(var l = 0, len3 = heightRanges.length; l < len3; l++) {
-                            var bp = heightRanges[l];
-                            if(selector == bp[0] && min == bp[1] && max == bp[2]) {
-                                duplicate = true;
-                                break;
-                            }
-                        }
-                        if(!duplicate) {
-                            heightRanges.push([selector, min, max]);
+                        // Check for height-ranges.
+                        if (heightRange != null) {
+                            var minMax = heightRange[0].match(/\[height-range~?=["|'](.*?)-(.*?)["|']\]/i);
+                            var min = minMax[1];
+                            var max = minMax[2];
+    
+                            pushToCache(heightRanges, selector, min, max);
                         }
                     }
                 }
diff --git a/uitest/src/com/vaadin/tests/extensions/ResponsiveWidthAndHeight.java b/uitest/src/com/vaadin/tests/extensions/ResponsiveWidthAndHeight.java
new file mode 100644
index 0000000000..1c3acbec8d
--- /dev/null
+++ b/uitest/src/com/vaadin/tests/extensions/ResponsiveWidthAndHeight.java
@@ -0,0 +1,50 @@
+/*
+ * 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.extensions;
+
+import com.vaadin.annotations.Theme;
+import com.vaadin.server.Responsive;
+import com.vaadin.server.VaadinRequest;
+import com.vaadin.tests.components.AbstractTestUI;
+import com.vaadin.ui.CssLayout;
+import com.vaadin.ui.Label;
+
+@Theme("tests-responsive")
+public class ResponsiveWidthAndHeight extends AbstractTestUI {
+
+    @Override
+    protected void setup(VaadinRequest request) {
+        CssLayout layout = new CssLayout();
+        layout.addStyleName("width-and-height");
+        layout.setSizeFull();
+        setContent(layout);
+        Responsive.makeResponsive(layout);
+
+        layout.addComponent(new Label(
+                "Resize the browser window in both dimensions to see the background color change."));
+    }
+
+    @Override
+    protected String getTestDescription() {
+        return "The CssLayout with both width-range and height-range defined";
+    }
+
+    @Override
+    protected Integer getTicketNumber() {
+        return 13587;
+    }
+}
diff --git a/uitest/src/com/vaadin/tests/extensions/ResponsiveWidthAndHeightTest.java b/uitest/src/com/vaadin/tests/extensions/ResponsiveWidthAndHeightTest.java
new file mode 100644
index 0000000000..8086457c24
--- /dev/null
+++ b/uitest/src/com/vaadin/tests/extensions/ResponsiveWidthAndHeightTest.java
@@ -0,0 +1,64 @@
+/*
+ * 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.extensions;
+
+import static org.junit.Assert.assertEquals;
+
+import org.junit.Before;
+import org.junit.Test;
+import org.openqa.selenium.support.ui.ExpectedConditions;
+import org.openqa.selenium.support.ui.WebDriverWait;
+
+import com.vaadin.testbench.By;
+import com.vaadin.testbench.elements.CssLayoutElement;
+import com.vaadin.tests.tb3.MultiBrowserTest;
+
+public class ResponsiveWidthAndHeightTest extends MultiBrowserTest {
+
+    @Before
+    public void setUp() throws Exception {
+        // We need this in order to ensure that the initial width-range is
+        // width: 600px- and height: 500px-
+        testBench().resizeViewPortTo(1024, 768);
+    }
+
+    @Test
+    public void testWidthAndHeightRanges() throws Exception {
+        openTestURL();
+
+        // IE sometimes has trouble waiting long enough.
+        new WebDriverWait(getDriver(), 30).until(ExpectedConditions
+                .presenceOfElementLocated(By
+                        .cssSelector(".v-csslayout-width-and-height")));
+
+        // Verify both width-range and height-range.
+        assertEquals("600px-",
+                $(CssLayoutElement.class).first().getAttribute("width-range"));
+        assertEquals("500px-",
+                $(CssLayoutElement.class).first().getAttribute("height-range"));
+
+        // Resize
+        testBench().resizeViewPortTo(550, 450);
+
+        // Verify updated width-range and height-range.
+        assertEquals("0-599px",
+                $(CssLayoutElement.class).first().getAttribute("width-range"));
+        assertEquals("0-499px",
+                $(CssLayoutElement.class).first().getAttribute("height-range"));
+    }
+
+}