Send article to new tab in jQuery mode

[Jaifroid]

This is currently a rough draft of a possible way to manage tabbed browsing in Kiwix JS in jQuery mode, addressing #678. This PR is not needed for Service Worker mode because the browser handles tabbed browsing natively.

With this PR, a user can launch a new tab or a new window (depending on OS/browser) in one of two ways:

1. Ctrl-click or command-click on a hyperlink (if user has a keyboard and mouse, or keyboard and tap);
2. Long press on a hyperlink for more than 600ms (only on browsers that support the touchstart event - i.e. FFOS and Windows Mobile are not supported, but I don't think anyone really wants to open a new window on these mobile apps).

The second option is an alternative to patching the right-click context menu. This menu has useful things a user may wish to do, so we should not override it. As a result, this PR does not support opening a new tab from the context menu (it is natively supported in SW mode). There is no reliable way to patch the "open in new tab" entry in the context menu without completely replacing the menu.

The tabs opened by this PR are independently browsable, i.e. the user can click on ZIM hyperlinks and they will open in the same window, and also ctrl-click on links to open yet another window/tab).

The new tabs do not have the Kiwix JS UI (no search bar, no random button or navigation buttons). I see this as an advantage for the user who would like to read a page fullscreen or print a page using the browser's native print function.

There is currently a small bug with setting the title of each tab on some browsers: it will intermittently show "Localhost" instead of the page title - this is probably a minor race condition with setting the title at the right moment.

[Jaifroid]

Here is a demo of what the PR does (jQuery mode in Firefox, using Ctrl-click):

[Jaifroid]

This is nearly ready, with the following mostly minor issues to resolve:

• Tests are failing consistently, even though QUnit tests are passing locally (I have yet to debug this) EDIT: tests are failing only in Firefox (UI tests, not QUnit).;
• History is stored manually by the app in jQuery mode, and so to go back and forward in a tab, it currently has to be done from the main app tab (where the controls are): maybe allowing native history in tabs could work, as the pages are not in an iframe;
• Tabbed browsing in IE11 works fine with non-WebP ZIMs, but with WebP, the images in a tab will not load unless the user switches back to the main (app) window and then back to the tab (the switching can be done a long time after the page has "finished" loading, and the images will then all show at once).

The last issue is curious: it is as if the Emscripten engine for WebP refuses to run "in the background" (only in IE11), and has to be in the foreground to complete a Promise or a timeout. I cannot find an obvious cause, and I cannot find a way to give focus to the main tab programmatically (blur and focus methods on tabs were blocked a long time ago due to abuse by spammers). Possibly the way image queueing is done inside a closure has this side-effect? I don't think it's a big deal if this feature remains a bit buggy in IE11.

[Jaifroid]

Tested and working as a Chromium Add-in and a Firefox Extension.
Firefox erroneously claims it has blocked popup windows (they are not actually blocked since they are initiated by a user action, which is allowed, and I didn't notice any blocked functionality). Dismissing the message about blocking (which only appears in the main app tab) makes it go away.

Chromium, Edge Legacy and IE11 do not flag the new tabs as popups.

[Jaifroid]

Hmm. I wonder why Travis tests are not running any more (only codefactor).
Last step for this PR is to be sure it is passing tests, but with the tests having gone down that's a bit tricky.

[Jaifroid]

@kelson42 @mossroy, I see the error below in Travis about our having run out of credit. Do you know what can be done about this?

[kelson42]

@Jaifroid No, I don't have changed anything. Actually we have moved two additional repos from Travis to Actions recently.

[Jaifroid]

@kelson42 The message says "Builds have been temporarily disabled for public repositories due to a negative credit balance. Please go to the Plan page to replenish your credit balance or alter your Consume paid credits for OSS setting." When I try to go to the Plan Page - https://travis-ci.com/organizations/kiwix/plan - I just get "There was an error, please try again". Is this something you can check, or @Popolechien (becuase it seems to be related to the Kiwix Organiziation Plan)?

[mossroy]

I discover that Travis CI announced last month that it stops providing free unlimited CI for OSS : https://blog.travis-ci.com/2020-11-02-travis-ci-new-billing
We're not the only affected OSS repo : https://www.jeffgeerling.com/blog/2020/travis-cis-new-pricing-plan-threw-wrench-my-open-source-works

Shit, I would have liked to be informed earlier, and have more time to think about our options, which seem to be :

• switch to a paid option : I doubt we'll do that
• beg Travis to give us some OSS credits : it would be a welcome temporary measure, to give us some time
• switch to another CI/CD provider, like GitHub Actions (see Switch to GitHub Actions instead of Travis #590) : it's probably what we'll need to do in the mid-term

And this will make #672 more complicated to merge (as we won't be able to test it in the short-term...)

[Jaifroid]

Hmm, so they're asking us either to request more credits by saying "please", or to pay USD $15 for 25,000 credits. @kelson42 what would you advise? I don't know how long 25,000 credits would last, but might be a short-term inexpensive solution while we work on migrating to github actions (#590). Of course I completely understand that there may be an issue of principle at stake rather than just the amount of money.

[kelson42]

@Jaifroid I would advise to move to Github actions. This was the plan (at least mine) anyway. We just need to speed up things.

[Jaifroid]

I guess this PR needs to wait until we have fixed Unit Tests. For my part, pending tests, it is ready.

[Jaifroid]

Add 'AREA' tagname here too.

[kelson42]

I guess we can carry on with this PR now? Maybe rebase and ask @mossroy to review?

[Jaifroid]

Yes, thanks @kelson42 . I need to rebase and get the new testing system to run on this PR (that was the previous blocker).

[Jaifroid]

OK tests have run, and it's the same as under Travis: tests are failing on Firefox (only). I'm guessing there is a race condition due to Firefox not running document.write() synchronously. The spec says it should be synchronous, but I'm pretty sure we've encountered this issue before when using document.write() with Firefox. Unfortunately, neither Edge Legacy nor IE11 expose the onload event for secondary tabs, so I have to insert a clunky workaround.

[Jaifroid]

I've restored asynchronous loading for browsers that support it. However, the Firefox error persists. I'm a bit stumped by this one. In the logs we have this identified as the issue:

And element <hd id="mweQ" ...> corresponds to the heading 'Life and career' (of Ray Charles). Checking the video on Sauce Labs for the problematic Firefox 84, I can clearly see that the corresponding text is shown as soon as the article is displayed:

The only thing I can think of is that there are two elements marked with id="mweQ" (one of them is a td element, the other is this h2 element). This clearly breaks the spec, since no two elements should have the same ID, but that would not be a reason for this particular PR to be failing while master passes.

I'm trying with the local Karma versions of the tests.

[mossroy]

I don't think it comes from the ZIM content. Because we look for the exact same element earlier in the test scenario, and it's found by all browsers.
I notice that the following error message appears in the log :

Error ECONNRESET: socket hang up

Could it be related to nightwatchjs/nightwatch#1936 ?
Maybe you could test to downgrade to version "=0.9.20" of nightwatch instead of "^1.5.1" (in package.json), to see if it helps

[Jaifroid]

Oh, OK, that's very helpful. It looks like it's navigating all the way back to the blank article.html, the very first document that is opened in a new tab/window before we populate the document. That would explain the spinner still being shown, because none of the loading code is run and therefore we never get to the point where the spinner is hidden. So I need to prevent navigation back beyond an actual ZIM article, or (better), prevent an empty article.html being recorded in the history.

[Jaifroid]

With commit 6580907, middle-click now works reliably for me with my bluetooth keyboard on Windows. On Ubuntu it is working reliably for me if i click on a normal ZIM link. However, on one of the graphical landing pages (in the Geography ZIM), I am still getting multiple tabs opening with a single middle click on one of the graphical tiles.

EDIT: Now d661f8f seems to fix the issue on Ubuntu as well (for me).

[Jaifroid]

Unfortunately, middle-click is still misbehaving, opening both a tab and a window on some occasions. Investigating...

[Jaifroid]

Hmm, this is interesting. Seems like recent browsers define an extra "auxclick" event. See https://developer.mozilla.org/en-US/docs/Web/API/Element/auxclick_event for middle-click, and I may need to handle that separately.

[Jaifroid]

I think I've fixed the problem with middle-click both on Windows and Ubuntu. Ctrl-click should also work correctly (if set to open a new window, it would open a new window AND a new empty tab). There's a fair amount of diagnostic logging left in the click code, but I'll leave it there for a while during testing.

EDIT: After testing, in Edge Legacy, it's not possible to suppress the hard-coded new tab that opens with middle-click because it doesn't support the auxclick event that was created for this purpose. Ironically middle-click works fine on IE11 (but IE11 is very slow communicating with another window).

[Jaifroid]

Having said that, I think I see a way to prevent popup blockers: there is an isTrusted property on the click event, and it is set to true if the event is the direct result of a click, and to false if it's called from another event (like mousedown) even if the latter is the direct results of a click. It would be best, therefore, not to break that trust chain, and in some cases I am breaking it. Javascript is never easy!!

EDIT: I've addressed this as far as I think is possible.

[mossroy]

Middle-click now works on my ubuntu computer (like with ctrl-click), on both Firefox and Chromium. Nice job.

But when I click on another link in the new tab, the spinner appears on the main tab : it's not visible. So the user does not have the usual feedback that the click is being processed. It's not a big issue but is not consistent with the main tab.
A new middle-click in the new tab does not do anything (which might be an acceptable limitation if it's too complicated to implement), but also triggers the popup blocker in the main tab.

Regarding the "empty tab" symptom (of my video above) when using back/forward, I only see it on Chromium, not on Firefox. And only when I use the "forward" button of my mouse (no problem when using the buttons of the browser). I have the same behavior with 2 different brands of mouse (that both have back/forward buttons)

[Jaifroid]

Middle-click now works on my ubuntu computer (like with ctrl-click), on both Firefox and Chromium. Nice job.

Good to hear.

But when I click on another link in the new tab, the spinner appears on the main tab : it's not visible. So the user does not have the usual feedback that the click is being processed. It's not a big issue but is not consistent with the main tab.

The spinner belongs to the chrome in the main window, and I have not attempted to replicate it in a secondary tab or window, as the window/tab is effectively a transparent container that only contains the HTML of the loaded article, much like the iframe. I could of course add the spinner to the secondary container in jQuery mode, but it would then be inconsistent with SW mode, and I believe I should not attempt to modify the DOM in SW mode. My "solution" was to show the spinner only in the main window. Please let me know if you think we could/should reconsider that.

A new middle-click in the new tab does not do anything (which might be an acceptable limitation if it's too complicated to implement), but also triggers the popup blocker in the main tab.

In jQuery mode, we parse the anchors in the usual way, so a middle-click should be caught and treated in the same way as a middle-click in the iframe, and should open a new tab or window. If you have permanently allowed popups in the main window for the domain in question, there should be no further popup blocks, as tabs are in the same domain (localhost, usually). In SW mode, the middle-click should be supported natively as it is in any other web page (we don't parse the DOM at all for SW mode).

Regarding the "empty tab" symptom (of my video above) when using back/forward, I only see it on Chromium, not on Firefox. And only when I use the "forward" button of my mouse (no problem when using the buttons of the browser). I have the same behavior with 2 different brands of mouse (that both have back/forward buttons)

OK, thanks. I'm not quite sure how to debug this, or why it is happening. I put in code that prevents navigating while the article is loading, but this is clearly not the issue. Possibly, because it is the mouse that is doing the navigation, it could count as an auxilliary click of some kind, so we are getting an aux mouse click as well as a navigation event... The best I can offer right now is to write more information in console.log about which event(s) have been triggered, which might give us a clue.

[mossroy]

I have more information to debug the white page issue :
When the problem happens, I have the following lines in the console of Chromium :

Uncaught TypeError: Cannot set property 'kiwixType' of null
    at onDetectedClick (app.js:1611)
    at HTMLBodyElement.<anonymous> (app.js:1661)
onDetectedClick @ app.js:1611
(anonymous) @ app.js:1661

auxclick

Uncaught TypeError: Cannot read property 'contentDocument' of null
    at Object.applyAppTheme (uiUtil.js:386)
    at windowLoaded (app.js:1467)

When using back/forward quickly, I also sometimes get :

Uncaught TypeError: Cannot read property 'title' of null
    at historyPop (app.js:812)

[Jaifroid]

Thank you @mossroy . There seem to be a few things here. First, a click has been detected, but it doesn't seem to be associated with a Window object, hence attempting to store the window's type (which we do for history manipulation) fails, since articleContainer is null. I had never seen that situation, since a click should only be trapped on elements in a loaded document, hence by definition the document and its Window are available. Possibly the window is in the process of being unloaded (wild guess).

I'm confused by why a back or forward button should produce a click at all on any DOM element. Could there be a proprietary mouse driver that reads a type of mouseclick and then programmatically does history.back() or history.forward(), possibly with a timeout? A clue might be in that auxclick event, which I'd hypothesized above.

The final error could be caused by going back in history to the initial load of the blank article.html which doesn't have a title (or a state object) associated with it.

I'll thiink more about this and see if I can reproduce.

[Jaifroid]

Looking into this, your mouse almost certainly comes with software that remaps buttons to custom actions, or else you have set up a Linux mapping/driver to do the same. On Windows there is this: https://www.addictivetips.com/windows-tips/remap-mouse-buttons-on-windows-10/ which allows various remappings for people with generic mice that don't come with a proprietary driver, so I'll see if I can simulate what's happening with that.

I guess we would not normally try to work around proprietary hardware, but in this case it could be revealing flaws in the implementation, so a good proxy for a testing for race conditions, etc.

[mossroy]

I did not have to install or configure anything for the mouse. The driver (generic or specific) must be open-source and bundled in the linux kernel. And the mapping of buttons is the default one
It's the same mapping as in https://medium.com/@Aenon/bind-mouse-buttons-to-keys-or-scripts-under-linux-with-xbindkeys-and-xvkbd-7e6e6fcf4cba : back button is button 8, and forward button is button 9. I checked with xev command-line
They are normally mapped with XF86Back and XF86Forward events

But I don't manage to reproduce the issue with the keyboard, even when mapping a real key (like pause) to XF86Forward with xmodmap -e "keycode 127=XF86Forward"

[Jaifroid]

OK, so I gues it's the combination of a mouseclck being registerd and a navigation event both happening (roughly) at the same time, since the underlying driver must see a click in order to convert it into the appropriate browser command, or map it to a keyboard equivalent. I think we're getting closer, thanks, and I can work with that!

[mossroy]

I think that we're spending too much time and code on this feature.
It's more complicated than initially expected in #678 and there will be limitations (see comments above).

I tend to think that we should not invest that time to improve the jQuery mode in kiwix-js. I've opened #727 to discuss that more broadly.

But this PR also contains some refactoring that might be kept (probably in a new PR) if it might ease to keep this feature in kiwix-js-windows

[Jaifroid]

That's fine, @mossroy . I think the feature works quite well when targeted at sepecific ecosystems where you can control the variables, but targeted across multiple OS's and a range of potential hardware inputs, it gets complicated to iron out all the bugs, as we've seen, particularly managing history and so on. And I'm still not 100% satisfied that the tests are working as expected: with every other push, I have to restart the tests (they do complete correctly on the second go), but that seems indicative of a race condition I haven't been able to get to the bottom of.

So I can close this if you like, but keep the branch to decide what from here could be kept in kiwix-js. On the UWP app, the feature seems to be working well. Admittedly I haven't tried a mouse with various buttons (other than right-, left- and middle-click, which work) but UWP is more touch-and-click-centric, and that seems to be covered. Electron/NWJS may be less controlled as they explicitly target various operating systems, but the usage is very small compared to the UWP app, and only really for those who can't run Kiwix Desktop (i.e., for 32-bit).