Fix incorrect system notifications with details styling (#18340)

Change-Id: Ia0d36147eb4ed9f170123771ac2674df584e6a4b

10 files changed, 152 insertions, 108 deletions

diff --git a/WebContent/VAADIN/themes/base/notification/notification.scss b/WebContent/VAADIN/themes/base/notification/notification.scss
index 9ccc29fd0e..5deb7e0e84 100644
--- a/WebContent/VAADIN/themes/base/notification/notification.scss
+++ b/WebContent/VAADIN/themes/base/notification/notification.scss
@@ -11,7 +11,8 @@
 	filter: alpha(opacity=90);
 }
 .#{$primaryStyleName}-caption,
-.#{$primaryStyleName}-description {
+.#{$primaryStyleName}-description,
+.#{$primaryStyleName}-details {
 	display: inline;
 	margin: 0 0.5em 0 0;
 }
diff --git a/WebContent/VAADIN/themes/valo/components/_notification.scss b/WebContent/VAADIN/themes/valo/components/_notification.scss
index 1b10e85d32..9538ebdc29 100644
--- a/WebContent/VAADIN/themes/valo/components/_notification.scss
+++ b/WebContent/VAADIN/themes/valo/components/_notification.scss
@@ -92,7 +92,7 @@ $v-notification-title-color: $v-focus-color !default;
     letter-spacing: 0;
   }
 
-  .#{$primary-stylename}-description {
+  .#{$primary-stylename}-description, .#{$primary-stylename}-details {
     margin: 0;
     display: inline-block;
     vertical-align: middle;
@@ -102,7 +102,8 @@ $v-notification-title-color: $v-focus-color !default;
     overflow: auto;
   }
 
-  .#{$primary-stylename}-caption ~ .#{$primary-stylename}-description {
+  .#{$primary-stylename}-caption ~ .#{$primary-stylename}-description,
+  .#{$primary-stylename}-caption ~ .#{$primary-stylename}-details {
     margin-left: round($v-font-size * 1.5);
   }
 
@@ -298,7 +299,7 @@ $v-notification-title-color: $v-focus-color !default;
   @include box-shadow(0 0 20px 0 rgba(0,0,0,0.25));
   padding: round($v-unit-size/3) round($v-unit-size/2.5);
 
-  .#{$primary-style}-description {
+  .#{$primary-style}-description, .#{$primary-style}-details {
     max-width: 50em;
   }
 
@@ -348,7 +349,7 @@ $v-notification-title-color: $v-focus-color !default;
     vertical-align: middle;
   }
 
-  .#{$primary-style}-description {
+  .#{$primary-style}-description, .#{$primary-style}-details {
     color: #e6e6e6;
   }
 }
diff --git a/client/src/com/vaadin/client/ApplicationConnection.java b/client/src/com/vaadin/client/ApplicationConnection.java
index 90fc9b2668..628559dd2a 100644
--- a/client/src/com/vaadin/client/ApplicationConnection.java
+++ b/client/src/com/vaadin/client/ApplicationConnection.java
@@ -90,7 +90,6 @@ import com.vaadin.client.ui.Icon;
 import com.vaadin.client.ui.ImageIcon;
 import com.vaadin.client.ui.VContextMenu;
 import com.vaadin.client.ui.VNotification;
-import com.vaadin.client.ui.VNotification.HideEvent;
 import com.vaadin.client.ui.VOverlay;
 import com.vaadin.client.ui.dd.VDragAndDropManager;
 import com.vaadin.client.ui.ui.UIConnector;
@@ -1234,7 +1233,7 @@ public class ApplicationConnection implements HasHandlers {
      * Shows the communication error notification.
      * 
      * @param details
-     *            Optional details for debugging.
+     *            Optional details.
      * @param statusCode
      *            The status code returned for the request
      * 
@@ -1248,7 +1247,7 @@ public class ApplicationConnection implements HasHandlers {
      * Shows the authentication error notification.
      * 
      * @param details
-     *            Optional details for debugging.
+     *            Optional details.
      */
     protected void showAuthenticationError(String details) {
         getLogger().severe("Authentication error: " + details);
@@ -1259,7 +1258,7 @@ public class ApplicationConnection implements HasHandlers {
      * Shows the session expiration notification.
      * 
      * @param details
-     *            Optional details for debugging.
+     *            Optional details.
      */
     public void showSessionExpiredError(String details) {
         getLogger().severe("Session expired: " + details);
@@ -1270,59 +1269,13 @@ public class ApplicationConnection implements HasHandlers {
      * Shows an error notification.
      * 
      * @param details
-     *            Optional details for debugging.
+     *            Optional details.
      * @param message
      *            An ErrorMessage describing the error.
      */
     protected void showError(String details, ErrorMessage message) {
-        showError(details, message.getCaption(), message.getMessage(),
-                message.getUrl());
-    }
-
-    /**
-     * Shows the error notification.
-     * 
-     * @param details
-     *            Optional details for debugging.
-     */
-    private void showError(String details, String caption, String message,
-            String url) {
-
-        StringBuilder html = new StringBuilder();
-        if (caption != null) {
-            html.append("<h1 class='");
-            html.append(VNotification.getDependentStyle(this,
-                    VNotification.CAPTION));
-            html.append("'>");
-            html.append(caption);
-            html.append("</h1>");
-        }
-        if (message != null) {
-            html.append("<p class='");
-            html.append(VNotification.getDependentStyle(this,
-                    VNotification.DESCRIPTION));
-            html.append("'>");
-            html.append(message);
-            html.append("</p>");
-        }
-
-        if (html.length() > 0) {
-
-            // Add error description
-            if (details != null) {
-                html.append("<p><i style=\"font-size:0.7em\">");
-                html.append(details);
-                html.append("</i></p>");
-            }
-
-            VNotification n = VNotification.createNotification(1000 * 60 * 45,
-                    uIConnector.getWidget());
-            n.addEventListener(new NotificationRedirect(url));
-            n.show(html.toString(), VNotification.CENTERED_TOP,
-                    VNotification.STYLE_SYSTEM);
-        } else {
-            redirect(url);
-        }
+        VNotification.showError(this, message.getCaption(),
+                message.getMessage(), details, message.getUrl());
     }
 
     protected void startRequest() {
@@ -1769,9 +1722,10 @@ public class ApplicationConnection implements HasHandlers {
                     if (meta.containsKey("appError")) {
                         ValueMap error = meta.getValueMap("appError");
 
-                        showError(error.getString("details"),
+                        VNotification.showError(ApplicationConnection.this,
                                 error.getString("caption"),
                                 error.getString("message"),
+                                error.getString("details"),
                                 error.getString("url"));
 
                         setApplicationRunning(false);
@@ -2747,7 +2701,7 @@ public class ApplicationConnection implements HasHandlers {
     }
 
     // Redirect browser, null reloads current page
-    private static native void redirect(String url)
+    public static native void redirect(String url)
     /*-{
     	if (url) {
     		$wnd.location = url;
@@ -3366,25 +3320,6 @@ public class ApplicationConnection implements HasHandlers {
                 + getUIConnector().getActiveTheme();
     }
 
-    /**
-     * Listens for Notification hide event, and redirects. Used for system
-     * messages, such as session expired.
-     * 
-     */
-    private class NotificationRedirect implements VNotification.EventListener {
-        String url;
-
-        NotificationRedirect(String url) {
-            this.url = url;
-        }
-
-        @Override
-        public void notificationHidden(HideEvent event) {
-            redirect(url);
-        }
-
-    }
-
     /* Extended title handling */
 
     private final VTooltip tooltip;
diff --git a/client/src/com/vaadin/client/ui/VNotification.java b/client/src/com/vaadin/client/ui/VNotification.java
index 4f0c2eb625..2f68976471 100644
--- a/client/src/com/vaadin/client/ui/VNotification.java
+++ b/client/src/com/vaadin/client/ui/VNotification.java
@@ -65,6 +65,7 @@ public class VNotification extends VOverlay {
 
     public static final String CAPTION = "caption";
     public static final String DESCRIPTION = "description";
+    public static final String DETAILS = "details";
 
     /**
      * Position that is only accessible for assistive devices, invisible for
@@ -597,4 +598,85 @@ public class VNotification extends VOverlay {
             DOM.addEventPreview(notification);
         }
     }
+
+    /**
+     * Shows an error notification and redirects the user to the given URL when
+     * she clicks on the notification.
+     * 
+     * If both message and caption are null, redirects the user to the url
+     * immediately
+     * 
+     * @param connection
+     *            A reference to the ApplicationConnection
+     * @param caption
+     *            The caption for the error or null to exclude the caption
+     * @param message
+     *            The message for the error or null to exclude the message
+     * @param details
+     *            A details message or null to exclude the details
+     * @param url
+     *            A url to redirect to after the user clicks the error
+     *            notification
+     */
+    public static void showError(ApplicationConnection connection,
+            String caption, String message, String details, String url) {
+
+        StringBuilder html = new StringBuilder();
+        if (caption != null) {
+            html.append("<h1 class='");
+            html.append(getDependentStyle(connection, CAPTION));
+            html.append("'>");
+            html.append(caption);
+            html.append("</h1>");
+        }
+        if (message != null) {
+            html.append("<p class='");
+            html.append(getDependentStyle(connection, DESCRIPTION));
+            html.append("'>");
+            html.append(message);
+            html.append("</p>");
+        }
+
+        if (html.length() > 0) {
+
+            // Add error description
+            if (details != null) {
+                html.append("<p class='");
+                html.append(getDependentStyle(connection, DETAILS));
+                html.append("'>");
+                html.append("<i style=\"font-size:0.7em\">");
+                html.append(details);
+                html.append("</i></p>");
+            }
+
+            VNotification n = VNotification.createNotification(1000 * 60 * 45,
+                    connection.getUIConnector().getWidget());
+            n.addEventListener(new NotificationRedirect(url));
+            n.show(html.toString(), VNotification.CENTERED_TOP,
+                    VNotification.STYLE_SYSTEM);
+        } else {
+            ApplicationConnection.redirect(url);
+        }
+    }
+
+    /**
+     * Listens for Notification hide event, and redirects. Used for system
+     * messages, such as session expired.
+     * 
+     */
+    private static class NotificationRedirect implements
+            VNotification.EventListener {
+        String url;
+
+        NotificationRedirect(String url) {
+            this.url = url;
+        }
+
+        @Override
+        public void notificationHidden(HideEvent event) {
+            ApplicationConnection.redirect(url);
+        }
+
+    }
+
 }
diff --git a/uitest/src/com/vaadin/tests/application/CriticalNotificationsTestBase.java b/uitest/src/com/vaadin/tests/application/CriticalNotificationsTestBase.java
index 2f32fa8026..f3813fce50 100644
--- a/uitest/src/com/vaadin/tests/application/CriticalNotificationsTestBase.java
+++ b/uitest/src/com/vaadin/tests/application/CriticalNotificationsTestBase.java
@@ -18,6 +18,8 @@ package com.vaadin.tests.application;
 import org.junit.Test;
 
 import com.vaadin.testbench.elements.ButtonElement;
+import com.vaadin.testbench.elements.CheckBoxElement;
+import com.vaadin.testbench.elements.NotificationElement;
 import com.vaadin.tests.tb3.MultiBrowserThemeTest;
 
 public abstract class CriticalNotificationsTestBase extends
@@ -64,23 +66,13 @@ public abstract class CriticalNotificationsTestBase extends
     }
 
     @Test
-    public void authenticationError() throws Exception {
-        testCriticalNotification("Authentication error");
-    }
-
-    @Test
-    public void communicationError() throws Exception {
-        testCriticalNotification("Communication error");
-    }
-
-    @Test
     public void internalError() throws Exception {
         testCriticalNotification("Internal error");
     }
 
     @Test
-    public void cookiesDisabled() throws Exception {
-        testCriticalNotification("Cookies disabled");
+    public void internalErrorDetails() throws Exception {
+        testCriticalNotification("Internal error", true);
     }
 
     @Test
@@ -93,13 +85,27 @@ public abstract class CriticalNotificationsTestBase extends
         testCriticalNotification("Session expired");
     }
 
+    @Test
+    public void sessionExpiredDetails() throws Exception {
+        testCriticalNotification("Session expired", true);
+    }
+
     private void testCriticalNotification(String buttonCaption)
             throws Exception {
+        testCriticalNotification(buttonCaption, false);
+    }
+
+    private void testCriticalNotification(String buttonCaption,
+            boolean withDetails) throws Exception {
         openTestURL(); // "theme=" + getTheme());
+        if (withDetails) {
+            click($(CheckBoxElement.class).caption("Include details").first());
+        }
         $(ButtonElement.class).caption(buttonCaption).first().click();
         // Give the notification some time to animate
         sleep(1000);
-        compareScreen("notification");
+        compareScreen($(NotificationElement.class).first(),
+                "systemnotification");
     }
 
     @Override
diff --git a/uitest/src/com/vaadin/tests/tb3/AbstractTB3Test.java b/uitest/src/com/vaadin/tests/tb3/AbstractTB3Test.java
index 5b05d1d50a..edcd07ee89 100644
--- a/uitest/src/com/vaadin/tests/tb3/AbstractTB3Test.java
+++ b/uitest/src/com/vaadin/tests/tb3/AbstractTB3Test.java
@@ -114,6 +114,15 @@ public abstract class AbstractTB3Test extends ParallelTest {
      */
     private static final int BROWSER_TIMEOUT_IN_MS = 30 * 1000;
 
+    protected static DesiredCapabilities PHANTOMJS2() {
+        DesiredCapabilities phantomjs2 = new VaadinBrowserFactory().create(
+                Browser.PHANTOMJS, "2");
+        // Hack for the test cluster
+        phantomjs2
+                .setCapability("phantomjs.binary.path", "/usr/bin/phantomjs2");
+        return phantomjs2;
+    }
+
     private boolean debug = false;
 
     private boolean push = false;
@@ -956,7 +965,12 @@ public abstract class AbstractTB3Test extends ParallelTest {
     }
 
     protected void click(CheckBoxElement checkbox) {
-        checkbox.findElement(By.xpath("input")).click();
+        WebElement cb = checkbox.findElement(By.xpath("input"));
+        if (BrowserUtil.isChrome(getDesiredCapabilities())) {
+            testBenchElement(cb).click(0, 0);
+        } else {
+            cb.click();
+        }
     }
 
     protected boolean isLoadingIndicatorVisible() {
diff --git a/uitest/src/com/vaadin/tests/tb3/MultiBrowserThemeTest.java b/uitest/src/com/vaadin/tests/tb3/MultiBrowserThemeTest.java
index 546588947c..0964e6eb65 100644
--- a/uitest/src/com/vaadin/tests/tb3/MultiBrowserThemeTest.java
+++ b/uitest/src/com/vaadin/tests/tb3/MultiBrowserThemeTest.java
@@ -17,8 +17,11 @@ package com.vaadin.tests.tb3;
 
 import java.util.Arrays;
 import java.util.HashSet;
+import java.util.List;
 import java.util.Set;
 
+import org.openqa.selenium.remote.DesiredCapabilities;
+
 /**
  * Test which uses theme returned by {@link #getTheme()} for running the test
  */
@@ -27,9 +30,21 @@ public abstract class MultiBrowserThemeTest extends MultiBrowserTest {
     protected abstract String getTheme();
 
     @Override
+    protected boolean requireWindowFocusForIE() {
+        return true;
+    }
+
+    @Override
     protected void openTestURL(Class<?> uiClass, String... parameters) {
         Set<String> params = new HashSet<String>(Arrays.asList(parameters));
         params.add("theme=" + getTheme());
         super.openTestURL(uiClass, params.toArray(new String[params.size()]));
     }
+
+    @Override
+    public List<DesiredCapabilities> getBrowsersToTest() {
+        List<DesiredCapabilities> browsersToTest = getBrowsersExcludingPhantomJS();
+        browsersToTest.add(PHANTOMJS2());
+        return browsersToTest;
+    }
 }
diff --git a/uitest/src/com/vaadin/tests/tb3/SingleBrowserTestPhantomJS2.java b/uitest/src/com/vaadin/tests/tb3/SingleBrowserTestPhantomJS2.java
index 9f4e5938e7..7447a3c056 100644
--- a/uitest/src/com/vaadin/tests/tb3/SingleBrowserTestPhantomJS2.java
+++ b/uitest/src/com/vaadin/tests/tb3/SingleBrowserTestPhantomJS2.java
@@ -20,15 +20,10 @@ import java.util.List;
 
 import org.openqa.selenium.remote.DesiredCapabilities;
 
-import com.vaadin.testbench.parallel.Browser;
-
 public abstract class SingleBrowserTestPhantomJS2 extends
         PrivateTB3Configuration {
     @Override
     public List<DesiredCapabilities> getBrowsersToTest() {
-        DesiredCapabilities p2 = Browser.PHANTOMJS.getDesiredCapabilities();
-        p2.setVersion("2");
-        p2.setCapability("phantomjs.binary.path", "/usr/bin/phantomjs2");
-        return Collections.singletonList(p2);
+        return Collections.singletonList(PHANTOMJS2());
     }
 }
diff --git a/uitest/src/com/vaadin/tests/tb3/VaadinBrowserFactory.java b/uitest/src/com/vaadin/tests/tb3/VaadinBrowserFactory.java
index f55b434ee0..294fe440d0 100644
--- a/uitest/src/com/vaadin/tests/tb3/VaadinBrowserFactory.java
+++ b/uitest/src/com/vaadin/tests/tb3/VaadinBrowserFactory.java
@@ -55,6 +55,8 @@ public class VaadinBrowserFactory extends DefaultBrowserFactory {
 
     @Override
     public DesiredCapabilities create(Browser browser, String version) {
-        return create(browser);
+        DesiredCapabilities capabilities = create(browser);
+        capabilities.setVersion(version);
+        return capabilities;
     }
 }
diff --git a/uitest/src/com/vaadin/tests/themes/valo/ValoThemeUITest.java b/uitest/src/com/vaadin/tests/themes/valo/ValoThemeUITest.java
index 4ed7e33e13..3c28b1f9eb 100644
--- a/uitest/src/com/vaadin/tests/themes/valo/ValoThemeUITest.java
+++ b/uitest/src/com/vaadin/tests/themes/valo/ValoThemeUITest.java
@@ -24,7 +24,6 @@ import com.vaadin.testbench.elements.CheckBoxElement;
 import com.vaadin.testbench.elements.CssLayoutElement;
 import com.vaadin.testbench.elements.LabelElement;
 import com.vaadin.testbench.elements.TreeElement;
-import com.vaadin.testbench.parallel.BrowserUtil;
 import com.vaadin.tests.tb3.MultiBrowserTest;
 
 public class ValoThemeUITest extends MultiBrowserTest {
@@ -204,13 +203,7 @@ public class ValoThemeUITest extends MultiBrowserTest {
      * workaround for http://dev.vaadin.com/ticket/13763
      */
     private void check(String caption) {
-        WebElement cb = $(CheckBoxElement.class).caption(caption).first()
-                .findElement(By.xpath("input"));
-        if (BrowserUtil.isChrome(getDesiredCapabilities())) {
-            testBenchElement(cb).click(0, 0);
-        } else {
-            cb.click();
-        }
+        click($(CheckBoxElement.class).caption(caption).first());
     }
 
     @Test