Support localStorage for storing settings

[Jaifroid]

This is a possible solution for #612 in contexts where the localStorage API is supported. It works in Firefox running from the file:// URL. I haven't tested in extension context. It should also enable Chromium to store settings from file:// context when the flag allowing file access is used.

This solution has been implemented in Kiwix JS Windows for a year or so, to solve some other issue that I forget, so I simply copied it over here to test.

How can I test this in an extension on Firefox?

[Jaifroid]

Note on how this works:

cookies.js runs on app launch and returns a cookies object with various methods. I have added code at the head of this file to test a) whether cookie support is available in the context and b) whether localStorage support is available. If localStorage is available, it is preferred. If it is not available, it falls back to using document.cookie.

The test only runs once on app launch, not every time a cookie method is used (you can see this in console.log).

The reason we have to prefer localStorage to solve the reported issue is that in file:// context, document.cookie appears to work and receive values, but does not remember them between tab launches. Therefore the provided test does not provide useful information. In extension context, the test will probably correctly show that cookie support is disabled, as per the console.log test I demonstrated in #612.

EDIT: If a version of this PR is accepted, we must remember to update the Privacy Policy to indicate use of localStorage.

[mossroy]

In https://developer.mozilla.org/en-US/docs/Mozilla/Add-ons/WebExtensions/API/storage, Mozilla says :

it is recommended that you don't use Window.localStorage in the extension code to store extension-related data. Firefox will clear data stored by extensions using the localStorage API in various scenarios where users clear their browsing history and data for privacy reasons, while data saved using the storage.local API will be correctly persisted in these scenarios

They suggest to use this https://developer.mozilla.org/en-US/docs/Mozilla/Add-ons/WebExtensions/API/storage API instead, that should work in other browser extensions too.

We have to keep in mind that, if we use these other storage APIs AND set them as default storage in some circumstances, we'll have to deal with data migration (move the existing cookie preferences to the other storage, then remove the cookies), and to deal with cases where some preferences would be set in different places (and decide which one we should use). Keeping cookies as the default preference storage would avoid these difficulties

[Jaifroid]

Firefox will clear data stored by extensions using the localStorage API in various scenarios where users clear their browsing history and data for privacy reasons, while data saved using the storage.local API will be correctly persisted in these scenarios

That's exactly the same scenario as cookies. If a user clears browsing data cookies will be cleared. In fact I think it's an advantage that localStorage would be cleared in the same circumstances, as it allows the user to "reset" all settings to default (say, where a particular setting is causing an issue due to an API change).

[Jaifroid]

I think we should respect a user's desire to clear their data for privacy reasons, and shouldn't try to circumvent that by using a more persistent API.

[Jaifroid]

PS Can easily add code to migrate settings to localStorage if supported and document.cookie is accessible. On the other hand, a user might not be too annoyed by having to choose two or three settings again in Config. Still, it's a few lines of code to migrate either way. Would you like me to add?

[mossroy]

OK, you're right. Our extension is more like an offline website with some preferences than an extension where we would need to keep settings even when the user clears the history/private data.
So localStorage could be OK.
And yes, it might not be worth adding too much code to migrate so few settings.
If we're lucky this would be in the same released version as #196

[Jaifroid]

Yes, I agree we should do #196 straight after. #196 would need some migration code of its own, because we will need to update the stored setting to Service Worker on a one-time basis, and let users know we've done it.

[Jaifroid]

Tested and working on Firefox OS. It uses localStorage.
Not working on IE11 from file:// protocol. It thinks localStorage is available, but attempting to write to it causes an exception. Therefore the test needs to attempt to write to localStorage to be sure it's supported.

[Jaifroid]

• Now fixed for IE11. It uses cookies from file:// protocol, and localStorage when running from localhost.
• Works in Chromium (Edge). It uses localStorage when running from localhost.
• Works in Edge legacy. It uses cookies from file:// protocol, and localStorage when running from localhost.

[Jaifroid]

Tested and working in Firefox 73.0.1 in an Extension installed using tools in about:debugging.

Almacenamiento local is Spanish for Local Storage. You can see the keys stored bottom right.

[Jaifroid]

I've just realized that if localStorage is preferred but it's not supported, there is no point checking for cookie support, because we don't have any other option, we have to fall back to something, and the most universal is likely to be cookie. Therefore logic dictates that we remove the test for cookie support and just assume it. If a cookie test were to fail, there'd be no other storage type to use anyway. In that hypothetical situation (no localStorage support and no cookie support), defaults will be used and the user will have to re-do settings between sessions.

[mossroy]

The implementation looks right to me. But I've added a few remarks

[Jaifroid]

This is ready for testing. I have added console log messages so we can see what exactly is migrated (it shows just the keys that are migrated).

In the contexts I've tested quickly, the migration is one-off. However, in FFOS simulator, the following key is recreated in document.cookie every time the app starts:

• permanent/fake-sdcard/wikipedia_en_medicine_novid_2018-08.zim

This causes the migration to run on every startup of the app. Is this just an artefact of the simulator?

[Jaifroid]

Tests

• Firefox 75.0 from localhost: uses localStorage and migrates settings once only
• Edge Chromium from localhost: uses localStorage and migrates settings once only
• Firefox ESR 52.5 from localhost uses localStorage and migrates settings once only
• Firefox OS simulator in Firefox ESR uses localStorage but migrates settings on each app launch due to fake-sdcard appearing in document.cookie even after it is expired
• Firefox ESR from file:// protocol uses localStorage
• Edge Legacy from file:// protocol uses cookies (and does not migrate settings obviously)
• Edge Legacy from localhost uses localStorage and migrates settings once only
• IE11 from file:// protocol uses cookies
• IE11 from localhost uses localStorage and migrates settings once only
• Firefox extension from about:debugging uses localStorage and does not migrate settings because cookies are always blank on first load of extension

Methodology: first load the app using master branch to ensure document.cookie is populated. Then switch to Use-localStorage branch and reload app with console open to see the migration. Reload again to see if migration is repeated.

[Jaifroid]

Now also tested in an Edge Chromium extension using developer mode. It uses localStorage and does not migrate settings because the extension loads with document.cookie empty. Settings are remembered between tab loads.

I think this is ready for your testing and review, @mossroy .

[mossroy]

Just starting the review : data migration worked perfectly on my Firefox OS device :

Migrating Settings Store from cookies to localStorage... settingsStore.js:126:5

• lastContentInjectionMode settingsStore.js:133:7
• listOfArchives settingsStore.js:133:7
• lastSelectedArchive settingsStore.js:133:7
• imageDisplayMode settingsStore.js:133:7
• showUIAnimations settingsStore.js:133:7
• appTheme settingsStore.js:133:7
• hideActiveContentWarning settingsStore.js:133:7
  Migration done. settingsStore.js:135:5

and on Chromium 80 (desktop) too.

I noticed on Chromium that there were some other settings in the localStorage :

The volume is the default volume of some videos (like the ones of dirtybiology).

So maybe we should add a prefix in our localStorage keys, to clearly distinguish them with the ones that might come from the ZIM contents, and avoid conflicts with them. It seems to be what MediaWiki does BTW. This prefix could be a constant, that we automatically add in the getItem/setItem/etc functions

[Jaifroid]

So maybe we should add a prefix in our localStorage keys, to clearly distinguish them with the ones that might come from the ZIM contents, and avoid conflicts with them.

Yes, I was thinking about that, as I'd seen some non-Kiwix settings being migrated too. I guess we can add a kiwixjs- prefix. But it doesn't solve the one-time migration problem, because the migration code won't find any keys prefixed with kiwixjs-, so won't migrate anything. The only solution to that is to add a kiwixjs-migrated key which would tell us that the migration has been run already, so we do not need to look for any non-prefixed keys. Note that we will then end up with spurious keys in localStorage like kiwixjs-volume (to use your example), but at least it wouldn't keep running the migration every time it finds volume in the cookies. But if we do have a kiwixjs-migrated key, is there any point in having the prefix? We would simply not run the migration if it's already been done.

The only way I can think of to be absolutely sure we don't migrate spurious keys would be to supply a list of known keys to migrate...

[mossroy]

As you seem to intend at the end of your last comment, we know which cookies kiwix-js uses, so we could look for and migrate only these specific cookies, and do not touch the other ones. No need to add kiwixjs-migrated setting in this case.

[Jaifroid]

OK, I've added the list of keys to migrate. I could only find seven in our app by searching for settingsStore.setItem. There is one in your list above from your FFOS device that is from a branch that has not been merged (manual image extraction / progressive image display), so I did not include that.

[mossroy]

The code looks good to me.
Apart from your own review comments, what is missing is the prefix added to setting keys (only in localStorage mode)

[Jaifroid]

Ah, OK, I had misunderstood about the prefix (I thought we no longer needed it as we were specifying which keys to migrate). But I agree that it makes sense just in case we get key collisions with code inside the ZIMs that might also use localStorage. Will do.

[Jaifroid]

Done, and tested so far only on Firefox 75. Methodology for testing:

• Switch to master
• Load Kiwix JS from localhost
• Go to Storage settings in dev console and delete the current localStorage entirely (e.g. right-click on http://localhost under Local Storage and delete), because it will probably have old key names already in it
• Add some cookie settings by toggling various settings in the app and loading a ZIM
• Switch to Use-localStorage branch
• With console log open, fully reload the app
• Finally, open the Storage tab in devtools and check what is in Local Storage. Experiment by toggling some settings in the app, and at least in Firefox, the values should change.

We'll need to test this in several different browsers/contexts before merging in case of any incompatibilities with the format of key names (I'm thinking of the hyphen and/or length -- shouldn't be a problem, but you never know).

[mossroy]

Successfully tested on my Firefox OS device, Chromium 80 and Firefox 75
Did not test inside extensions but you already did.

You might introduce a constant variable in settingsStore.js for kiwixjs- string, to avoid repeating it many times. https://developer.mozilla.org/fr/docs/Web/JavaScript/Reference/Instructions/const seems to be handled by all the browsers we support

[Jaifroid]

Interesting. I always associate const and let with ES6, and thought IE11 therefore didn't support them (like it doesn't support arrow notation). Interesting to see we could have been using some of the features. We actually have quite a few constants that could use this, though I guess the effect on memory usage/speed is tiny, it's more for readability.

Happy to experiment with that.

[Jaifroid]

I've implemented that and have tested on Firefox 75 with file:// protocol, and on IE11 from localhost. It works fine on the latter, i.e. const works as expected (which is good to know).

I'll do tests on the other platforms before squashing/merging (but not today).

EDIT 14/4: Tested and working on FFOS simulator.
EDIT 14/4: Tested and working on Chromium extension.
Tested and working on Firefox temporary extension.

[Jaifroid]

Summary of final tests conducted:

FFOS simulator: working
Firefox 75 from file://: working
IE11 from localhost: working
Edge Chromium developer extension: working
Firefox temporary extension: working

So I'll squash and merge.