Improved fix for #11614 (#11618)

* #11614: Added test which demonstrates that the fix is incomplete

* #11614: UI.doRefresh() only calls navigator when the navigation state actually changes

* Added javadoc to Navigator.getCurrentNavigationState()

* Minor: Removed accidental star imports

server/src/main/java/com/vaadin/navigator/Navigator.java

@@ -1272,4 +1272,17 @@ public class Navigator implements Serializable {
         ui.setNavigator(null);
     }
 
+    /**
+     * Returns the current navigation state for which the
+     * {@link #getCurrentView()} has been constructed. This may differ to
+     * {@link #getState()} in case the URL has been changed on the browser and
+     * the navigator wasn't yet given an opportunity to construct new view. The
+     * state is in the form of
+     * <code>current-view-name/optional/parameters</code>
+     *
+     * @return the current navigation state, may be {@code null}.
+     */
+    public String getCurrentNavigationState() {
+        return currentNavigationState;
+    }
 }

server/src/main/java/com/vaadin/ui/UI.java

@@ -31,6 +31,7 @@ import java.util.LinkedHashSet;
 import java.util.List;
 import java.util.Map;
 import java.util.Map.Entry;
+import java.util.Objects;
 import java.util.concurrent.Future;
 import java.util.logging.Level;
 import java.util.logging.Logger;
@@ -161,12 +162,6 @@ public abstract class UI extends AbstractSingleComponentContainer
     private LoadingIndicatorConfiguration loadingIndicatorConfiguration = new LoadingIndicatorConfigurationImpl(
             this);
 
-    /**
-     * Holder for old navigation state, needed in doRefresh in order not to call
-     * navigateTo too often
-     */
-    private String oldNavigationState;
-
     /**
      * Scroll Y position.
      */
@@ -880,12 +875,10 @@ public abstract class UI extends AbstractSingleComponentContainer
         // PushStateNavigation. Call navigateTo only if state have
         // truly changed
         Navigator navigator = getNavigator();
-        if (navigator != null) {
-            if (oldNavigationState == null) oldNavigationState = getNavigator().getState();
-            if (!navigator.getState().equals(oldNavigationState)) {
-                navigator.navigateTo(navigator.getState());
-                oldNavigationState = navigator.getState();
-            }
+        if (navigator != null
+                && !Objects.equals(navigator.getCurrentNavigationState(),
+                        navigator.getState())) {
+            navigator.navigateTo(navigator.getState());
         }
     }
 

uitest/src/main/java/com/vaadin/tests/components/ui/RefreshUI.java

@@ -5,7 +5,9 @@ import com.vaadin.navigator.Navigator;
 import com.vaadin.navigator.View;
 import com.vaadin.server.VaadinRequest;
 import com.vaadin.tests.components.AbstractTestUI;
+import com.vaadin.ui.Button;
 import com.vaadin.ui.Label;
+import com.vaadin.ui.UI;
 import com.vaadin.ui.VerticalLayout;
 
 @PreserveOnRefresh
@@ -15,7 +17,10 @@ public class RefreshUI extends AbstractTestUI {
     protected void setup(VaadinRequest request) {
         final Navigator navigator = new Navigator(this, this);
         navigator.addView("", MyView.class);
+        navigator.addView("otherview", OtherView.class);
         setNavigator(navigator);
+        MyView.instanceNumber = 0;
+        OtherView.instanceNumber = 0;
     }
 
     public static class MyView extends VerticalLayout implements View {
@@ -24,6 +29,18 @@ public class RefreshUI extends AbstractTestUI {
         public MyView() {
             instanceNumber++;
             addComponent(new Label("This is instance no " + instanceNumber));
+            addComponent(new Button("Navigate to otherview", e -> UI
+                    .getCurrent().getNavigator().navigateTo("otherview")));
+        }
+    }
+
+    public static class OtherView extends VerticalLayout implements View {
+        private static int instanceNumber = 0;
+
+        public OtherView() {
+            instanceNumber++;
+            addComponent(new Label(
+                    "This is otherview instance no " + instanceNumber));
         }
     }
 }

uitest/src/test/java/com/vaadin/tests/components/ui/RefreshUITest.java

@@ -1,5 +1,6 @@
 package com.vaadin.tests.components.ui;
 
+import com.vaadin.testbench.elements.ButtonElement;
 import org.junit.Test;
 
 import com.vaadin.testbench.elements.LabelElement;
@@ -17,9 +18,31 @@ public class RefreshUITest extends MultiBrowserTest {
                 $(LabelElement.class).first().getText());
 
         // Reload the page; UI.refresh should be invoked
-        openTestURL();
+        reloadPage();
         assertEquals("The Label content is not matching",
                 "This is instance no 1",
                 $(LabelElement.class).first().getText());
     }
+
+    @Test
+    public void testViewNotRecreatedAfterNavigation() {
+        openTestURL();
+        assertEquals("This is instance no 1",
+                $(LabelElement.class).first().getText());
+
+        // Reload the page; UI.refresh should be invoked
+        reloadPage();
+        assertEquals("This is instance no 1",
+                $(LabelElement.class).first().getText());
+
+        // now open the other view using the navigator
+        $(ButtonElement.class).first().click();
+        assertEquals("This is otherview instance no 1",
+                $(LabelElement.class).first().getText());
+
+        // Reload the page; UI.refresh should be invoked
+        reloadPage();
+        assertEquals("This is otherview instance no 1",
+                $(LabelElement.class).first().getText());
+    }
 }

uitest/src/test/java/com/vaadin/tests/tb3/AbstractTB3Test.java

@@ -31,10 +31,8 @@ import org.junit.rules.TestName;
 import org.junit.runner.Description;
 import org.junit.runner.RunWith;
 import org.openqa.selenium.By;
-import org.openqa.selenium.Dimension;
 import org.openqa.selenium.JavascriptExecutor;
 import org.openqa.selenium.NoSuchElementException;
-import org.openqa.selenium.Point;
 import org.openqa.selenium.WebDriver;
 import org.openqa.selenium.WebElement;
 import org.openqa.selenium.interactions.Actions;
@@ -254,6 +252,13 @@ public abstract class AbstractTB3Test extends ParallelTest {
         openTestURL(uiClass, new HashSet<>(Arrays.asList(parameters)));
     }
 
+    /**
+     * Reloads the current page, as if the user pressed F5.
+     */
+    protected void reloadPage() {
+        driver.navigate().refresh();
+    }
+
     private void openTestURL(Class<?> uiClass, Set<String> parameters) {
         String url = getTestURL(uiClass);