Non-file Dragging Bug Fix

[D3V-D]

For issue #1037

Tests done:
npm run serve
npm run preview
npm run build, followed by npm run serve and using the localhost:xxxx/dist/? page. (should be same as running the extension)

Tested on both firefox and chromium.
Tested with both mdn web docs (which uses workers) and a smaller page that doesn't use workers. Tested in both jQuery and workers mode. Passed npm test.

Ran e2e-test for firefox, just one error as seen below:

1) Gutenberg_ro test [ZSTD compression] [SW mode]
       Sorting books by name:

      AssertionError [ERR_ASSERTION]: 'Poezii' == 'Creierul, O Enigma Descifrata'
      + expected - actual

      -Poezii
      +Creierul, O Enigma Descifrata

On e2e-test for chromium, also just one error, as seen below:

1) Gutenberg_ro test [ZSTD compression] [SW mode]
       Change Language:
     JavascriptError: javascript error: {"status":11,"value":"Element is not currently visible and may not be manipulated"}
  (Session info: chrome=124.0.6367.91)

I suspect these are unrelated to my changes, but please let me know if I broke something.

Other than that, I do have one question; is there a way to know if the ZIM page has already been loaded? With my current solution, it takes them unnecessarily to home if they dragleave the page, even if no ZIM file has been loaded. Also, if there's a way to keep track of their last visited page, that could greatly improve the experience when having dragged a file over but not having dropped it on the page (right now, it will take you back to the home page of the ZIM). @Jaifroid

[Jaifroid]

@D3V-D Thanks very much for this PR! It's passed testing, and I've also tested manually -- it seems to be working perfectly as intended. Well done. I'll do a code review shortly.

[D3V-D]

Sounds good! Do let me know if you find any answers to my questions above. @Jaifroid

[Jaifroid]

@D3V-D Regarding your test failures, I'm not seeing them in the CI (you can probably look at the CI test runs below), so they are probably a result of your environment / speed of CPU, etc. E2E tests are quite sensitive to timings and have been optimized to run on GitHub, where they are passing. We also have BrowserStack texts, but they can't run from a PR without access to the secrets, but I don't anticipate any issues given what your PR touches (we'll know for sure after merge).

Regarding knowing whether a ZIM is loaded: yes, simply call selectedArchive.isReady() in app.js (not currently available in other modules, but that could be changed if necessary). If you want to know whether a specific ZIM is loaded, then look at the selectedArchive.name field (if it exists, then the archive is loaded).

[Jaifroid]

It's looking good! Just some very minor suggestions mostly about house style.

[D3V-D]

Ok! I'll work on the commenting and variable names. Thanks for the answer about seeing if a page has been loaded and what page it is; I'll try to add those to the implementation while I'm at it.

[D3V-D]

I'm not at my computer (not home rn) so I don't know if this would be trivial or more difficult, but if I store the current page at time of drag over, I should be able to just load back into that page after going to home right? (When they drag leave)

[Jaifroid]

@D3V-D I tested the compiled version in IE11, and unfortunately drag-and-drop now only works if Config is open. It no longer works dragging the ZIM onto the app with a ZIM already open and displayed. Additionally (and it's probably related), the red "drop-zone" outline no longer shows in IE11, whereas it does in modern browsers. On main, both of these things work.

We use IE11 as a proxy for older browsers (if it works in IE11 it'll work everywhere!). I think this is why I originally coded two drop zones, because the global drop zone isn't working in all browsers (I realize I should have left a comment explaining this if it's the case). Alternatively, the explanation may be because some of the newer events you are using are not supported in older browsers, and in that case it might be enough to test if the API exists before attempting to use it, and fall back to the default API if not.

In Windows, you can easily test on IE11 by using IE11 Mode in Microsoft Edge. However, debugging is a bit trickier in that context, and it may be easier to use a simple trick to open IE11 itself (I can give you details via Slack if you have access to Windows 10/11).

[D3V-D]

Hey, yeah I was a little worried about IE11 since I'm on a Linux machine and I don't have it installed. If needed I can run a windows VM and run it for testing; let me know if that would work

Though, I think I checked most things I used with caniuse for IE11 compatibilit, so it's probably more about weird iFrame behavior. Probably a modification to the drag over event on the iFrame might be necessary; not 100% sure though.

[Jaifroid]

I'm not at my computer (not home rn) so I don't know if this would be trivial or more difficult, but if I store the current page at time of drag over, I should be able to just load back into that page after going to home right? (When they drag leave)

No need to store anything. In fact there is an easy way to restore the currently viewed article without reloading the landing page. Look for function showReturnLink in uiUtil.js, and copy what happens in the viewArticle click event listener in that function. That hides config and shows the currently loaded article, without reloading anything.

[D3V-D]

That sounds very convenient; thanks for letting me know about it.

[Jaifroid]

Hey, yeah I was a little worried about IE11 since I'm on a Linux machine and I don't have it installed. If needed I can run a windows VM and run it for testing; let me know if that would work

Yes, that would work. Or you can try using https://caniuse.com/ to check which APIs you're using aren't available on IE11 and add a fallback, and I'd be happy to test. If that doesn't work, you may have to restore the Config zone in addition to the global drop zone.

[D3V-D]

@D3V-D I tested the compiled version in IE11, and unfortunately drag-and-drop now only works if Config is open. It no longer works dragging the ZIM onto the app with a ZIM already open and displayed. Additionally (and it's probably related), the red "drop-zone" outline no longer shows in IE11, whereas it does in modern browsers. On main, both of these things work.

Won't know till I can do some testing, but this may be part of the problem, seems to be an IE11 quirk. Do contact me on slack if you want to discuss it more in-depth

[D3V-D]

I figured out the issues with IE11; both the .startsWith() and the .includes() functions are not supported, so I had to use some more basic functions that effectively achieve the same goal.

Additionally, I implemented the other features, so now dragging over on config page when no ZIM is loaded no longer unnecessarily takes you back to the home page, and dragging over and exiting doesn't unnecessarily load the home page of the current ZIM but rather just restores the last page that was open.

I also fixed up the commenting and variable names.

Please test for IE11 @Jaifroid and let me know if its working as intended now!

[Jaifroid]

OK, thanks -- I'll check, although note that we use core-js so startsWith, etc., should be polyfilled when the app is built (which is necessary for IE11 to work in any case). However, there's no problem with using more universal JS in any case -- often a lot of modern methods are just sugar.

[Jaifroid]

Congratulations @D3V-D, I confirm that IE11 is now working fine! Tests are passing, so I'll do another code review asap.

[Jaifroid]

This is looking very good. I've got some minor suggestions and a query. But this is functioning very well!

[D3V-D]

Didn't mean to resolve that one, but please do read the comment under it.

As for the others, I'll make some last edits.

[D3V-D]

Alright, I've removed the blank line and shortened the checks on the if to just use the enteredElement.

Please let me know if there's anything else @Jaifroid! If not, I can make that PR for moving the function to uiUtil once this has been merged.

[Jaifroid]

That's great! My last manual tests -- including dropping all the parts of a split ZIM -- all passed, so this is ready to merge. Let me know when you're happy for me to go ahead. It would be great if you could make an issue for moving the returnToCurrentPage function to uiUtil.js (before making a PR for it), so I can formally assign you to it. It should be a very quick PR.

[D3V-D]

Feel free to merge right now. I'll write up the issue