From b541e8e4e9346971f33025135619120166aac425 Mon Sep 17 00:00:00 2001 From: Artur Signell Date: Thu, 25 Jun 2015 00:06:17 +0300 Subject: [PATCH] Fix incorrect system notifications with details styling (#18340) Change-Id: Ia0d36147eb4ed9f170123771ac2674df584e6a4b --- .../base/notification/notification.scss | 3 +- .../themes/valo/components/_notification.scss | 9 +- .../vaadin/client/ApplicationConnection.java | 83 ++----------------- .../com/vaadin/client/ui/VNotification.java | 82 ++++++++++++++++++ .../CriticalNotificationsTestBase.java | 32 ++++--- .../com/vaadin/tests/tb3/AbstractTB3Test.java | 16 +++- .../tests/tb3/MultiBrowserThemeTest.java | 15 ++++ .../tb3/SingleBrowserTestPhantomJS2.java | 7 +- .../tests/tb3/VaadinBrowserFactory.java | 4 +- .../tests/themes/valo/ValoThemeUITest.java | 9 +- 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("

"); - html.append(caption); - html.append("

"); - } - if (message != null) { - html.append("

"); - html.append(message); - html.append("

"); - } - - if (html.length() > 0) { - - // Add error description - if (details != null) { - html.append("

"); - html.append(details); - html.append("

"); - } - - 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("

"); + html.append(caption); + html.append("

"); + } + if (message != null) { + html.append("

"); + html.append(message); + html.append("

"); + } + + if (html.length() > 0) { + + // Add error description + if (details != null) { + html.append("

"); + html.append(""); + html.append(details); + html.append("

"); + } + + 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 @@ -63,24 +65,14 @@ 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 */ @@ -26,10 +29,22 @@ 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 params = new HashSet(Arrays.asList(parameters)); params.add("theme=" + getTheme()); super.openTestURL(uiClass, params.toArray(new String[params.size()])); } + + @Override + public List getBrowsersToTest() { + List 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 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 -- 2.39.5