kiwix / kiwix-js

Fully portable & lightweight ZIM reader in Javascript
https://www.kiwix.org/
GNU General Public License v3.0
310 stars 135 forks source link

Backport popover code from Kiwix PWA #1252

Closed Jaifroid closed 5 months ago

Jaifroid commented 6 months ago

Fixes #719. This backports the new feature from the PWA.

So far, I have ported the mouseover and the focus events, together with the popover code. Focus event provides keyboard support (tabbing into a link).

To do:

audiodude commented 6 months ago

image

Jaifroid commented 6 months ago

Nice one @audiodude! For now, I'm still working through things and fixing stuff, but I'll be very pleased to request a review when it's ready! Still some rough stuff...

Jaifroid commented 6 months ago

Pointerdown fallback is working on Edge Legacy at least, but user has to dismiss the popover with the X icon if evoked by touch rather than by hover. That seems like an acceptable compromise for such an old browser. However, it will be important to prevent popup from loading if a click event is registered, as currently with pointerdown fallback, the browser opens the popover and simultaneously navigates to the requested page if user completes a click (rather than long press for example).

Jaifroid commented 6 months ago

Now working in Safari on iOS, but user has to lift finger after a long-press for the popover to show. Apparently iPad users are used to this weird behaviour, so it's OK... ㄟ( ▔, ▔ )ㄏ

Jaifroid commented 6 months ago

On Safari for macOS, the popover loads fine when hovering a ZIM link, but it does not remain open when the user moves the mouse over the popover (to select a link in it, for example). However, this works in the PWA on the same browser, so I need to debug the difference between the two implementations. On other browsers, the popover does remain open while hovering the popover div... Sigh. EDIT: I've determined this issue ONLY affects the landing page of Ray Charles ZIM, which has some formatting issues. On mdwiki ZIMs, the issue doesn't exist, and also on any normal Ray Charles page. So I'm putting this down to a quirk that is not worth trying to fix.

Jaifroid commented 6 months ago

@audiodude I'm not sure if this PR will make much sense outside of the convoluted context of Kiwix JS, but you did kindly offer to take a look! Some considerations / background:

Anyway, I hope this context is useful! Please don't feel you have to spend much time on this. It's a large new feature arising from the Hackathon...

Jaifroid commented 6 months ago

Implementations of this branch for in-browser testing:

Be aware that if user opens a new tab / window, links in it are not currently tracked and processed by the popover code, unlike in the PWA. See #1253.

audiodude commented 6 months ago

Okay! Let me dive and and take a look at this tomorrow (Thursday)!

Jaifroid commented 6 months ago

Thanks for those useful comments 😊. I'll deal with those before you get to uiUtil.js.

Jaifroid commented 6 months ago

@audiodude All done, should be ready now for you to move on to uiUtil.js when/if you have time. I've left a few comments open above for you to review, and I think everything else you suggested is resolved. Thanks once again! It's been a while since I've had the benefits of review (since the previous lead developer left the project). 😊

Jaifroid commented 6 months ago

New test deployment as v4.0.9 here:

Ensure it has updated to v4.0.9 (message in Configuration should appear after 15s) and restart or reset if necessary. In Firefox a Reset (Expert Settings) is often needed due to few-hours limit on Service Worker checking.

Jaifroid commented 6 months ago

@audiodude Many thanks for helpful comments once again. It's getting towards Friday evening here, so I'll try to resolve everything this weekend. Have a great weekend when you get there! 😊🍾

Jaifroid commented 6 months ago

New test deployment as v4.0.91 here:

Ensure it has updated to v4.0.91 (message in Configuration should appear after 15s) and restart or reset if necessary. In Firefox a Reset (Expert Settings) is often needed due to few-hours limit on Service Worker checking.

Jaifroid commented 6 months ago

Screenshots in light and dark mode:

image

image

Jaifroid commented 5 months ago

@audiodude Are you happy with the resolved issues? OK to merge after final testing?

NB future PRs will certainly not be as massive as this! This was the outcome of work at the Hackathon, hence a major feature. It's rare to do major features! At some point there will be a PR (see #1253) to add these popovers to new tabs / windows controlled by the app's Service Worker, but not for this PR which is feature-complete as far as I can manage for now. Adding to new tabs or windows needs careful thought, as we have to keep track of opened windows, which we currently don't do explicitly (just let the SW do that job, but it knows nothing about the DOM).

audiodude commented 5 months ago

Sorry for the delay. LGTM.

Jaifroid commented 5 months ago

Sorry for the delay. LGTM.

Many thanks for your help @audiodude -- you really helped improve the code! 🙏

Jaifroid commented 5 months ago

Last manual tests:

Good to merge.