riganti / dotvvm

Open source MVVM framework for Web Apps
https://www.dotvvm.com
Apache License 2.0
743 stars 97 forks source link

Chrome doesn't remember viewmodel after history change #685

Open GerardSmit opened 5 years ago

GerardSmit commented 5 years ago

When you leave the page by going back and then going forward, the viewmodel is reset to its initial state.

Example (GIF)

This is because Chrome doesn't remember the value of the <input type="hidden"> across history states. When we change the following in BodyResourceLinks.cs:

             // render the serialized viewmodel
             var serializedViewModel = ((DotvvmRequestContext) context).GetSerializedViewModel();
-            writer.AddAttribute("type", "hidden");
+            writer.AddAttribute("type", "text");
+            writer.AddAttribute("style", "display:none!important;");
             writer.AddAttribute("id", "__dot_viewmodel_root");
             writer.AddAttribute("value", serializedViewModel);
             writer.RenderSelfClosingTag("input");

then Chrome will remember the viewmodel:

Example (GIF)

When I searched for this issue, I found an issue from 2010 with the same problem (here) in WebForms. So I don't think this behavior won't change in Chrome if it didn't change in 9 years.

What would be the best solution here? Should we change the viewmodel input to an text-field and hide it (with maybe an setting in DotvvmConfiguration for compability)?


Browser: Google Chrome Version: 73.0.3683.103 (Official Build) (64-bit) Dotvvm build commit: https://github.com/riganti/dotvvm/commit/dccddcf2bc8f2688af454e3831f3324801371e8d Hosting: ASP.Net Core

GerardSmit commented 5 years ago

I've created an PR (#686) with the change I made in the first message to fix the persisted viewmodel.

exyi commented 5 years ago

Hmm, that's strange, we actually introduced that hidden field back in 2015 for exactly this reason - https://github.com/riganti/dotvvm/issues/37. However, I can replicate this issue, it does not work in Chrome (while it works in FF). @tomasherceg You are using Chrome, aren't you? Do you know if it actually ever worked?

tomasherceg commented 5 years ago

I am sure it worked in Chrome at the time we implemented this. I wonder if there is any other input type that would persist the value - input[type=text] can be a problem for many scripts and libraries.

GerardSmit commented 5 years ago

I've done some research and on this page they suggest to use the History API.

The history API is available in most browser (https://caniuse.com/#search=history) with the exception of <IE9 and Opera Mini.

Maybe this is an good alternative for the input[type=text].


Edit: However the window.onpopstate is after the DOMContentLoaded event, which would break libraries that depends DotVVM to be loaded on window.onloaded.

This is a really complicated issue... 😕

tomasherceg commented 5 years ago

I think we can use the History API. The only supported version of IE we support is 11.

Actually, I can imagine much more problems with changing the field to input[type=text] than with using window.onpopstate. Using input[type=text] can break many selectors (including dozens of our UI tests which rely on the fact there is only one text field in the page). Also, I have seen a couple of libraries that search for all form fields and do something with them (animations, validations or focus handling). It can be tricky to convince the library to ignore this field. I am not so worried about the fact that window.onpopstate comes after window.onload. Although it is unpleasant that DotVVM won't be able to detect whether the event onpopstate will be fired or not, so it has to load the default viewmodel which gets updated later, still - most of the scripts that rely on DotVVM are Knockout binding handlers which must be registered as early as possible and they expect that the viewmodel will be changed at any time, so it shouldn't be a big issue. The only problem I see is that is can slow down the loading of the page as the default viewmodel will be prepared and then immediately updated with the last state. I think that we should keep the hidden field and apply the new state in onpopstate only when if it differs from the value in the hidden field.

GerardSmit commented 5 years ago

Alright. I'll try to replace the input[type=hidden] with the History API this weekend. I'll close the PR #686 and make a new PR with the History API.

I also found out that there is a history.state property which returns the current state without having to wait for window.onpopstate 🎉.

GerardSmit commented 5 years ago

Done. I've created a new PR (#687) that uses the History API. Everything works fine in Edge, Chrome, Firefox and Internet Explorer. Since I don't have an Apple device at home I cannot test if the implementation works on Safari, maybe someone can test this?

Also I don't have checked if SPA mode works with the new implementation since this also works with the History API. It should work fine since I combine the old state and the new state, but it's better that someone can verify if this is still working.