Upgrade Bootstrap to version 4

[Jaifroid]

This implements #526. I will document the reasons why certain changes had to be made for this version.

[Jaifroid]

These are the main changes.

• The built-in colours in Bootstrap 4 have become very garish. See https://getbootstrap.com/docs/4.0/utilities/colors/#background-color . I tried the standard colours, and frankly, they look horrible! So I have included a code block in app.css that overrides them with the current colours we use.

• Panels have been replaced with cards. See https://getbootstrap.com/docs/4.3/components/card/ . They are pretty similar, but you have to do border styling, etc., yourself. Basically, Bootstrap has separated out all the colouring elements, probably because people wanted to customize them anyway.

• Glyphicons have been removed from Bootstrap 4, and are no longer licensed. It is recommended to use Fontawesome instead. I've done this, but haven't yet added the licence info for Fontawesome (it is compatible with Open Source) [EDIT: now done]. I have only used the solid icons, as it minimizes the size, and we only use a small subset.

• Headings no longer play nicely with plain text that is not wrapped in a <p> or similar, as the margin-bottom is provided by the <p> element. Otherwise there is no spacing between text and the next header. This mostly affects our About page. I've inserted <p> instead of <br> where it makes a difference. I made some incidental updates (e.g. new Ray Charles), but About could generally do with a rewrite. That's a separate issue.

[Jaifroid]

NB I haven't done the alerts yet or other dynamic Bootstrap content. With the alerts, we may be able to tackle #470 at the same time, but I haven't looked into the changes yet. [EDIT: alerts now done, see below]

[Jaifroid]

FYI Commit c463ba4 is a proposed fix for #470, i.e., we no longer have to create the alert boxes' html in code each time we want to display them.

However, as you can see, it doesn't save any code; on the contrary, it adds to the amount of code, because we have to implement an override on Bootstrap's own .alert('close') function. The only advantage of this code is that it maintains the separation between html and code, but at the cost of a little bit more complexity.

This commit (as opposed to the whole PR) could be added to a separate branch and we could have a PR on it as a solution to #470, if you'd prefer to do it that way, @mossroy . Just let me know, and also let me know if you think this way of doing it is better than what it replaces (just focus on commit c463ba4).

[kelson42]

@Jaifroid Version of Bootstrap in www/index.html should be updated too.

[Jaifroid]

I've rebased this branch/PR on master.

[mossroy]

I don't forget this PR, but will need more time to look at it.

[Jaifroid]

No rush! After your holiday is fine.

[mossroy]

I finally took a bit of time to have a look at this PR.
It seems to work fine : nice job!
I like the fact that the current field or the clicked/tapped button is highlighted. It also indicates in which section we just jumped, which is convenient.
And the flat design is trendy.

I'm OK with fixing #470 in this same PR, and happy to see that we do not put HTML strings in javascript files any more. I find that much better than the previous version, even if it adds a few lines of code.

There are a few minor issues that I noticed :

• there is a small useless gap between the top of the window and the top bar. It seems to come from the margin of [role=region] > header in app.css
• on Firefox OS, in the Configure section, there is a hyperlink to rescan the SD cards Click here to rescan... but the here hyperlink is not visible : it looks like some standard text (see screenshot below)
• on Firefox OS, when you open the menu, the items are surrounded like buttons, which is not very beautiful (see screenshot below). But, if it's FirefoxOS-specific, do not bother to try to fix it, it's very minor
  
• on any browser, if the screen is small enough to have the sandwich menu and you click on an item, the menu stays expanded, where it was automatically collapsed in previous versions. I found the previous behavior more convenient

[Jaifroid]

Thanks @mossroy . I'll take a look at those issues. One thing to note, however, is that if the intention is to use this as a stepping-stone towards a similar interface to that provided in Kiwix JS Windows, then it may not be worth trying to fix these issues yet, but to tidy up once we have the full new interface. I'm not sure. On the other hand, I think most of the issues are probably pertinent to functionality we'll want with the new interface. I don't currently use a collapsing toggle, but it may be necessary to continue supporting very narrow screens. We'll see.

[mossroy]

I did not look into the bootstrap 4 implementation details : I trust you on that.
I just added a few minor comments.

If you prefer, I agree to merge this PR and handle the issues I mentioned afterwards.
IMHO, the main one is the fact that the menu items do not collapse when you choose one of these items. In #523, we were talking about having 2 menus like that, so it might be worth dealing with that.

[Jaifroid]

on any browser, if the screen is small enough to have the sandwich menu and you click on an item, the menu stays expanded, where it was automatically collapsed in previous versions. I found the previous behavior more convenient

Fixed in local copy

[Jaifroid]

there is a small useless gap between the top of the window and the top bar. It seems to come from the margin of [role=region] > header in app.css

I think this is also fixed (but I found it hard to see, so not entirely sure). I've pushed all fixes so far.

I'm still missing:

• Menu button surround. This seems to be specific to FFOS: in browsers, the buttons are only surrounded when selected, but we could probably replace the buttons with links. However, as we're going to replace the whole interface, I think this one should be fixed later if it persists.
• SD card rescan. This could be turned into a button perhaps.
• Selection of ZIM in FFOS: we probably need to remove or add a style.

I propose to fix the above after replacing the text-based system, because lots of things could change. Please let me know if you're happy for me to squash rebase.

[Jaifroid]

Hmm. "CodeFactor" doesn't seem to like the bootstrap library code. Can it be marked as library code, so it doesn't get checked? EDIT: It seems that after we merge, we'll be able to mark the relevant files as "Ignore", but it's not possible to ignore them in a PR. As they are new files, we'll just have to swallow this poor code and then ignore it after merge.

[mossroy]

The code of your latest commits looks good.
I did not have time to test, but I think you can rebase/squash/merge if you did.
Don't bother for FirefoxOS. I was mentioning theses minor issues in case they would affect other platforms too.
Replacing the hyperlink to scan for ZIM files by a button is a valid workaround.
Regarding the button surround, I think it's not worth investing time on it.

I did not do anything to activate CodeFactor. I suppose it's a general modification done at the hackathon. It should not prevent you from merging this PR.

[Jaifroid]

I changed the FFOS rescan link to a button, and it now looks like this in the simulator (NB I intentionally opened the menu here):

As you see here, the code I pushed yesterday means the Configure button no longer remains selected, so no longer has the surround.

[mossroy]

Great!

[Jaifroid]

Hmm, the code yesterday seems to have a race condition (I guess), with jQuery not being ready for a bootstrap function. I'm getting this in SW mode on Edge. Need to do more testing and fix before I merge this.

[Jaifroid]

So I've rebased on master, and it unfortunately doesn't cure the Chrome test failure. I've checked in sauce labs, but as you said, @mossroy, the keys R-a-y are never registered by the search field. This only occurs in Chrome. They are clearly visible in the other browsers. The http response (visible in the screenshot) is the same as in Firefox (status 200 and null). So it's a bit of a mystery. I'm back to wondering if our timeout delay (to avoid a race condition when typing fast) is interacting with sendkeys and Bootstrap in a weird way in Chrome. Could Chrome have its own form of rate limiter? I'll see if I can test more at the weekend.

[Jaifroid]

Looking at my Commit list, one other idea occurs to me: we remove the focus from UI elements as soon as the document is loaded in order to fix one of the issues with FFOS display in the collapsed menu. It could be that, due to the async "optimizations" in Chrome, this blurring occurs after Travis has selected the prefix field, and before the keypresses are sent (maybe exacerbated by the setTimeout on the prefix field). It seems like a plausible explanation that needs testing.

[Jaifroid]

So that was indeed the problem. The diagnostic commit comments out the line that removes focus from the UI elements, and it now passes.

So now we have to decide how to deal with this. In some ways this error can be considered an artefact of the testing system, because no user will select the field and type so fast after article loads. These possibilities occur to me:

• Introduce a delay in Travis between checking that the article has loaded and selecting the prefix field, so that it gets selected after we have removed focus;
• Introduce a delay (a setTimeout) on the command to blur the UI elements. This would be a race condition, and I'm not sure it's a good idea, as we'd have to hit the sweet-spot between Travis working and the user not being inconvenienced by a delayed blur while they are typing;
• Do not remove focus from the UI elements on article load. The effect of this on mobile devices would be that the keyboard would pop up always at least on the landing page. I know we used to have that behaviour, but at some point it appears to have been dropped. There probably is some other effect, because the test error occurred after I fixed one of the FFOS issues mentioned above.

Any other thoughts?

[mossroy]

What if you moved this piece of code, so that it is ran synchronously, when starting to read the article, instead of running asynchronously in the onload event (after the article is displayed)?

[Jaifroid]

Thanks for the helpful suggestion. Although it didn't solve the issue with Chrome directly, it did help simplify the code a little, and to understand what we're doing. It looks like we have a solution. Below is the explanation in case you're curious! But, subject to a bit more testing, I think the code could be ready to merge now, if you agree @mossroy.

Explanation:

In response to your noticing that in FFOS some buttons remained pressed, I introduced a line to blur any focused UI element on page load. It returns the interface to a neutral state once an article is loaded. In most browsers this worked fine, but in Chrome it caused a failure because the blur conflicts with the sendkey routine for an article search. In fact, the reason there is no failure in the other browsers may be accidental: many browsers defer or batch UI rendering, and maybe Chrome was being the "speedy guy" here, blurring "too soon".

Whatever the case, it turns out that the Unit Test expects the search field to be selected when the landing page loads, and doesn't actively select it before sending the search keys. This is consistent with expected behaviour, as traditionally this app selects the search field for the user on pressing the Home button. So, the solution is to blur only if we are not loading the landing page. We already have an isLandingPage parameter, so it's easy.

A very rare side effect of this is that it's possible that the Random key will remain pressed if hitting it results in the user loading the Landing page randomly. It's rather unlikely and very minor, with no negative effect. EDIT: This may not in fact be true, as we set the parameter in the gotoMainArticle function.

[mossroy]

All right you can merge this.
If we find some other problems, we'll open some other github issues