[RFC] Use event.defaultMuiPrevented to prevent default events

[oliviertassinari]

Summary 💡

This RFC highlights issues with the current solution and explores an alternative with fewer drawbacks. The conversation started with #1381.

Motivation 🔦

The cell edit feature is soon to be released with the introduction of a new concept: event.stopPropagation() as a means to prevent the default behavior of the component.

https://github.com/mui-org/material-ui-x/blob/ca286ad1aa388a7ee8b6cb3609124f583679a45d/packages/grid/x-grid/src/tests/events.XGrid.test.tsx#L221-L230

I would like to highlight 5 important problems that the approach creates (before we release it).

1. The semantic is wrong (IMHO), but to be unbaised: confusing. event.stopPropagation() in the DOM and React stops the bubbling of the event to the parents but has no impacts on the element it listens to. It keeps calling the other listeners on the currentTarget element. In order to stop the other listeners to trigger on the same element, developers have to use event.stopImmediatePropagation in the DOM, which doesn't exist in the React realm.
2. event.stopPropagation() only works for events that are React synthetic events. The approach doesn't work on DOM events because there are no equivalents to event.defaultPrevented like React has (event.isPropagationStopped). So we can't scale the solution to all our use cases.
3. event.stopPropagation() stops the bubbling of the event. Meaning, if you want to handle the event on the parent, but still want to prevent the default behavior. You can't, you are screwed. Proof: https://codesandbox.io/s/material-demo-forked-1sstp?file=/demo.js. This is why it's commonly considered a practice to void. I agree that calling stopPropagation on the events the data grid is going to use for the cell editing makes sense, it's only when we want to disable the feature that it doesn't make any sense.
4. This is inconsistent. There is a prior-art in the main repository for using a different solution for the very same problem. It's currently deployed on the SwipeableDrawer and the Autocomplete. You can find it documented at https://next.material-ui.com/components/autocomplete/#events. When we introduced it, we thought about these problems, event.stopPropagation was an option we ignored because of the downsides I have listed before.
5. The parent callbacks are called before the component's callback. It's commonly the opposite of how callbacks run so that once the component yields to the parent, all the necessary actions have already been performed and the parent can continue.

Proposed Solution 🧾

We can solve 1. 2. 3. and 4. by using the approach used in the core. The only downside it has I'm aware of with this solution is 5. I actually think that the discussion should be reversed. Why is the solution in the core not good enough that it prevents being applied in the data grid component?

<DataGrid
  onCellDoubleClick={(event) => {
    // Prevent's default 'edit cell' behavior.
    event.defaultMuiPrevented = true;
  }}
/>

Unresolved Questions ❓

I have no idea how we could come up with a solution that also resolves 5.

[dtassone]

To solve 3 & 5 you can use a capture event.
Not sure about 2.

https://codesandbox.io/s/material-demo-forked-3mhuk?file=/demo.js

[m4theushw]

@oliviertassinari Could you detail which callbacks you refer in 5? I'm not sure if I understood the problem.

Do you mean that onCellDoubleClick should fire AFTER the DataGrid has handled the double click? Should we add callbacks to fire at the beginning and at the end of each listener?

[oliviertassinari]

Could you detail which callbacks you refer in 5?

@m4theushw Yeah, I wasn't very clear. My point assumed too much context.
There are two possible way events are processed. The one way we try (in the core components) to default to is:

• 1 The DOM event is fired
• 2 The DataGrid handles the event
• 3 The DataGrid yields the event to the developer

However, in order to make event.stopPropagation() or event.defaultMuiPrevented = true work, we need to revert 3 and 2:

• 1 The DOM event is fired
• 3 The DataGrid yields the event to the developer
• 2 The DataGrid handles the event (check of the developer didn't cancel it)

To be honest, I have never seen a real issue with the reversal of the call order. But on paper, it's not sounds, so be careful, there are mutations done in each handler. It's more a theoretical concern than a practical one.

[dtassone]

for 3., the first listener is always options.onCellClick so developer can control the event

[oliviertassinari]

@dtassone Yes, this is the problem I'm describing, in theory, it shouldn't.

[m4theushw]

However, in order to make event.stopPropagation() or event.defaultMuiPrevented = true work, we need to revert 3 and 2:

1 The DOM event is fired
3 The DataGrid yields the event to the developer
2 The DataGrid handles the event (check of the developer didn't cancel it)

All event handlers registered in the useEvents hook will be called before the default behavior. If we reverse this, we need to add another callback to allow users to cancel them.

This seems to me similar to the old React lifecycle callbacks: componentWillUpdate and componentDidUpdate. They are all related but fired at different times.

[oliviertassinari]

@m4theushw I would personally advocate for ignoring 5. if the solution it requires is too heavy, the downside is limited.

[dtassone]

We also control a part of the publication of events as apiRef is based on eventEmitter, so we could block an event from being propagated to a second listener if it is being published by apiRef.

ATM. if apiRef.current.publishEvent publish an event that is being caught by 2 hooks. Then if the first one stop propagation, it would still go to the second hook (apiRef.current.emit). However, the handler of the first hook will not publish anymore events (apiRef.current.publishEvent).

[dtassone]

some more thoughts.

1. Using the defaultMuiPrevented approach won't prevent the event from bubbling.
   So we would need checks to avoid running the code in almost every events. ie. cellClick will bubble to rowClick.
2. If we take this path, 2 ways of implementing it.
   a. we modify the original event as in autoComplete, not ideal, types won't match, and it's not a great practice
   b. we wrap it like MuiEvent<T extends React.SyntheticEvent>. Keep the original event, and add behavior on top of it. So we can forward the original event to the user.
3. If we use event propagation, we stay part of the react and web standards. No learning curve. The propagation works as in any other react component.

[oliviertassinari]

1. row click comes from the GRID_ROW_CLICK event. I assume the solution would be identical to
   https://github.com/mui-org/material-ui-x/blob/fa346f0fbe3d9b9eea9bb403fe4675f544d6abf9/packages/grid/_modules_/grid/hooks/root/useApi.ts#L18
2. The defaultMuiPrevented pattern was inspired by feat(handlers): prevent default behavior with preventDownshiftDefault downshift-js/downshift#358, solving the same problem we have: Change how to prevent default events downshift-js/downshift#353.
3. Point 1 to 3 in the initial comments shows the fundamental issues with stopping the propagation of the event. As for the drawback of the defaultMuiPrevented approach, we have raised one: the mutation of the event, and yes, TypeScript will struggle to handle it. See TypeScript: definitions missing preventDownshiftDefault downshift-js/downshift#734. I personally think that any is fine here.

[dtassone]

For 1. there is a small distinction here, that I was trying to explain.
So a native event can be stopped by event.stopPropagation() see https://codesandbox.io/s/material-demo-forked-3mhuk?file=/demo.js

However for events that are specific to the grid such as 'cellClick' or cellEditStart then we would need to do the check. And publishEvent is sort of copying the behavior of a native event propagation.

[oliviertassinari]

Regarding the TypeScript downside of this proposal. I personally think that a if ((event as any).defaultMuiPrevented) { is an acceptable tradeoff compared to the other wins.

[oliviertassinari]

There might be a third way to solve this problem, a stateReducer which would allow developers to make the events they are interested in as no-op. However, it would require refactoring the update of the state to attach them corresponding event names.

[oliviertassinari]

@m4theushw, @flaviendelangle Are we happy to move forward with: event.defaultMuiPrevented? We would need to remove the documentation of event.stopPropagation.

If my issue description is not enough, see for another example of why #2146 (comment).

It's potentially a breaking change if we drop custom logic for (event.stopPropagation). We don't have to do it now, we could do it later on, in the next major.

[m4theushw]

I'm happy to adopt event.defaultMuiPrevented but I think we should provide a solution to allow to cancel the default behavior of an event while allowing another one to run: #2146 (comment). That means, not sharing the same event object across handlers. I would propose to introduce a third argument containing the "event" that caused a certain handler to be called. This is different from the React.SyntheticEvent passed.

onCellFocusOut?: (params: GridCellParams, originalEvent?: React.SyntheticEvent, event: MuiEvent<MouseEvent>) => void;

The third event is the one that we check for defaultMuiPrevented. Since it's unique for each event, users can specifically cancel one while other listeners will continue to run.

The problem with the event.defaultMuiPrevented proposed in this issue is that it's actually stopping the propagation of subsequent listeners and cancelling the default behavior.

[oliviertassinari]

@m4theushw Interesting. Two thoughts:

1. If we keep sharing the same event, we could reset event.defaultMuiPrevented when a grid event handler starts to process it. It would solve the isolation issue.
2. The third argument API proposal you have makes me think that we could actually have a custom API. We could have a synthetic preventDefault() callback developers can call, we wouldn't even need an "event".

[m4theushw]

I'm thinking about how we can implement 1. Taking as example #2146 (comment):

cellFocusOut triggers cellEditPropsChangeCommitted, then cellEditExit, all using the same event.

I want to cancel cellEditPropsChangeCommitted but allow cellEditExit. To do that, we need to reset event.defaultMuiPrevented after the last listenter of the first event. The problem comes if the user is using a promise, then we may lost sync.

About 2. could you elaborate more your proposal? Is about wrapping the event? It has to work both with synthetic events and native events.

[oliviertassinari]

for option 1. I had this reset in mind. When the grid fires a new event, we make sure it's pristine.

diff --git a/packages/grid/_modules_/grid/hooks/root/useApi.ts b/packages/grid/_modules_/grid/hooks/root/useApi.ts
index 91637e88..8041c29d 100644
--- a/packages/grid/_modules_/grid/hooks/root/useApi.ts
+++ b/packages/grid/_modules_/grid/hooks/root/useApi.ts
@@ -11,6 +11,9 @@ export function useApi(apiRef: GridApiRef): void {
   const publishEvent = React.useCallback(
     (name: string, params: any, event?: React.SyntheticEvent) => {
       if (!event || !event.isPropagationStopped || !event.isPropagationStopped()) {
+        if (event) {
+          event.defaultMuiPrevented = false;
+        }
         apiRef.current.emit(name, params, event);
       }
     },

for option 2. I guess I didn't get how it would work in practice. I didn't get how we could have a unique event object per grid event we fire, say in the example we have used since the beginning.

for option 3 (the alternative to 2 that you made me think of). The concept is simply to recognize that we use the event as a vector to cancel the handles of the grid. We could use a custom data structure to do it too.

---

As a side note, the current handling of .defaultMuiPrevented and isPropagationStopped is not great. AFAIK, we don't need to repeat the exit branch logic in each handler. Instead, we could move the logic at the useGridApiEventHandler level, to make it a systematic behavior.

[m4theushw]

I liked this option 1. About the potential issue I raised with promises, I don't think we have to worry. If the developer cancels the default behavior outside the promise it will work. We can extend the MuiEvent we already use to accommodate both synthetic and native events:

export type MuiEvent<T extends keyof GlobalEventHandlersEventMap> = (
  | GlobalEventHandlersEventMap[T]
  | React.SyntheticEvent
) & {
  defaultMuiPrevented?: boolean;
};

keyof GlobalEventHandlersEventMap means "mouseover", "click", "dragover" and so on.

Some events are published without the event argument. Even when there's no subscriber listening to them, for consistency, we have to provide a default one.

https://github.com/mui-org/material-ui-x/blob/76aeea1f76b4c0118f7b9c4eeaaff4e4e2f31bea/packages/grid/_modules_/grid/hooks/features/columns/useGridColumns.ts#L236