Refine handling of null and empty URI fragments (#12207)

Change-Id: Ie133694b010a586c6336e9b04be7bcd94d2525e9

client/src/com/vaadin/client/ui/ui/UIConnector.java

@@ -40,6 +40,7 @@ import com.google.gwt.user.client.Event;
 import com.google.gwt.user.client.History;
 import com.google.gwt.user.client.Timer;
 import com.google.gwt.user.client.Window;
+import com.google.gwt.user.client.Window.Location;
 import com.google.gwt.user.client.ui.RootPanel;
 import com.google.gwt.user.client.ui.Widget;
 import com.google.web.bindery.event.shared.HandlerRegistration;
@@ -352,14 +353,30 @@ public class UIConnector extends AbstractSingleComponentContainerConnector
         if (uidl.hasAttribute(UIConstants.LOCATION_VARIABLE)) {
             String location = uidl
                     .getStringAttribute(UIConstants.LOCATION_VARIABLE);
+            String newFragment;
+
             int fragmentIndex = location.indexOf('#');
             if (fragmentIndex >= 0) {
                 // Decode fragment to avoid double encoding (#10769)
-                getWidget().currentFragment = URL.decodePathSegment(location
+                newFragment = URL.decodePathSegment(location
                         .substring(fragmentIndex + 1));
+
+                if (newFragment.isEmpty()
+                        && Location.getHref().indexOf('#') == -1) {
+                    // Ensure there is a trailing # even though History and
+                    // Location.getHash() treat null and "" the same way.
+                    Location.assign(Location.getHref() + "#");
+                }
+            } else {
+                // No fragment in server-side location, but can't completely
+                // remove the browser fragment since that would reload the page
+                newFragment = "";
             }
-            if (!getWidget().currentFragment.equals(History.getToken())) {
-                History.newItem(getWidget().currentFragment, true);
+
+            getWidget().currentFragment = newFragment;
+
+            if (!newFragment.equals(History.getToken())) {
+                History.newItem(newFragment, true);
             }
         }
 

server/src/com/vaadin/server/Page.java

@@ -552,10 +552,11 @@ public class Page implements Serializable {
      * The fragment is the optional last component of a URI, prefixed with a
      * hash sign ("#").
      * <p>
-     * Passing <code>null</code> as <code>newFragment</code> clears the fragment
-     * (no "#" in the URI); passing an empty string sets an empty fragment (a
-     * trailing "#" in the URI.) This is consistent with the semantics of
-     * {@link java.net.URI}.
+     * Passing an empty string as <code>newFragment</code> sets an empty
+     * fragment (a trailing "#" in the URI.) Passing <code>null</code> if there
+     * is already a non-null fragment will leave a trailing # in the URI since
+     * removing it would cause the browser to reload the page. This is not fully
+     * consistent with the semantics of {@link java.net.URI}.
      * 
      * @param newUriFragment
      *            The new fragment.
@@ -570,6 +571,11 @@ public class Page implements Serializable {
      */
     public void setUriFragment(String newUriFragment, boolean fireEvents) {
         String oldUriFragment = location.getFragment();
+        if (newUriFragment == null && getUriFragment() != null) {
+            // Can't completely remove the fragment once it has been set, will
+            // instead set it to the empty string
+            newUriFragment = "";
+        }
         if (newUriFragment == oldUriFragment
                 || (newUriFragment != null && newUriFragment
                         .equals(oldUriFragment))) {

uitest/src/com/vaadin/tests/components/ui/UriFragmentTest.html

@@ -71,7 +71,66 @@
 	<td>vaadin=runcomvaadintestscomponentsuiUriFragmentTest::/VVerticalLayout[0]/ChildComponentContainer[1]/VVerticalLayout[0]/ChildComponentContainer[0]/VLabel[0]</td>
 	<td>Current URI fragment: test</td>
 </tr>
-
+<!--Open other URL in between to ensure the page is loaded again (testbench doesn't like opening a URI that only changes the fragment)-->
+<tr>
+	<td>open</td>
+	<td>/run/</td>
+	<td></td>
+</tr>
+<tr>
+	<td>open</td>
+	<td>/run/com.vaadin.tests.components.ui.UriFragmentTest?restartApplication</td>
+	<td></td>
+</tr>
+<!--Empty initial fragment-->
+<tr>
+	<td>assertText</td>
+	<td>vaadin=runcomvaadintestscomponentsuiUriFragmentTest::/VVerticalLayout[0]/Slot[1]/VVerticalLayout[0]/Slot[0]/VLabel[0]</td>
+	<td>No URI fragment set</td>
+</tr>
+<tr>
+	<td>click</td>
+	<td>vaadin=runcomvaadintestscomponentsuiUriFragmentTest::/VVerticalLayout[0]/Slot[1]/VVerticalLayout[0]/Slot[3]/VButton[0]/domChild[0]/domChild[0]</td>
+	<td></td>
+</tr>
+<!--Still no # after setting to null-->
+<tr>
+	<td>assertText</td>
+	<td>vaadin=runcomvaadintestscomponentsuiUriFragmentTest::/VVerticalLayout[0]/Slot[1]/VVerticalLayout[0]/Slot[0]/VLabel[0]</td>
+	<td>No URI fragment set</td>
+</tr>
+<tr>
+	<td>click</td>
+	<td>vaadin=runcomvaadintestscomponentsuiUriFragmentTest::/VVerticalLayout[0]/Slot[1]/VVerticalLayout[0]/Slot[2]/VButton[0]/domChild[0]/domChild[0]</td>
+	<td></td>
+</tr>
+<!--Empty # is added when setting to ""-->
+<tr>
+	<td>assertText</td>
+	<td>vaadin=runcomvaadintestscomponentsuiUriFragmentTest::/VVerticalLayout[0]/Slot[1]/VVerticalLayout[0]/Slot[0]/VLabel[0]</td>
+	<td>Current URI fragment:</td>
+</tr>
+<tr>
+	<td>click</td>
+	<td>vaadin=runcomvaadintestscomponentsuiUriFragmentTest::/VVerticalLayout[0]/Slot[1]/VVerticalLayout[0]/Slot[1]/VButton[0]/domChild[0]/domChild[0]</td>
+	<td></td>
+</tr>
+<tr>
+	<td>assertText</td>
+	<td>vaadin=runcomvaadintestscomponentsuiUriFragmentTest::/VVerticalLayout[0]/Slot[1]/VVerticalLayout[0]/Slot[0]/VLabel[0]</td>
+	<td>Current URI fragment: test</td>
+</tr>
+<tr>
+	<td>click</td>
+	<td>vaadin=runcomvaadintestscomponentsuiUriFragmentTest::/VVerticalLayout[0]/Slot[1]/VVerticalLayout[0]/Slot[3]/VButton[0]/domChild[0]/domChild[0]</td>
+	<td></td>
+</tr>
+<!--Setting to null when there is a fragment actually sets it to #-->
+<tr>
+	<td>assertText</td>
+	<td>vaadin=runcomvaadintestscomponentsuiUriFragmentTest::/VVerticalLayout[0]/Slot[1]/VVerticalLayout[0]/Slot[0]/VLabel[0]</td>
+	<td>Current URI fragment:</td>
+</tr>
 </tbody></table>
 </body>
 </html>

uitest/src/com/vaadin/tests/components/ui/UriFragmentTest.java

@@ -29,6 +29,21 @@ public class UriFragmentTest extends AbstractTestUI {
                         getPage().setUriFragment("test");
                     }
                 }));
+
+        addComponent(new Button("Navigate to #", new Button.ClickListener() {
+            @Override
+            public void buttonClick(ClickEvent event) {
+                getPage().setUriFragment("");
+            }
+        }));
+
+        addComponent(new Button("setUriFragment(null)",
+                new Button.ClickListener() {
+                    @Override
+                    public void buttonClick(ClickEvent event) {
+                        getPage().setUriFragment(null);
+                    }
+                }));
     }
 
     private void updateLabel() {