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

Add skipBubbling property to dispatch config #23366

Merged
merged 1 commit into from
Mar 14, 2022

Conversation

lunaleaps
Copy link
Contributor

Support skip bubbling for React Native bubbled events.

Introducing this to offer similar event dispatching for event behavior on the web. In particular, there are some events that we want to support in React Native that do not bubble. These are not synonymous with direct events on React Native as they still go through a capture phase.

How did you test this change?

Added a test case to demonstrate that ancestors do not receive bubbled events when skipBubbling is true

@sizebot
Copy link

sizebot commented Feb 26, 2022

Comparing: 5d08a24...51f9345

Critical size changes

Includes critical production bundles, as well as any change greater than 2%:

Name +/- Base Current +/- gzip Base gzip Current gzip
oss-stable/react-dom/cjs/react-dom.production.min.js = 131.32 kB 131.32 kB = 42.01 kB 42.01 kB
oss-experimental/react-dom/cjs/react-dom.production.min.js = 136.15 kB 136.15 kB = 43.43 kB 43.43 kB
facebook-www/ReactDOM-prod.classic.js = 434.32 kB 434.32 kB = 79.42 kB 79.42 kB
facebook-www/ReactDOM-prod.modern.js = 421.06 kB 421.06 kB = 77.41 kB 77.41 kB
facebook-www/ReactDOMForked-prod.classic.js = 434.32 kB 434.32 kB = 79.42 kB 79.42 kB

Significant size changes

Includes any change greater than 0.2%:

Expand to show
Name +/- Base Current +/- gzip Base gzip Current gzip
react-native/shims/ReactNativeViewConfigRegistry.js +0.87% 3.57 kB 3.60 kB +1.23% 1.22 kB 1.23 kB
react-native/shims/ReactNativeTypes.js +0.43% 7.16 kB 7.19 kB +0.40% 2.00 kB 2.01 kB
react-native/implementations/ReactFabric-prod.js +0.22% 274.55 kB 275.15 kB +0.15% 49.49 kB 49.57 kB
react-native/implementations/ReactNativeRenderer-prod.js +0.21% 283.17 kB 283.78 kB +0.14% 51.06 kB 51.13 kB
react-native/implementations/ReactFabric-prod.fb.js +0.21% 286.03 kB 286.63 kB +0.17% 51.65 kB 51.74 kB
react-native/implementations/ReactNativeRenderer-prod.fb.js +0.21% 289.98 kB 290.59 kB +0.13% 52.54 kB 52.61 kB
react-native/implementations/ReactFabric-profiling.js +0.21% 293.54 kB 294.15 kB +0.15% 52.67 kB 52.75 kB

Generated by 🚫 dangerJS against 51f9345

@lunaleaps lunaleaps force-pushed the optional-bubble-dispatch-config branch from e85940c to 6a1c679 Compare March 1, 2022 22:11
Comment on lines 19 to 23
phasedRegistrationNames: $ReadOnly<{|
captured: string,
bubbled: string,
skipBubbling?: ?boolean,
|}>,
Copy link
Contributor

Choose a reason for hiding this comment

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

This type implies the possibility of a nonsensical value:

phasedRegistrationNames: {
  captured: 'onFooCapture',
  bubbled: 'onFooWhateverIFeelLike', // Not actually used, so why does it matter?
  skipBubbling: true,
},

Could you have accomplished the same by making bubbled nullable?

Suggested change
phasedRegistrationNames: $ReadOnly<{|
captured: string,
bubbled: string,
skipBubbling?: ?boolean,
|}>,
phasedRegistrationNames: $ReadOnly<{|
captured: string,
bubbled: null | string,
|}>,

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yea I thought about this but we still need a way to define what the bubbled event listener name is. Because even if we don't bubble, we still need to dispatch that listener on the target. Unless we can infer the name of event by the captured listener?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I guess clarification -- the bubbled listener is still dispatched on the target even if the event is non-bubbling

Copy link
Contributor

Choose a reason for hiding this comment

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

Ahh, right. This really reveals how terrible the current event config is at describing the events, heh. Thanks for the explanation.

Copy link
Member

Choose a reason for hiding this comment

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

I may be missing some previous discussions on this, but what do we think about calling this bubbles to bring it in line with the name used on the Web for custom events?

Long term, the event config could match even closer to the Web spec for dispatching custom events, something like:

{
  type: string,
  bubbles: boolean,
  
  // Not in web spec, but could be implemented in React.
  captures: boolean,
}

Which would turn this:

{
  bubblingEventTypes: {
    topDefaultBubblingEvent: {
      phasedRegistrationNames: {
        captured: 'onDefaultBubblingEventCapture',
        bubbled: 'onDefaultBubblingEvent',
      },
    },
  }
  directEventTypes: {
    topDirectEvent: {
       registrationName: 'onDirectEvent',
    },
  }
}

Into this:

[
  {
    // Bubbling phase is onDefaultBubblingEvent
    // Capture phase is onDefaultBubblingEventCapture
    // Top is topDefaultBubblingEvent
    // Captures and bubbles
    type: 'DefaultBubblingEvent',
    bubbles: true,
    captures: true,
  },
  {
    // Direct event name is onDirectEvent, no capture, no bubble.
    type: 'DirectEvent',
    bubbles: false,
    captures: false,
  }
]

Notice that there's only one config (instead of bubbling vs direct), and all of the event names like top*, on*, and on*Capture are automatically derived from the event type, similar to what we do in React DOM with things like onClick and onClickCapture.

@lunaleaps lunaleaps force-pushed the optional-bubble-dispatch-config branch from 6a1c679 to 36bfd46 Compare March 2, 2022 02:07
Copy link
Contributor

@yungsters yungsters left a comment

Choose a reason for hiding this comment

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

Looks good to me. Thanks again.

const skipBubbling =
event != null &&
event.dispatchConfig.phasedRegistrationNames != null &&
event.dispatchConfig.phasedRegistrationNames.skipBubbling;
Copy link
Contributor

Choose a reason for hiding this comment

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

Can use ?. here?

Comment on lines 19 to 23
phasedRegistrationNames: $ReadOnly<{|
captured: string,
bubbled: string,
skipBubbling?: ?boolean,
|}>,
Copy link
Contributor

Choose a reason for hiding this comment

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

Ahh, right. This really reveals how terrible the current event config is at describing the events, heh. Thanks for the explanation.

@lunaleaps lunaleaps force-pushed the optional-bubble-dispatch-config branch from 36bfd46 to 24f0216 Compare March 10, 2022 21:46
@lunaleaps
Copy link
Contributor Author

cc @rickhanlonii, @gaearon could I get your guys' thoughts on this?

@lunaleaps lunaleaps force-pushed the optional-bubble-dispatch-config branch from 24f0216 to 42d24da Compare March 11, 2022 20:39
@lunaleaps
Copy link
Contributor Author

@lunaleaps lunaleaps force-pushed the optional-bubble-dispatch-config branch from 42d24da to 51f9345 Compare March 14, 2022 17:28
@lunaleaps lunaleaps merged commit 43eb283 into facebook:main Mar 14, 2022
@lunaleaps lunaleaps deleted the optional-bubble-dispatch-config branch March 14, 2022 17:59
@@ -73,6 +73,7 @@ export type ViewConfig = $ReadOnly<{
phasedRegistrationNames: $ReadOnly<{
captured: string,
bubbled: string,
skipBubble?: ?boolean,
Copy link

Choose a reason for hiding this comment

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

@lunaleaps seems like this property name is (accidentally?) different here, flow check during React sync caught this

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh my mistake! How should I address? Can we fix forward on the sync?

Copy link
Contributor Author

@lunaleaps lunaleaps Mar 16, 2022

Choose a reason for hiding this comment

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

Submitting a fix now: #24109 -- waiting for tests to pass. If anyone knows what I should've run to check this .. let me know!

facebook-github-bot pushed a commit to facebook/react-native that referenced this pull request Mar 24, 2022
Summary:
This sync includes the following changes:
- **[3f8990898](facebook/react@3f8990898 )**: Fix test-build-devtools if build was generated by build-for-devtools ([#24088](facebook/react#24088)) //<Sebastian Silbermann>//
- **[577f2de46](facebook/react@577f2de46 )**: enableCacheElement flag ([#24131](facebook/react#24131)) //<David McCabe>//
- **[2e0d86d22](facebook/react@2e0d86d22 )**: Allow updating dehydrated root at lower priority without forcing client render ([#24082](facebook/react#24082)) //<Andrew Clark>//
- **[dbe9e732a](facebook/react@dbe9e732a )**: Avoid conditions where control flow is sufficient ([#24126](facebook/react#24126)) //<Sebastian Markbåge>//
- **[b075f9742](facebook/react@b075f9742 )**: Fix dispatch config type for skipBubbling ([#24109](facebook/react#24109)) //<Luna>//
- **[ef23a9ee8](facebook/react@ef23a9ee8 )**: Flag for text hydration mismatch ([#24107](facebook/react#24107)) //<salazarm>//
- **[0412f0c1a](facebook/react@0412f0c1a )**: add offscreen state node ([#24026](facebook/react#24026)) //<Luna Ruan>//
- **[43eb28339](facebook/react@43eb28339 )**: Add skipBubbling property to dispatch config ([#23366](facebook/react#23366)) //<Luna>//
- **[832e2987e](facebook/react@832e2987e )**: Revert accdientally merged PR ([#24081](facebook/react#24081)) //<Andrew Clark>//
- **[02b65fd8c](facebook/react@02b65fd8c )**: Allow updates at lower pri without forcing client render //<Andrew Clark>//
- **[83b941a51](facebook/react@83b941a51 )**: Add isRootDehydrated function //<Andrew Clark>//
- **[c8e4789e2](facebook/react@c8e4789e2 )**: Pass children to hydration root constructor //<Andrew Clark>//
- **[581f0c42e](facebook/react@581f0c42e )**: [Flight] add support for Lazy components in Flight server ([#24068](facebook/react#24068)) //<Josh Story>//
- **[72a933d28](facebook/react@72a933d28 )**: Gate legacy hidden ([#24047](facebook/react#24047)) //<Sebastian Markbåge>//
- **[b9de50d2f](facebook/react@b9de50d2f )**: Update test to reset modules instead of using private state ([#24055](facebook/react#24055)) //<Sebastian Markbåge>//
- **[c91892ec3](facebook/react@c91892ec3 )**: [Fizz] Don't flush empty segments ([#24054](facebook/react#24054)) //<Sebastian Markbåge>//
- **[d5f1b067c](facebook/react@d5f1b067c )**: [ServerContext] Flight support for ServerContext ([#23244](facebook/react#23244)) //<salazarm>//
- **[6edd55a3f](facebook/react@6edd55a3f )**: Gate unstable_expectedLoadTime on enableCPUSuspense ([#24038](facebook/react#24038)) //<Sebastian Markbåge>//
- **[57799b912](facebook/react@57799b912 )**: Add more feature flag checks ([#24037](facebook/react#24037)) //<Sebastian Markbåge>//
- **[e09518e5b](facebook/react@e09518e5b )**: [Fizz] write chunks to a buffer with no re-use ([#24034](facebook/react#24034)) //<Josh Story>//
- **[14c2be8da](facebook/react@14c2be8da )**: Rename Node SSR Callbacks to onShellReady/onAllReady and Other Fixes ([#24030](facebook/react#24030)) //<Sebastian Markbåge>//
- **[cb1e7b1c6](facebook/react@cb1e7b1c6 )**: Move onCompleteAll to .allReady Promise ([#24025](facebook/react#24025)) //<Sebastian Markbåge>//
- **[566285761](facebook/react@566285761 )**: [Fizz] Export debug function for FB ([#24024](facebook/react#24024)) //<salazarm>//
- **[05c283c3c](facebook/react@05c283c3c )**: Fabric HostComponent as EventEmitter: support add/removeEventListener (unstable only) ([#23386](facebook/react#23386)) //<Joshua Gross>//
- **[08644348b](facebook/react@08644348b )**: Added unit Tests in the ReactART, increasing the code coverage ([#23195](facebook/react#23195)) //<BIKI DAS>//
- **[feefe437f](facebook/react@feefe437f )**: Refactor Cache Code ([#23393](facebook/react#23393)) //<Luna Ruan>//

Changelog:
[General][Changed] - React Native sync for revisions 1780659...1159ff6

jest_e2e[run_all_tests]

Reviewed By: lunaleaps

Differential Revision: D34928167

fbshipit-source-id: 8c386f2be5871981d217ab9a514892ed88eafcfb
zhengjitf pushed a commit to zhengjitf/react that referenced this pull request Apr 15, 2022
Saadnajmi pushed a commit to Saadnajmi/react-native-macos that referenced this pull request Jan 15, 2023
Summary:
This sync includes the following changes:
- **[3f8990898](facebook/react@3f8990898 )**: Fix test-build-devtools if build was generated by build-for-devtools ([facebook#24088](facebook/react#24088)) //<Sebastian Silbermann>//
- **[577f2de46](facebook/react@577f2de46 )**: enableCacheElement flag ([facebook#24131](facebook/react#24131)) //<David McCabe>//
- **[2e0d86d22](facebook/react@2e0d86d22 )**: Allow updating dehydrated root at lower priority without forcing client render ([facebook#24082](facebook/react#24082)) //<Andrew Clark>//
- **[dbe9e732a](facebook/react@dbe9e732a )**: Avoid conditions where control flow is sufficient ([facebook#24126](facebook/react#24126)) //<Sebastian Markbåge>//
- **[b075f9742](facebook/react@b075f9742 )**: Fix dispatch config type for skipBubbling ([facebook#24109](facebook/react#24109)) //<Luna>//
- **[ef23a9ee8](facebook/react@ef23a9ee8 )**: Flag for text hydration mismatch ([facebook#24107](facebook/react#24107)) //<salazarm>//
- **[0412f0c1a](facebook/react@0412f0c1a )**: add offscreen state node ([facebook#24026](facebook/react#24026)) //<Luna Ruan>//
- **[43eb28339](facebook/react@43eb28339 )**: Add skipBubbling property to dispatch config ([facebook#23366](facebook/react#23366)) //<Luna>//
- **[832e2987e](facebook/react@832e2987e )**: Revert accdientally merged PR ([facebook#24081](facebook/react#24081)) //<Andrew Clark>//
- **[02b65fd8c](facebook/react@02b65fd8c )**: Allow updates at lower pri without forcing client render //<Andrew Clark>//
- **[83b941a51](facebook/react@83b941a51 )**: Add isRootDehydrated function //<Andrew Clark>//
- **[c8e4789e2](facebook/react@c8e4789e2 )**: Pass children to hydration root constructor //<Andrew Clark>//
- **[581f0c42e](facebook/react@581f0c42e )**: [Flight] add support for Lazy components in Flight server ([facebook#24068](facebook/react#24068)) //<Josh Story>//
- **[72a933d28](facebook/react@72a933d28 )**: Gate legacy hidden ([facebook#24047](facebook/react#24047)) //<Sebastian Markbåge>//
- **[b9de50d2f](facebook/react@b9de50d2f )**: Update test to reset modules instead of using private state ([facebook#24055](facebook/react#24055)) //<Sebastian Markbåge>//
- **[c91892ec3](facebook/react@c91892ec3 )**: [Fizz] Don't flush empty segments ([facebook#24054](facebook/react#24054)) //<Sebastian Markbåge>//
- **[d5f1b067c](facebook/react@d5f1b067c )**: [ServerContext] Flight support for ServerContext ([facebook#23244](facebook/react#23244)) //<salazarm>//
- **[6edd55a3f](facebook/react@6edd55a3f )**: Gate unstable_expectedLoadTime on enableCPUSuspense ([facebook#24038](facebook/react#24038)) //<Sebastian Markbåge>//
- **[57799b912](facebook/react@57799b912 )**: Add more feature flag checks ([facebook#24037](facebook/react#24037)) //<Sebastian Markbåge>//
- **[e09518e5b](facebook/react@e09518e5b )**: [Fizz] write chunks to a buffer with no re-use ([facebook#24034](facebook/react#24034)) //<Josh Story>//
- **[14c2be8da](facebook/react@14c2be8da )**: Rename Node SSR Callbacks to onShellReady/onAllReady and Other Fixes ([facebook#24030](facebook/react#24030)) //<Sebastian Markbåge>//
- **[cb1e7b1c6](facebook/react@cb1e7b1c6 )**: Move onCompleteAll to .allReady Promise ([facebook#24025](facebook/react#24025)) //<Sebastian Markbåge>//
- **[566285761](facebook/react@566285761 )**: [Fizz] Export debug function for FB ([facebook#24024](facebook/react#24024)) //<salazarm>//
- **[05c283c3c](facebook/react@05c283c3c )**: Fabric HostComponent as EventEmitter: support add/removeEventListener (unstable only) ([facebook#23386](facebook/react#23386)) //<Joshua Gross>//
- **[08644348b](facebook/react@08644348b )**: Added unit Tests in the ReactART, increasing the code coverage ([facebook#23195](facebook/react#23195)) //<BIKI DAS>//
- **[feefe437f](facebook/react@feefe437f )**: Refactor Cache Code ([facebook#23393](facebook/react#23393)) //<Luna Ruan>//

Changelog:
[General][Changed] - React Native sync for revisions 1780659...1159ff6

jest_e2e[run_all_tests]

Reviewed By: lunaleaps

Differential Revision: D34928167

fbshipit-source-id: 8c386f2be5871981d217ab9a514892ed88eafcfb
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants