Conditional statement 'bug'

[rasmuskt]

General info

• Version: 1.0.0-rc3

Issue
After reviewing the newest version of the code, the following conditional statement has an issue in the second case (line 921-922).

if (new_range_start_index >= old_range_start_index && old_range_end_index === new_range_end_index
|| new_range_end_index === old_range_end_index && old_range_end_index >= new_range_end_index) {

The second case new_range_end_index === old_range_end_index && old_range_end_index >= new_range_end_index equals comparison does not make sense . The intention of the case is to test for 'near top', to mirror the case of 'near bottom'. By mistakenly referring to the end-ranges in the equals comparison, it fails to be a 'near top' test, and instead sort of nullifies the second case.

Fix
The second case should instead be changed to new_range_start_index === old_range_start_index && old_range_end_index >= new_range_end_index

[bigopon]

Thanks @rasmuskt . What file are you pointing at? Can you give some link. Also, if it's a bug you are hitting, maybe a reproduction of the bug will make it easier to make progress.

[rasmuskt]

The file I looked at, is located at the following path:
node_modules\aurelia-ui-virtualization\dist\native-modules\aurelia-ui-virtualization.js

I tried locally changing the condition to see if it would resolve my issue, which it did not. As I am still too unsure what might be causing the issue I am having, I think mixing that issue with this issue would be a mistake.

I can briefly mention that I seem to have an issue, where I do not hit any of the scrolling cases, as I can see in the console that I reach 'Scroll intersection not handled...'. Which was the reason I was reading through this part of the code.

[bigopon]

Thanks, though it's still confusing what issues you are running into, please help with a minimal repro that demonstrates the issue.

[rasmuskt]

(Jakob Gaardsted here, writing instead of Rasmus).

The issue is that the logic in _handleScroll is incorrect (in line 657),
in https://github.com/aurelia/ui-virtualization/blob/master/src/virtual-repeat.ts
which we discovered reading through it.
It splits the range-change into 6(7) cases.
The first two cases cover scrolling-to-top and scrolling-to-bottom.
The outer 'if' is supposed to catch those two cases.
The expression intended to catch 'reached top' is where the 'bug' is.
It is supposed to act on xxx_range_start, but is incorrectly expressed with range_end,
which renders the whole expression void, since the actual range-check also acts on range_end.

I therefore do not believe there is a reason to make a repro as it will be 'reproduced' whenever someone tries to use
the scroll effect of virtual-repeat-for.

Triggering it is not related to how you use virtual-repeat, but to how the user interacts with the UI
(ie it needs to trigger/involve scrolling-to-top).

We are really after a wholly different bug - we routinely see _handleScroll's case 7 (the 'error/conflict catch')
trigger both in development and production, which demonstrates buggy scrolling.
That is the reason we are scrutinizing/reviewing the file
https://github.com/aurelia/ui-virtualization/blob/master/src/virtual-repeat.ts
During our reading of that file, we stumbled upon this bug in virtual-repeat.ts,
and thought it best to report it to you. We don't currently believe these two bugs are related,
but we thought it unfortunate that the code for _handleScroll is not correct,
and that's why we are bringing it to you.

If the following expression is really correct, then I think it is a very weird way to write it??
what would be the POINT of that ">=" check, if we just established "===" to the left??
On the other hand, IF it's replaced with new_range_START_index === new_range_END_index,
then the code makes logical sense, and is consistent with the rest of the function.

// scrolling up and reach top
|| new_range_end_index === old_range_end_index && old_range_end_index >= new_range_end_index

Rasmus will go on vacation from today, but I will continue to check up on this issue ticket, please let us know if we have overlooked anything. Or if you would like us/me to elaborate on something.

Best regards,
Jakob Gaardsted

[bigopon]

Thanks for the explanation. There' cases where the logic maybe isn't good enough, maybe with the small movement of the scrollbar. I don't have a test for this so it's often quite hard to get to the bottom of it.
Though I'll be following up with your points above at a later time.

[pylgrym]

I have been looking further into this.
As mentioned, there is more than one bug.
The first bug was line 657, ie
(new_range_end_index === old_range_end_index && old_range_end_index >= new_range_end_index)

The second bug is triggering 'Scroll intersection not handled.' in line 735.
That section has a comment guessing at probable causes e.g. 'scrolled too little'.
But it appears to me, that the bug/behaviour instead stems
from a mismatch between reality and the mental model '_handleScroll' is built from.

The 6 cases that _handleScroll tries to distribute (start_range_change, end_range_change) across,
do not adequately cover the actual combinations that arise on runtime.

We routinely experience the combination
(range_start: UNCHANGED, range_end: INCR).

That combination is not covered by the 6 cases _handleScroll contains,
so it of course rams into 'scroll intersection not handled',
whereby scrolling consistently misbehaves.

I am not sure what causes it, but I'm guessing it maybe happens
when virtual-repeat's 'infinite-scroll-next' appends the next batch of items
to the container?

I would love to (?) ignore the problem, but unfortunately our application console log looks like this:
14:56:37.775 [!] Scroll intersection not handled. With indices: new [0, 55] / old [0, 49]
14:56:37.792 [!] Scroll intersection not handled. With indices: new [0, 55] / old [0, 49]
14:56:37.825 [!] Scroll intersection not handled. With indices: new [1, 56] / old [1, 50]
14:56:37.909 [!] Scroll intersection not handled. With indices: new [5, 60] / old [5, 54]
14:56:38.159 [!] Scroll intersection not handled. With indices: new [23, 78] / old [23, 72]
14:56:38.192 [!] Scroll intersection not handled. With indices: new [25, 80] / old [25, 74]
14:56:38.242 [!] Scroll intersection not handled. With indices: new [27, 82] / old [27, 76]
14:56:38.292 [!] Scroll intersection not handled. With indices: new [29, 84] / old [29, 78]
14:56:38.326 [!] Scroll intersection not handled. With indices: new [30, 85] / old [30, 79]
14:56:38.359 [!] Scroll intersection not handled. With indices: new [31, 86] / old [31, 80]
14:56:38.392 [!] Scroll intersection not handled. With indices: new [32, 87] / old [32, 81]
14:56:38.409 [!] Scroll intersection not handled. With indices: new [32, 87] / old [32, 81]
14:56:38.426 [!] Scroll intersection not handled. With indices: new [32, 87] / old [32, 81]
14:56:38.442 [!] Scroll intersection not handled. With indices: new [32, 87] / old [32, 81]
14:56:38.700 [!] Scroll intersection not handled. With indices: new [32, 87] / old [33, 82]
14:57:34.122 [!] Scroll intersection not handled. With indices: new [32, 87] / old [32, 81]
14:57:34.155 [!] Scroll intersection not handled. With indices: new [33, 88] / old [33, 82]
14:57:34.188 [!] Scroll intersection not handled. With indices: new [34, 89] / old [34, 83]
14:57:34.205 [!] Scroll intersection not handled. With indices: new [34, 89] / old [34, 83]
14:57:34.238 [!] Scroll intersection not handled. With indices: new [35, 90] / old [35, 84]
14:57:34.255 [!] Scroll intersection not handled. With indices: new [35, 90] / old [35, 84]
14:57:34.288 [!] Scroll intersection not handled. With indices: new [36, 91] / old [36, 85]
14:57:34.305 [!] Scroll intersection not handled. With indices: new [36, 91] / old [36, 85]

[pylgrym]

Thanks for the explanation. There' cases where the logic maybe isn't good enough, maybe with the small movement of the >scrollbar. I don't have a test for this so it's often quite hard to get to the bottom of it. Though I'll be following up with your >points above at a later time.

I have some updated details, possibly related to what you intend by a 'minimal repro'(?):
That is, I have some observations on what triggers it.
(1) it relates to updating or changing the contents of the observed container.
(*)
(2) specifically, if the observed container SHRINKS.

(3) apparently, the _handleScroll subsystem is not happy,
if the observed container changes contents/size abruptly.

(4) so, if I just assign (or append/splice/push) to the observed container, it gets angry/sad.
(5) but if I instead first TRUNCATE the observed container, before appending to it,
then it is possible to make _handleScroll happy/less sad.

(6) however, it is not enough to truncate it.
I must both truncate it, AND do an async-await (**), before doing the append.

(7) also, the truncate-method must also be specific: It works, if I assign an empty array [] to it.
But not, if I assign a zero length to the array!

(this is the two-star note - I give up typing two asterisks in markdown :-/.)
( * . * ) it seems - I'm guessing here - that if I allow for some async actions,
this gives aurelia a chance to do some intermediate refreshing, and 'discover' that the array has been truncated. (***)

(*) The reason for changing the contents of a currently displayed container, is
that we have many filter/search screens, and whenever the user changes the query,
the result-list is updated with the new search result. Maybe this is an anti-pattern towards aurelia;
does aurelia not like that we dynamically update-replace the contents of a list currently displayed..?
(in these concrete instances, apparently not..?)

(***) I'm currently "in love with" that we can circumvent this issue by writing our container-updates
in this weird and peculiar way.. But I'm wondering.. maybe this isn't really solving the problem;
maybe the reason it 'fixes', is because temporarily emptying the entire list, resets the scroll-settings
to accomodate an empty list? (ie, clearing scroll-config?).. I'm just speculating here..

Anyway, to recap: We experience this problem, whenever searching on an open screen,
reduces the amount of items displayed in the virtual-repeat list.

To elaborate on that: When we do this, I can see, that the case-7 error ranges tells us,
that OLD-range-ends match the fresh search result (ie the search reduced originally 100 items to now 50,
and the OLD-end-range reported has the value 49, which seems to fit with the updated list-contents).
At the same time, the NEW-end-range contains a value that relates to the PREVIOUS list-contents
(ie the 100 items, which were more than the now 50) - the new-end-range value is some value larger than 50,
e.g. 67.

So it looks like new-end-range 'didnt get the memo', it still lives in a world of 100 items (before we updated the container).

xxx-start-range doesn't really enter the picture(?) - both old and new start is zero/0.

But, to recap: If we violently empty the observed container, before we update,
AND if we make some await-async thing happen (to allow aurelia to process, I speculate),
then we can avoid/hide the problem. However, it is very unsatisfactory - having to resort
to that kind of no-guarantee multi-week shenanigans to get basic scrolling lists to work,
sort of defeats the purpose of using aurelia (which we otherwise appreciate very much/most days).

Here some examples again, of the indices appearing outside the expected range of [0-49]:

[!] Scroll intersection not handled. With indices: new [0, 67] / old [0, 49] aurelia-ui-virtualization.js:967
HS-8 aurelia-ui-virtualization.js:900
[!] Scroll intersection not handled. With indices: new [0, 67] / old [0, 49] aurelia-ui-virtualization.js:967
HS-8 aurelia-ui-virtualization.js:900
[!] Scroll intersection not handled. With indices: new [0, 67] / old [0, 49] aurelia-ui-virtualization.js:967
HS-8 aurelia-ui-virtualization.js:900