Add a modern CSS spinner

[Jaifroid]

@mossroy I've taken the CSS spinner code as far as I can without hooking into the Service Worker messaging channel. So it works fully in jQuery mode, and in a limited way in Service Worker mode: when loading a new page by searching for it, with the Random button, or when loading the landing page, but not when clicking on a link within a page. The latter would require hooking into the messaging channel, and so would require a policy decision if we want uniformity across the app. It can be left for future decision.

Regarding your comment in #425 that keeping the old page on-screen until we have the new html to replace it might cause the app to keep requesting images unnecessarily, I have done extensive testing in #426 : there is a broader issue affecting master, not this PR specifically, so I have opened a separate issue for it.

Could you please check this code and test? I'd be grateful if you could test in particular that #361 ("TypeError: can't access dead objects") is not reintroduced. It shouldn't be, as we still have the specific fix for that in place. There is also a related query embedded in a comment in app.js line 748 addressed to you, which I'll remove when the query is resolved. No rush at all!

[mossroy]

Thanks a lot for this PR!

I agree with you about #426 being a separate issue.
I did not see #361 symptoms.

A few minor remarks :

• I tend to find the new spinner could be a bit more visible. For example by using a bit stronger colors. But it's only a personal opinion
• The new "Caching styles" is pretty, but might be a bit more visible too. For example by adding a border around. Or displaying it next to the spinner. But that might be discussed too.
• In jQuery mode, when scrolling down and clicking on a hyperlink, there's an annoying behavior : the spinner is visible, then the page scrolls up, and afterwards this page is replaced by the new page. I suppose the $("#articleContent").contents().scrollTop(0); is done too early. I think this scrolling is not necessary any more? Else it might be moved in the onload event, so that it happens later?
• Regarding $('#articleContent').contents().remove(); in ServiceWorker mode, I don't remember why this line was needed at some time. It seems to work fine without it.

[Jaifroid]

Thanks @mossroy :

• I'll experiment with a darker colour -- I agree it's sometimes hard to notice;
• I was having trouble finding a good place for "Caching styles..."; it's not so necessary now that xzdec is much faster, though could still be useful on low-end devices. I'll experiment with what it looks like placing it over the spinner, otherwise use more noticeable style;
• Good catch about the scrolling -- I think it's not necessary, will check;
• I think the only purpose of $('#articleContent').contents().remove(); would be to tell jQuery to remove its hooks, so I think in SW mode it only needs to be performed once when the user switches to SW mode after having used jQuery mode -- I'll look for a place to relocate it in the switching code.

[Jaifroid]

Hi @mossroy , if you get a moment, could you give me an opinion on the (visual) implementation now?

• More visible foreground and background colours for spinner;
• In jQuery mode, Caching styles... message now centred beneath spinner;
• Caching styles... message is updated with abbreviated name of each css file as it is extracted.

Note that the caching message(s) will only show in jQuery mode, as the cache isn't implemented for SW mode. Also the caching message will only be displayed on first load of the landing page, and very rarely on a page that needs a new CSS file that has not yet been cached.

Specifically, does the caching message strike a good balance between being discreet but still being noticeable/informative? Is it too obtrusive? Would it be better relocated to bottom-right or top-right corners? (We can't use bottom left because many browsers use that area to show the URL of a link that has been clicked.)

An example over blank background shown below (in reality it's smaller and more discreet than shown here, as Github expands the screenshot).

[mossroy]

I quickly tested that on Firefox and IE11 (both in jQuery mode). Visually, I find that great! I would keep it like this.
I did not look again at the code.

But I found another possible improvement that we might do (maybe it should be a separate issue) :
I tested with very slow I/Os (by choosing a ZIM file on a remote filesystem over a slow network), and noticed that the spinner does not appear right away when the user clicks on a hyperlink or on a button of the UI (random, home etc). It only appears after the binary search that reads the DirEntry.
With these very slow I/Os, it gives a noticeable delay.
Anyway, this is clearly an edge case, not a priority. It could be discussed separately.

[Jaifroid]

Great, thanks! I can certainly test adding a line to the onclick code to activate the spinner sooner for these edge cases. I think it can be part of this PR.

I've still got code clearing-up to do and one small thing to fix from earlier discussion. I'll ask you to look over final code when it's ready (probably at weekend) before merging.

[Jaifroid]

@mossroy I've completed the pending issues (see commit titles). Could you test your slow I/O files with this final code? The spinner should display on click of UI elements in both modes, and inline article links in jQuery mode.

If you get a moment to look through the code, it would be great. Quite a lot of redundant hiding and showing has been deleted.

There is one thing that might need decision: I got rid of the "Reading article xxxxx ...." message before I implemented the css caching info box beneath the spinner, on the basis that users know what they've clicked on, and the spinner is enough feedback. However, it would now be possible to use the css caching info box to present that message. I tend to think it's still not necessary, and displaying the message on each page load would mean that we would always see the box. Currently it is only shown when there is new CSS to cache, so only for the first couple of page loads usually. I feel it's neater just to use the spinner as feedback. The "loss" of feedback is mitigated by the fact that the article list stays in place now until the page is ready, and when a user clicks on an inline link, browsers usually display the link in their status message bottom left.

However, I thought I should point this out, since it is removing a feature that exists in master.

[mossroy]

I tested with the same slow I/Os (on Firefox 60ESR and IE11, both in jQuery mode) : the user feedback is much better now, with the spinner appearing right after the click.
A small regression : the spinner does not seem to appear any more when clicking on "Random" button.

Regarding the "loss" of "Reading article xxx", I don't have a strong opinion. We can probably leave it this way : it's still possible to add it back later if we feel it's necessary. In a sense, it's also more consistent with the SW mode.

I had a quick look at the code : I like to see some code removal in this hiding/showing code :-), see #78.

I did not test in SW mode, but if you did, you might squash/rebase/merge this PR.

[Jaifroid]

Thanks for spotting the regression. It was a bit "random", and a puzzle, because I can't see the rogue hide() statement that turns off the spinner. It's clearly shown at the very beginning of the goToRandomArticle() function, and nothing in the chain should turn it off until the html is received and all CSS is loaded. I wonder if there is a manifestation of #426 here: a delayed CSS file from a previous page comes through async and triggers turning off the spinner (because we have a tendency when testing to mash the Random button (at least I do), and because I moved the show() statement to before the dirEntry is sourced, it gets turned off before we read the file. In any case, I've fixed it with an extra show() statement. But it feels like a brute force kludge... Hmmm.

[Jaifroid]

Found another regression in this PR with FFOS, when clicking on a search result, we get a severely truncated display area once the new page is shown, at least in the simulator. It's to do with display of the on-screen keyboard, I guess. I need to remove the article list before that code is called... Or ensure the on-screen keyboard is removed as soon as an article is clicked, might do the trick.

It's never easy!!

[Jaifroid]

OK, this commit solves that one. Again, feels a bit like brute force, but it does make sense to call resizeIFrame() after we render CSS.

I'm going to leave this till tomorrow before squashing etc. @mossroy , I don't know if you want to test on a real FFOS device first? If not, it does seem to work in the simulator. Anyway, I aim to merge early tomorrow morning unless I hear to the contrary.

[mossroy]

I tested on a Firefox OS device : it works perfectly fine.
Maybe it would be worth adding some comments in the code about these workarounds.