Use articleContent.write()

[Jaifroid]

This PR implements just the code that injects HTML into the iframe using articleContent.write() instead of .innerHTML. For explanation see #341. It is for testing, because it cannot be merged as is for the reason shown in the screenshot (explanation continues after screenshot):

I.e., the Headings are closed, and there is no way to open them, since clicking on the headers does nothing on FFOS (because the inline .onclick event is blocked (and errors are swallowed by Q) and the opening routines rely on jQuery which is not attached to the iframe).

This example is the page "Tuberculosis management" from the latest WikiMed ZIM. But any page on new-style mobile-first ZIMs will have this problem.

@mossroy , when you've had a chance to review this (absolutely no rush, I have a lot on this coming week), please request changes, so that I can then add the next step. Possible next steps:

• Get rid of the class attached to the html tag (class="client-js") that causes all headers to be closed;

• Or, we can remove this class and neutralize all JavaScript on the page as an interim workaround pending implementations of Enable injection of javascript content coming from ZIM files in jQuery mode #152 .

[mossroy]

On which ZIM file do you reproduce the issue?
I tried with wikipedia_en_medicine_novid_2018-03.zim but it does not seem to be a mobile version

[mossroy]

NB : I think it should be avoided to use special characters like parenthesis in a branch name, as it's not always properly handled by git tools (for example autocompletion on command-line)

[Jaifroid]

On which ZIM file do you reproduce the issue?
I tried with wikipedia_en_medicine_novid_2018-03.zim but it does not seem to be a mobile version

That's strange. The above screenshot is taken in the FFOS simulator on this PR using wikipedia_en_medicine_novid_2018-03.zim. Do you get a different result? I get the same result in wikivoyage_en_all_novid_2018-03.zim:

[Jaifroid]

I think it should be avoided to use special characters like parenthesis in a branch name

OK, I'll try to rename.

[Jaifroid]

Seems only way to rename remote branch is to delete this branch and push the renamed copy, but not sure how that would affect the PR. Seems confusing, so I'll leave as is for now.

[Jaifroid]

Just to add a data point: running this PR in Firefox ESR produces this error (screenshot below) on every page load from mobile-style WikiMed in the Console log (the error is not reported in the FFOS simulator -- this is running the full browser). Error also with onclick events. It's possible that FFOS also blocks window.onload along with inline onclick events, which is why no error is seen in that context.

[Jaifroid]

More info from Internet Explorer for onclick error:

[mossroy]

I pushed some commits on this branch : the first one fixes a race-condition, the second one fixes UI tests on Travis.
With this, it seems to work fine on Firefox 59, Chromium 65 and Firefox OS devices. Both in jQuery and ServiceWorker modes. But I did not test a lot.

I now reproduce the header toggle issue (due to missing jquery javascript) : it was because my desktop screen was considered too wide by the CSS to collapse the headers. It is indeed a real (and separate) issue.

NB : never mind for the branch name, we'll see if it causes other issues or not.

[mossroy]

@Jaifroid : can you confirm that this PR fixes #341?
If it's the case, we might test a bit more, then merge this PR

[mossroy]

I think it also fixes #347 , but it needs to be verified.

[mossroy]

I opened #355 for the toggleSection issue

[Jaifroid]

Hi @mossroy , thanks for your work on this! Can I just ask if you introduced this .onload function with the iframe in order to cure a race condition you observed with articleContent.write()? I've got absolutely no problem with doing it this way, but I'm curious as to whether you were still noticing the race problem with this way of injecting to the iframe -- I thought it was a problem pertaining to the loading of dummyArticle only, and went away once we got rid of dummyArticle. Again, no problem here, it looks like a good way of avoiding potential problems.

I'll try to do some testing of the PR this evening and will get back to you.

[mossroy]

In my first test, the links were simply not parsed (on my desktop Firefox 59). So they were leading to an error page, because the href was not found.
On the other hand, I tried to remove the onload trick, and can not reproduce the issue now...

So I can't say I'm 100% sure. I should test more

[mossroy]

I just reproduced it a few times, on several ZIM files

[Jaifroid]

OK, thanks for confirming. So it turns out document.write() is asynchronous, at least in the latest Firefox. Good to know.

[mossroy]

I reproduced that also on Firefox ESR 52.7.3 and Chrome 68, with the Ray Charles archive.
I suppose the browser enfines interpret the iframe in a separate thread, which makes sense.

I'm wondering if it would be necessary to move the statement "articleContent.close();" after the onload declarations. If the iframe loading is really in another thread, it might finish before we put the onload.
It's probably safer to move this line.

[Jaifroid]

I think I read somewhere that document.close(); may be necessary to trigger the document.onload event. May be wrong. But if it works as is, and #347 is not reproducible with this PR, then maybe we're OK...

[mossroy]

I think you're right about document.close() : the onload event is certainly fired after the document.close(). To illustrate my proposal, I've commited it in the branch : I've moved the document.close() after setting the onload

[mossroy]

When running this PR on Chromium, it displays the following warning in the console :
[Violation] Avoid using document.write().

It does work, but it's not a good sign of proper support in the long term : not cool...

[Jaifroid]

See:

https://stackoverflow.com/questions/12574098/is-document-write-actually-deprecated

and

https://stackoverflow.com/questions/802854/why-is-document-write-considered-a-bad-practice

[Jaifroid]

Discussion in second link seems pretty balanced. It's more a conceptual problem people have with it. For loading content into an iframe from a known and controlled source (the ZIM in our case) where you want active content to run, it seems like the perfect use case...

[mossroy]

Thanks for the links. You seem to be right : it's not deprecated, but seems to be often misused.

[mossroy]

Regarding the document.write() method, can you test it a bit more on Edge? Because there is the following comment on https://developer.mozilla.org/en-US/docs/Web/API/Document/write :

Note: In Edge only, calling document.write more than once in an iframe causes the error SCRIPT70: Permission denied.

See https://stackoverflow.com/questions/39954217/several-document-write-in-iframe-in-edge-cause-script70-permission-denied

[Jaifroid]

On the Edge bug: I've just thoroughly tested this PR as it currently is in Edge, and there is no sign of this bug SCRIPT70 in Wikimedia ZIMs. The only error I'm getting in the console is the one we get in other browsers about $ not being defined. But pages are all loading fine and fast in Edge with this PR.

In Stackexchange ZIMs I'm getting a lot of console errors relating to JavaScript being executed, I think (see screenshot), but none of them are SCRIPT70 and the page still loads correctly and looks fine, and all links work. In UWP context, I would have to neutralize the JavaScript because otherwise I will cause the app to hang, but this can be done in the Windows version only if it doesn't affect any other apps.

[Jaifroid]

Actually, ignore all those script errors, they are caused by a completely unrelated issue switching between non-console use and opening the console. If I run the pages with the console open the whole time, the only error I'm getting on Edge from Stackexchange ZIMs relates to cross-site access for the font file, which I also get in Firefox.

In any case, I can safely answer that the document.write() bug with iframes in Edge is not being triggered by our code. This makes sense because we are only calling it once, then we empty the contentWindow before calling it again for a new document.

[Jaifroid]

From testing in Edge and IE, far as I can tell, this PR is working fine with the exception of what I'd consider to be a blocker bug of not being able to open content on narrow screens/mobile devices.

How would you like to proceed? I would personally be a bit wary of merging a PR that has such a problem. It seems to me there are now two courses of action:

1. Leave this PR unmerged until we can get Enable injection of javascript content coming from ZIM files in jQuery mode #152 working.

2. Remove the "client-js" class (if any) from the html tag as a temporary workaround pending Enable injection of javascript content coming from ZIM files in jQuery mode #152 (and comment it as such in the code). This would, I believe, allow us to merge this PR.

Optionally I would also recommend including JavaScripts in the regex that currently replaces <img src=...> with <img data-kiwixsrc=...> in jQuery mode, as it prevents the browser launching a useless request for a file it won't be able to find. We will give the scripts a new src once we have extracted the JavaScript blob. It's a one-word addition to our current regex.

[mossroy]

The sections can not be toggled in kiwix-js for now (in this branch as in the master branch).
It is indeed a real problem, but merging this PR does not make things worse.
It fixes a bug, and leaves the toggling issue as it was before.

On the other hand, as far as I understood, it does create a regression on UWP apps, where running the inline javascript can crash the app if the javascript dependencies are not loaded? Maybe this could be temporarily workarounded in kiwix-js-windows, by removing the onload/onclick attributes?

[mossroy]

Let's talk on #355 about the workarounds or solutions to the toggling issue

[Jaifroid]

OK, no need to worry about the regression in Kiwix JS Windows -- it is already worked around, as I discovered a long time ago that clicking on links in mobile-style ZIMs caused an app crash (not because of the document.write() code which is new here, but because the app attempts to run the inline onclick event). I had thought the issue was unique to UWP (which it is in a way, at least in so far as causing a crash, though some other systems do attempt to run the onclick code), so I hadn't at the time investigated it further or raised it as an issue here.

I'll squash/merge this branch and we can work on the other issues separately.

[Jaifroid]

Merged to master.

[mossroy]

Cool! Thanks a lot for this PR
I think it gives us a saner base to work on the other issues.

I suppose we can now close #341 , #347, and even #343 ?

[Jaifroid]

Yes, I think all of those can be closed now. I didn't manage to do work on JavaScript at the w/e, but will have a go soon.