Refactor conditional event emitting to the C++ layer #38674
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
facebook-github-bot
added
CLA Signed
|
This pull request was exported from Phabricator. Differential Revision: D47852371 |
Base commit: d655d44 |
vincentriemer
added a commit
to vincentriemer/react-native
that referenced
this pull request
Jul 31, 2023
Summary: Pull Request resolved: facebook#38674 Changelog: [Internal] - Refactor conditional pointer event emitting to the C++ layer Some background: early on in the implementation of Pointer Events a concern was brought up that events related to hovering pointers could saturate the JS thread if they were fired all the time unconditionally, so as a mitigation we would check in native to see if listeners in the tree were listening for those events and only fire them if there were listeners. Now since we're going to be moving some of the event derivation logic to the C++ layer we need to receive all the events — but recreate the conditional firing in the C++ layer so we can still avoid saturating the JS thread. That's what this diff does. The only change I see being potentially contraversial is the fact that I needed a way to turn an `EventTarget` (the only information I receive regarding which node the event is firing on) to its cooresponding `ShadowNode` which I did in the method `GetShadowNodeFromEventTarget`. It essentially does the exact same thing the `getNodeFromInternalInstanceHandle` method in `ReactNativePublicCompat.js`, but in C++ against the JSI API. I don't know if there's a better way to do this but this was the best one I came up with that actually works. Differential Revision: D47852371 fbshipit-source-id: a935852ce26c372e61a35025b61eec1d9133854c
vincentriemer
force-pushed
the
export-D47852371
branch
from
62ee737
to
3cd1fe9
Compare
|
This pull request was exported from Phabricator. Differential Revision: D47852371 |
vincentriemer
added a commit
to vincentriemer/react-native
that referenced
this pull request
Aug 3, 2023
Summary: Pull Request resolved: facebook#38674 Changelog: [Internal] - Refactor conditional pointer event emitting to the C++ layer Some background: early on in the implementation of Pointer Events a concern was brought up that events related to hovering pointers could saturate the JS thread if they were fired all the time unconditionally, so as a mitigation we would check in native to see if listeners in the tree were listening for those events and only fire them if there were listeners. Now since we're going to be moving some of the event derivation logic to the C++ layer we need to receive all the events — but recreate the conditional firing in the C++ layer so we can still avoid saturating the JS thread. That's what this diff does. The only change I see being potentially contraversial is the fact that I needed a way to turn an `EventTarget` (the only information I receive regarding which node the event is firing on) to its cooresponding `ShadowNode` which I did in the method `GetShadowNodeFromEventTarget`. It essentially does the exact same thing the `getNodeFromInternalInstanceHandle` method in `ReactNativePublicCompat.js`, but in C++ against the JSI API. I don't know if there's a better way to do this but this was the best one I came up with that actually works. Differential Revision: D47852371 fbshipit-source-id: d33fbecef4490a3fe94c7a8c927c958eae12d49e
|
This pull request was exported from Phabricator. Differential Revision: D47852371 |
vincentriemer
force-pushed
the
export-D47852371
branch
from
3cd1fe9
to
7fc1bd0
Compare
|
This pull request was exported from Phabricator. Differential Revision: D47852371 |
vincentriemer
force-pushed
the
export-D47852371
branch
from
7fc1bd0
to
6b192fa
Compare
vincentriemer
added a commit
to vincentriemer/react-native
that referenced
this pull request
Aug 4, 2023
Summary: Pull Request resolved: facebook#38674 Changelog: [Internal] - Refactor conditional pointer event emitting to the C++ layer Some background: early on in the implementation of Pointer Events a concern was brought up that events related to hovering pointers could saturate the JS thread if they were fired all the time unconditionally, so as a mitigation we would check in native to see if listeners in the tree were listening for those events and only fire them if there were listeners. Now since we're going to be moving some of the event derivation logic to the C++ layer we need to receive all the events — but recreate the conditional firing in the C++ layer so we can still avoid saturating the JS thread. That's what this diff does. The only change I see being potentially contraversial is the fact that I needed a way to turn an `EventTarget` (the only information I receive regarding which node the event is firing on) to its cooresponding `ShadowNode` which I did in the method `GetShadowNodeFromEventTarget`. It essentially does the exact same thing the `getNodeFromInternalInstanceHandle` method in `ReactNativePublicCompat.js`, but in C++ against the JSI API. I don't know if there's a better way to do this but this was the best one I came up with that actually works. Reviewed By: NickGerleman Differential Revision: D47852371 fbshipit-source-id: a71f52205282d8f3cda3afd39e95de2e1348fe02
vincentriemer
added a commit
to vincentriemer/react-native
that referenced
this pull request
Aug 7, 2023
Summary: Pull Request resolved: facebook#38674 Changelog: [Internal] - Refactor conditional pointer event emitting to the C++ layer Some background: early on in the implementation of Pointer Events a concern was brought up that events related to hovering pointers could saturate the JS thread if they were fired all the time unconditionally, so as a mitigation we would check in native to see if listeners in the tree were listening for those events and only fire them if there were listeners. Now since we're going to be moving some of the event derivation logic to the C++ layer we need to receive all the events — but recreate the conditional firing in the C++ layer so we can still avoid saturating the JS thread. That's what this diff does. The only change I see being potentially contraversial is the fact that I needed a way to turn an `EventTarget` (the only information I receive regarding which node the event is firing on) to its cooresponding `ShadowNode` which I did in the method `GetShadowNodeFromEventTarget`. It essentially does the exact same thing the `getNodeFromInternalInstanceHandle` method in `ReactNativePublicCompat.js`, but in C++ against the JSI API. I don't know if there's a better way to do this but this was the best one I came up with that actually works. Reviewed By: NickGerleman Differential Revision: D47852371 fbshipit-source-id: e883b6cd27f0ae154c884cd375295bd864242393
vincentriemer
force-pushed
the
export-D47852371
branch
from
6b192fa
to
3fa9780
Compare
|
This pull request was exported from Phabricator. Differential Revision: D47852371 |
vincentriemer
added a commit
to vincentriemer/react-native
that referenced
this pull request
Aug 8, 2023
Summary: Pull Request resolved: facebook#38674 Changelog: [Internal] - Refactor conditional pointer event emitting to the C++ layer Some background: early on in the implementation of Pointer Events a concern was brought up that events related to hovering pointers could saturate the JS thread if they were fired all the time unconditionally, so as a mitigation we would check in native to see if listeners in the tree were listening for those events and only fire them if there were listeners. Now since we're going to be moving some of the event derivation logic to the C++ layer we need to receive all the events — but recreate the conditional firing in the C++ layer so we can still avoid saturating the JS thread. That's what this diff does. The only change I see being potentially contraversial is the fact that I needed a way to turn an `EventTarget` (the only information I receive regarding which node the event is firing on) to its cooresponding `ShadowNode` which I did in the method `GetShadowNodeFromEventTarget`. It essentially does the exact same thing the `getNodeFromInternalInstanceHandle` method in `ReactNativePublicCompat.js`, but in C++ against the JSI API. I don't know if there's a better way to do this but this was the best one I came up with that actually works. Differential Revision: https://internalfb.com/D47852371 fbshipit-source-id: dc05970c75db1599ab62087b4def21fda85c1fc4
vincentriemer
added a commit
to vincentriemer/react-native
that referenced
this pull request
Aug 8, 2023
Summary: Pull Request resolved: facebook#38674 Changelog: [Internal] - Refactor conditional pointer event emitting to the C++ layer Some background: early on in the implementation of Pointer Events a concern was brought up that events related to hovering pointers could saturate the JS thread if they were fired all the time unconditionally, so as a mitigation we would check in native to see if listeners in the tree were listening for those events and only fire them if there were listeners. Now since we're going to be moving some of the event derivation logic to the C++ layer we need to receive all the events — but recreate the conditional firing in the C++ layer so we can still avoid saturating the JS thread. That's what this diff does. The only change I see being potentially contraversial is the fact that I needed a way to turn an `EventTarget` (the only information I receive regarding which node the event is firing on) to its cooresponding `ShadowNode` which I did in the method `GetShadowNodeFromEventTarget`. It essentially does the exact same thing the `getNodeFromInternalInstanceHandle` method in `ReactNativePublicCompat.js`, but in C++ against the JSI API. I don't know if there's a better way to do this but this was the best one I came up with that actually works. Differential Revision: https://internalfb.com/D47852371 fbshipit-source-id: 073fa794a6c6ad6d676b529e2e69ced49d949a42
|
This pull request was exported from Phabricator. Differential Revision: D47852371 |
vincentriemer
force-pushed
the
export-D47852371
branch
from
3fa9780
to
38e3219
Compare
vincentriemer
added a commit
to vincentriemer/react-native
that referenced
this pull request
Aug 10, 2023
Summary: Pull Request resolved: facebook#38674 Changelog: [Internal] - Refactor conditional pointer event emitting to the C++ layer Some background: early on in the implementation of Pointer Events a concern was brought up that events related to hovering pointers could saturate the JS thread if they were fired all the time unconditionally, so as a mitigation we would check in native to see if listeners in the tree were listening for those events and only fire them if there were listeners. Now since we're going to be moving some of the event derivation logic to the C++ layer we need to receive all the events — but recreate the conditional firing in the C++ layer so we can still avoid saturating the JS thread. That's what this diff does. The only change I see being potentially contraversial is the fact that I needed a way to turn an `EventTarget` (the only information I receive regarding which node the event is firing on) to its cooresponding `ShadowNode` which I did in the method `GetShadowNodeFromEventTarget`. It essentially does the exact same thing the `getNodeFromInternalInstanceHandle` method in `ReactNativePublicCompat.js`, but in C++ against the JSI API. I don't know if there's a better way to do this but this was the best one I came up with that actually works. Reviewed By: NickGerleman Differential Revision: D47852371 fbshipit-source-id: be47c616274bb0a9bd3e226152fa1c4ce100e617
vincentriemer
added a commit
to vincentriemer/react-native
that referenced
this pull request
Aug 10, 2023
Summary: Pull Request resolved: facebook#38674 Changelog: [Internal] - Refactor conditional pointer event emitting to the C++ layer Some background: early on in the implementation of Pointer Events a concern was brought up that events related to hovering pointers could saturate the JS thread if they were fired all the time unconditionally, so as a mitigation we would check in native to see if listeners in the tree were listening for those events and only fire them if there were listeners. Now since we're going to be moving some of the event derivation logic to the C++ layer we need to receive all the events — but recreate the conditional firing in the C++ layer so we can still avoid saturating the JS thread. That's what this diff does. The only change I see being potentially contraversial is the fact that I needed a way to turn an `EventTarget` (the only information I receive regarding which node the event is firing on) to its cooresponding `ShadowNode` which I did in the method `GetShadowNodeFromEventTarget`. It essentially does the exact same thing the `getNodeFromInternalInstanceHandle` method in `ReactNativePublicCompat.js`, but in C++ against the JSI API. I don't know if there's a better way to do this but this was the best one I came up with that actually works. Reviewed By: NickGerleman Differential Revision: D47852371 fbshipit-source-id: 55bb8ec18be877d570992d51d772d5418682fe25
vincentriemer
force-pushed
the
export-D47852371
branch
from
38e3219
to
febaabf
Compare
|
This pull request was exported from Phabricator. Differential Revision: D47852371 |
vincentriemer
added a commit
to vincentriemer/react-native
that referenced
this pull request
Aug 10, 2023
Summary: Pull Request resolved: facebook#38674 Changelog: [Internal] - Refactor conditional pointer event emitting to the C++ layer Some background: early on in the implementation of Pointer Events a concern was brought up that events related to hovering pointers could saturate the JS thread if they were fired all the time unconditionally, so as a mitigation we would check in native to see if listeners in the tree were listening for those events and only fire them if there were listeners. Now since we're going to be moving some of the event derivation logic to the C++ layer we need to receive all the events — but recreate the conditional firing in the C++ layer so we can still avoid saturating the JS thread. That's what this diff does. The only change I see being potentially contraversial is the fact that I needed a way to turn an `EventTarget` (the only information I receive regarding which node the event is firing on) to its cooresponding `ShadowNode` which I did in the method `GetShadowNodeFromEventTarget`. It essentially does the exact same thing the `getNodeFromInternalInstanceHandle` method in `ReactNativePublicCompat.js`, but in C++ against the JSI API. I don't know if there's a better way to do this but this was the best one I came up with that actually works. Reviewed By: NickGerleman Differential Revision: D47852371 fbshipit-source-id: d2a0a460df985051d406240b5345f45781ab164b
vincentriemer
force-pushed
the
export-D47852371
branch
from
febaabf
to
48716aa
Compare
|
This pull request was exported from Phabricator. Differential Revision: D47852371 |
1 similar comment
|
This pull request was exported from Phabricator. Differential Revision: D47852371 |
vincentriemer
force-pushed
the
export-D47852371
branch
from
48716aa
to
8e716e7
Compare
vincentriemer
added a commit
to vincentriemer/react-native
that referenced
this pull request
Aug 11, 2023
Summary: Pull Request resolved: facebook#38674 Changelog: [Internal] - Refactor conditional pointer event emitting to the C++ layer Some background: early on in the implementation of Pointer Events a concern was brought up that events related to hovering pointers could saturate the JS thread if they were fired all the time unconditionally, so as a mitigation we would check in native to see if listeners in the tree were listening for those events and only fire them if there were listeners. Now since we're going to be moving some of the event derivation logic to the C++ layer we need to receive all the events — but recreate the conditional firing in the C++ layer so we can still avoid saturating the JS thread. That's what this diff does. The only change I see being potentially contraversial is the fact that I needed a way to turn an `EventTarget` (the only information I receive regarding which node the event is firing on) to its cooresponding `ShadowNode` which I did in the method `GetShadowNodeFromEventTarget`. It essentially does the exact same thing the `getNodeFromInternalInstanceHandle` method in `ReactNativePublicCompat.js`, but in C++ against the JSI API. I don't know if there's a better way to do this but this was the best one I came up with that actually works. Reviewed By: NickGerleman Differential Revision: D47852371 fbshipit-source-id: cd0df9c43f654ba48dc6bef8dc7e501275d5613c
|
This pull request was exported from Phabricator. Differential Revision: D47852371 |
Summary: Pull Request resolved: facebook#38674 Changelog: [Internal] - Refactor conditional pointer event emitting to the C++ layer Some background: early on in the implementation of Pointer Events a concern was brought up that events related to hovering pointers could saturate the JS thread if they were fired all the time unconditionally, so as a mitigation we would check in native to see if listeners in the tree were listening for those events and only fire them if there were listeners. Now since we're going to be moving some of the event derivation logic to the C++ layer we need to receive all the events — but recreate the conditional firing in the C++ layer so we can still avoid saturating the JS thread. That's what this diff does. The only change I see being potentially contraversial is the fact that I needed a way to turn an `EventTarget` (the only information I receive regarding which node the event is firing on) to its cooresponding `ShadowNode` which I did in the method `GetShadowNodeFromEventTarget`. It essentially does the exact same thing the `getNodeFromInternalInstanceHandle` method in `ReactNativePublicCompat.js`, but in C++ against the JSI API. I don't know if there's a better way to do this but this was the best one I came up with that actually works. Reviewed By: NickGerleman Differential Revision: D47852371 fbshipit-source-id: 5bedf4cb25807704a5cb703310b0db1c215607a3
vincentriemer
force-pushed
the
export-D47852371
branch
from
8e716e7
to
0634e42
Compare
|
This pull request has been merged in 7cd6996. |
rozele
added a commit
to rozele/react-native-macos
that referenced
this pull request
Aug 12, 2023
…book#38973) Summary: Pull Request resolved: facebook#38973 Original commit changeset: 6c00c2fcfdfd Original Phabricator Diff: D47852371 When facebook#38674 and facebook#38959 are combined, they cause Android apps on Fabric to crash. facebook#38959 is the more urgent fix, so backing out facebook#38674. ## Changelog: [General] [Fixed] - Rolling back change that broke e2e tests until we can figure out why the e2e tests are broken Reviewed By: NickGerleman Differential Revision: D48288752 fbshipit-source-id: 52dac650e0ca43ef094d28f5df4388bc98e20fb9
rozele
added a commit
to rozele/react-native-macos
that referenced
this pull request
Aug 12, 2023
…book#38973) Summary: Pull Request resolved: facebook#38973 Original commit changeset: 6c00c2fcfdfd Original Phabricator Diff: D47852371 When facebook#38674 and facebook#38959 are combined, they cause Android apps on Fabric to crash. facebook#38959 is the more urgent fix, so backing out facebook#38674. ## Changelog: [General] [Fixed] - Rolling back change that broke e2e tests until we can figure out why the e2e tests are broken Reviewed By: NickGerleman Differential Revision: D48288752 fbshipit-source-id: d9c8b2d1b01ecf6532e92ef6dbce0a5be9c2b57b
rozele
added a commit
to rozele/react-native-macos
that referenced
this pull request
Aug 12, 2023
…book#38973) Summary: Pull Request resolved: facebook#38973 Original commit changeset: 6c00c2fcfdfd Original Phabricator Diff: D47852371 When facebook#38674 and facebook#38959 are combined, they cause Android apps on Fabric to crash. facebook#38959 is the more urgent fix, so backing out facebook#38674. ## Changelog: [General] [Fixed] - Rolling back change that broke e2e tests until we can figure out why the e2e tests are broken Reviewed By: NickGerleman Differential Revision: D48288752 fbshipit-source-id: a3f8efe9bb2657a420a6af7f6b52af37e28d3f3f
|
This pull request has been reverted by ae88aef. |
facebook-github-bot
pushed a commit
that referenced
this pull request
Aug 12, 2023
Summary: Pull Request resolved: #38973 Original commit changeset: 6c00c2fcfdfd Original Phabricator Diff: D47852371 When #38674 and #38959 are combined, they cause Android apps on Fabric to crash. #38959 is the more urgent fix, so backing out #38674. ## Changelog: [General] [Fixed] - Rolling back change that broke e2e tests until we can figure out why the e2e tests are broken Reviewed By: NickGerleman Differential Revision: D48288752 fbshipit-source-id: b52e28936bbebd21bd3b2f49f9a233f295ba6248
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary:
Changelog: [Internal] - Refactor conditional pointer event emitting to the C++ layer
Some background: early on in the implementation of Pointer Events a concern was brought up that events related to hovering pointers could saturate the JS thread if they were fired all the time unconditionally, so as a mitigation we would check in native to see if listeners in the tree were listening for those events and only fire them if there were listeners.
Now since we're going to be moving some of the event derivation logic to the C++ layer we need to receive all the events — but recreate the conditional firing in the C++ layer so we can still avoid saturating the JS thread. That's what this diff does.
The only change I see being potentially contraversial is the fact that I needed a way to turn an
EventTarget(the only information I receive regarding which node the event is firing on) to its coorespondingShadowNodewhich I did in the methodGetShadowNodeFromEventTarget. It essentially does the exact same thing thegetNodeFromInternalInstanceHandlemethod inReactNativePublicCompat.js, but in C++ against the JSI API. I don't know if there's a better way to do this but this was the best one I came up with that actually works.Differential Revision: D47852371