Search results list fails to close when typing quickly

[danielzgtg]

The search results sometimes don't close after navigating to the page. This occurs mainly when performing actions quickly.

Steps

1. Open a zim
2. Type an article into the search box. Do so quickly enough so that the results don't pop up before you finish typing
3. Spam press the enter key
4. Repeat steps 2-3 until the bug occurs

I am running locally off of fdb4238, but this occurs on the deployed kiwix-js 3.7.2 too.

Workaround

header#top:has(>form#formArticleSearch>div.container>div.row>span.col-10>span.input-group>input#prefix:placeholder-shown)+article.view-content>div.container#articleListWithHeader { display: none !important } should work on Chrome. Sadly, I'm on Firefox which hasn't enabled the :has selector because they haven't implemented this selector's support for pseudo-classes.

Unfocusing the input box by clicking somewhere else on the page might work. It's not very convenient because I want to use applications with my keyboard only when I need to work efficiently.

Expected Behavior

The search results list should close after navigating to each page. It should be closed whenever the search box is empty.

Actual Behavior

The bug does not occur all the time. In the video below, it happens after 1:00.

2023-03-12.09-10-34.mp4

2023-03-12.09-41-00.mp4

[danielzgtg]

I would like to work on this today.

[Jaifroid]

Very happy for you to work on it! Will assign you.

[Jaifroid]

Just a guess: the field is designed so that if a user clicks out of it, the search results disappear, but if the user then clicks back in it without typing anything more, the previous search results will show again. That part is intended, and is useful. But it looks like there is a race condition between the hiding and the showring. It might be worth checking if there is any jQuery still being used for hiding and showing (can be slower than native DOM methods, and isn't necessary). But that's just a guess. Otherwise, it's possible that a chain of mousedown -> click -> mouseup events is not properly synchronized.

[danielzgtg]

That part is intended, and is useful

Thank you for telling me about this feature. I never noticed as I always press enter right away. I was trying to find it after I pressed enter, but only found it when I stopped pressing enter.

can be slower than native DOM methods, and isn't necessary

I don't think slowness matters for Javascript synchronization problems. The only thing it could do to break things is to make things asynchronous with a setTimeout or something, and I don't see the reason why it would do something like that.

mousedown -> click -> mouseup

I had my hand off the mouse except at the start. Though along the same lines it might be kepup -> keydown.

I just finished comparing the network requests to be identical, and also watching the videos in slow motion. Next I will start with console.log.

[danielzgtg]

I traced down the root cause of the bug, and increased to reproduction rate above 10%. This occurs 100% of the time after adding .then(x => new Promise(resolve => setTimeout(() => resolve(x), 5000))) to

kiwix-js/www/js/lib/zimArchive.js

Line 255 in f5c749a

if (LZ) that.findDirEntriesFromFullTextSearch(search, dirEntries).then(function (fullTextDirEntries) {

. Further debugging with console.log(search.status) before the line setting it confirms this is the problem.

So the normal sequence is "Search Click -> Variant Callback -> Fulltext Callback -> Results Entry Click". If the enter key is pressed quickly enough, we get "Search Click -> Variant Callback -> Results Entry Click -> Fulltext Callback", which kiwix-js didn't handle properly.

[Jaifroid]

Ah, looks like a race condition caused by handling things outside of a Promise that should be done inside. Thanks for the diagnosis! (Full-text search was somewhat monkey-wrenched into the code.)

N.B. Full-text search results are pretty slow, hence we shouldn't wait for them if the user has already got what they want (which occurs 95% of the time just with title search).

[danielzgtg]

I also tested kiwix-js-windows. The check is similarly missing but even with the delay it is not affected by this bug. It is neither affected by the bug of older fulltext searches overwriting the next search. There is a minor thing where you can see the search results popping up after the delay if you refocus the search box right away and wait. That little thing isn't important as not many users would intentionally seek to expose this behavior, and the code is longer there.

[Jaifroid]

Thanks! Well I might as well backport (or feel free to if you would like, especially given it's pretty much a one-liner). I'm just waiting on Code QL to approve your PR (which I quickly tested, though I didn't try to reproduce the actual race condition, I just tested the cancellation was working as expected: all good).

[danielzgtg]

When I looked at the code in kiwix-js-windows, it didn't seem like a one-liner. There are many more callers of various methods. Eventually I narrowed it down to 2 lines that needed be be added. I backported it in case we forgot the other line.

[Jaifroid]

Many thanks indeed! I'll take a proper look / test the KJSWL PR tomorrow.