Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

Refactor hover tracking logic to the shared C++ renderer #41519

Closed
wants to merge 2 commits into from

Conversation

vincentriemer
Copy link
Contributor

Summary:
Changelog: [Internal] - Refactor hover tracking logic to the shared c++ renderer

This diff refactors our hover tracking logic out of the platform (in this case only iOS, integrating Android is going to require extra work due to their pointer events being untyped) and puts it into the event intercepting infra in Fabric's C++ core. This is a big-ish diff so I'm going to try my best to use this summary to guide you through the changes.

To begin with — the changes inside RCTSurfacePointerHandler.mm are mostly about removing the existing hover tracking logic. The logic of the hover tracking is largely the same between the objective-c and c++ implementations with minor tweaks in order to be a better citizen when it comes to storing node references. One small "addition" to this file is explicitly firing a pointerleave event from the iOS layer when we detect that the pointer has left the app entirely because the C++ would otherwise not know when the pointer leaves the app. We don't need to include a special case for pointerenter because we can derive that in C++ from the first pointermove event that gets sent once the pointer re-enters the app's root view.

Next I think it makes most sense to continue onto the PointerEventsProcessor.h/mm which is the central class we're working in. One small change is adding a flag to the ActivePointer struct (shouldLeaveWhenReleased) which we will set during ActivePointer registration — setting to false if the pointer in question exists in the (also) newly added previousHoverTrackersPerPointer_ registry. This logic is primarily used for knowing later when the pointer is released and whether we should emit the synthetic leave/out event on release. If the pointer existed in previousHoverTrackersPerPointer_ before the ActivePointer registration that implies that the pointer is capable of hovering to some degree and we should not emit those leave/out events yet.

The real meat & potatoes of this diff is the handleIncomingPointerEventOnNode method which matches the handleIncomingPointerEvent method we removed from RCTSurfacePointerHandler.mm. This method derives the enter/leave/over/out pointer events by comparing the current event's target path (list of nodes from the root node to the target node of the event) to the previously recorded event target path. The representation of this event path is through the new PointerHoverTracker class which stores a pointer to just the root node and the target node as we can recreate the entire event path from these.

For over/out events all that matters is when the deepest-most target changes which is checked in handleIncomingPointerEventOnNode by leveraging PointerHoverTracker's hasSameTarget method. For enter/leave events we need to fire discrete events for every node in the path which has either been removed or added, so the diffEventPath method was introduced on PointerHoverTracker to provide that.

Differential Revision: D51317492

…acebook#41471)

Summary:

Changelog: [Internal] - Clean up eventTarget retaining logic in the pointer event processor

This refactors calls to EventTarget::retain/release to occur in the actual methods that require the event target to be retained instead of expecting the caller to manage that which should be more maintainable.

Differential Revision: D51279974
Summary:
Changelog: [Internal] - Refactor hover tracking logic to the shared c++ renderer

This diff refactors our hover tracking logic out of the platform (in this case only iOS, integrating Android is going to require extra work due to their pointer events being untyped) and puts it into the event intercepting infra in Fabric's C++ core. This is a big-ish diff so I'm going to try my best to use this summary to guide you through the changes.

To begin with — the changes inside `RCTSurfacePointerHandler.mm` are mostly about removing the existing hover tracking logic. The logic of the hover tracking is largely the same between the objective-c and c++ implementations with minor tweaks in order to be a better citizen when it comes to storing node references. One small "addition" to this file is explicitly firing a `pointerleave` event from the iOS layer when we detect that the pointer has left the app entirely because the C++ would otherwise not know when the pointer leaves the app. We don't need to include a special case for `pointerenter` because we can derive that in C++ from the first `pointermove` event that gets sent once the pointer re-enters the app's root view.

Next I think it makes most sense to continue onto the `PointerEventsProcessor.h/mm` which is the central class we're working in. One small change is adding a flag to the `ActivePointer` struct (`shouldLeaveWhenReleased`) which we will set during `ActivePointer` registration — setting to false if the pointer in question exists in the (also) newly added `previousHoverTrackersPerPointer_` registry. This logic is primarily used for knowing later when the pointer is released and whether we should emit the synthetic leave/out event on release. If the pointer existed in `previousHoverTrackersPerPointer_` **before** the `ActivePointer` registration that implies that the pointer is capable of hovering to some degree and we should **not** emit those leave/out events yet.

The real meat & potatoes of this diff is the `handleIncomingPointerEventOnNode` method which matches the `handleIncomingPointerEvent` method we removed from `RCTSurfacePointerHandler.mm`. This method derives the enter/leave/over/out pointer events by comparing the current event's target path (list of nodes from the root node to the target node of the event) to the previously recorded event target path. The representation of this event path is through the new `PointerHoverTracker` class which stores a pointer to just the root node and the target node as we can recreate the entire event path from these.

For over/out events all that matters is when the deepest-most target changes which is checked in `handleIncomingPointerEventOnNode` by leveraging `PointerHoverTracker`'s `hasSameTarget` method. For enter/leave events we need to fire discrete events for every node in the path which has either been removed or added, so the `diffEventPath` method was introduced on `PointerHoverTracker` to provide that.

Differential Revision: D51317492
@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Nov 16, 2023
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D51317492

@analysis-bot
Copy link

Platform Engine Arch Size (bytes) Diff
android hermes arm64-v8a 17,666,170 +20,474
android hermes armeabi-v7a n/a --
android hermes x86 n/a --
android hermes x86_64 n/a --
android jsc arm64-v8a 21,054,189 +20,471
android jsc armeabi-v7a n/a --
android jsc x86 n/a --
android jsc x86_64 n/a --

Base commit: fa89dd6
Branch: main

Copy link

This pull request was successfully merged by @vincentriemer in 72b876a.

When will my fix make it into a release? | Upcoming Releases

@github-actions github-actions bot added the Merged This PR has been merged. label Dec 21, 2023
rshest added a commit to rshest/react-native that referenced this pull request Dec 27, 2023
…hain

Summary:
## Changelog:
[Internal] - 

In facebook#41519 we introduced usage of C++20s range operations, which broke MacOSX desktop builds for the x86_64 targets (e.g. on Intel Mac laptops).

This appears to be a [known issue](https://stackoverflow.com/questions/73929080/error-with-clang-15-and-c20-stdviewsfilter), fixed in the later clang versions, however we need to support the earlier ones as well.

This changes the code to use the good old imperative style to do the same thing, but without using `std::views::filter`, thus working around the problem.

Differential Revision: D52428984
rshest added a commit to rshest/react-native that referenced this pull request Dec 27, 2023
…hain (facebook#42076)

Summary:

## Changelog:
[Internal] -

In facebook#41519 we introduced usage of C++20s range operations, which broke MacOSX desktop builds for the x86_64 targets (e.g. on Intel Mac laptops).

This appears to be a [known issue](https://stackoverflow.com/questions/73929080/error-with-clang-15-and-c20-stdviewsfilter), fixed in the later clang versions, however we need to support the earlier ones as well.

This changes the code to use the good old imperative style to do the same thing, but without using `std::views::filter`, thus working around the problem.

Reviewed By: christophpurrer

Differential Revision: D52428984
rshest added a commit to rshest/react-native that referenced this pull request Dec 27, 2023
…hain (facebook#42076)

Summary:

## Changelog:
[Internal] -

In facebook#41519 we introduced usage of C++20s range operations, which broke MacOSX desktop builds for the x86_64 targets (e.g. on Intel Mac laptops).

This appears to be a [known issue](https://stackoverflow.com/questions/73929080/error-with-clang-15-and-c20-stdviewsfilter), fixed in the later clang versions, however we need to support the earlier ones as well.

This changes the code to use the good old imperative style to do the same thing, but without using `std::views::filter`, thus working around the problem.

Reviewed By: christophpurrer

Differential Revision: D52428984
facebook-github-bot pushed a commit that referenced this pull request Dec 27, 2023
…hain (#42076)

Summary:
Pull Request resolved: #42076

## Changelog:
[Internal] -

In #41519 we introduced usage of C++20s range operations, which broke MacOSX desktop builds for the x86_64 targets (e.g. on Intel Mac laptops).

This appears to be a [known issue](https://stackoverflow.com/questions/73929080/error-with-clang-15-and-c20-stdviewsfilter), fixed in the later clang versions, however we need to support the earlier ones as well.

This changes the code to use the good old imperative style to do the same thing, but without using `std::views::filter`, thus working around the problem.

Reviewed By: christophpurrer

Differential Revision: D52428984

fbshipit-source-id: 6d0a390549c462b7040b5c0e669c00932bd99af7
Othinn pushed a commit to Othinn/react-native that referenced this pull request Jan 9, 2024
)

Summary:
Pull Request resolved: facebook#41519

Changelog: [Internal] - Refactor hover tracking logic to the shared c++ renderer

This diff refactors our hover tracking logic out of the platform (in this case only iOS, integrating Android is going to require extra work due to their pointer events being untyped) and puts it into the event intercepting infra in Fabric's C++ core. This is a big-ish diff so I'm going to try my best to use this summary to guide you through the changes.

To begin with — the changes inside `RCTSurfacePointerHandler.mm` are mostly about removing the existing hover tracking logic. The logic of the hover tracking is largely the same between the objective-c and c++ implementations with minor tweaks in order to be a better citizen when it comes to storing node references. One small "addition" to this file is explicitly firing a `pointerleave` event from the iOS layer when we detect that the pointer has left the app entirely because the C++ would otherwise not know when the pointer leaves the app. We don't need to include a special case for `pointerenter` because we can derive that in C++ from the first `pointermove` event that gets sent once the pointer re-enters the app's root view.

Next I think it makes most sense to continue onto the `PointerEventsProcessor.h/mm` which is the central class we're working in. One small change is adding a flag to the `ActivePointer` struct (`shouldLeaveWhenReleased`) which we will set during `ActivePointer` registration — setting to false if the pointer in question exists in the (also) newly added `previousHoverTrackersPerPointer_` registry. This logic is primarily used for knowing later when the pointer is released and whether we should emit the synthetic leave/out event on release. If the pointer existed in `previousHoverTrackersPerPointer_` **before** the `ActivePointer` registration that implies that the pointer is capable of hovering to some degree and we should **not** emit those leave/out events yet.

The real meat & potatoes of this diff is the `handleIncomingPointerEventOnNode` method which matches the `handleIncomingPointerEvent` method we removed from `RCTSurfacePointerHandler.mm`. This method derives the enter/leave/over/out pointer events by comparing the current event's target path (list of nodes from the root node to the target node of the event) to the previously recorded event target path. The representation of this event path is through the new `PointerHoverTracker` class which stores a pointer to just the root node and the target node as we can recreate the entire event path from these.

For over/out events all that matters is when the deepest-most target changes which is checked in `handleIncomingPointerEventOnNode` by leveraging `PointerHoverTracker`'s `hasSameTarget` method. For enter/leave events we need to fire discrete events for every node in the path which has either been removed or added, so the `diffEventPath` method was introduced on `PointerHoverTracker` to provide that.

Reviewed By: yungsters

Differential Revision: D51317492

fbshipit-source-id: e15ac3a396d5afa7ab921e4589861b43b07a33b5
Othinn pushed a commit to Othinn/react-native that referenced this pull request Jan 9, 2024
…hain (facebook#42076)

Summary:
Pull Request resolved: facebook#42076

## Changelog:
[Internal] -

In facebook#41519 we introduced usage of C++20s range operations, which broke MacOSX desktop builds for the x86_64 targets (e.g. on Intel Mac laptops).

This appears to be a [known issue](https://stackoverflow.com/questions/73929080/error-with-clang-15-and-c20-stdviewsfilter), fixed in the later clang versions, however we need to support the earlier ones as well.

This changes the code to use the good old imperative style to do the same thing, but without using `std::views::filter`, thus working around the problem.

Reviewed By: christophpurrer

Differential Revision: D52428984

fbshipit-source-id: 6d0a390549c462b7040b5c0e669c00932bd99af7
sammy-SC added a commit to sammy-SC/react-native that referenced this pull request Oct 28, 2024
Summary:

changelog: [internal]

This looks like a missing break statement when it was refactored in facebook#41519

Differential Revision: D65040504
facebook-github-bot pushed a commit that referenced this pull request Oct 28, 2024
Summary:
Pull Request resolved: #47233

changelog: [internal]

This looks like a missing break statement when it was refactored in #41519

Reviewed By: rubennorte

Differential Revision: D65040504

fbshipit-source-id: c4d52792099a5764d00c16708e400fa8a0046c93
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. fb-exported Merged This PR has been merged. p: Facebook Partner: Facebook Partner
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants