Open external links in new tabs, and warn the user #862

mossroy · 2022-05-30T17:54:00Z

Fixes #404

This is just a refresh of the work of Jaifroid in branch https://github.com/kiwix/kiwix-js/tree/Check-for-external-links-and-warn

www/js/app.js

mossroy · 2022-05-30T19:46:24Z

I still need to add a setting to optionally disable this behavior, as we decided in #404
Will do that tomorrow

Jaifroid · 2022-05-30T19:50:57Z

www/js/app.js

+                            // We also do it for ftp even if it's not supported any more by recent browsers...
+                            if (/^(?:http|ftp)/i.test(href)) {
+                                var target = e.target.getAttribute('target');
+                                // We don't override the default browser behavior if there's already a target attribute


Actually, I'm not sure about this. If I ask to be warned before the browser opens a link on the Internet (albeit in a new tab), I would expect to be warned uniformly in all cases, not only if the link happens not to have a target. So I think the warning should happen anyway, just don't set the target if it is already set. What do you think @mossroy ?

I have tested the functionality, and it works well.

Based on https://developer.mozilla.org/en-US/docs/Web/HTML/Element/a#attr-target , the target can be used to communicate between tabs/windows/frames/iframes etc
I thought it might be safer to not do anything in such cases.
But you're right: we're talking about the case of an external target URL, for which it should be better to warn the user anyway.
I'll change accordingly

mossroy · 2022-05-31T08:58:30Z

I've added the UI option to disable this behavior, in the "display" settings.
But I'm wondering if I should move it to the "expert" settings (and invert the logic, to call it "disableOpenExternalLinksInNewTabs", unchecked by default). It's what we initially intended in #404

Jaifroid · 2022-05-31T09:03:16Z

You mean the warning should be on by default, right? If so, I agree. We like this app to be very cautious about contacting external servers for privacy reasons (which, for example, in a certain large country right now could even entail considerable danger to someone). So I think it's best to warn before accessing such URLs by default. If it is interfering (this is extremely unlikely with currently available ZIMs) then the user can turn it off. Would it be worth adding a line at bottom of dialogue box to say "This warning can be disabled in configuration", or is it better to let user discover the setting if it's annoying them?

mossroy · 2022-05-31T09:21:54Z

The warning is already on by default. I'll had the comment in the dialog box to let the user know he/she can change this behavior.

My questioning is if it should be in "display" settings or in "expert" settings.
If we move it in "expert" settings, it's probably better to invert the setting: checked meaning "disable the opening in new tab" (it's currently the contrary: checked means "open in new tab")

Jaifroid · 2022-05-31T09:25:17Z

Ah, so it's really just about how discoverable this is, and whether we want to discourage people from turning it off. I'm not sure it's really and "Expert Setting". I think of those as settings for the more technically minded, and I don't know if this is the case.

mossroy · 2022-05-31T09:27:51Z

I realize we in fact mix 2 different topics here:

warn the user when an external link is clicked
open the external link in a new tab (which is usually necessary to not break kiwix-js UI)

I don't think it's a big issue anyway

mossroy · 2022-05-31T09:30:06Z

Ah, so it's really just about how discoverable this is, and whether we want to discourage people from turning it off. I'm not sure it's really and "Expert Setting". I think of those as settings for the more technically minded, and I don't know if this is the case.

OK. I'll just add a comment to discourage people from changing the default value

Jaifroid · 2022-05-31T09:31:05Z

Well, they are closely related, so it's OK!

My personal feeling is that it's not an expert setting. I also think that it would be trivial to add the warning also in jQuery mode, for uniformity and for the same privacy reasons. But that's up to you!

mossroy · 2022-05-31T09:36:44Z

I also think that it would be trivial to add the warning also in jQuery mode, for uniformity and for the same privacy reasons. But that's up to you!

You're right, I'm trying to do that

mossroy · 2022-05-31T10:02:32Z

I've implemented the same behavior for jQuery mode: it was indeed straightforward.

To me, this PR is ready for review.
I did not implement the CSP/sandbox discussed in #404 (comment) : I read your comments and see it's not trivial. I see it as a separate issue (#753) that would be in a different PR

Jaifroid · 2022-05-31T10:29:06Z

I've been doing a quick bit of testing. This link fails, and opens in the iframe instead. Any idea why? It's in the English Wikipedia article on poetry.

Jaifroid · 2022-05-31T10:30:02Z

Could it be because the click is registered on the span element, and we're not catching it in the correct bubbling phase?

Jaifroid

It looks good, but I've only tested briefly, and haven't yet tested in IE11 or IE mode with jQuery, only in Chromium.

Jaifroid · 2022-05-31T10:35:29Z

www/js/app.js

+            message += ' (in a new tab)';
+        }
+        message += '<br />' + event.target.href + '<br />(this behavior can be disabled in settings if you know what you do)';
+        uiUtil.systemAlert(message, 'Opening external link', true).then(function (response) {


Instead of <br />, I would try wrapping each part of this message in <p> tags. At the moment it looks crowded and a bit ugly. The URL should be on its own line, with some vertical whitespace between the URL and the surrounding messages.

I agree, will use <p> tags

www/js/app.js

Jaifroid · 2022-05-31T10:42:21Z

www/js/app.js

@@ -1456,6 +1463,20 @@ define(['jquery', 'zimArchiveLoader', 'uiUtil', 'settingsStore','abstractFilesys
                    // Configure home key press to focus #prefix only if the feature is in active state
                    if (params.useHomeKeyToFocusSearchBar)
                        iframeArticleContent.contentWindow.addEventListener('keydown', focusPrefixOnHomeKey);
+                    if (params.openExternalLinksInNewTabs) {
+                        // Add event listener to iframe window to check for links to external resources
+                        iframeArticleContent.contentWindow.addEventListener('click', function (e) {


As mentioned in another comment, it looks like we need some logic to detect a click on an element that contains an anchor element. I think it can be caught by specifying the bubbling phase without having to add lots of new logic, but that's always been a bit obscure to me and it needs some experimentation. This seems to affect some links in Wikipedia citations, for example. If we can't catch it in a simple way, simply inspecting the innerHTML (or other content) of the trapped element for an anchor target should not be hard.

I think I've faced this issue before somewhere, and there was a way to get, using DOM methods, the final target of a click....

mossroy · 2022-05-31T13:21:26Z

This link fails, and opens in the iframe instead. Any idea why?

I managed to reproduce. Will investigate

mossroy · 2022-05-31T13:39:32Z

In the ZIM file where I reproduced the issue, it comes from the fact that there is a <cite> tag inside the <a> tag.
So event.target is the cite tag, and its tagName is CITE instead of A
In this particular case, testing event.parentElement.target would work.
To work in all cases, I suppose we need to browse all parent elements to find the closest <a> tag (if any)

Jaifroid · 2022-05-31T13:55:24Z

Ah, so I think there is a difference between event.target and event.currentTarget (entirely from memory, I haven't checked). Maybe you can check?

Jaifroid · 2022-05-31T13:56:26Z

See https://developer.mozilla.org/en-US/docs/Web/API/Event/currentTarget

Jaifroid · 2022-05-31T14:07:07Z

Come to think of it event.currentTarget will probably be the iframe, right?

mossroy · 2022-05-31T14:14:33Z

Come to think of it event.currentTarget will probably be the iframe, right?

I think so.

In any case, I implemented a fix that seems to work well, in both SW and jQuery modes. When the click event happens, I look for the closest enclosing <a> tag

Jaifroid · 2022-05-31T14:19:50Z

Great! Do you know about Element.closest()? https://developer.mozilla.org/en-US/docs/Web/API/Element/closest. I presume you didn't use because not supported by IE11 (though there is a polyfill, it's overkill for this use case).

mossroy · 2022-05-31T14:35:27Z

Do you know about Element.closest()?

Actually, I did not, thanks a lot for the pointer! Our support of old browsers tend to give me bad habits to do things by hand...

I think it would be a good idea to use that instead of my custom code, and add the polyfill for IE.
It's much more generic

mossroy · 2022-05-31T15:20:44Z

The code is much simpler with closest(), thanks for the hint!
I've included the poyfill, but could not test with IE (or any other browser that would need the polyfill)

mossroy · 2022-05-31T15:29:10Z

The test-offline-files github actions did very well its job to warn me I had to add the new file in the precache list of the serviceworker 👍

Jaifroid · 2022-05-31T16:50:39Z

I've included the poyfill, but could not test with IE (or any other browser that would need the polyfill)

I should be able to test with IE11.

Jaifroid · 2022-05-31T17:36:17Z

While testing I came across this case:

As you can see, because the URL doesn't have any space/break at which to wrap, it bleeds outside of the dialogue box. I think there might be a way to make it wrap regardless of whitespace. But I don't know without checking.

EDIT: to reproduce, search for "wired" in the English Wikipedia "Poetry" article.

Jaifroid · 2022-05-31T17:49:11Z

Not working in IE11, unfortunately. I'll try to determine why. I didn't get the dialogue box, it just showed me this.

Jaifroid · 2022-05-31T18:28:50Z

Looks like the polyfill didn't load correctly.

Jaifroid · 2022-05-31T18:45:07Z

Polyfill is loaded. It's in the Element.prototype, but somehow hasn't attached itself to event.target. When pausing debugger on the click event (jQuery mode) I did some checks in console:

Jaifroid · 2022-05-31T19:54:49Z

OK, I tried several things, I even tried a different polyfill, but none seems to work with IE11. I wonder if it's something to do with RequireJS unloading modules it thinks aren't being used.

Then I remembered that jQuery has a $.closest() function. This is very unlike me, but "JQuery to the rescue!" in this instance! The jQuery function returns the clicked target as the first [0] element of an array, so code needs small adaptation.

We only need to use it in the JQuery mode (obvious, really!). I'll make some suggested changes that I've tested, and they work (on quick test) in IE11. It works with a straight anchor target, and also with the cite element that is wrapped in an anchor target that we tested above.

www/js/app.js

service-worker.js

mossroy · 2022-06-01T07:45:42Z

Many thanks for your tests
I'm reluctant to use jQuery here, as we would like to remove it in #367

Maybe a compromise could be to revert to commit 17e9f19 and, in uiUtil.closestAnchorEnclosingElement function, test if closest is supported. Use it if it's supported, else use the code I had written

Jaifroid · 2022-06-01T08:00:23Z

OK. I'm really puzzled by why the Polyfill wouldn't run in IE11. It must be something to do with Require isolating it, though the Element prototype is patched by the Polyfill.

I think using your handcrafted solution if closest isn't available is a good idea if you would like to avoid jQuery here.

mossroy · 2022-06-01T08:07:33Z

I just pushed it.
There are still 2 things to implement: the text wrap you mentioned, and also consider supporting the same feature with <area> tags (because it's what we already do in jQuery mode).
I'm on it

mossroy · 2022-06-01T10:17:00Z

It's pushed.
But no rush for the review (I'll be AFK for the next 24 hours)

Jaifroid

I tested in Chromium (Edge) in SW and jQuery modes, and in IE11.
It worked fine on the links I tested in both browsers. I didn't test AREA tags that might have external links (I didn't find an example).
I had some very minor code suggestions you can take or leave! (They are just for clarity.)

www/js/lib/uiUtil.js

Jaifroid · 2022-06-02T14:56:51Z

PS Very useful PR, that makes behaviour consistent between Kiwix JS and other Kiwix apps. I'll also add it downstream when merged.

Fixes #404 This is a refresh of the work of Jaifroid in branch https://github.com/kiwix/kiwix-js/tree/Check-for-external-links-and-warn

Jaifroid reviewed May 30, 2022

View reviewed changes

www/js/app.js Outdated Show resolved Hide resolved

Jaifroid reviewed May 30, 2022

View reviewed changes

mossroy changed the title ~~Open external links in new tabs in SW mode~~ Open external links in new tabs, and warn the user May 31, 2022

Jaifroid requested changes May 31, 2022

View reviewed changes

Jaifroid reviewed May 31, 2022

View reviewed changes

www/js/app.js Outdated Show resolved Hide resolved

Jaifroid reviewed May 31, 2022

View reviewed changes

service-worker.js Outdated Show resolved Hide resolved

Jaifroid approved these changes Jun 2, 2022

View reviewed changes

www/js/lib/uiUtil.js Outdated Show resolved Hide resolved

www/js/lib/uiUtil.js Outdated Show resolved Hide resolved

Open external links in new tabs and warn the user

6bc0b93

Fixes #404 This is a refresh of the work of Jaifroid in branch https://github.com/kiwix/kiwix-js/tree/Check-for-external-links-and-warn

mossroy force-pushed the open-external-links-in-new-tab-in-SW-mode branch from 2bb7544 to 6bc0b93 Compare June 2, 2022 15:17

mossroy merged commit acd99a5 into master Jun 2, 2022

mossroy deleted the open-external-links-in-new-tab-in-SW-mode branch June 2, 2022 15:22

Jaifroid mentioned this pull request Jun 16, 2022

Support "page preview" feature of Wikipedia and Wikivoyage kiwix/kiwix-js-pwa#278

Closed

Jaifroid mentioned this pull request Mar 5, 2023

Restrict what an iframe can do, for security reasons #753

Closed

Open external links in new tabs, and warn the user #862

Open external links in new tabs, and warn the user #862

Conversation

mossroy commented May 30, 2022 • edited Loading

mossroy commented May 30, 2022

Jaifroid May 30, 2022

Choose a reason for hiding this comment

mossroy May 30, 2022

Choose a reason for hiding this comment

mossroy commented May 31, 2022

Jaifroid commented May 31, 2022

mossroy commented May 31, 2022

Jaifroid commented May 31, 2022

mossroy commented May 31, 2022

mossroy commented May 31, 2022

Jaifroid commented May 31, 2022

mossroy commented May 31, 2022

mossroy commented May 31, 2022

Jaifroid commented May 31, 2022

Jaifroid commented May 31, 2022

Jaifroid left a comment

Choose a reason for hiding this comment

Jaifroid May 31, 2022

Choose a reason for hiding this comment

mossroy May 31, 2022

Choose a reason for hiding this comment

Jaifroid May 31, 2022

Choose a reason for hiding this comment

mossroy commented May 31, 2022

mossroy commented May 31, 2022

Jaifroid commented May 31, 2022

Jaifroid commented May 31, 2022

Jaifroid commented May 31, 2022

mossroy commented May 31, 2022

Jaifroid commented May 31, 2022

mossroy commented May 31, 2022

mossroy commented May 31, 2022

mossroy commented May 31, 2022

Jaifroid commented May 31, 2022

Jaifroid commented May 31, 2022 • edited Loading

Jaifroid commented May 31, 2022

Jaifroid commented May 31, 2022

Jaifroid commented May 31, 2022

Jaifroid commented May 31, 2022 • edited Loading

mossroy commented Jun 1, 2022 • edited Loading

Jaifroid commented Jun 1, 2022

mossroy commented Jun 1, 2022 • edited Loading

mossroy commented Jun 1, 2022

Jaifroid left a comment

Choose a reason for hiding this comment

Jaifroid commented Jun 2, 2022

mossroy commented May 30, 2022 •

edited

Loading

Jaifroid commented May 31, 2022 •

edited

Loading

Jaifroid commented May 31, 2022 •

edited

Loading

mossroy commented Jun 1, 2022 •

edited

Loading

mossroy commented Jun 1, 2022 •

edited

Loading