Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Switch to sw mode by default #903

Merged
merged 36 commits into from
Oct 24, 2022
Merged

Switch to sw mode by default #903

merged 36 commits into from
Oct 24, 2022

Conversation

Jaifroid
Copy link
Member

@Jaifroid Jaifroid commented Oct 16, 2022

This branch contains work in progress that was commenced by @mossroy. The linked issue is #196.

Initial list of points to cover:

  • new user/installation with SW supported: use SW (but warn about the switch to PWA for Firefox)
  • new user/installation but SW unsupported: fallback to jQuery mode with a one-time warning
  • existing installation with SW mode selected: keep SW
  • existing installation with jQuery mode and SW supported: we should switch to SW mode with a one-time information (but warn about the switch to PWA for Firefox)
  • existing installation with jQuery mode but SW unsupported: we should stay in jQuery mode with a one-time warning

@Jaifroid
Copy link
Member Author

Rebased on master to incorporate dependabot updates to testing dependencies.

@Jaifroid
Copy link
Member Author

I determined that using a Bootstrap alert similar to our Active Content Warning was not ideal because it would only be possible to show the alert once the user has loaded a ZIM archive, which might be some time after initial app launch. Therefore I changed this to a Bootsrap-based System Alert (using the code we have for that in uiUtil.js). The result is as below (tested so far in a Chromium Extension):

image

@Jaifroid Jaifroid marked this pull request as draft October 16, 2022 14:38
@Jaifroid
Copy link
Member Author

In the Firefox extension (if the message channel is supported), it will attempt to switch to SW mode automatically, and show the message below. If the user is switching BACK to SW mode after having successfully (on some previous occaion) launched SW mode and then deliberately returned to jQuery mode, then the standard message (as at present) is shown instead.

image

@Jaifroid
Copy link
Member Author

I now need to set the development PWA to this branch, and temporarily set this branch's extension code to access the development PWA, in order to test what happens "on the other side".

@Jaifroid
Copy link
Member Author

One-time message for browsers that do not support Service Worker mode:

image

@Jaifroid
Copy link
Member Author

Tested on IceCat. It's a bit clunky: it attempts to load the PWA, then falls back to jQuery mode, but remains redirected to the PWA online until the user exits and relaunches. Then it works as expected in jQuery mode in the extension.

Testing for the message channel API is not enough, and it looks like I'll have to put in a specific test (similar to the one we did for WebP support) in order to detect such cases.

@Jaifroid
Copy link
Member Author

Relying on canvasElementWorkaround detection is not enough because the user can turn this anti-fingerprinting off for the site.

@Jaifroid
Copy link
Member Author

Jaifroid commented Oct 16, 2022

N.B. Several of the dialog boxes have changed in small ways (usually to make them more concise).

How to test PR's logic under various condiitons:

  1. Uncomment params['PWAServer'] = 'https://kiwix.github.io/kiwix-js/'; in app.js (don't commit this change);
  2. Load the extension you want to test from your local Repo;
  3. Test behaviour when resetting the app with Reset button (=clean install);
  4. Put the app in jQuery mode;
  5. Open DevTools, go to Application (or Storage), and under localStorage delete all settings except contentInjectionMode=jquery;
  6. Reload the app with Ctrl-Shift-R and observe "upgrade" process.

@mossroy
Copy link
Contributor

mossroy commented Oct 17, 2022

All this looks good!
(even if I did not test and only had a very quick look at the code)

@Jaifroid
Copy link
Member Author

As a sidenote, this PR improves the situation outlined in #877 and #878, in that instead of leaving users of old Firefox in the PWA once it is discovered that Service Workers are not supported, this PR returns the user to local code. It is possible to prevent the return to local code by cancelling the relevant dialogue box (this is primarily for testing the PWA, but also to ensure the user has ultimate control over what is happening).

@Jaifroid
Copy link
Member Author

On Firefox OS (simulator), if the user doesn't have an archive on their device (i.e. on their SD card), then the Welcome to Kiwix alert below will show instead of the alert about deprecation of jQuery mode:

image

To address this issue, I have added code in the resolve function of the above alert to remove the key that indicates that the user has seen the deprecation notice. The effect of this is that once the user adds an archive to the device, and on next restart of the app, they will see the deprecation notice like this:

image

@Jaifroid
Copy link
Member Author

@mossroy If you would like to test on a physical Firefox OS device, let me know.

I still have some tests to do in various scenarios, because I removed a setTimeout delay on displaying the deprecation or the upgrade notice, and I need to be sure that the timeout is no longer needed in all situations.

@mossroy
Copy link
Contributor

mossroy commented Oct 22, 2022

I just tested on my Firefox OS device, and it seems to behave as expected: the warning about unsupported SW mode appears (only once), and the app still works.
I tested both with an upgrade of an existing app, and with a new installation.

However, when the popup appears, you have 2 ways to close it: the top-right X, or the bottom-right "okay" button.
It's clear that "okay" button should dismiss the warning (i.e. not make it appear again). But, to me, closing it with the "X" should let it appear again on next launch (to me, it means: "I don't have the time to read that, I just need to use the app, but remind me later")

@Jaifroid
Copy link
Member Author

However, when the popup appears, you have 2 ways to close it: the top-right X, or the bottom-right "okay" button.
It's clear that "okay" button should dismiss the warning (i.e. not make it appear again). But, to me, closing it with the "X" should let it appear again on next launch (to me, it means: "I don't have the time to read that, I just need to use the app, but remind me later")

That's a good point. It's probably best to be explicit and have a Cancel button alongside the OK button, and then the "X" would be equivalent to Cancel and we can handle it in the code (it's an option in the systemAlert function).

@Jaifroid
Copy link
Member Author

Cancel, 'X' or backdrop (also Esc key) are now handled so that the deprecation warning will show again next session if the user cancelled.

Note this does NOT apply to the notice that the user has been upgraded to ServiceWorker mode. In that case, there is no cancel button (but there is an X, as that can't easily be removed). IMHO "Cancel" is meaningless in that context, because we don't want to put the upgraded user back into JQuery mode, and we hope they'll just carry on using SW mode without thinking about it too much.

There is one other case when I explicitly show a button that says "Use JQuery mode" (and "Cancel"). This is for users of the Firefox Extension in old browsers (e.g. IceCat). In that case, the user has been sent to the PWA only to discover that their browser doesn't support Service Worker mode. Then the button "Use JQuery mode" explicitly takes them back to the extension. This is a special case.

@Jaifroid
Copy link
Member Author

Jaifroid commented Oct 22, 2022

Final tests completed:

  • Internet Explorer
  • Firefox OS Simulator
  • Windows Mobile browser
  • Extension in IceCat (Ubuntu via WSL)
  • Extension in Chrome 49.0.2623.112 - in Exttension only runs in jQuery mode; in PWA attempts to run in SW mode, but transferrable ArrayBuffer via message port not supported, so only usable in jQuery mode
  • Extension in latest Edge/Chromium - Edge 106
  • Extension in Firefox 107 (Developer)

@Jaifroid
Copy link
Member Author

Although we don't officially support it, the last version of Chrome to run on Windows XP/Vista is 49.0.2623.112, so it may be a good proxy for other issues. In this browser, the extension installs fine, but there is no Service Worker support in extensions, because they are not considered a secure origin. When loading the extension, we get the security error below, and this prevents display of the deprecation notice. So I need to handle that so the notice is displayed after this alert.

image

@Jaifroid
Copy link
Member Author

Jaifroid commented Oct 23, 2022

In Firefox 107 (Developer Edition), I'm getting a failure to communicate between the PWA (latest code in this branch) and the Extension, which leads to a message on each launch that the last attempt to launch the PWA may have failed. Clearly this needs debugging before we can make SW mode the default.

@Jaifroid Jaifroid marked this pull request as draft October 23, 2022 10:17
@Jaifroid
Copy link
Member Author

Panic over! It was because the PWA version had the wrong setting for params.PWAServer (set to production instead of test).

@Jaifroid Jaifroid marked this pull request as ready for review October 23, 2022 10:49
@Jaifroid
Copy link
Member Author

I've now completed all tests on the latest code. This is ready to merge if you don't have any more testing to do, @mossroy.

@mossroy
Copy link
Contributor

mossroy commented Oct 24, 2022

I won't have the time to test more (I only tested on Firefox OS) or review in detail (I just had a quick look at the code and did not find anything obviously wrong)
But it seems to me that you already tested well, so you can probably squash/merge this

@Jaifroid Jaifroid merged commit b678034 into master Oct 24, 2022
@Jaifroid Jaifroid deleted the Switch-to-SW-mode-by-default branch October 24, 2022 15:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants