kiwix-js
kiwix-js copied to clipboard
Backport popover code from Kiwix PWA
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:
- [x] Add touch support (long press)
- [x] Add pointerdown fallback for browsers that don't support touchstart (e.g. Safari on macOS, and Edge legacy)
- [x] Fix hover over popover in Safari for macOS
- [x] Disable the contextmenu event if popovers are enabled
- [x] Enable for Safe mode (without image support, as in the PWA)
- [x] Add the option to disable popover preview in the UI, with translations
- [x] Fix support for dark modes
- [x] Move as many inline styles as possible to stylesheet
- [x] Suppress popover loading if user has clicked the link
- [x] Add e2e test for presence of popover on tab into link (Ray Charles ZIM)
- [x] Make the breakout icon open a new window
- [x] Fix clicking on a link inside the popover
- [x] ~~Add popover support to new windows~~ Open a new issue to add popover support to new tabs/windows: #1253
- [x] Clean up code hangovers from the PWA that are not needed here
- [ ] Final testing
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...
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).
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... ㄟ( ▔, ▔ )ㄏ
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.
@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:
- The main things to focus on are the functions
attachPopoverTriggerEventsandhandlePopoverEventsin www/js/app.js, andgetArticleLede,populateKiwixPopoverDivandcreateNewkiwixPopoverContainerfrom www/js/lib/uiUtil.js. Most other changes are either incidental variable changes/renaming and a few other utility functions for closing stuff and cleaning up. - Be aware that this Repo supports pretty much every browser since IE11, given target users often have very old devices/OSs, and this means lots of little quirks and tweaks and fallbacks, worked out empirically with manual testing... i.e., a royal pain in the proverbial. For example, Firefox OS (the app's original target!) doesn't support adding stylesheets to the DOM in an iframe even if same origin, so instead I have to add a style element with the style content. Loads of other stuff like that even for some modern browsers.
- Historic coding target was ES5 with JQuery and Common-JS modules. I've ripped out almost all JQuery, converted the codebase to ES6 modules, and we now use rollup and core-js as bundlers / polyfillers, but they don't cover everything.
- Because of historic coding target, there are no classes or other more complex structures, and the original coding style was object-oriented as opposed to functional. Constructors are functions rather than classes, etc. We can use async functions thanks to rollup, but a lot is originally coded with Promises, even callbacks. I tend to use Promises liberally because I got used to them...
- The app runs in two different content injection modes, one for browsers that support Service Workers, and a mode originally called "JQuery mode" now renamed "Safe Mode" in the UI, but you'll see references to jquery mode in code. The popovers are simplified in this mode: only textContent and no image.
- We have e2e tests of most old platforms via BrowserStack. I added a UI test for these popovers in tests/e2e/spec/legacy-ray_charles.e2e.spec.js. It tests tabbing into a link (hovering is way too complicated to test in WebDriver) and then tests that a popover was displayed.
- Unit tests are........... very basic and use a really old QUnit. They run in browser context rather than in Node context....... (I know)..... And basically need a major upgrade. So I haven't (shock horror) written Unit tests for this PR. Testing relies entirely on the e2e UI test. That's not satisfactory, but until I've done the work to upgrade Unit tests to Mocha, it would be pointless to try to write such tests now.
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...
Implementations of this branch for in-browser testing:
- Unbundled (only works in browsers supporting ES6 modules natively); https://kiwix.github.io/kiwix-js/www/index.html
- Bundled but not minified: https://kiwix.github.io/kiwix-js/dist/www/index.html
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.
Okay! Let me dive and and take a look at this tomorrow (Thursday)!
Thanks for those useful comments 😊. I'll deal with those before you get to uiUtil.js.
@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). 😊
New test deployment as v4.0.9 here:
- (browsers with ES module support) https://kiwix.github.io/kiwix-js/www/index.html
- (bundled but not minified) https://kiwix.github.io/kiwix-js/dist/www/index.html
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.
@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! 😊🍾
New test deployment as v4.0.91 here:
- (browsers with ES module support) https://kiwix.github.io/kiwix-js/www/index.html
- (bundled but not minified) https://kiwix.github.io/kiwix-js/dist/www/index.html
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.
Screenshots in light and dark mode:
@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).
Sorry for the delay. LGTM.
Sorry for the delay. LGTM.
Many thanks for your help @audiodude -- you really helped improve the code! 🙏
Last manual tests:
- Works fine in Firefox 65 (tested on Big Sur). Below Firefox 65, WebP images are not supported by Firefox, so the images will not show. I think it would complicate the code tremendously to try to run the WebP Hero polyfill on popover images for such barely used and obsolete browsers. If user wants to see image, they can open the page and the polyfill will then be run. To be clear, there are only a tiny number of Firefox versions affected: those that don't support WebP and DO support Service Workers, i.e. Firefox 56-64 (our minimum supported Firefox version for ServiceWorker mode is Firefox 56, though in practice SW mode only works reliably on all platforms on non-ESR versions of Firefox 60+).
- Works fine in Safari 14 on macOS
- Works fine in Safari 14 on iPad
- Works fine on Edge 18 on W10
- Works fine (Safe Mode only) in Edge 16 on W10,
Good to merge.