Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

kiwix-serve 3.4.0: Opening in a new tab opens the content frame only #591

Open
JensKorte opened this issue Dec 18, 2022 · 26 comments
Open
Assignees
Milestone

Comments

@JensKorte
Copy link

In past I was able to open a link in a new tab and got the normal page served. Since 3.4.0 I just get the content frame opened in a new tab.

Would it be possible to link to the the whole frameset?

@kelson42
Copy link
Contributor

@JensKorte What do you do exactly from kiwix-serve welcome page?

@kelson42 kelson42 added this to the 3.5.0 milestone Dec 18, 2022
@kelson42 kelson42 self-assigned this Dec 18, 2022
@kelson42 kelson42 changed the title kiwix-serve 3.40: Opening in a new tab open the content frame only kiwix-serve 3.4.0: Opening in a new tab open the content frame only Dec 18, 2022
@JensKorte
Copy link
Author

/kiwix-tools_linux-armhf-3.4.0/kiwix-serve -zp 8888 *zim

Starting with the start page: http://pi400.fritz.box:8888/viewer#wikipedia_en_all_maxi/A/User:The_other_Kiwix_guy/Landing when middle clicking or using "Open in new tab" http://pi400.fritz.box:8888/content/wikipedia_en_all_maxi/A/Architecture gets opened. The difference is in /viewer#... and /content/ .

@kelson42 kelson42 assigned veloman-yunkan and unassigned kelson42 Dec 18, 2022
@kelson42
Copy link
Contributor

Indeed, I really wonder how we have missed that one!

@JensKorte JensKorte changed the title kiwix-serve 3.4.0: Opening in a new tab open the content frame only kiwix-serve 3.4.0: Opening in a new tab opens the content frame only Dec 18, 2022
@kelson42 kelson42 added bug and removed question labels Dec 19, 2022
@kelson42 kelson42 pinned this issue Jan 6, 2023
@kelson42
Copy link
Contributor

@veloman-yunkan Can we move forward on that one?

@veloman-yunkan
Copy link
Collaborator

@kelson42 I have thought about this issue a few times since it was filed and it looks like there is no simple way to fix it.

An approach that might work is like follows:

  1. Rewrite links inside an iframe so that they represent a viewer URL that would display the target inside a viewer.
  2. If the user opens such a link inside a new tab it will be automatically displayed inside a viewer.
  3. If the user simply clicks such a link the result would be a nested viewer inside a viewer. The viewer must detect such a situation and fix it by eliminating the nested viewer.

But I am afraid that this workaround (which will make the code more complex) will run against certain issues in some corner cases (like we had with the iframe and PDFs) that - if solvable at all - will require further sophistication of the code. I don't want to end up in a situation where we eventually regret that we chose to try to fix this ticket.

@JensKorte
Copy link
Author

How about always linking to the whole frameset and always open it in the whole tab? The menu frame would always be reloaded.

@kelson42
Copy link
Contributor

kelson42 commented Apr 28, 2023

@veloman-yunkan First of all, we will have to do something because we can not live with kiwix-serve disallowing proper opening of links in new tabs.

I share as well your disatisfaction of polluting the viewer with DOM impacting Javascript. This for the very same reasons you have given. On my side there is no return. I still consider our move to iframe like a good move... by far.

I'm pretty concern of modifying the DOM and this should be avoided AFAP. What about attaching an event to links a catch and handle like this?

@Jaifroid @juuz0 Your experience with these kind of things could be helpful here.

@Jaifroid
Copy link
Member

It's the same situation with the PWA: if you right-click / middle-click / ctrl-click, you'll open a new browsable tab, but that tab won't have any chrome (UI, buttons, etc.). Personally, I think this is acceptable. People usually want to open a new tab in order to keep a particular piece of information available, and there is one advantage of being able to open a new chromeless tab: it is much easier to print an article this way, or print/save it to PDF (for example).

Personally, therefore, I wouldn't fix this, but see it as a feature.

However, if you want to "fix" it, then instead of rewriting the DOM, you can attach an on-click event listener quite easily to the iframe content window. When anything is clicked in the iframe, the event listener inspects what was clicked and, if it's a ZIM link, then it can easily be redirected to the framed version. Of course this might not be "safe" in all cases, because Zimit ZIMs may have event listeners of their own attached to lnks.

You could make it an option, but then you have to deal with cookies / local storage and storing user preferences.

I can suggest event-listener code (I previously provided it for the case of opening PDFs in a new tab), if you decide to go down this route.

@JensKorte
Copy link
Author

JensKorte commented Apr 28, 2023

If people use the text frame without the upper frame for printing, maybe there could be an extra url, like /print or "/contentonly" instead of viewer content, where every page get viewed without the upper frame.

At https://stackoverflow.com/a/18428669 I found the following. Is it the same as @Jaifroid suggested in the answer before?

If you want to work around it and force the framesets to be present in new windows, only viable option that comes to my mind is to have a javascript check on top of every child page for the frameset, and if there are none, redirect back to frameset page.

var isFramesetAvailable = (window.top !== window.self);
if (!isFramesetAvailable) {
  window.location.href = 'your_frameset_page.html#target_page';
}

and then your frameset page would check for the #target_page url fragment, and if it exists, load that frame.

@rgaudin
Copy link
Member

rgaudin commented Apr 28, 2023

I've noticed it but it didn't bother me. On a second thought, it is indeed unexpected and inconsistent.

I am also in favor of adding an event listener to the viewer.

@mgautierfr
Copy link
Member

I see several solutions for this issue (without being sure the solutions are actually doable):

  • Intercept all user events and change the url (as proposed)
  • Set a "context" (headers, cookies, referrer ?) to the iframe and let the server detect this context and redirect to the viewer when needed.
  • Use a service worker to catch requests

It is not clear to me how the target=blank or middle-click opening interfere with event listener of service workers we may install. I don't know if the target is applied "after" (and so we can catch requests) or before (and so we are in a new context "too soon").


Having a way to catch everything (service worker ?) may also help us to fix all issues with securing zim files don't break navigation by changing top target and securing specific contents (pdf, external links) are still working.

@rgaudin
Copy link
Member

rgaudin commented May 3, 2023

I suggest we give the first option a try as this is the one with the least uncertainty.

Mandatory warning about SW:

  • two SW can't operate on the same prefix (that's the point of the SW)
  • SW requires a different security context: being accessed via HTTPS or via localhost

@Jaifroid
Copy link
Member

Jaifroid commented May 3, 2023

Service Worker would be a sledgehammer to crack a small nut, even if it could be made interoperable with the Service Worker in the ZIMs. This could be dealt with using the same script as suggested for kiwix/libkiwix#943. In fact I'd suggest considering these two issues together, as a fix for one should be tooled in such a way that it fixes the other.

@veloman-yunkan
Copy link
Collaborator

veloman-yunkan commented May 4, 2023

However, if you want to "fix" it, then instead of rewriting the DOM, you can attach an on-click event listener quite easily to the iframe content window. When anything is clicked in the iframe, the event listener inspects what was clicked and, if it's a ZIM link, then it can easily be redirected to the framed version. Of course this might not be "safe" in all cases, because Zimit ZIMs may have event listeners of their own attached to lnks.

The problem is that open-in-new-tab/window actions, when selected from a context menu or initiated via a middle-click, don't generate on-click events.

@JensKorte
Copy link
Author

The problem is that open-in-new-tab/window actions, when selected from a context menu or initiated via a middle-click, don't generate on-click events.

How about declaring the middle click to be the default, making suitable links for it and the left-clicks get rewritten using on-click events?

@Jaifroid
Copy link
Member

Jaifroid commented May 4, 2023

The problem is that open-in-new-tab/window actions, when selected from a context menu or initiated via a middle-click, don't generate on-click events.

Ctrl-click can be trapped pretty decently. Middle click is a bit more complex but possible. Right-click is very complicated. You can override it, but then the context menu won't show, and there's no API for showing context menu in JS.

@JensKorte I'm not sure what you mean by declaring middle-click to be the default. Middle-click is only available on some machines/mice and also only really supported on Linux. Remember Mac users only have one mouse button, and we can't really cater to mobile users because long-press is REALLY complicated to support in JS.

I'd say it's only really feasible to support ctrl-click in this situation. I have some code for testing if ctrl-key (or command key) is down when the click is trapped.

@JensKorte
Copy link
Author

@Jaifroid: How about providing links that are suitable for opening in a new tab. If the user uses left-click and opens it in the same tab, the link can be rewritten, as far as I understand this.

@Jaifroid
Copy link
Member

Jaifroid commented May 4, 2023

I'm not sure what "providing links that are suitable for opening in a new tab" means. If you mean rewriting all the links server-side, it can of course be done (as we control the server), but you risk destroying functionality in highly dynamic ZIMs such as Zimit. And although we can intervene in right-click, people may use it because they want to download an image rather than open a new tab (in fact more often than not). So we want to intervene as little as possible.

I don't know what others feel, but I would say that the only thing we really want to do is automatically open external links in a new tab (not inside any iframe). Opening ZIM links in a new tab is something that a user may choose to do through ctrl-click, middle-click or right-click (standard browser habits they know), and it will work, but the new tab won't have the iframe or any chrome. It will be just like browsing the "pure" original Web site. To me, this actually feels more like a feature than a bug...


TLDR: I'm advocating for fixing only kiwix/libkiwix#943 (because it's terrible UX otherwise), and closing this issue as won't fix. Or if you like, you can go down the rabbit-hole of trying to trap every user gesture and second-guess what they want to do. 😯 (Full disclosure, I went down this rabbit-hole in Kiwix JS, and we ended up not merging a time-consuming PR because it just wasn't safe.)

@juuz0
Copy link
Collaborator

juuz0 commented May 5, 2023

there is one advantage of being able to open a new chromeless tab: it is much easier to print an article this way, or print/save it to PDF (for example).

Out of context for this discussion but this sounds like a different issue, if we want we can override the Print shortcut/add a print button in the taskbar.

@Jaifroid
Copy link
Member

Jaifroid commented May 5, 2023

I don't think it's out of context to explain that some people may want to open a window or tab without any chrome (and then explain why). Printing was just one example of why...

@juuz0
Copy link
Collaborator

juuz0 commented May 6, 2023

I meant my suggestion to fixing that was out of context of this issue. I made an issue #619 to bring it to light

@mgautierfr
Copy link
Member

This make me think about a old idea I had at the time to add the toolbar into the content.
Could warc2zim insert a link to a small js in all html contents ? Something like : <script src="/viewer_check.js"></script> in head section.

This script would be provided by kiwix-serve itself and its checks if viewer is actually in place and redirect to /viewer#url if needed.

@Jaifroid
Copy link
Member

Jaifroid commented May 9, 2023

No problem if you want to inject such a script into the landing page HTML when Kiwix Serve reads the HTML. However, I don't think it should be in the ZIM archives themselves. Warc2Zim would only be a solution for Zimit archives in any case, not for all the others. Other readers that provide their own UI would need to eliminate such a script. Much better to add it if needed only.

Or... simply accept that new windows are opened without any chrrome, and see it as a feature! It's like you're browsing the original web site...

@mgautierfr
Copy link
Member

Well, I have mixed things. It is not about warc2zim. It is about our viewer.
My proposition is that the inclusion should be in all zim files. The script itself is not in the zim file and is provided by the reader (kiwix-serve here). Another readers (kiwix-js, kiwix-desktop) may return different content as they need it.

@JensKorte
Copy link
Author

It's like you're browsing the original web site...

No, Kiwix lacks the search bar (and even the template at the end of the page). Compare https://en.wikipedia.org/wiki/Kiwix#External_links and https://library.kiwix.org/content/wikipedia_en_all_maxi_2023-04/A/Kiwix (full page: https://library.kiwix.org/viewer#wikipedia_en_all_maxi_2023-04/A/Kiwix )

@kelson42
Copy link
Contributor

@veloman-yunkan I think this is really important and urgent now to fix this ticket. Could you please have a look?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

7 participants