Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Refactor, Fixes, and functionality to avoid processing nested scroll events on internal webview elements scrolls #8

Merged
merged 4 commits into from
May 25, 2023

Conversation

dpastor
Copy link
Contributor

@dpastor dpastor commented May 25, 2023

🎟️ Jira tickets

ANDROID-13326
ANDROID-11710

🥅 What's the goal?

  1. I decided to revert the "helper" approach to extract to an specific file code extracted from NestedScrollView support library code, this was leading to issues due methods not calling directly WebView methods (Specially all the ones related with NestedScrollingChild3 interface implementation). So I just added this directly to the NestedScrollWebView class, identifying support library code with an specific comment block (trying to keep it as unaltered as possible for future updates).

  2. Fixed also an issue where scroll was maintained after navigating to a new page (ANDROID-11710) -->

Record_2023-05-11-10-14-24.mp4
  1. Added a new functionality to block nested scrolling in cases we are able to detect when user is performing scroll gestures that does not produce page scrolling, so nested scrolling should not be performed.
Previous behaviour Behaviour when blocking nested scrolling on internal scrolls
Record_2023-05-24-19-03-54.mp4
Record_2023-05-24-19-02-56.mp4

@dpastor dpastor changed the title Refactor, Fixes, and functionality to not process as nested scroll events internal webview scrolls Refactor, Fixes, and functionality to avoid processing nested scroll events on internal webview elements scrolls May 25, 2023
@dpastor dpastor requested review from gmerinojimenez and dagonco May 25, 2023 10:45
val y = event.getY(activePointerIndex)
val x = event.getX(activePointerIndex)
val deltaY: Float = initialY!! - y
val deltaX: Float = initialX!! - x
Copy link
Member

@dagonco dagonco May 25, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we have a concurrency issue here?
I mean, initialX for example could be nullable. To avoid a possible problem could we store in this method these two coordinates and use them instead using initialX/Y? Just to be sure these two values couldnt be null any time in this instance

Copy link
Contributor Author

@dpastor dpastor May 25, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It shouldn't because all class methods should be always executed from main thread (All are related with touch events processing or scrolls performed that will be sequentially invoked).

In any case it would be clear and we won't need the !!. I'm applying it --> d629d4c

Copy link
Member

@dagonco dagonco left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TOP 🚀

@dpastor dpastor merged commit 9f806d8 into main May 25, 2023
@dpastor dpastor deleted the fixes_and_improvements branch May 25, 2023 13:56
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants