Fix(Card Browser): ANR on using scrollbar

[ShaanNarendran]

Purpose / Description

ANR when the scrollbar was dragged or tapped in larger collections, arose due to the addition of pr #19980

• Enhancement (Card Browser): Tap and drag scrollbar #19980

Fixes

• Fixes The scrollbar can cause lag #20173

Approach

I deleted the previous delta based logic implemented and let the new function handle dragging and tapping which doesn't cause ANRs, for extra optimisation added a scrolltopositionwithoffset.

How Has This Been Tested?

Tested the scrolling with a deck of 100k cards and found no issue on rapid scrolling for 30 seconds along with taps throughout the browser

Checklist

Please, go through these checks before submitting the PR.

• You have a descriptive commit message with a short title (first line, max 50 chars).
• You have commented your code, particularly in hard-to-understand areas
• You have performed a self-review of your own code
• UI changes: include screenshots of all affected screens (in particular showing any new or changed strings)
• UI Changes: You have tested your change using the Google Accessibility Scanner

[david-allison]

the new function

Please clarify this

---

In general, if you can list 'reverted X', then name the commit which is in HEAD, as this makes it easier to understand what was reverted, and what was fixed forward

I believe the commit is: ca95728

[ShaanNarendran]

the new function

Please clarify this

Ah, by new function I meant the overriding of onTouchEvent which I had done for the main issue of tap and drag, should've phrased it better oops. Will edit the commit message now.

[ShaanNarendran]

In general, if you can list 'reverted X', then name the commit which is in HEAD, as this makes it easier to understand what was reverted, and what was fixed forward

I believe the commit is: ca95728

Would I not have to mention 3c0b2bb since I removed the onTouchListener introduced in that commit, wouldn't it be better to write 'Reverts the onTouchListener introduced in 3c0b2bb'? I could be confusing the meaning of revert here so I just wanted to clarify.
Edit: 'Reverts ca95728' also seems right since introducing that pr is what caused the issue, so I suppose that's more appropriate like you said.

[ShaanNarendran]

Not sure if the new commit description works, but it made sense like this in my head

[ShaanNarendran]

@david-allison do we want to draft since the new browser redesign is coming?

[david-allison]

Let's keep going with this.

The redesign will be dev only for a long time, and I expect "production" features to break it.

In addition, I'm mostly reworking search, the rendered rows won't change

[criticalAY]

I tested it since you removed had the logic set by pixels and you are counting items by number, add cards with uneven content (basically not equal height for all cards) then scroll the cards you will see a sudden snap while browsing the list

[ShaanNarendran]

I tested it since you removed had the logic set by pixels and you are counting items by number, add cards with uneven content (basically not equal height for all cards) then scroll the cards you will see a sudden snap while browsing the list

Could I have the deck file? I think either way, it has to be a tradeoff since if we revert to pixels then ANR is possible for decks with a large number of cards so we wouldn't be able to implement the tap and drag functionality otherwise we stick to using indexes to solve this issue but there would be the snapping you mentioned. I will look into it more though once I reproduce and see what I can do

Edit: I've reproduced it and I see the issue, will try and figure something out
Edit 2: Think I can use offset scroll with some decimal approximations to get it to work, should have it out tomorrow

[ShaanNarendran]

I implemented a fix but it's more of just a band-aid. We basically just use proportional scrolling rather than calculating pixels so it changes according to the size of the card, thereby making it smoother and not jumpy but in my testing there still is a little jump when you have very varied sizes.

[david-allison]

LGTM (from a code perspective), feel free to accept/reject comments, as I feel they're all subjective nitpicks

⚠️ I feel this needs a second physical test before merging

[ShaanNarendran]

I think I really messed up the git in my branch..

[david-allison]

undo the 'merge' and rebase instead

[david-allison]

My review still stands, Cheers!
Needs a quick fix to make CI happy

I'm going to unwatch this one, ping me if I'm needed

[criticalAY]

Ok! now I am happy, LGTM