Skip to content

Commit a846fdc

Browse files
vaadin-botmcollovati
vaadin-bot
and
mcollovati
authored
fix: prevent duplicated history entry with vaadin-router (#21305) (CP: 24.7) (#21319)
* fix: prevent duplicated history entry with vaadin-router (#21305) When using legacy vaadin-router, browser history for client side navigation is updated by the router itself when navigation completes. However, in some situations the history gets updated also by the server side, causing two history entries for the same navigation and consequently a wrong behavior if pressing browser back button. This change: - Prevents updating history on the server side, unless the server is handling a postponed navigation for which vaadin-router does not update the history - Adds a test module for vaadin-router with the most common navigation tests - Re-enables several tests that are now passing, likely because previous changes fixed the related issues Fixes #21243 Fixes #19494 Fixes vaadin/hilla#335 * Fix module parent pom version --------- Co-authored-by: Marco Collovati <[email protected]>
1 parent 1458b16 commit a846fdc

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

41 files changed

+2623
-35
lines changed

‎flow-server/src/main/java/com/vaadin/flow/component/internal/JavaScriptNavigationStateRenderer.java

+10-5
Original file line numberDiff line numberDiff line change
@@ -106,7 +106,10 @@ protected boolean shouldPushHistoryState(NavigationEvent event) {
106106
return super.shouldPushHistoryState(event);
107107
}
108108
if (NavigationTrigger.CLIENT_SIDE.equals(event.getTrigger())
109-
|| NavigationTrigger.ROUTER_LINK.equals(event.getTrigger())) {
109+
&& isPostponedClientSideNavigation()) {
110+
// When navigation is postponed, the legacy router does not update
111+
// the history, so it should be done on the server side when
112+
// proceeding.
110113
return true;
111114
}
112115
return super.shouldPushHistoryState(event);
@@ -116,14 +119,16 @@ protected boolean shouldPushHistoryState(NavigationEvent event) {
116119
protected void pushHistoryState(NavigationEvent event) {
117120
super.pushHistoryState(event);
118121

119-
if (continueNavigationAction != null
120-
// We're trying to navigate to a client view.
121-
&& UI.ClientViewPlaceholder.class.isAssignableFrom(
122-
getNavigationState().getNavigationTarget())) {
122+
if (isPostponedClientSideNavigation()) {
123123
event.getUI().navigateToClient(
124124
event.getLocation().getPathWithQueryParameters());
125125
}
126+
}
126127

128+
private boolean isPostponedClientSideNavigation() {
129+
return continueNavigationAction != null
130+
&& UI.ClientViewPlaceholder.class.isAssignableFrom(
131+
getNavigationState().getNavigationTarget());
127132
}
128133

129134
private static Logger getLogger() {

‎flow-server/src/main/java/com/vaadin/flow/router/BeforeLeaveEvent.java

-13
Original file line numberDiff line numberDiff line change
@@ -83,19 +83,6 @@ public void proceed() {
8383
.executeJs("this.serverConnected($0)", false);
8484
}
8585

86-
// Change the trigger to programmatic as the url will be
87-
// updated/added by router when we continue for a Router_link.
88-
// If the server updates the url also we will get 2 history
89-
// changes instead of 1.
90-
if (NavigationTrigger.ROUTER_LINK.equals(event.getTrigger())
91-
&& !event.getUI().getSession().getService()
92-
.getDeploymentConfiguration()
93-
.isReactEnabled()) {
94-
event = new NavigationEvent(event.getSource(),
95-
event.getLocation(), event.getUI(),
96-
NavigationTrigger.PROGRAMMATIC);
97-
}
98-
9986
handler.handle(event);
10087
setReferences(null, null);
10188
}

‎flow-server/src/main/java/com/vaadin/flow/router/internal/AbstractNavigationStateRenderer.java

+9-8
Original file line numberDiff line numberDiff line change
@@ -400,25 +400,26 @@ protected List<Class<? extends RouterLayout>> getTargetParentLayouts(
400400
}
401401

402402
private void pushHistoryStateIfNeeded(NavigationEvent event, UI ui) {
403+
boolean reactEnabled = ui.getInternals().getSession().getService()
404+
.getDeploymentConfiguration().isReactEnabled();
405+
Location currentLocation = ui.getInternals().getActiveViewLocation();
406+
NavigationTrigger eventTrigger = event.getTrigger();
403407
if (event instanceof ErrorNavigationEvent errorEvent) {
404408
if (isRouterLinkNotFoundNavigationError(errorEvent)) {
405409
// #8544
406410
event.getState().ifPresent(s -> ui.getPage().executeJs(
407411
"this.scrollPositionHandlerAfterServerNavigation($0);",
408412
s));
409413
}
410-
} else if (NavigationTrigger.REFRESH != event.getTrigger()
414+
} else if (NavigationTrigger.REFRESH != eventTrigger
411415
&& !event.isForwardTo()
412-
&& (event.getUI().getInternals().getActiveViewLocation() == null
413-
|| !event.getLocation().getPathWithQueryParameters()
414-
.equals(event.getUI().getInternals()
415-
.getActiveViewLocation()
416-
.getPathWithQueryParameters()))) {
416+
&& (currentLocation == null || !event.getLocation()
417+
.getPathWithQueryParameters().equals(currentLocation
418+
.getPathWithQueryParameters()))) {
417419
if (shouldPushHistoryState(event)) {
418420
pushHistoryState(event);
419421
}
420-
} else if (ui.getInternals().getSession().getService()
421-
.getDeploymentConfiguration().isReactEnabled()) {
422+
} else if (reactEnabled) {
422423
if (shouldPushHistoryState(event)) {
423424
pushHistoryState(event);
424425
}

‎flow-tests/pom.xml

+2
Original file line numberDiff line numberDiff line change
@@ -330,6 +330,8 @@
330330
<module>test-custom-frontend-directory</module>
331331

332332
<module>vaadin-spring-tests</module>
333+
<module>test-vaadin-router</module>
334+
<module>test-vaadin-router/pom-production.xml</module>
333335
<module>test-react-router</module>
334336
<module>test-react-router/pom-production.xml</module>
335337
<module>test-react-adapter</module>

‎flow-tests/test-react-router/src/main/java/com/vaadin/flow/BackNavFirstView.java

+3-1
Original file line numberDiff line numberDiff line change
@@ -19,12 +19,14 @@
1919
import com.vaadin.flow.component.html.Div;
2020
import com.vaadin.flow.component.html.NativeButton;
2121
import com.vaadin.flow.router.Route;
22+
import com.vaadin.flow.router.RouterLink;
2223

2324
@Route("com.vaadin.flow.BackNavFirstView")
2425
public class BackNavFirstView extends Div {
2526

2627
public BackNavFirstView() {
27-
add(new NativeButton("Navigate", event -> getUI()
28+
add(new NativeButton("Server side navigation", event -> getUI()
2829
.ifPresent(ui -> ui.navigate(BackNavSecondView.class))));
30+
add(new RouterLink("Client side navigation", BackNavSecondView.class));
2931
}
3032
}

‎flow-tests/test-react-router/src/test/java/com/vaadin/flow/BackNavIT.java

+18-3
Original file line numberDiff line numberDiff line change
@@ -1,12 +1,16 @@
11
package com.vaadin.flow;
22

3+
import java.util.function.Supplier;
4+
35
import org.junit.Assert;
46
import org.junit.Test;
57
import org.openqa.selenium.TimeoutException;
68

9+
import com.vaadin.flow.component.html.testbench.AnchorElement;
710
import com.vaadin.flow.component.html.testbench.NativeButtonElement;
811
import com.vaadin.flow.component.html.testbench.SpanElement;
912
import com.vaadin.flow.testutil.ChromeBrowserTest;
13+
import com.vaadin.testbench.TestBenchElement;
1014

1115
public class BackNavIT extends ChromeBrowserTest {
1216

@@ -15,7 +19,18 @@ public class BackNavIT extends ChromeBrowserTest {
1519

1620
// Test for https://github.com/vaadin/flow/issues/19839
1721
@Test
18-
public void testBackButtonAfterHistoryStateChange() {
22+
public void serverSideNavigation_testBackButtonAfterHistoryStateChange() {
23+
navigateAndPressBack(() -> $(NativeButtonElement.class).first());
24+
}
25+
26+
// Test for https://github.com/vaadin/flow/issues/21243
27+
@Test
28+
public void routerLinkNavigation_testBackButtonAfterHistoryStateChange() {
29+
navigateAndPressBack(() -> $(AnchorElement.class).first());
30+
}
31+
32+
private void navigateAndPressBack(
33+
Supplier<TestBenchElement> navigationElement) {
1934
getDriver().get(getTestURL(getRootURL(), BACK_NAV_FIRST_VIEW, null));
2035

2136
try {
@@ -26,7 +41,7 @@ public void testBackButtonAfterHistoryStateChange() {
2641
+ BACK_NAV_FIRST_VIEW);
2742
}
2843

29-
$(NativeButtonElement.class).first().click();
44+
navigationElement.get().click();
3045

3146
try {
3247
waitUntil(arg -> driver.getCurrentUrl()
@@ -47,7 +62,7 @@ public void testBackButtonAfterHistoryStateChange() {
4762
}
4863

4964
Assert.assertTrue("Expected button is not available.",
50-
$(NativeButtonElement.class).first().isDisplayed());
65+
navigationElement.get().isDisplayed());
5166
}
5267

5368
@Test

‎flow-tests/test-root-context/src/test/java/com/vaadin/flow/uitest/ui/NavigationTriggerIT.java

-1
Original file line numberDiff line numberDiff line change
@@ -28,7 +28,6 @@
2828

2929
public class NavigationTriggerIT extends ChromeBrowserTest {
3030

31-
@Ignore("https://github.com/vaadin/flow/issues/19494")
3231
@Test
3332
public void testNavigationTriggers() {
3433
String url = getTestURL() + "/abc/";

‎flow-tests/test-root-context/src/test/java/com/vaadin/flow/uitest/ui/RouterLinkIT.java

+2-4
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,6 @@
1010
public class RouterLinkIT extends ChromeBrowserTest {
1111

1212
@Test
13-
@Ignore("Ignored because of issue in fusion : https://github.com/vaadin/flow/issues/7575")
1413
public void testRoutingLinks_insideServletMapping_updateLocation() {
1514
open();
1615

@@ -57,12 +56,11 @@ public void testRoutingLinks_externalLink_pageChanges() {
5756

5857
// Chrome changes url to whatever it can, removing www part, forcing
5958
// https.
60-
Assert.assertTrue("Invalid URL: " + currentUrl,
61-
currentUrl.equals("https://example.net/"));
59+
Assert.assertEquals("Invalid URL: " + currentUrl,
60+
"https://example.net/", currentUrl);
6261
}
6362

6463
@Test
65-
@Ignore("Ignored because of issue in fusion : https://github.com/vaadin/flow/issues/7575")
6664
public void testImageInsideRouterLink() {
6765
open();
6866

‎flow-tests/test-vaadin-router/pom-production.xml

+66
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,66 @@
1+
<?xml version="1.0" encoding="UTF-8"?>
2+
<project xmlns="http://maven.apache.org/POM/4.0.0" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/xsd/maven-4.0.0.xsd">
3+
<modelVersion>4.0.0</modelVersion>
4+
<parent>
5+
<artifactId>flow-tests</artifactId>
6+
<groupId>com.vaadin</groupId>
7+
<version>24.7-SNAPSHOT</version>
8+
</parent>
9+
<artifactId>flow-test-vaadin-router-prod</artifactId>
10+
<name>Flow tests for routing using vaadin-router in production mode</name>
11+
12+
<packaging>war</packaging>
13+
<properties>
14+
<maven.deploy.skip>true</maven.deploy.skip>
15+
<surefire.reportNameSuffix>production</surefire.reportNameSuffix>
16+
</properties>
17+
18+
<dependencies>
19+
<dependency>
20+
<groupId>com.vaadin</groupId>
21+
<artifactId>flow-test-resources</artifactId>
22+
<version>${project.version}</version>
23+
</dependency>
24+
<dependency>
25+
<groupId>com.vaadin</groupId>
26+
<artifactId>flow-html-components-testbench</artifactId>
27+
<version>${project.version}</version>
28+
<scope>test</scope>
29+
</dependency>
30+
31+
<dependency>
32+
<groupId>com.vaadin</groupId>
33+
<artifactId>flow-test-common</artifactId>
34+
<version>${project.version}</version>
35+
</dependency>
36+
37+
</dependencies>
38+
39+
<build>
40+
<plugins>
41+
<plugin>
42+
<groupId>com.vaadin</groupId>
43+
<artifactId>flow-maven-plugin</artifactId>
44+
<executions>
45+
<execution>
46+
<goals>
47+
<goal>prepare-frontend</goal>
48+
<goal>build-frontend</goal>
49+
</goals>
50+
<phase>compile</phase>
51+
</execution>
52+
</executions>
53+
<configuration>
54+
<cleanFrontendFiles>false</cleanFrontendFiles>
55+
<reactEnable>false</reactEnable>
56+
</configuration>
57+
</plugin>
58+
<!-- This module is mapped to default web context -->
59+
<plugin>
60+
<groupId>org.eclipse.jetty.ee10</groupId>
61+
<artifactId>jetty-ee10-maven-plugin</artifactId>
62+
</plugin>
63+
</plugins>
64+
</build>
65+
66+
</project>

‎flow-tests/test-vaadin-router/pom.xml

+68
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,68 @@
1+
<?xml version="1.0" encoding="UTF-8"?>
2+
<project xmlns="http://maven.apache.org/POM/4.0.0" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/xsd/maven-4.0.0.xsd">
3+
<modelVersion>4.0.0</modelVersion>
4+
<parent>
5+
<artifactId>flow-tests</artifactId>
6+
<groupId>com.vaadin</groupId>
7+
<version>24.7-SNAPSHOT</version>
8+
</parent>
9+
<artifactId>flow-test-vaadin-router</artifactId>
10+
<name>Flow tests for routing using vaadin-router</name>
11+
12+
<packaging>war</packaging>
13+
<properties>
14+
<maven.deploy.skip>true</maven.deploy.skip>
15+
</properties>
16+
17+
<dependencies>
18+
<dependency>
19+
<groupId>com.vaadin</groupId>
20+
<artifactId>flow-test-resources</artifactId>
21+
<version>${project.version}</version>
22+
</dependency>
23+
<dependency>
24+
<groupId>com.vaadin</groupId>
25+
<artifactId>vaadin-dev-server</artifactId>
26+
<version>${project.version}</version>
27+
</dependency>
28+
<dependency>
29+
<groupId>com.vaadin</groupId>
30+
<artifactId>flow-html-components-testbench</artifactId>
31+
<version>${project.version}</version>
32+
<scope>test</scope>
33+
</dependency>
34+
35+
<dependency>
36+
<groupId>com.vaadin</groupId>
37+
<artifactId>flow-test-common</artifactId>
38+
<version>${project.version}</version>
39+
</dependency>
40+
41+
</dependencies>
42+
43+
<build>
44+
<plugins>
45+
<plugin>
46+
<groupId>com.vaadin</groupId>
47+
<artifactId>flow-maven-plugin</artifactId>
48+
<configuration>
49+
<reactEnable>false</reactEnable>
50+
</configuration>
51+
<executions>
52+
<execution>
53+
<goals>
54+
<goal>prepare-frontend</goal>
55+
</goals>
56+
<phase>compile</phase>
57+
</execution>
58+
</executions>
59+
</plugin>
60+
<!-- This module is mapped to default web context -->
61+
<plugin>
62+
<groupId>org.eclipse.jetty.ee10</groupId>
63+
<artifactId>jetty-ee10-maven-plugin</artifactId>
64+
</plugin>
65+
</plugins>
66+
</build>
67+
68+
</project>

‎flow-tests/test-vaadin-router/src/main/java/com/vaadin/flow/AddQueryParamView.java

+51
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,51 @@
1+
/*
2+
* Copyright 2000-2024 Vaadin Ltd.
3+
*
4+
* Licensed under the Apache License, Version 2.0 (the "License"); you may not
5+
* use this file except in compliance with the License. You may obtain a copy of
6+
* the License at
7+
*
8+
* http://www.apache.org/licenses/LICENSE-2.0
9+
*
10+
* Unless required by applicable law or agreed to in writing, software
11+
* distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
12+
* WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
13+
* License for the specific language governing permissions and limitations under
14+
* the License.
15+
*/
16+
17+
package com.vaadin.flow;
18+
19+
import com.vaadin.flow.component.UI;
20+
import com.vaadin.flow.component.html.Div;
21+
import com.vaadin.flow.component.html.NativeButton;
22+
import com.vaadin.flow.component.page.Page;
23+
import com.vaadin.flow.router.BeforeEnterEvent;
24+
import com.vaadin.flow.router.BeforeEnterObserver;
25+
import com.vaadin.flow.router.Route;
26+
27+
@Route("com.vaadin.flow.AddQueryParamView")
28+
public class AddQueryParamView extends Div {
29+
30+
public static final String PARAM_BUTTON_ID = "setParameter";
31+
public static final String QUERY_ID = "query";
32+
33+
public AddQueryParamView() {
34+
NativeButton button = new NativeButton("Add URL Parameter", e -> {
35+
updateUrlRequestParameter("test", "HELLO!");
36+
});
37+
button.setId(PARAM_BUTTON_ID);
38+
add(button);
39+
}
40+
41+
public void updateUrlRequestParameter(String key, String value) {
42+
Page page = UI.getCurrent().getPage();
43+
page.fetchCurrentURL(url -> {
44+
String newLocation = url + "?" + key + "=" + value;
45+
page.getHistory().replaceState(null, newLocation);
46+
Div div = new Div(newLocation);
47+
div.setId(QUERY_ID);
48+
add(div);
49+
});
50+
}
51+
}

0 commit comments

Comments
 (0)