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.

Sign up for GitHub

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

Jump to bottom

Add skipBubbling property to dispatch config #23366

Merged
merged 1 commit into from
Mar 14, 2022
Merged

Add skipBubbling property to dispatch config #23366

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
44 changes: 39 additions & 5 deletions packages/react-native-renderer/src/ReactNativeBridgeEventPlugin.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,12 @@ function getParent(inst) {
/**
* Simulates the traversal of a two-phase, capture/bubble event dispatch.
*/
export function traverseTwoPhase(inst: Object, fn: Function, arg: Function) {
export function traverseTwoPhase(
inst: Object,
fn: Function,
arg: Function,
skipBubbling: boolean,
) {
const path = [];
while (inst) {
path.push(inst);
Expand All @@ -76,21 +81,42 @@ export function traverseTwoPhase(inst: Object, fn: Function, arg: Function) {
for (i = path.length; i-- > 0; ) {
fn(path[i], 'captured', arg);
}
for (i = 0; i < path.length; i++) {
fn(path[i], 'bubbled', arg);
if (skipBubbling) {
// Dispatch on target only
fn(path[0], 'bubbled', arg);
} else {
for (i = 0; i < path.length; i++) {
fn(path[i], 'bubbled', arg);
}
}
}

function accumulateTwoPhaseDispatchesSingle(event) {
if (event && event.dispatchConfig.phasedRegistrationNames) {
traverseTwoPhase(event._targetInst, accumulateDirectionalDispatches, event);
traverseTwoPhase(
event._targetInst,
accumulateDirectionalDispatches,
event,
false,
);
}
}

function accumulateTwoPhaseDispatches(events) {
forEachAccumulated(events, accumulateTwoPhaseDispatchesSingle);
}

function accumulateCapturePhaseDispatches(event) {
if (event && event.dispatchConfig.phasedRegistrationNames) {
traverseTwoPhase(
event._targetInst,
accumulateDirectionalDispatches,
event,
true,
);
}
}

/**
* Accumulates without regard to direction, does not look for phased
* registration names. Same as `accumulateDirectDispatchesSingle` but without
Expand Down Expand Up @@ -162,7 +188,15 @@ const ReactNativeBridgeEventPlugin = {
nativeEventTarget,
);
if (bubbleDispatchConfig) {
accumulateTwoPhaseDispatches(event);
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?

if (skipBubbling) {
accumulateCapturePhaseDispatches(event);
} else {
accumulateTwoPhaseDispatches(event);
}
} else if (directDispatchConfig) {
accumulateDirectDispatches(event);
} else {
Expand Down
1 change: 1 addition & 0 deletions packages/react-native-renderer/src/ReactNativeTypes.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 •
edited
Loading

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!

}>,
}>,
...,
Expand Down
100 changes: 100 additions & 0 deletions packages/react-native-renderer/src/__tests__/ReactFabric-test.internal.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -633,6 +633,106 @@ describe('ReactFabric', () => {
expect(touchStart2).toBeCalled();
});

describe('skipBubbling', () => {
it('should skip bubbling to ancestor if specified', () => {
const View = createReactNativeComponentClass('RCTView', () => ({
validAttributes: {},
uiViewClassName: 'RCTView',
bubblingEventTypes: {
topDefaultBubblingEvent: {
phasedRegistrationNames: {
captured: 'onDefaultBubblingEventCapture',
bubbled: 'onDefaultBubblingEvent',
},
},
topBubblingEvent: {
phasedRegistrationNames: {
captured: 'onBubblingEventCapture',
bubbled: 'onBubblingEvent',
skipBubbling: false,
},
},
topSkipBubblingEvent: {
phasedRegistrationNames: {
captured: 'onSkippedBubblingEventCapture',
bubbled: 'onSkippedBubblingEvent',
skipBubbling: true,
},
},
},
}));
const ancestorBubble = jest.fn();
const ancestorCapture = jest.fn();
const targetBubble = jest.fn();
const targetCapture = jest.fn();

const event = {};

act(() => {
ReactFabric.render(
<View
onSkippedBubblingEventCapture={ancestorCapture}
onDefaultBubblingEventCapture={ancestorCapture}
onBubblingEventCapture={ancestorCapture}
onSkippedBubblingEvent={ancestorBubble}
onDefaultBubblingEvent={ancestorBubble}
onBubblingEvent={ancestorBubble}>
<View
onSkippedBubblingEventCapture={targetCapture}
onDefaultBubblingEventCapture={targetCapture}
onBubblingEventCapture={targetCapture}
onSkippedBubblingEvent={targetBubble}
onDefaultBubblingEvent={targetBubble}
onBubblingEvent={targetBubble}
/>
</View>,
11,
);
});

expect(nativeFabricUIManager.createNode.mock.calls.length).toBe(2);
expect(nativeFabricUIManager.registerEventHandler.mock.calls.length).toBe(
1,
);
const [
,
,
,
,
childInstance,
] = nativeFabricUIManager.createNode.mock.calls[0];
const [
dispatchEvent,
] = nativeFabricUIManager.registerEventHandler.mock.calls[0];

dispatchEvent(childInstance, 'topDefaultBubblingEvent', event);
expect(targetBubble).toHaveBeenCalledTimes(1);
expect(targetCapture).toHaveBeenCalledTimes(1);
expect(ancestorCapture).toHaveBeenCalledTimes(1);
expect(ancestorBubble).toHaveBeenCalledTimes(1);
ancestorBubble.mockReset();
ancestorCapture.mockReset();
targetBubble.mockReset();
targetCapture.mockReset();

dispatchEvent(childInstance, 'topBubblingEvent', event);
expect(targetBubble).toHaveBeenCalledTimes(1);
expect(targetCapture).toHaveBeenCalledTimes(1);
expect(ancestorCapture).toHaveBeenCalledTimes(1);
expect(ancestorBubble).toHaveBeenCalledTimes(1);
ancestorBubble.mockReset();
ancestorCapture.mockReset();
targetBubble.mockReset();
targetCapture.mockReset();

dispatchEvent(childInstance, 'topSkipBubblingEvent', event);
expect(targetBubble).toHaveBeenCalledTimes(1);
expect(targetCapture).toHaveBeenCalledTimes(1);
expect(ancestorCapture).toHaveBeenCalledTimes(1);
expect(ancestorBubble).not.toBeCalled();
});
});

it('dispatches event with target as instance', () => {
const View = createReactNativeComponentClass('RCTView', () => ({
validAttributes: {
Expand Down
2 changes: 2 additions & 0 deletions packages/react-native-renderer/src/legacy-events/ReactSyntheticEventType.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ export type DispatchConfig = {|
phasedRegistrationNames: {|
bubbled: null | string,
captured: null | string,
skipBubbling?: ?boolean,
|},
registrationName?: string,
|};
Expand All @@ -24,6 +25,7 @@ export type CustomDispatchConfig = {|
phasedRegistrationNames: {|
bubbled: null,
captured: null,
skipBubbling?: ?boolean,
|},
registrationName?: string,
customEvent: true,
Expand Down
1 change: 1 addition & 0 deletions scripts/rollup/shims/react-native/ReactNativeViewConfigRegistry.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ const customBubblingEventTypes: {
phasedRegistrationNames: $ReadOnly<{|
captured: string,
bubbled: string,
skipBubbling?: ?boolean,
|}>,
Comment on lines 19 to 23
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.

|}>,
...,
Expand Down